Never set session cookies for API requests #3941
Conversation
|
|
| @@ -0,0 +1,17 @@ | |||
| from flask import sessions, request | |||
|
|
|||
| class SessionThatIgnoresAPI(sessions.SecureCookieSessionInterface): | |||
There was a problem hiding this comment.
Happy to take suggestions for a better name here :)
There was a problem hiding this comment.
I would call this simply JournalistInterfaceSession since we may want to add other features in the future. I would also prefer this just be in the module journalist_app.utils since I don't think it deserves its own module with a complex name.
rjmackay
force-pushed
the
no-session-for-api-3876
branch
from
639ce21
to
bb835d0
Compare
There was a problem hiding this comment.
$ http HEAD localhost:8081/api/v1
HTTP/1.0 301 MOVED PERMANENTLY
Content-Length: 265
Content-Type: text/html; charset=utf-8
Date: Mon, 26 Nov 2018 03:08:30 GMT
Location: http://localhost:8081/api/v1/
Server: Werkzeug/0.14.1 Python/2.7.6
Vary: Cookie
The Vary: Cookie line still exists.
|
@kushaldas thanks. I missed that. I'll try to take a look over the weekend if I have time. |
There was a problem hiding this comment.
This should carry an accompanying test to verify that both Set-Cookie is not present and Vary does not contain Cookie when hitting /api/v1/ as a guard against regressions.
Actually, it should go one step further and include Vary: Authorization since that is technically correct and if we're going to fiddle with the Vary header, we might as well be pedantic about it :D
rjmackay
force-pushed
the
no-session-for-api-3876
branch
from
bb835d0
to
ae83fb4
Compare
|
I've updated to add tests, and remove the Vary header for cookies.
I had a bit of a look into this. I'd rather not deal with it in the PR since I'm not really messing with the Vary header, but rather just preventing the session being saved at all and the letting the default session logic handle the rest. Also while looking at how to set the Vary head appropriately I found a couple of RFC refeneces that suggest Authorization shouldn't be included in the Vary response anyway: rfc7234 section 3 says in part:
rfc7321 section 7.1.4 says:
|
rjmackay
force-pushed
the
no-session-for-api-3876
branch
from
ae83fb4
to
b737e3a
Compare
|
@heartsucker @kushaldas This is be ready for another review now. Thanks |
There was a problem hiding this comment.
Thanks for adding these tests. After reading the relevant RFCs, I realized you were correct. This is good to merge once @kushaldas approves this or dismisses his request for changes.
kushaldas
dismissed
their stale review
Going with @heartsucker's approval, did not test the change again.
|
Heya @rjmackay, it looks like this needs a quick rebase and then we can merge this in! |
Implement a custom session interface that doesn't save sessions for API requests Fixes freedomofpress#3876
rjmackay
force-pushed
the
no-session-for-api-3876
branch
from
b737e3a
to
f66259d
Compare
|
@heartsucker rebased. This should be ready to merge as soon as the build passes. |
|
Looks like the lint build timed out. I can't restart the build but It passes locally. |
|
Thanks for the pr :D |
Status
Ready for review
Description of Changes
Fixes #3876.
Changes proposed in this pull request:
Implement a custom session interface that never sets session cookies
on API requests
Testing
How should the reviewer test this PR?
curl -v localhost:8081/admin/
curl -v localhost:8081/api/v1/
Deployment
Any special considerations for deployment? Consider both:
Checklist
If you made changes to the server application code:
make ci-lint) and tests (make -C securedrop test) pass in the development containerIf you made changes to
securedrop-admin:make -C admin test) pass in the admin development containerIf you made changes to the system configuration:
If you made non-trivial code changes:
If you made changes to documentation:
make docs-lint) passed locally