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

HttpClientXsrfModule breaks compatibility with HttpModule by not handling GET requests #19885

Open
Meligy opened this Issue Oct 24, 2017 · 14 comments

Comments

10 participants
@Meligy
Contributor

Meligy commented Oct 24, 2017

I'm submitting a...


[x] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

In HttpModule you could use XSRF support for GET requests. It added the XSRF header for all requests including GET requests

But HttpClientModule does not send the XSRF header for GET requests.

I understand why it's implemented this way, but in corporate world you might want to protect everything as well. This is not the point though.

The point is that this is an undocumented breaking change when moving from HttpModule to HttpClientModule, that could cost a lot of debugging time if you had code depending on old behaviour.

You can only tell the problem by looking at source code

https://github.com/angular/angular/blob/master/packages/common/http/src/xsrf.ts#L81-L82

if (req.method === 'GET' || req.method === 'HEAD' || lcUrl.startsWith('http://') ||
    lcUrl.startsWith('https://')) {

Which I only did by accident after long debugging and then googling and seeing #18859 (comment)

Expected behavior

Either:

  • Document the different behaviour
  • Allow overriding it with options
  • Or just keep it as before

Minimal reproduction of the problem with instructions

For a server that supports XSRF, create a GET Api call with HttpModule.

Observe the call working fine.

Change HttpModule to HttpClientModule` and HttpClientXsrfModule`.

See the call fail.

What is the motivation / use case for changing the behavior?

Because it was working before and now it breaks.

Environment


Angular version: 4.3+


Browser:
- [ ] Chrome (desktop) version XX
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX
 
For Tooling issues:
- Node version: XX  
- Platform:  

Others:

@Meligy Meligy changed the title from HTTPClient XSRF support does not work for GET requests to HttpClientXsrfModule does not work for GET requests, different from HttpModule Oct 24, 2017

@Meligy

This comment has been minimized.

Show comment
Hide comment
@Meligy

Meligy Oct 24, 2017

Contributor

A workaround is to copy the code after the check in

https://github.com/angular/angular/blob/master/packages/common/http/src/xsrf.ts#L81-L82

Into a new interceptor.

That's of course once you figure out that this is the issue.

Update: This is not the simplest task unless you duplicate XSRF header value. See the note about exposing the injection token in #19885 (comment)

Contributor

Meligy commented Oct 24, 2017

A workaround is to copy the code after the check in

https://github.com/angular/angular/blob/master/packages/common/http/src/xsrf.ts#L81-L82

Into a new interceptor.

That's of course once you figure out that this is the issue.

Update: This is not the simplest task unless you duplicate XSRF header value. See the note about exposing the injection token in #19885 (comment)

@Meligy Meligy changed the title from HttpClientXsrfModule does not work for GET requests, different from HttpModule to HttpClientXsrfModule handles GET requests differently from HttpModule Oct 24, 2017

@Meligy Meligy changed the title from HttpClientXsrfModule handles GET requests differently from HttpModule to HttpClientXsrfModule breaks compatibility with HttpModule by not handling GET requests Oct 25, 2017

@alxhub

This comment has been minimized.

Show comment
Hide comment
@alxhub

alxhub Oct 25, 2017

Contributor

I don't believe you can call it a breaking change, as HttpClientModule is not designed to be a drop-in replacement for HttpModule - it explicitly does not have the same behavior.

I do agree we could do a better job on the documentation, though - where would you have liked to see this documented that would've helped you?

Contributor

alxhub commented Oct 25, 2017

I don't believe you can call it a breaking change, as HttpClientModule is not designed to be a drop-in replacement for HttpModule - it explicitly does not have the same behavior.

I do agree we could do a better job on the documentation, though - where would you have liked to see this documented that would've helped you?

@jesussobrino

This comment has been minimized.

Show comment
Hide comment
@jesussobrino

jesussobrino Nov 6, 2017

Hi all.

I have the same problem that @Meligy described above: My Backend is currently using XSRF protection for these http methods: GET, PUT, PATCH, POST. So, I had to find a solution in order to be able to use Angular 5.

As @Meligy suggested, avoiding these lines it's working again:
https://github.com/angular/angular/blob/master/packages/common/http/src/xsrf.ts#L81-L84

if (req.method === 'GET' || req.method === 'HEAD' || lcUrl.startsWith('http://') || lcUrl.startsWith('https://')) { return next.handle(req); }

So I created my own HttpXsrfInterceptor, avoiding that lines:

`
@Injectable()
export class HttpXsrfInterceptor implements HttpInterceptor {
constructor(private tokenService: HttpXsrfTokenExtractor) {
}

intercept(req: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {
    const headerName = 'X-XSRF-TOKEN';
    const lcUrl = req.url.toLowerCase();
    // DO NOT Skip both non-mutating requests and absolute URLs. (Override angular 5 default behaviour)
    const token = this.tokenService.getToken();

    // Be careful not to overwrite an existing header of the same name.
    if (token !== null && !req.headers.has(headerName)) {
        req = req.clone({ headers: req.headers.set(headerName, token) });
    }
    return next.handle(req);
}

}
`
Later I create a provider for this:

export let HttpXSRFInterceptorProvider = { provide: HTTP_INTERCEPTORS, useClass: HttpXsrfInterceptor, multi: true };

And finally I add these provider into my "CoreModule". (Actually as you can see I have 2 "custom" interceptors implementing from "HttpInterceptor"):

@NgModule({ imports: [], providers: [ CustomHttpInterceptorProvider, HttpXSRFInterceptorProvider ] })

I hope this helps :)

