angular: HttpClientXsrfModule breaks compatibility with HttpModule by not handling GET requests

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 https://github.com/angular/angular/issues/18859#issuecomment-333844607

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:

About this issue

  • Original URL
  • State: open
  • Created 7 years ago
  • Reactions: 14
  • Comments: 17 (9 by maintainers)

Most upvoted comments

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

https://github.com/angular/angular/blob/3997d9780615dc323eb357bf7077d0264a1c35e9/packages/common/http/src/xsrf.ts#L18

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 😃

Just ran into this issue when upgrading an AngularJS app.

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.

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)

I like the idea of making this configurable, I think it’s a use case we should discuss.

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 😃

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 https://github.com/angular/angular/issues/19885#issuecomment-342136992