CherryPy should handle If-Range's properly #1760

ian-otto · 2018-12-02T10:40:17Z

What kind of change does this PR introduce?

What is the related issue number (starting with #)

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

In the presence of an If-Range header, CP will check if the time is before the current time, and if so, return the entire file as opposed to respecting the Range header. This is the RFC compliant way to handle this.

Other information:

Checklist:

I think the code is well written
I wrote good commit messages
I have squashed related commits together after the changes have been approved
Unit tests for the changes exist
Integration tests for the changes exist (if applicable)
I used the same coding conventions as the rest of the project
The new code doesn't generate linter offenses
Documentation reflects the changes
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences

webknjaz · 2018-12-05T23:31:50Z

cherrypy/lib/static.py

@@ -174,7 +176,21 @@ def _serve_fileobj(fileobj, content_type, content_length, debug=False):
                cherrypy.log(message, 'TOOLS.STATIC')
            raise cherrypy.HTTPError(416, message)

-        if r:
+        IF_RANGE_IN_PAST = False


Since it's a variable and not a constant, you should use lowercase

webknjaz · 2018-12-05T23:33:27Z

cherrypy/lib/static.py

+            # and the server sends back a 206 Partial Content answer with
+            # the appropriate body. If the condition is not fulfilled,
+            # the full resource is sent back, with a 200 OK status.
+            if datetime.datetime(*parsedate(request.headers.get('If-Range'))[:6]) \


We prefer wrapping clause with braces instead of escaping EOL

ian-otto · 2018-12-05T23:44:18Z

Thanks for cleaning this up for me, I'm still getting used to the coding conventions on this project, as well as my IDE doing things for me that don't match what the convention is!

webknjaz · 2018-12-05T23:52:35Z

No worries, just try to follow PEP8 + PEP257 at first :)

webknjaz · 2018-12-06T00:01:20Z

cherrypy/lib/static.py

@@ -174,7 +176,22 @@ def _serve_fileobj(fileobj, content_type, content_length, debug=False):
                cherrypy.log(message, 'TOOLS.STATIC')
            raise cherrypy.HTTPError(416, message)

-        if r:
+        is_if_range_in_past = False


I think that this check would be moved before httputil.get_ranges() call, so that we could skip calling another heavy function.
Also, this whole check could be put into its own function (maybe in httputil) for readability.

webknjaz · 2018-12-06T00:02:17Z

It looks like flake8 made checks stricter so this will need to be fixed separately.

cherrypy/lib/static.py

webknjaz · 2018-12-06T08:21:08Z

cherrypy/lib/static.py

+            # the appropriate body. If the condition is not fulfilled,
+            # the full resource is sent back, with a 200 OK status.
+            # Ref: https://tools.ietf.org/html/rfc7233#section-3.2
+            if datetime(*parsedate(if_range_header)[:6]) < datetime.now():


This will probably fail with E-Tag value:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-Range

I'll write a change for this. Do you think searching for exactly two double quotes will be enough for detecting ETAG vs Date? That's what I'm working with at the moment.

I'd probably go for try/except

And it should probably go to httputil module

Migrated to httputil, and caught exception when there is an ETag instead of an httpdate. However, I treat the ETag as an always-fail condition as to my knowledge we don't support that yet? Correct me if I'm wrong.

It's actually supported in caching tool: https://github.com/cherrypy/cherrypy/search?q=expires&unscoped_q=expires&type=Code

And it just occurred to me that maybe this check should be done there, not in this module.

There's also a tool called expires + there's tests for etag as well.

Could you please explore this?

…unction usage in static.py.

webknjaz · 2019-01-04T21:51:09Z

This pull request introduces 2 alerts when merging 2da1314 into e10b984 - view on LGTM.com

new alerts:

2 for Unused import

Comment posted by LGTM.com

cherrypy/lib/httputil.py

cherrypy/lib/static.py

Correct passes_if_range_check to not take request object, but rather take given if_range_header

webknjaz · 2019-04-09T03:06:06Z

This pull request introduces 1 alert when merging 302c2d9 into b648977 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

Comment posted by LGTM.com

cherrypy/lib/httputil.py

cherrypy/lib/static.py

Co-Authored-By: duper51 <ianotto24@gmail.com>

webknjaz · 2019-04-10T00:14:17Z

This pull request introduces 1 alert when merging 64a52cc into b648977 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

Comment posted by LGTM.com

…heck Invert logic in matches_if_range_check to save on indentation

# Conflicts: # cherrypy/lib/httputil.py

webknjaz · 2019-04-10T22:32:19Z

This pull request introduces 1 alert when merging 13e149b into b648977 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

Comment posted by LGTM.com

cherrypy/lib/static.py

cherrypy/lib/httputil.py

Co-Authored-By: duper51 <ianotto24@gmail.com>

…ix-1699

webknjaz · 2019-04-11T22:39:10Z

This pull request introduces 1 alert when merging ae98c9b into b648977 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

Comment posted by LGTM.com

flake8 fixes

ensure old functionality is still present (logging parameter for non-requested byterange)

webknjaz · 2019-04-12T10:34:44Z

cherrypy/lib/httputil.py

@@ -66,6 +67,29 @@ def protocol_from_http(protocol_str):
    return int(protocol_str[5]), int(protocol_str[7])


+def matches_if_range_check(if_range_header):
+    """
+    Determines if an If-Range header is present and passes its conditions.


Imperative plz.

Suggested change

Determines if an If-Range header is present and passes its conditions.

Determine if an If-Range header conditions are fulfilled.

webknjaz · 2019-04-12T10:43:46Z

cherrypy/lib/static.py

-                    stop = content_length
-                r_len = stop - start
+        if_range_valid = httputil.matches_if_range_check(
+            request.headers.get('Range')


plz add a trailing comma here

Suggested change

request.headers.get('Range')

request.headers.get('Range'),

webknjaz · 2019-04-12T10:44:01Z

cherrypy/lib/static.py

+        if if_range_valid:
+            r = httputil.get_ranges(
+                request.headers.get('Range'),
+                content_length


trailing comma plz

Suggested change

content_length

content_length,

codecov · 2019-04-12T22:34:13Z

Codecov Report

Merging #1760 into master will increase coverage by 20.68%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master   #1760       +/-   ##
==========================================
+ Coverage   60.32%     81%   +20.68%     
==========================================
  Files         104     104               
  Lines       18156   13654     -4502     
==========================================
+ Hits        10953   11061      +108     
+ Misses       7203    2593     -4610

Issue cherrypy#1699: CherryPy should handle If-Range's properly

be26099

webknjaz reviewed Dec 5, 2018

View reviewed changes

webknjaz added 3 commits December 6, 2018 00:36

Reorder imports

d17e701

Make is_if_range_in_past var lowercase

8c4e3b0

Log more precise error messages

ff16ef6

Refactor conditionals

de1c62f

Drop extra empty line

e35a43c

webknjaz reviewed Dec 6, 2018

View reviewed changes

cherrypy deleted a comment Dec 6, 2018

webknjaz reviewed Dec 6, 2018

View reviewed changes

cherrypy/lib/static.py Outdated Show resolved Hide resolved

webknjaz reviewed Dec 6, 2018

View reviewed changes

ian-otto added 2 commits January 4, 2019 14:09

migrate if-range check to httputil, check if value is not an ETag.

9af77aa

if_range_check should return True if no header is present. Refactor f…

2da1314

…unction usage in static.py.

ian-otto changed the title ~~CherryPy should handle If-Range's properly~~ [WIP] CherryPy should handle If-Range's properly Jan 4, 2019

webknjaz reviewed Jan 4, 2019

View reviewed changes

cherrypy/lib/httputil.py Outdated Show resolved Hide resolved

webknjaz reviewed Jan 4, 2019

View reviewed changes

cherrypy/lib/httputil.py Outdated Show resolved Hide resolved

webknjaz reviewed Jan 4, 2019

View reviewed changes

cherrypy/lib/httputil.py Outdated Show resolved Hide resolved

webknjaz reviewed Jan 4, 2019

View reviewed changes

cherrypy/lib/static.py Outdated Show resolved Hide resolved

ian-otto added 3 commits April 8, 2019 18:48

change documentation for passes_if_range_check

103bdcb

Clean up code in _serve_fileobj to make if_range_check make more sense.

aa7f65e

Correct passes_if_range_check to not take request object, but rather take given if_range_header

Clean up unused imports from testing

302c2d9

ian-otto changed the title ~~[WIP] CherryPy should handle If-Range's properly~~ CherryPy should handle If-Range's properly Apr 9, 2019

webknjaz reviewed Apr 9, 2019

View reviewed changes

cherrypy/lib/httputil.py Outdated Show resolved Hide resolved

webknjaz reviewed Apr 9, 2019

View reviewed changes

cherrypy/lib/httputil.py Outdated Show resolved Hide resolved

webknjaz reviewed Apr 9, 2019

View reviewed changes

cherrypy/lib/httputil.py Outdated Show resolved Hide resolved

webknjaz reviewed Apr 9, 2019

View reviewed changes

cherrypy/lib/static.py Show resolved Hide resolved

webknjaz reviewed Apr 9, 2019

View reviewed changes

cherrypy/lib/static.py Show resolved Hide resolved

webknjaz reviewed Apr 9, 2019

View reviewed changes

cherrypy/lib/static.py Outdated Show resolved Hide resolved

webknjaz and others added 2 commits April 9, 2019 16:54

Update cherrypy/lib/httputil.py

c277004

Co-Authored-By: duper51 <ianotto24@gmail.com>

Update cherrypy/lib/static.py

64a52cc

Co-Authored-By: duper51 <ianotto24@gmail.com>

ian-otto added 3 commits April 10, 2019 15:01

Change function name from passes_if_range_check to matches_if_range_c…

3f2aee2

…heck Invert logic in matches_if_range_check to save on indentation

place result of matches_if_range_check in variable, rather than inline

912c8b4

Merge remote-tracking branch 'origin/fix-1699' into fix-1699

13e149b

# Conflicts: # cherrypy/lib/httputil.py

webknjaz reviewed Apr 10, 2019

View reviewed changes

cherrypy/lib/static.py Outdated Show resolved Hide resolved

webknjaz reviewed Apr 10, 2019

View reviewed changes

cherrypy/lib/static.py Outdated Show resolved Hide resolved

webknjaz reviewed Apr 10, 2019

View reviewed changes

cherrypy/lib/httputil.py Outdated Show resolved Hide resolved

webknjaz and others added 5 commits April 11, 2019 15:18

Add Period to docs

3f7a1ab

Co-Authored-By: duper51 <ianotto24@gmail.com>

Update syntax for format string

ae98c9b

Co-Authored-By: duper51 <ianotto24@gmail.com>

begin work on pep257

9fbfc45

Merge branch 'fix-1699' of https://github.com/duper51/cherrypy into f…

cebe7d9

…ix-1699

simplify content-length removal logic

8fa3da8

ian-otto added 3 commits April 11, 2019 16:06

fix up alert for imports

1a1b718

flake8 fixes

flake8 compliance

fad68d7

ensure old functionality is still present (logging parameter for non-requested byterange)

flake8 compliance

f410985

webknjaz reviewed Apr 12, 2019

View reviewed changes

jaraco changed the base branch from master to main March 23, 2021 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CherryPy should handle If-Range's properly #1760

CherryPy should handle If-Range's properly #1760

ian-otto commented Dec 2, 2018 •

edited by webknjaz

webknjaz Dec 5, 2018

webknjaz Dec 5, 2018

ian-otto commented Dec 5, 2018

webknjaz commented Dec 5, 2018

webknjaz Dec 6, 2018

webknjaz commented Dec 6, 2018

webknjaz Dec 6, 2018

ian-otto Jan 4, 2019

webknjaz Jan 4, 2019

webknjaz Jan 4, 2019

ian-otto Jan 4, 2019

webknjaz Jan 4, 2019

webknjaz commented Jan 4, 2019

webknjaz commented Apr 9, 2019

webknjaz commented Apr 10, 2019

webknjaz commented Apr 10, 2019

webknjaz commented Apr 11, 2019

webknjaz Apr 12, 2019

webknjaz Apr 12, 2019

webknjaz Apr 12, 2019

codecov bot commented Apr 12, 2019 •

edited

	Determines if an If-Range header is present and passes its conditions.
	Determine if an If-Range header conditions are fulfilled.

CherryPy should handle If-Range's properly #1760

Are you sure you want to change the base?

CherryPy should handle If-Range's properly #1760

Conversation

ian-otto commented Dec 2, 2018 • edited by webknjaz

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-otto commented Dec 5, 2018

webknjaz commented Dec 5, 2018

Choose a reason for hiding this comment

webknjaz commented Dec 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz commented Jan 4, 2019

webknjaz commented Apr 9, 2019

webknjaz commented Apr 10, 2019

webknjaz commented Apr 10, 2019

webknjaz commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 12, 2019 • edited

Codecov Report

ian-otto commented Dec 2, 2018 •

edited by webknjaz

codecov bot commented Apr 12, 2019 •

edited