Regards,
Jesús.

jesussobrino commented Nov 6, 2017

Hi all.

I have the same problem that @Meligy described above: My Backend is currently using XSRF protection for these http methods: GET, PUT, PATCH, POST. So, I had to find a solution in order to be able to use Angular 5.

As @Meligy suggested, avoiding these lines it's working again:
https://github.com/angular/angular/blob/master/packages/common/http/src/xsrf.ts#L81-L84

if (req.method === 'GET' || req.method === 'HEAD' || lcUrl.startsWith('http://') || lcUrl.startsWith('https://')) { return next.handle(req); }

So I created my own HttpXsrfInterceptor, avoiding that lines:

`
@Injectable()
export class HttpXsrfInterceptor implements HttpInterceptor {
constructor(private tokenService: HttpXsrfTokenExtractor) {
}

intercept(req: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {
    const headerName = 'X-XSRF-TOKEN';
    const lcUrl = req.url.toLowerCase();
    // DO NOT Skip both non-mutating requests and absolute URLs. (Override angular 5 default behaviour)
    const token = this.tokenService.getToken();

    // Be careful not to overwrite an existing header of the same name.
    if (token !== null && !req.headers.has(headerName)) {
        req = req.clone({ headers: req.headers.set(headerName, token) });
    }
    return next.handle(req);
}

}
`
Later I create a provider for this:

export let HttpXSRFInterceptorProvider = { provide: HTTP_INTERCEPTORS, useClass: HttpXsrfInterceptor, multi: true };

And finally I add these provider into my "CoreModule". (Actually as you can see I have 2 "custom" interceptors implementing from "HttpInterceptor"):

@NgModule({ imports: [], providers: [ CustomHttpInterceptorProvider, HttpXSRFInterceptorProvider ] })

I hope this helps :)

Regards,
Jesús.

@Meligy

This comment has been minimized.

Show comment
Hide comment
@Meligy

Meligy Nov 6, 2017

Contributor

@alxhub it's more than docs.

Technically speaking one might argue that it's not a breaking change because HttpClient isn't the next version of Http.

Except:

  • Http is being deprecated and will eventually be removed, users are advised to stop using it
  • And the only official recommended migration path is HttpClient
  • Angular docs already only mention HttpClient
  • The common word is that the migration is just a simple code change, except when it isn't!

So, the API people rely on is being deprecated and set to be removed, and the single pushed alternative API breaks the existing behaviour. Yes, this is breaking.

Anyway, to answer your question about where to add the note in docs, I suggest the one page where HTTP is documented https://angular.io/guide/http, specifically in the XSRF section.

Please make it a clear warning to people that states it's different from old Http. It'd be helpful if it's geared towards people who may be upgrading and not aware of it, not a justification why the decision was made.

Also, in addition to docs, implementing your own interceptor that handles GET needs to be made easy.

export const XSRF_COOKIE_NAME = new InjectionToken<string>('XSRF_COOKIE_NAME');

XSRF_COOKIE_NAME InjectionToken needs to be exposed in public_api.ts, so people can implement a copy of the Angular interceptor without hard coding its value or trying to read private members of the Angular interceptor instance.

The result will still be ugly mind you, for an added check that arguably doesn't benefit Angular or anyone anything really, but at least it would be better than it is today. Thanks for keeping the code DRY and using a separate HttpXsrfTokenExtractor class BTW.

Finally, sorry if this comment sounds annoyed. I got personal embarrassment when I had to spend unplanned hours debugging, documenting, and working around this when I originally told my team the change was a simple one that's required due to deprecation.

That is not to contradict that Angular also saved the whole team SO MANY hours and that the net is certainly positive. I really appreciate all the great work you folks do in the framework which I loved so much that I even run a monthly meetup around. You folks are the best :)

Thanks :)

Contributor

Meligy commented Nov 6, 2017

@alxhub it's more than docs.

Technically speaking one might argue that it's not a breaking change because HttpClient isn't the next version of Http.

Except:

  • Http is being deprecated and will eventually be removed, users are advised to stop using it
  • And the only official recommended migration path is HttpClient
  • Angular docs already only mention HttpClient
  • The common word is that the migration is just a simple code change, except when it isn't!

So, the API people rely on is being deprecated and set to be removed, and the single pushed alternative API breaks the existing behaviour. Yes, this is breaking.

Anyway, to answer your question about where to add the note in docs, I suggest the one page where HTTP is documented https://angular.io/guide/http, specifically in the XSRF section.

Please make it a clear warning to people that states it's different from old Http. It'd be helpful if it's geared towards people who may be upgrading and not aware of it, not a justification why the decision was made.

Also, in addition to docs, implementing your own interceptor that handles GET needs to be made easy.

export const XSRF_COOKIE_NAME = new InjectionToken<string>('XSRF_COOKIE_NAME');

XSRF_COOKIE_NAME InjectionToken needs to be exposed in public_api.ts, so people can implement a copy of the Angular interceptor without hard coding its value or trying to read private members of the Angular interceptor instance.

The result will still be ugly mind you, for an added check that arguably doesn't benefit Angular or anyone anything really, but at least it would be better than it is today. Thanks for keeping the code DRY and using a separate HttpXsrfTokenExtractor class BTW.

Finally, sorry if this comment sounds annoyed. I got personal embarrassment when I had to spend unplanned hours debugging, documenting, and working around this when I originally told my team the change was a simple one that's required due to deprecation.

That is not to contradict that Angular also saved the whole team SO MANY hours and that the net is certainly positive. I really appreciate all the great work you folks do in the framework which I loved so much that I even run a monthly meetup around. You folks are the best :)

Thanks :)

@mjeffrey

This comment has been minimized.

Show comment
Hide comment
@mjeffrey

mjeffrey Mar 23, 2018

I have the same issue.

