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

Add xsrf token header if cookie is present (#9471) #10953

Closed
wants to merge 1 commit into from

Conversation

tepi
Copy link
Contributor

@tepi tepi commented May 30, 2018

Adds custom X-XSRF-TOKEN header to requests, if XSRF-TOKEN cookie is set

Fixes #9471

Check when you have completed
[ ] Valid tests for the pull request
[x] Contributing guidelines implemented


This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented May 30, 2018

CLA assistant check
All committers have signed the CLA.

@tepi tepi force-pushed the issue_9471 branch 2 times, most recently from 59a3981 to 778e751 Compare May 31, 2018 10:02
@elmot
Copy link
Contributor

elmot commented May 31, 2018

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved, some commit checks failed.


a discussion (no related file):
I have a feeling this change is actually an application security degradation. Having XSRF value in cookies does not look safe to me. What do you think, @Legioth ?


server/src/main/resources/VAADIN/vaadinBootstrap.js, line 39 at r2 (raw file):

        }
    };
    

extra spaces
please run mvn process-source or mvn install before commiting


server/src/main/resources/VAADIN/vaadinBootstrap.js, line 203 at r2 (raw file):

                // send parameters as POST data
                r.setRequestHeader("Content-type", "application/x-www-form-urlencoded");
                

extra spaces


Comments from Reviewable

@tepi
Copy link
Contributor Author

tepi commented May 31, 2018

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


a discussion (no related file):

Previously, elmot (Ilia Motornyi) wrote…

I have a feeling this change is actually an application security degradation. Having XSRF value in cookies does not look safe to me. What do you think, @Legioth ?

This change does nothing to application security as-is. It just checks for the value in the cookie and adds it to the custom header. This enables use cases where the server is adding this cookie and expects the header to be present in all requests.


Comments from Reviewable

@tepi
Copy link
Contributor Author

tepi commented May 31, 2018

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


server/src/main/resources/VAADIN/vaadinBootstrap.js, line 39 at r2 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

extra spaces
please run mvn process-source or mvn install before commiting

I run both, it did no changes to this file (or the other one you commented on). What exactly is wrong?


Comments from Reviewable

@tepi
Copy link
Contributor Author

tepi commented May 31, 2018

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


server/src/main/resources/VAADIN/vaadinBootstrap.js, line 39 at r2 (raw file):

Previously, tepi (Teppo Kurki) wrote…

I run both, it did no changes to this file (or the other one you commented on). What exactly is wrong?

Never mind, found the issue and fixed.


server/src/main/resources/VAADIN/vaadinBootstrap.js, line 203 at r2 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

extra spaces

Never mind, found the issue and fixed.


Comments from Reviewable

@elmot
Copy link
Contributor

elmot commented Jul 10, 2018

Closed in favor of #11034

@elmot elmot closed this Jul 10, 2018
@tsuoanttila tsuoanttila added this to the Invalid milestone Jul 10, 2018
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

Successfully merging this pull request may close these issues.

None yet

4 participants