Skip to content

Subclass HTTPBadCSRFToken from HTTPBadRequest and have request.session.c... #1149

Closed
wants to merge 1 commit into from

3 participants

@kpinc
kpinc commented Oct 8, 2013

...heck_csrf_token use the new exception.

This supports a more fine-grained exception trapping.

There's no possibility of a real namspace collision here, but should the name be something other than HTTPBadCSRFToken to avoid a possible brainspace collision with future http response codes?

Should check_csrf_token() really report itself when raising the exception or am I too used to shell? What about case in the error text?

Should the docs for HTTPBadCSRFToken and HTTPBadResponse state the http error code?

Note that I was unable to get the big exception list in the docs to format properly without the additional blank lines around HTTPBadCSRFToken. There might be a better way.

Note that while I plan to need this in the future I have no immediate need for this feature. This was done more as an exercise in design and hacking the pyramid code than out of present need.

@kpinc kpinc Subclass HTTPBadCSRFToken from HTTPBadRequest and have request.sessio…
…n.check_csrf_token use the new exception.

This supports a more fine-grained exception trapping.
0905d20
@mmerickel
Pylons Project member

This is not an http exception. I'd expect to see a CSRFError or BadCSRFToken exception in pyramid.exceptions that subclasses HTTPBadRequest.

@kpinc
kpinc commented Oct 8, 2013
@mmerickel
Pylons Project member

It's a framework-level exception, whereas pyramid.httpexceptions has been so far reserved for exception/responses that are directly mapping the http specs, so I don't think I'd expect to see a reference in there.

@mcdonc
Pylons Project member
mcdonc commented Oct 9, 2013

I think the motivation for this is good. Like @mmerickel said, for bw compat purposes, it should subclass HTTPBadRequest, it should live in exceptions.py, and should just be called BadCSRFToken (without the HTTP-prefix).

@mmerickel
Pylons Project member

Closing this pull request, changes merged into #1169.

@mmerickel mmerickel closed this Oct 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.