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

feat(common): on-by-default XSRF support in HttpClient #18108

Merged
merged 1 commit into from
Jul 14, 2017

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Jul 13, 2017

No description provided.

@mary-poppins
Copy link

You can preview 3f24287 at https://pr18108-3f24287.ngbuilds.io/.

* found in the LICENSE file at https://angular.io/license
*/

import {DOCUMENT, ɵparseCookieValue as parseCookieValue} from '@angular/common';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might as well move ɵparseCookieValue into common/http, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would introduce a dependency from @angular/platform-browser on @angular/common/http, which I'd rather avoid.

@@ -1127,6 +1127,20 @@ export declare class HttpXhrBackend implements HttpBackend {
}

/** @experimental */
export declare class HttpXsrfModule {
static disabled(): ModuleWithProviders;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about "disable()"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok with me.

@mary-poppins
Copy link

You can preview 65a508c at https://pr18108-65a508c.ngbuilds.io/.

@IgorMinar IgorMinar merged commit dd04f09 into angular:master Jul 14, 2017
Copy link

@zygimantas zygimantas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question about absolute URLs

// Skip both non-mutating requests and absolute URLs.
// Non-mutating requests don't require a token, and absolute URLs require special handling
// anyway as the cookie set
// on our origin is not the same as the token expected by another origin.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if the URL is absolute and the origin is the same? Still testing, but I am experiencing a breaking change here with absolute API URL.

Copy link

@zygimantas zygimantas Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed. XSRF token header is not added if the URL is absolute. While it is not a breaking change exactly, because the HttpClient is a new class, I see no reasons why absolute URLs are wrong. A case where absolute URL is a good thing: I store the base URL in environment.ts and use it in HttpClient, as well as in WebSocket where I replace "https://" with "wss://" (WebSocket requires absolute URL). Having a relative URL will force me to think about how to make URL relative before each HTTP request.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran into this issue. My API is actually located at a totally different URL (and server) from where my app is served which requires me to use absolute URL. It sounds like some sort of white-list is necessary for this option, or another flag on HttpClient.Post() options.

Copy link

@MrBlaise MrBlaise Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My API uses collection-json which relies heavily on links (absolute URLs), my app constantly uses those links to POST, DELETE etc. This breaks my app and I can't migrate from Http unless I manually convert every link to relative before the request.

@IgorMinar @alxhub Is it necessary to disallow xsrf when using absolute url?

asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 26, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants