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)
@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 ofHttp
.Except:
Http
is being deprecated and will eventually be removed, users are advised to stop using itHttpClient
HttpClient
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 inpublic_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) { }
} ` 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.
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?
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