WIP: Bypass URI as path only if absolute for OPTIONS #74

webknjaz · 2018-01-26T23:29:41Z

I'm not sure whether it's right from the RFC point of view, but this allows to put path w/o query-string into PATH_INFO in case of origin-form.

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix (possibly)
What is the related issue number (starting with #)
cherrypy/cherrypy#1662
What is the current behavior? (You can also link to an open issue here)
PATH_INFO in WSGI env is assigned URI as is if proxy is enabled.
What is the new behavior (if this is a feature change)?
PATH_INFO is assigned to path part if URI is not absolute.

This change is

Fix cherrypy/cherrypy#1662

webknjaz · 2018-01-26T23:34:24Z

@bertjwregeer @josephtate @jaraco Could you please share your opinion regarding RFC compliance?

codecov · 2018-01-26T23:35:48Z

Codecov Report

Merging #74 into master will decrease coverage by 0.51%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
- Coverage   76.87%   76.35%   -0.52%     
==========================================
  Files          24       24              
  Lines        3770     3769       -1     
==========================================
- Hits         2898     2878      -20     
- Misses        872      891      +19

digitalresistor · 2018-01-27T03:30:55Z

That change looks fine to me from a RFC perspective.

webknjaz · 2018-01-27T06:27:31Z

@bertjwregeer thanks.
What about proxy_mode? Maybe it's better to raise an error when there's no absolute path present?

webknjaz · 2018-01-27T11:59:55Z

Here's what I think:

If we expect it to act like proxy, there's not enough info if only origin-form is present in Request-Line. absolute-form is needed for proxy to know where to connect to. So in case of enabled proxy mode I'd raise an error if absolute-form validation fails.
It should be okay to set PATH_INFO to absolute-path in case of disabled proxy mode, but
- RFC encourages to accept absolute-form for all queries and instructs servers implementing Virtual Hosts to override Host header with a host from absolute-form. It looks like this should be handled on another level, but WSGI requires to have just absolute-path part there.
Based on that, I'd just turn off proxy mode in cherrypy adapter.

jaraco · 2018-02-04T20:02:32Z

I'm okay with this change. Will be good to stabilize the behavior reported in the ticket.

webknjaz · 2018-02-04T20:10:56Z

Actually, I'm not okay with it now:) But haven't had time to put a new patch together

danlamanna · 2018-04-06T14:25:32Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thlank you for your contributions.

I would be sad to see this issue/PR fall off the radar since it's caused us to pin CherryPy to an older version in our downstream project (https://github.com/girder/girder/).

Seeing as how the related issue has broken long lived expectations of CherryPy's handling of OPTIONS requests, is it possible to get this patched in the meantime?

webknjaz · 2018-04-09T23:58:07Z

@danlamanna I need to re-do this patch in order to have it merged. I just didn't have time for this. I wish there were more maintainers who could help.

digitalresistor · 2018-07-06T17:45:49Z

Stale bot... go away. This is still an issue.

webknjaz · 2018-07-16T11:20:47Z

Now bot won't close it with actual bug label.

webknjaz · 2018-09-06T11:10:20Z

@jaraco should we think of deferring HTTP parsing to https://github.com/python-hyper/h11?

fviard · 2019-01-17T17:13:06Z

Hi, today I encountered this issue also using cherrypy and just trying to use query params...
That would be nice if that could be fixed soon... And also if there was a "hack" that can be used with existing versions.
FYI, applying the fix proposed here solve my issue.
thanks

ian-otto · 2019-01-28T17:31:05Z

@fviard You're going to end up using a version of cheroot that is not RFC compliant, so you might have some user agents that misbehave and/or introduce yourself to various vulnerabilities.

jaraco · 2019-10-14T22:22:33Z

cheroot/server.py

        if self.method == b'OPTIONS':
            # TODO: cover this branch with tests
            path = (
                uri
                # https://tools.ietf.org/html/rfc7230#section-5.3.4
-                if self.proxy_mode or uri == ASTERISK
+                if (self.proxy_mode and uri_is_absolute_form)
+                or uri == ASTERISK


I'm pretty sure this line can be eliminated as urllib.parse.urlsplit('*').path == '*' anyway.

Removed in dc9592b.

@jaraco where did you see urllib.parse.urlsplit('*').path == '*'?

Because I don't see any code checking for ASTERISK now.

I think that this change is wrong. And the problem with this PR was this hack: https://github.com/cherrypy/cherrypy/blob/535605c/cherrypy/_cpwsgi_server.py#L31

It was temporary until this PR's figured out (cherrypy/cherrypy@c87bc9f). We should revert it to proxy_mode=False now and see if it's ok.

webknjaz · 2019-10-17T13:35:01Z

FTR I recall there have been some corner cases there in conjunction with CherryPy.

Bypass URI as path only if absolute for OPTIONS

97c8635

Fix cherrypy/cherrypy#1662

webknjaz requested review from jaraco, a team and josephtate January 26, 2018 23:29

jaraco approved these changes Feb 4, 2018

View reviewed changes

webknjaz changed the title ~~Bypass URI as path only if absolute for OPTIONS~~ WIP: Bypass URI as path only if absolute for OPTIONS Feb 4, 2018

This comment has been minimized.

Sign in to view

stale bot added the stale This thing has been ignored for too long label Apr 5, 2018

stale bot removed the stale This thing has been ignored for too long label Apr 6, 2018

This comment has been minimized.

Sign in to view

stale bot added the stale This thing has been ignored for too long label Jul 5, 2018

stale bot removed the stale This thing has been ignored for too long label Jul 6, 2018

webknjaz added the bug Something is broken label Jul 16, 2018

webknjaz mentioned this pull request May 15, 2019

Defer HTTP/1.1 parsing to Hyper (h11 project) #201

Open

3 tasks

jaraco added 2 commits October 14, 2019 18:12

Merge branch 'master' into bugfix/options-path-processing

3f82016

Update changelog

210f81e

jaraco reviewed Oct 14, 2019

View reviewed changes

Eliminate 'uri == ASTERISK' test as it has no impact on the behavior.

dc9592b

jaraco merged commit 02f7a17 into master Oct 14, 2019

jaraco deleted the bugfix/options-path-processing branch October 14, 2019 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Bypass URI as path only if absolute for OPTIONS #74

WIP: Bypass URI as path only if absolute for OPTIONS #74

webknjaz commented Jan 26, 2018 •

edited

Loading

webknjaz commented Jan 26, 2018

codecov bot commented Jan 26, 2018 •

edited

Loading

digitalresistor commented Jan 27, 2018

webknjaz commented Jan 27, 2018

webknjaz commented Jan 27, 2018

jaraco commented Feb 4, 2018

webknjaz commented Feb 4, 2018

This comment has been minimized.

danlamanna commented Apr 6, 2018 •

edited

Loading

webknjaz commented Apr 9, 2018

This comment has been minimized.

digitalresistor commented Jul 6, 2018

webknjaz commented Jul 16, 2018

webknjaz commented Sep 6, 2018

fviard commented Jan 17, 2019

ian-otto commented Jan 28, 2019

jaraco Oct 14, 2019

jaraco Oct 14, 2019

webknjaz Oct 17, 2019

webknjaz Oct 17, 2019

webknjaz Oct 17, 2019

webknjaz commented Oct 17, 2019

WIP: Bypass URI as path only if absolute for OPTIONS #74

WIP: Bypass URI as path only if absolute for OPTIONS #74

Conversation

webknjaz commented Jan 26, 2018 • edited Loading

webknjaz commented Jan 26, 2018

codecov bot commented Jan 26, 2018 • edited Loading

Codecov Report

digitalresistor commented Jan 27, 2018

webknjaz commented Jan 27, 2018

webknjaz commented Jan 27, 2018

jaraco commented Feb 4, 2018

webknjaz commented Feb 4, 2018

This comment has been minimized.

danlamanna commented Apr 6, 2018 • edited Loading

webknjaz commented Apr 9, 2018

This comment has been minimized.

digitalresistor commented Jul 6, 2018

webknjaz commented Jul 16, 2018

webknjaz commented Sep 6, 2018

fviard commented Jan 17, 2019

ian-otto commented Jan 28, 2019

jaraco Oct 14, 2019

Choose a reason for hiding this comment

jaraco Oct 14, 2019

Choose a reason for hiding this comment

webknjaz Oct 17, 2019

Choose a reason for hiding this comment

webknjaz Oct 17, 2019

Choose a reason for hiding this comment

webknjaz Oct 17, 2019

Choose a reason for hiding this comment

webknjaz commented Oct 17, 2019

webknjaz commented Jan 26, 2018 •

edited

Loading

codecov bot commented Jan 26, 2018 •

edited

Loading

danlamanna commented Apr 6, 2018 •

edited

Loading