feat($http): support sending XSRF token to whitelisted origins #14890

gkalpak · 2016-07-10T08:19:36Z

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

What is the current behavior? (You can also link to an open issue here)
The XSRF token header is not set on requests to cross-origin requests. This is usually desired, but there are cases where you want to whitelist specific origins.
See #7862 for context.

What is the new behavior (if this is a feature change)?
It is possible to whitelist specific origins, so requests to URLs on these origins will also have the XSRF token header set.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/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)

Other information:
Fixes #7862

gkalpak · 2016-07-10T08:20:13Z

/fyi @IgorMinar

gkalpak · 2016-07-10T08:23:30Z

test/ng/httpSpec.js

    customParamSerializer: customParamSerializer
  }));
-  beforeEach(module(function($exceptionHandlerProvider) {
-    $exceptionHandlerProvider.mode('log');
-  }));


Just refactoring beforeEach blocks merging unnecessarily separate blocks.

Narretz · 2018-03-16T07:29:02Z

LGTM but needs a rebase

gkalpak · 2018-03-16T11:15:18Z

Rebased. PTAL

Narretz

Only a few cosmetic changes requested, otherwise LGTM

Narretz · 2018-03-21T09:35:57Z

src/.eslintrc.json

@@ -161,6 +161,7 @@
    "urlResolve": false,
    "urlIsSameOrigin": false,
    "urlIsSameOriginAsBaseUrl": false,
+    "urlIsAllowedOriginChecker": false,


I would prefer a different name here, like isUrlAllowedOrigin. This fits better with our other methods that "check" something.

But this one is not like the other functions 😁
This one returns a function that checks (i.e. returns what one would call isUrlAllowedOrigin()).

So, I think naming it isUrlAllowedOrigin would be confusing. Open to other naming suggestions.

How about urlIsAllowedOriginFactory()?

Narretz · 2018-03-21T14:27:05Z

src/ng/http.js

+   * @name $httpProvider#xsrfWhitelistedOrigins
+   * @description
+   *
+   * Array containing URLs whose origins are considered trusted enough to receive the XSRF token.


Could be made simpler as "are trusted to receive the XSRF token"

Narretz · 2018-03-21T14:30:42Z

src/ng/http.js

-     * properties of either $httpProvider.defaults at config-time, $http.defaults at run-time,
-     * or the per-request config object.
+     * The header will &mdash; by default &mdash; **not** be set for cross-domain requests. This
+     * prevents unauthorized servers (e.g. malicious or compromized 3rd-party APIs) from gaining


compromize -> compromise

Narretz · 2018-03-22T09:36:09Z

Factory is good - I noticed that this wasn't the actual function during the review, but then forgot to change my comment 😅

petebacondarwin · 2018-03-26T10:19:50Z

src/ng/http.js

+   * @name $httpProvider#xsrfWhitelistedOrigins
+   * @description
+   *
+   * Array containing URLs whose origins are trusted to receive the XSRF token. See the


Array containing "origins" that are trusted?
Or if you explicitly accept URLs (but ignore the path etc) then change the name to xsrfWhitelistedUrls.

I do want to explicitly accept URLs (but ignore the path etc), but I think xsrfWhitelistedUrls will be misleading in that case. It would sound like you can whitelist specific URLs.

How about I change the description to "...containing origins that are trusted..." and keep the name as is (and also mention below that path etc will be ignored)?

Changing the description works for me.

petebacondarwin · 2018-03-26T10:20:38Z

src/ng/http.js