We have a 3rd party payment API that we need to support and unfortunately we have to use GET requests that change state. (we'd also like to protect all requests anyway just in case someone makes a mistake and creates a get with side effects).

The docs state:
By default, an interceptor sends this cookie on all mutating requests (POST, etc.) to relative URLs but not on GET/HEAD requests or on requests with an absolute URL.

The "By default" implies that it should be possible to override this choice but unfortunately it is not. So yes the docs are confusing but we should have the option of supporting GETs.

Maybe an option something like this?

        HttpClientXsrfModule.withOptions({
            includeMethods: [GET, POST],

It is marked frequency:low and this is a sad indictment on the state of Web development that people are not protecting against CSRF. Angular should make it as easy as possible.

thanks @jesussobrino wIll try your workaround.
It is currently marked docs, shall I submit an enhancement request for this?

p.s. thanks for an awesome framework :-)

mjeffrey commented Mar 23, 2018

I have the same issue.

We have a 3rd party payment API that we need to support and unfortunately we have to use GET requests that change state. (we'd also like to protect all requests anyway just in case someone makes a mistake and creates a get with side effects).

The docs state:
By default, an interceptor sends this cookie on all mutating requests (POST, etc.) to relative URLs but not on GET/HEAD requests or on requests with an absolute URL.

The "By default" implies that it should be possible to override this choice but unfortunately it is not. So yes the docs are confusing but we should have the option of supporting GETs.

Maybe an option something like this?

        HttpClientXsrfModule.withOptions({
            includeMethods: [GET, POST],

It is marked frequency:low and this is a sad indictment on the state of Web development that people are not protecting against CSRF. Angular should make it as easy as possible.

thanks @jesussobrino wIll try your workaround.
It is currently marked docs, shall I submit an enhancement request for this?

p.s. thanks for an awesome framework :-)

@justin-ruffin

This comment has been minimized.

Show comment
Hide comment
@justin-ruffin

justin-ruffin Mar 29, 2018

Security Issue

The real issue here is the fact that some Enterprise API Frameworks secure all Http requests irrespective of the Method (e.g Get vs Post).

This is done for security reasons (e.g An inexperienced developer uses a "GET" request and mutates data on the back-end).

The fact that the HttpClient doesn't support this option out of the box, makes communication with server APIs more insecure than the previous version, because the Server API's will have to remove the XSRF token verification on all Get Requests, now opening a possible vulnerability on the server side.

justin-ruffin commented Mar 29, 2018

Security Issue

The real issue here is the fact that some Enterprise API Frameworks secure all Http requests irrespective of the Method (e.g Get vs Post).

This is done for security reasons (e.g An inexperienced developer uses a "GET" request and mutates data on the back-end).

The fact that the HttpClient doesn't support this option out of the box, makes communication with server APIs more insecure than the previous version, because the Server API's will have to remove the XSRF token verification on all Get Requests, now opening a possible vulnerability on the server side.

@IgorMinar IgorMinar added comp: docs and removed type: docs labels Apr 10, 2018

magnastrazh added a commit to stuartshay/ImageGallery.Client that referenced this issue May 29, 2018

@jenniferfell

This comment has been minimized.

Show comment
Hide comment
@jenniferfell

jenniferfell Jun 4, 2018

Contributor

@IgorMinar @alxhub : Hi. I'm triaging the "tricky" PRs now. Can I ask you to confirm that this is a doc issue? Thanks.

Contributor

jenniferfell commented Jun 4, 2018

@IgorMinar @alxhub : Hi. I'm triaging the "tricky" PRs now. Can I ask you to confirm that this is a doc issue? Thanks.

@adammendoza

This comment has been minimized.

Show comment
Hide comment
@adammendoza

adammendoza Jun 7, 2018

I'm getting this error that seems to be related.

core.js:1598 ERROR TypeError: req.url.toLowerCase is not a function
    at HttpXsrfInterceptor.push../node_modules/@angular/common/fesm5/http.js.HttpXsrfInterceptor.intercept (http.js:2027)
    at HttpInterceptorHandler.push../node_modules/@angular/common/fesm5/http.js.HttpInterceptorHandler.handle (http.js:1407)
    at HttpInterceptingHandler.push../node_modules/@angular/common/fesm5/http.js.HttpInterceptingHandler.handle (http.js:2080)
    at MergeMapSubscriber.project (http.js:1158)
    at MergeMapSubscriber.push../node_modules/rxjs/_esm5/internal/operators/mergeMap.js.MergeMapSubscriber._tryNext (mergeMap.js:117)
    at MergeMapSubscriber.push../node_modules/rxjs/_esm5/internal/operators/mergeMap.js.MergeMapSubscriber._next (mergeMap.js:107)
    at MergeMapSubscriber.push../node_modules/rxjs/_esm5/internal/Subscriber.js.Subscriber.next (Subscriber.js:93)
    at Observable._subscribe (scalar.js:5)
    at Observable.push../node_modules/rxjs/_esm5/internal/Observable.js.Observable._trySubscribe (Observable.js:176)
    at Observable.push../node_modules/rxjs/_esm5/internal/Observable.js.Observable.subscribe (Observable.js:161)

adammendoza commented Jun 7, 2018

I'm getting this error that seems to be related.

core.js:1598 ERROR TypeError: req.url.toLowerCase is not a function
    at HttpXsrfInterceptor.push../node_modules/@angular/common/fesm5/http.js.HttpXsrfInterceptor.intercept (http.js:2027)
    at HttpInterceptorHandler.push../node_modules/@angular/common/fesm5/http.js.HttpInterceptorHandler.handle (http.js:1407)
    at HttpInterceptingHandler.push../node_modules/@angular/common/fesm5/http.js.HttpInterceptingHandler.handle (http.js:2080)
    at MergeMapSubscriber.project (http.js:1158)
    at MergeMapSubscriber.push../node_modules/rxjs/_esm5/internal/operators/mergeMap.js.MergeMapSubscriber._tryNext (mergeMap.js:117)
    at MergeMapSubscriber.push../node_modules/rxjs/_esm5/internal/operators/mergeMap.js.MergeMapSubscriber._next (mergeMap.js:107)
    at MergeMapSubscriber.push../node_modules/rxjs/_esm5/internal/Subscriber.js.Subscriber.next (Subscriber.js:93)
    at Observable._subscribe (scalar.js:5)
    at Observable.push../node_modules/rxjs/_esm5/internal/Observable.js.Observable._trySubscribe (Observable.js:176)
    at Observable.push../node_modules/rxjs/_esm5/internal/Observable.js.Observable.subscribe (Observable.js:161)
@RegalMedia

This comment has been minimized.

Show comment
Hide comment
@RegalMedia

RegalMedia Aug 12, 2018

Interested in this as well. More and more enterprise setups include pseudo-microservices where the UI and backend REST are on different URLs and have CORS setup to allow all/most requests in. Can't assume endpoints will always be relative, or that an outside caller will be unable to see the results. Understood this is only part of the security picture, but it'd be a great help to have this available out of the box instead of having to roll our own, regardless how easy it might be.

RegalMedia commented Aug 12, 2018

Interested in this as well. More and more enterprise setups include pseudo-microservices where the UI and backend REST are on different URLs and have CORS setup to allow all/most requests in. Can't assume endpoints will always be relative, or that an outside caller will be unable to see the results. Understood this is only part of the security picture, but it'd be a great help to have this available out of the box instead of having to roll our own, regardless how easy it might be.

@abnerh69

This comment has been minimized.

Show comment
Hide comment
@abnerh69

abnerh69 Aug 22, 2018

Hey @rudyhadoux , what exactly works for you with Angular 6? I really need help on it, I am relatively new on Angular and in my very first project I need CSRF control. Please, can you share what solve this problem on Angular 6?

abnerh69 commented Aug 22, 2018

Hey @rudyhadoux , what exactly works for you with Angular 6? I really need help on it, I am relatively new on Angular and in my very first project I need CSRF control. Please, can you share what solve this problem on Angular 6?

@Meligy

This comment has been minimized.

Show comment
Hide comment
@Meligy

Meligy Aug 22, 2018

Contributor

@rudyhadoux this issue is not about sanitizing. An image SRC with a direct url wouldn't go through the Angular Http services anyway. So it works not because the issue is solved, but because your case is not relevant to the issue at all.

Contributor

Meligy commented Aug 22, 2018

@rudyhadoux this issue is not about sanitizing. An image SRC with a direct url wouldn't go through the Angular Http services anyway. So it works not because the issue is solved, but because your case is not relevant to the issue at all.

@Meligy

This comment has been minimized.

Show comment
Hide comment
@Meligy

Meligy Aug 23, 2018

Contributor

@rudyhadoux my reply was for everybody's interest since the comment that this works in Angular v6 was confusing. This issue is about CSRF, and the limitation it describes has not been removed in v6.

If you used other techniques for a specific use case that do not involve CSRF, good for you, your specific use case was met by avoiding CSRF, but for those who need CSRF for their use cases, this issue still stands.

Contributor

Meligy commented Aug 23, 2018

@rudyhadoux my reply was for everybody's interest since the comment that this works in Angular v6 was confusing. This issue is about CSRF, and the limitation it describes has not been removed in v6.

If you used other techniques for a specific use case that do not involve CSRF, good for you, your specific use case was met by avoiding CSRF, but for those who need CSRF for their use cases, this issue still stands.

@Meligy

This comment has been minimized.

Show comment
Hide comment
@Meligy

Meligy Aug 23, 2018

Contributor

The question is out of scope of this issue. Please let's stop this branch of discussion here and leave the issue back to the request about removing the XSRF limitation. You have made your point already that people might be able to explore JWT based alternatives to using XSRF at all.

Discussing whether this is feasible in varying use cases, and whether it's equivalent, etc. is outside of the scope of communicating with the Angular team about a limitation in the built-in XSRF utilities they provide, for those who choose to use it.

Contributor

Meligy commented Aug 23, 2018

The question is out of scope of this issue. Please let's stop this branch of discussion here and leave the issue back to the request about removing the XSRF limitation. You have made your point already that people might be able to explore JWT based alternatives to using XSRF at all.

Discussing whether this is feasible in varying use cases, and whether it's equivalent, etc. is outside of the scope of communicating with the Angular team about a limitation in the built-in XSRF utilities they provide, for those who choose to use it.

@jenniferfell

This comment has been minimized.

Show comment
Hide comment
@jenniferfell

jenniferfell Sep 27, 2018

Contributor

It's not clear that this is a doc issue, so I'm removing comp: docs so it will bounce back into the triage queue.

Looks like the discussion has ended on a code request: "back to the request about removing the XSRF limitation"

If there is a doc update to be made in lieu of a code change, then we'll need clarification from engineering. Thanks.

Contributor

jenniferfell commented Sep 27, 2018

It's not clear that this is a doc issue, so I'm removing comp: docs so it will bounce back into the triage queue.

Looks like the discussion has ended on a code request: "back to the request about removing the XSRF limitation"

If there is a doc update to be made in lieu of a code change, then we'll need clarification from engineering. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment