feat(security): Automatic XSRF handling. #8898

mprobst · May 28, 2016

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Feature

Does this PR introduce a breaking change?

Yes
No

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.

mprobst · May 28, 2016

@rjamet PTAL from a security perspective.
@mhevery PTAL for the new API addition (see public_api_spec.ts).

rjamet · May 30, 2016

rjamet reviewed May 30, 2016

View changes

rjamet · May 30, 2016

modules/@angular/http/http.ts

+ * `XSRFHandler` allows customizing how the application protects itself against Cross Site Request
+ * Forgery (XSRF) attacks. By default, Angular will look for a cookie called `'XSRF-TOKEN'`, and set
+ * an HTTP request header called `'X-XSRF-TOKEN'` with the value of the cookie on each request,
+ * allowing the server side to validate that the request comes from its own front end.


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 ?

rjamet · May 30, 2016

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.

koto · May 30, 2016

koto reviewed May 30, 2016

View changes

koto · May 30, 2016

modules/@angular/http/src/backends/xhr_backend.ts

+  configureRequest(req: Request) {
+    let xsrfToken = getDOM().getCookie(this.cookieName);
+    if (xsrfToken && !req.headers.has(this.headerName)) req.headers.set(this.headerName, xsrfToken);
+  }


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.

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)

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.

koto · May 30, 2016

LGTM (modulo the notes above).

molnarg · May 31, 2016

LGTM

mprobst · May 31, 2016

@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.

vicb · May 31, 2016

vicb reviewed May 31, 2016

View changes

vicb · May 31, 2016

modules/@angular/http/http.ts

@@ -164,7 +191,8 @@ export const HTTP_PROVIDERS: any[] = [
  BrowserXhr,
  provide(RequestOptions, {useClass: BaseRequestOptions}),
  provide(ResponseOptions, {useClass: BaseResponseOptions}),
-  XHRBackend
+  XHRBackend,
+  provide(XSRFHandler, {useValue: new CookieXSRFHandler()}),


useClass ?

Nope. Our injector does not support arguments with default values, it'll complain about a binding for String missing.

vicb · May 31, 2016

vicb reviewed May 31, 2016

View changes

vicb · May 31, 2016

modules/@angular/http/src/backends/xhr_backend.ts

-  constructor(private _browserXHR: BrowserXhr, private _baseResponseOptions: ResponseOptions) {}
+  constructor(
+      private _browserXHR: BrowserXhr, private _baseResponseOptions: ResponseOptions,
+      private xsrfHandler: XSRFHandler) {}


_xsrfHandler

mhevery · May 31, 2016

mhevery reviewed May 31, 2016

View changes

mhevery · May 31, 2016

modules/@angular/http/src/interfaces.ts

@@ -20,6 +20,11 @@ export abstract class Connection {
  response: any;  // TODO: generic of <Response>;
 }

+/** An XSRFHandler configures XSRF protection (e.g. via headers) on an HTTP request. */
+export abstract class XSRFHandler {


How about Strategy instead of Handler?

vicb · May 31, 2016

vicb reviewed May 31, 2016

View changes

vicb · May 31, 2016

modules/@angular/http/src/backends/xhr_backend.ts

+ */
+export class CookieXSRFHandler implements XSRFHandler {
+  constructor(
+      private cookieName: string = 'XSRF-TOKEN', private headerName: string = 'X-XSRF-TOKEN') {}


private -> _ prefix

mhevery · May 31, 2016

mhevery reviewed May 31, 2016

View changes

mhevery · May 31, 2016

modules/@angular/http/src/backends/xhr_backend.ts

+ * with different `cookieName` and `headerName` values. See the main HTTP documentation for more
+ * details.
+ */
+export class CookieXSRFHandler implements XSRFHandler {


Handler -> Strategy

mhevery · May 31, 2016

LGTM

tlaverdure · Jun 17, 2016

tlaverdure reviewed Jun 17, 2016

View changes

tlaverdure · Jun 17, 2016

modules/@angular/http/src/backends/xhr_backend.ts

+      private _cookieName: string = 'XSRF-TOKEN', private _headerName: string = 'X-XSRF-TOKEN') {}
+
+  configureRequest(req: Request) {
+    let xsrfToken = getDOM().getCookie(this._cookieName);


Seems related to this issue - #9294

googlebot added the cla: yes label May 28, 2016

mprobst assigned robwormald May 28, 2016

mprobst referenced this pull request May 28, 2016
Closed
Angular 2 Security Tracking Issue #8511
17 of 17 tasks complete

mprobst added PR action: review action: review comp: security type: feature labels May 28, 2016

mprobst added pr_state: LGTM PR action: merge and removed action: review PR action: review labels May 31, 2016

laskoviymishka referenced this pull request Jun 13, 2016
Closed
Create Universal cookies module #273

angular/angular

Join GitHub today

feat(security): Automatic XSRF handling. #8898

Conversation

mprobst commented May 28, 2016

googlebot added the cla: yes label May 28, 2016

mprobst assigned robwormald May 28, 2016

This comment has been minimized.

mprobst commented May 28, 2016

mprobst referenced this pull request May 28, 2016

mprobst added PR action: review action: review comp: security type: feature labels May 28, 2016

rjamet reviewed May 30, 2016 View changes

This comment has been minimized.

rjamet May 30, 2016 • edited

This comment has been minimized.

rjamet commented May 30, 2016

koto reviewed May 30, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

koto commented May 30, 2016

This comment has been minimized.

molnarg commented May 31, 2016

This comment has been minimized.

mprobst commented May 31, 2016

mprobst added pr_state: LGTM PR action: merge and removed action: review PR action: review labels May 31, 2016

vicb reviewed May 31, 2016 View changes

This comment has been minimized.

This comment has been minimized.

vicb reviewed May 31, 2016 View changes

This comment has been minimized.

This comment has been minimized.

mhevery reviewed May 31, 2016 View changes

This comment has been minimized.

This comment has been minimized.

vicb reviewed May 31, 2016 View changes

This comment has been minimized.

This comment has been minimized.

mhevery reviewed May 31, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mhevery commented May 31, 2016

Hide details View details mprobst merged commit 4d793c4 into angular:master May 31, 2016 1 of 4 checks passed

1 of 4 checks passed

laskoviymishka referenced this pull request Jun 13, 2016

tlaverdure reviewed Jun 17, 2016 View changes

This comment has been minimized.

tlaverdure Jun 17, 2016 • edited

rjamet reviewed May 30, 2016

View changes

rjamet May 30, 2016 •

edited

koto reviewed May 30, 2016

View changes

vicb reviewed May 31, 2016

View changes

vicb reviewed May 31, 2016

View changes

mhevery reviewed May 31, 2016

View changes

vicb reviewed May 31, 2016

View changes

mhevery reviewed May 31, 2016

View changes

mprobst merged commit `4d793c4` into angular:master May 31, 2016
1 of 4 checks passed

tlaverdure reviewed Jun 17, 2016

View changes

tlaverdure Jun 17, 2016 •

edited