+   * angular.
+   *   module('xsrfWhitelistedOriginsExample', []).
+   *   config(['$httpProvider', function($httpProvider) {
+   *     $httpProvider.xsrfWhitelistedOrigins.push('https://api.example.com/');


Note that the / at the end of the URL is part of the path and not the origin.

petebacondarwin · 2018-03-26T10:34:56Z

src/ng/urlUtils.js

+   * @returns {boolean} - Whether the specified URL is of an allowed origin.
+   */
+  return function urlIsAllowedOrigin(requestUrl) {
+    var parsedUrl = isString(requestUrl) ? urlResolve(requestUrl) : requestUrl;


Not related to this PR but perhaps we should change urlResolve to be a noop if the url is already parsed (i.e. not a string)? This would save some of this boilerplate here and further down.

petebacondarwin · 2018-03-26T10:45:57Z

test/ng/httpSpec.js

+    var $httpBackend;
+
+    beforeEach(module(function($httpProvider) {
+      $httpProvider.xsrfWhitelistedOrigins.push('https://whitelisted.example.com/');


Note that the trailing slash is ignored. Should we have tests for URLs with different paths - to demonstrate that they are ignored?

Normally, the XSRF token will not be set for cross-origin requests. With this commit, it is possible to whitelist additional origins, so that requests to these origins will include the XSRF token header. Fixes angular#7862

gkalpak · 2018-03-26T14:32:28Z

@petebd, PTAL.

Closes #14890

Accidentally broken while backporting angular#14890.

Accidentally broken while backporting #14890. Since #14890, `$$sanitizeUri()` can no longer handle `null`/`undefined` values. In 1.7.x, there are no such calls. In 1.6.x, there is still one such calls inside `Attributes.$set()`, so it needs to be adjusted accordingly. Closes #16520

gkalpak added type: feature component: $http frequency: low needs: review severity: inconvenient labels Jul 10, 2016

gkalpak added this to the Backlog milestone Jul 10, 2016

googlebot added the cla: yes label Jul 10, 2016

gkalpak reviewed Jul 10, 2016
View reviewed changes

gkalpak force-pushed the feat-http-xsrf-origin-whitelist branch from 0455b09 to 00ef7e3 Compare August 8, 2016 15:10

gkalpak mentioned this pull request Sep 6, 2016

Setting xsrfHeader for cross-domain requests has no effect on requests (the header is missing) #5122

Closed

gkalpak mentioned this pull request Mar 2, 2018

Adding XSRF header when request is NOT same origin, but is TRUSTED origin (subdomain) #7862

Closed

gkalpak force-pushed the feat-http-xsrf-origin-whitelist branch 2 times, most recently from f5e4abc to 587610f Compare March 16, 2018 11:02

Narretz modified the milestones: Backlog, 1.6.x Mar 20, 2018

Narretz suggested changes Mar 21, 2018

View reviewed changes

gkalpak force-pushed the feat-http-xsrf-origin-whitelist branch from 587610f to bb17964 Compare March 21, 2018 19:10

Narretz approved these changes Mar 22, 2018

View reviewed changes

petebacondarwin approved these changes Mar 26, 2018

View reviewed changes

gkalpak added 4 commits March 26, 2018 15:27

feat($http): support sending XSRF token to whitelisted origins

eafc458

Normally, the XSRF token will not be set for cross-origin requests. With this commit, it is possible to whitelist additional origins, so that requests to these origins will include the XSRF token header. Fixes angular#7862

fixup! feat($http): support sending XSRF token to whitelisted origins

34e0440

fixup! feat($http): support sending XSRF token to whitelisted origins

679741e

refactor(urlResolve): return already parsed URLs unchanged

45bf5e9

gkalpak force-pushed the feat-http-xsrf-origin-whitelist branch from bb17964 to 45bf5e9 Compare March 26, 2018 14:31

petebacondarwin approved these changes Apr 4, 2018

View reviewed changes

gkalpak closed this in 059d4f6 Apr 4, 2018

gkalpak added a commit that referenced this pull request Apr 5, 2018

refactor(urlResolve): return already parsed URLs unchanged

da3d96c

Closes #14890

gkalpak deleted the feat-http-xsrf-origin-whitelist branch April 5, 2018 07:15

gkalpak added a commit to gkalpak/angular.js that referenced this pull request Apr 5, 2018

fix($compile): correctly handle null/undefined href attrs.$set()

ae0e07c

Accidentally broken while backporting angular#14890.

gkalpak mentioned this pull request Apr 5, 2018

fix($compile): correctly handle null/undefined href attrs.$set() #16520

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat($http): support sending XSRF token to whitelisted origins #14890

feat($http): support sending XSRF token to whitelisted origins #14890

gkalpak commented Jul 10, 2016 •

edited

Loading

gkalpak commented Jul 10, 2016

gkalpak Jul 10, 2016

Narretz commented Mar 16, 2018

gkalpak commented Mar 16, 2018

Narretz left a comment

Narretz Mar 21, 2018

gkalpak Mar 21, 2018

gkalpak Mar 21, 2018

Narretz Mar 21, 2018

Narretz Mar 21, 2018

Narretz commented Mar 22, 2018

petebacondarwin Mar 26, 2018

gkalpak Mar 26, 2018

petebacondarwin Apr 4, 2018

petebacondarwin Mar 26, 2018

petebacondarwin Mar 26, 2018

petebacondarwin Mar 26, 2018

gkalpak commented Mar 26, 2018

feat($http): support sending XSRF token to whitelisted origins #14890

feat($http): support sending XSRF token to whitelisted origins #14890

Conversation

gkalpak commented Jul 10, 2016 • edited Loading

gkalpak commented Jul 10, 2016

Choose a reason for hiding this comment

Narretz commented Mar 16, 2018

gkalpak commented Mar 16, 2018

Narretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Mar 26, 2018

gkalpak commented Jul 10, 2016 •

edited

Loading