docs(security): improve xsrf description and add it to http chapter as well #2652
Conversation
|
Perhaps it would be worth mentioning why the malicious code cant simply read the cookie and do the same thing. |
| cookies, and the attacker could—for example—cause a bank transfer in the user's name with | ||
| the right request. | ||
| In a cross-site request forgery (CSRF or XSRF), an attacker tricks the user into visiting | ||
| a different web page with malicious code that secretly sends a request to your application's web server. |
There was a problem hiding this comment.
this might be clearer if you give the websites names, e.g. evil.com and example.com.
| the right request. | ||
| In a cross-site request forgery (CSRF or XSRF), an attacker tricks the user into visiting | ||
| a different web page with malicious code that secretly sends a request to your application's web server. | ||
| If the user is logged into your application, the browser sends the request along with authentication cookies. |
There was a problem hiding this comment.
it sends authentication cookies along with the request, right?
There was a problem hiding this comment.
That's what I think I'm saying here. Did I leave that in doubt? That is, after all, how this attack works. I'll try to clarify.
| If the user is logged into your application, the browser sends the request along with authentication cookies. | ||
| An _unprotected_ server can't tell the difference between a legitimate request and a forged request. | ||
|
|
||
| In this manner, for example, a knowledgable attacker could transfer money from the user's account to the attacker's account. |
There was a problem hiding this comment.
... , if the bank's website does not protect itself against XSRF.?
| CSRF tokens should be unique per user and session, have a large random value generated by a | ||
| cryptographically secure random number generator, and expire. | ||
| XSRF/CSRF tokens should be unique per user and session, have a large random value generated by a | ||
| cryptographically secure random number generator, and should expire after a reasonably short period of time. |
| `XSRF-TOKEN` cookie and validate the response header for each state-modifying request. | ||
| Angular's `http` has built-in support for the client-side half of this technique in its `XSRFStrategy`. | ||
| The default `CookieXSRFStrategy` is turned on automatically. | ||
| During each HTTP request, the `CookieXSRFStrategy` looks for a cookie called `XSRF-TOKEN` and |
There was a problem hiding this comment.
The during is a bit misleading, this all happens before the request is sent. Maybe:
When sending an HTTP request, `CookieXSRFStrategy` looks for a cookie called `XSRF-TOKEN` and sets a header named `X-XSRF-TOKEN` with the value of that cookie.
| [Cross-Site Request Forgery (CSRF)](https://www.owasp.org/index.php/Cross-Site_Request_Forgery_%28CSRF%29) and | ||
| [Cross-Site Request Forgery (CSRF) Prevention Cheat Sheet](https://www.owasp.org/index.php/CSRF_Prevention_Cheat_Sheet). The Stanford University | ||
| paper [Robust Defenses for Cross-Site Request Forgery](https://seclab.stanford.edu/websec/csrf/csrf.pdf) is also a rich source of detail. | ||
| <a href="https://www.owasp.org/index.php/Cross-Site_Request_Forgery_%28CSRF%29" target="_blank">Cross-Site Request Forgery (CSRF)</a> and |
There was a problem hiding this comment.
any particular reason why you're changing the link syntax here?
There was a problem hiding this comment.
Yes. We want links to external sites to appear in a new browser tab by default. Can't specify that in markdown
| <a href="https://www.owasp.org/index.php/Cross-Site_Request_Forgery_%28CSRF%29" target="_blank">Cross-Site Request Forgery (CSRF)</a> and | ||
| <a href="https://www.owasp.org/index.php/CSRF_Prevention_Cheat_Sheet" target="_blank">Cross-Site Request Forgery (CSRF) Prevention Cheat Sheet</a>. | ||
| The Stanford University paper | ||
| <a href="" target="_blank">Robust Defenses for Cross-Site Request Forgery</a> is a rich source of detail. |
| The Stanford University paper | ||
| <a href="" target="_blank">Robust Defenses for Cross-Site Request Forgery</a> is a rich source of detail. | ||
|
|
||
| See also Dave Smith's masterful, easy-to-understand |
There was a problem hiding this comment.
I'm happy that you like this, but maybe a more neutral tone of voice would be appropriate in our docs?
| :marked | ||
| ## Guarding against Cross-Site Request Forgery | ||
|
|
||
| In a cross-site request forgery (CSRF or XSRF), an attacker tricks the user into visiting |
There was a problem hiding this comment.
why do we repeat the same content here? should this link to the original doc, so we don't have two sources of truth that can diverge?
There was a problem hiding this comment.
Thought about that. We need some context here. I only repeat the intro and this is material that shouldn't change even if our implementation does. I didn't think making up a new summary description would make a difference. I'll crisp it up in the Http chapter.
|
Re whether this works with AoT, it should. The static reflector understands class instantiations and string literals, AFAIK. |
|
@johnpapa I didn't want to get into this depth in our Angular docs. But I took a crack at it in an update on the security page |
|
@mprobst I wasn't sure about AoT's ability to instantiate with string literals. Not sure what the rules are. Good to know. Thanks. And thanks for your other comments which I will incorporate. |
|
@wardbell please note that most folks don't work on weekends, so might be nice to leave a workday at least for comments. I think we also do mandatory code reviews in most Angular repos now, whether that's useful here for docs is probably your call. |
Improves the English description of XSRF in the Security chapter and adds a shorter version (with references) to the Http chapter where developers would expect to find it..
@mprobst Please review and confirm that the
XSRFStrategyoverride, described in the original text, actually works in AoT:I fear that it might not because it involves executing code (newing the class). It may be necessary to do this:
Please confirm which approach is best.