Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pyramid 2.0 could restrict the request methods by default in add_view, view_config. #2796

Closed
ztane opened this issue Oct 13, 2016 · 10 comments
Closed

Comments

@ztane
Copy link
Contributor

ztane commented Oct 13, 2016

The current behaviour of Pyramid is that by default all views handle all HTTP verbs, unless explicitly stated otherwise. This means that for example CORS preflight OPTIONS requests will trigger lots of inadvertent code etc. As as far as I know no one really wants to respond to OPTIONS with whatever body the view returns, one thing to consider would be that in Pyramid 2.0 the request_method would default to ('GET', 'POST') (and including HEAD naturally) instead; after all, this is mostly what people think the behaviour would be, unless explicitly excepted otherwise.

@ztane
Copy link
Contributor Author

ztane commented Oct 13, 2016

#1484 is tangentially related.

@mmerickel
Copy link
Member

mmerickel commented Oct 13, 2016

So just as an FYI I've implemented CORS using a global approach here: https://gist.github.com/mmerickel/1afaf64154b335b596e4

Another option is something similar to what we did with CSRF checks where we add something like config.set_default_cors_options(..) and then on a per-view basis you can opt-out of cors checking if required or set different cors constraints. This is trivial to implement using the new machinery in a view deriver which can intercept any OPTIONS request and execute per-view logic.

I don't think this is compelling enough to switch the defaults, but that's my 2 cents right now.

@digitalresistor
Copy link
Member

If you set request_method to GET it will automatically add HEAD: https://github.com/Pylons/pyramid/blob/master/pyramid/config/predicates.py#L33

I on all of my views set request_method manually to what I want them to be. I would recommend this as a best practice anyway.

With your proposed changes, then users can't have a single view that handles verbs and then have a large if statement that switches on the request method.

@ztane
Copy link
Contributor Author

ztane commented Oct 13, 2016

It was just an option; my point is that even though explicit is better than implicit, people write software so that "no request method" means request_method=('GET', 'POST') and make the request_method=ANYTHING_IS_POSSIBLE one explicit. After all, the recommendation is that 501 should be given to request methods that are not supported at all and 405 for those that are not supported for the resource:

When a request method is received
that is unrecognized or not implemented by an origin server, the
origin server SHOULD respond with the 501 (Not Implemented) status
code. When a request method is received that is known by an origin
server but not allowed for the target resource, the origin server
SHOULD respond with the 405 (Method Not Allowed) status code.

so in the end the "any request method" views are almost always not very correct.

@mmerickel
Copy link
Member

The biggest issue here is that the implementation is not near as easy as you probably think it is right now. A naive approach will conflict with #1863 in which a default view with request_method=['GET', 'POST'] will not have an appropriate ordering with another view that has request_method='POST'. The lookup will be ambiguous.

@digitalresistor
Copy link
Member

The 405 and 501 and other errors discussion has happened before. It's something I'd like to see improved, but I am not sure there is a good way to complete the task with the way that Pyramid is currently written and the way the views are added (and the way view lookup can work with traversal).

I did a longer write up in one of those issues before...

#1526
and
#1873

@mmerickel
Copy link
Member

I haven't yet seen a compelling reason for such a drastic change to add_view defaults. This would be a major incompatibility. From my perspective this can be solved using the CORS code I pasted above or on a per-view level using view derivers added in 1.7 if the global approach is not sufficient.

@ztane
Copy link
Contributor Author

ztane commented Jul 18, 2017

The thing is now it is pretty much impossible to do the sane things. The implicit behaviour is dangerous and people do not realize it works like that. It would be better if the view_config defaults to the sane set of request methods that is used to request an object; for any others you would need to do something like request_method='*'. The ordering should naturally be "explicit method" matches first, then default methods, and lastly the wildcard request method. That would also aid in adding a general 405 view for unsupported views.

@digitalresistor
Copy link
Member

405 for unsupported views is almost impossible in Pyramid to implement, especially if you are using something like traversal, due to the way that view matching is allowed to walk up the traversal tree to find a potential match. We can't say "sure, if you changed your request method for the request it may succeed", it's just the nature of the beast.

I've spent an awful lot of time trying to reason through and think through how we could add support for 405's, but it is simply not something that Pyramid can reliably do.

Things like Cornice and others add it as a later step and handle the 405 themselves, which is possible because they can know whether the request could succeed if the request method was changed.

@ztane
Copy link
Contributor Author

ztane commented Jul 21, 2017

I don't mean that Pyramid should necessarily do 405, as it would indeed be quite difficult to work out what would be allowed, as RFC 2616 says that the response "MUST include an Allow header containing a list of valid methods for the requested resource."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants