Repository navigation
feat(security): Automatic XSRF handling. - #8898
Conversation
mprobst
added
action: review
There was a problem hiding this comment.
Somewhat out of scope, but it's probably the right place to put a warning saying that Angular won't do that for you, and your backend has to check that in every relevant api point ?
|
LGTM on the code, +@koto and @molnarg anyways. However, this might need more doc, or links to https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF) for instance ? The problem is straightforward, but it's not obvious how Angular having this automatic header addition from cookie values helps, and what should a developer do with this to prevent issues. |
There was a problem hiding this comment.
Usually the GET requests don't need the XSRF token, but if Angular 1 did it for GET as well (I don't know that), it's better to have backwards compatibility here.
There was a problem hiding this comment.
I think it did, a quick review of the doc and implementation doesn't seem to discriminate on that (doc at https://github.com/angular/angular.js/blob/0727bfc141db6e60bce2fb1e09ad4f53963dec5d/src/ng/http.js#L770)
There was a problem hiding this comment.
Yeah, not needed, but I think it's better to make it a decision of the server side when to enforce XSRF, and just unconditionally send the headers by default here.
|
LGTM (modulo the notes above). |
|
LGTM |
|
@rjamet I added a link in a somewhat random location. We're working on the security documentation, I plan to link to a central "here's all about security in Angular 2" doc once we have that. |
mprobst
added
pr_state: LGTM
action: merge
There was a problem hiding this comment.
Nope. Our injector does not support arguments with default values, it'll complain about a binding for String missing.
|
LGTM |
Automatically recognize XSRF protection cookies, and set a corresponding XSRF header. Allows applications to configure the cookie names, or if needed, completely override the XSRF request configuration by binding their own XSRFHandler implementation. Part of angular#8511.
| constructor( | ||
| private _cookieName: string = 'XSRF-TOKEN', private _headerName: string = 'X-XSRF-TOKEN') {} | ||
|
|
||
| configureRequest(req: Request) { |
There was a problem hiding this comment.
Seems related to this issue - #9294
9 participants
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Does this PR introduce a breaking change?
Automatically recognize XSRF protection cookies, and set a corresponding XSRF
header. Allows applications to configure the cookie names, or if needed,
completely override the XSRF request configuration by binding their own
XSRFHandler implementation.
Part of #8511.