-
Notifications
You must be signed in to change notification settings - Fork 5
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
Turn on CSRF protection. #90
Conversation
…e CSRF tokens. Update tests for CSRF tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, Mark! The spring security configuration looks like it pretty much matches what is in the docs for SPA CSRF which is good. I only had a few minor comments.
The question re: what to do about what to do about requests coming from the backend user (stateless basic auth)
and CSRF, I will do some more reading on this, but we can also talk about this if you want to try and come to a reasonable decision.
pass-core-main/src/test/java/org/eclipse/pass/main/security/AccessControlTest.java
Show resolved
Hide resolved
pass-core-main/src/main/java/org/eclipse/pass/main/security/SecurityConfiguration.java
Show resolved
Hide resolved
pass-core-main/src/main/java/org/eclipse/pass/main/security/CsrfCookieFilter.java
Outdated
Show resolved
Hide resolved
pass-core-main/src/main/java/org/eclipse/pass/main/security/SecurityConfiguration.java
Show resolved
Hide resolved
pass-core-main/src/main/java/org/eclipse/pass/main/security/SpaCsrfTokenRequestHandler.java
Outdated
Show resolved
Hide resolved
pass-core-main/src/main/java/org/eclipse/pass/main/security/SpaCsrfTokenRequestHandler.java
Outdated
Show resolved
Hide resolved
pass-core-main/src/test/java/org/eclipse/pass/main/security/AccessControlTest.java
Show resolved
Hide resolved
pass-core-main/src/test/java/org/eclipse/pass/main/security/AccessControlTest.java
Show resolved
Hide resolved
I'm ready to approve, but I know there are still some pending issues on the UI/tests side. I will approve once those issue have been fixed jic there are changes need in pass-core. |
Turns on CSRF protection using cookies and headers. Responses from the API will set a cookie XSRF-TOKEN and non-GET requests from the client must set the header X-XSRF-TOKEN with the value of the cookie. GET requests to the /doi service are also protected because they have side effects.
In addition the integration tests have been refactored to handle the CSRF token and do a little more testing.
By default CSRF protection requires a POST to to /logout. This is disabled for now because the a POST to that endpoint with the header set returns a login form with a CSRF token hidden in the form. Submitting that form does do the logout. The format of the token in the in the form is different from the cookie. In addition doing a POST to /logout will initiate the SAML logout process. The complications don't seem to make protecting /logout worthwhile at the moment.
Clients can user any CSRF token they want. The cookie and header just must be consistent.
Required prs:
Acceptance tests pass, but require the updated pass-core and pass-support images running in the updated pass-docker.