FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(http): run root interceptors in the terminal request chain by SkyZeroZx · Pull Request #69778 · angular/angular · GitHub

fix(http): run root interceptors in the terminal request chain - #69778

Merged
thePunderWoman merged 6 commits into
angular:mainfrom
SkyZeroZx:fix/leak-parent-transfercache
Jul 31, 2026
Merged

fix(http): run root interceptors in the terminal request chain#69778
thePunderWoman merged 6 commits into
angular:mainfrom
SkyZeroZx:fix/leak-parent-transfercache

Conversation

SkyZeroZx commented Jul 15, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

Represent withRequestsMadeViaParent() with an internal delegating backend so the interceptor handler can distinguish delegated clients from independent child configurations.

Delegated clients leave inherited root interceptors to the parent chain. This prevents duplicate execution and lets HttpTransferCache evaluate authentication and cache filters after parent request interceptors, while independent child clients continue to inherit framework root interceptors.

Add coverage for independent root inheritance, request and response ordering, authenticated requests, cache filters, and public cache hits.

Fixes #69777

This seems similar to GHSA-q6f4-qqrg-jv6x ( In fact, no filter or exclusion is currently applied )

pullapprove Bot requested a review from JeanMeche July 15, 2026 05:00
angular-robot Bot added the area: common/http Issues related to HTTP and HTTP Client label Jul 15, 2026
ngbot Bot added this to the Backlog milestone Jul 15, 2026
Comment thread packages/common/http/src/provider.ts Outdated
);
}
return handlerFromParent;
return new HttpParentBackend(handlerFromParent!);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What would you think of defining a separate token instead of creating a separate HttpBackend instance ?

SkyZeroZx Jul 15, 2026
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Are you referring to using a DI token as a boolean flag here?

I considered it, but after reviewing the git history, it seems we might have a case like this #55652

A boolean token could become stale if another provider overrides HttpBackend, causing inherited root interceptors to be skipped incorrectly.


Or did you mean using a token for the parent handler and comparing it with the current HttpBackend instance?

In that case, I believe it would be something like the following (I haven't tested it), personally I would prefer the class that seemed simpler to me

const HTTP_PARENT_HANDLER = new InjectionToken<HttpHandler>('HTTP_PARENT_HANDLER');

// withRequestsMadeViaParent()
[
 {
   provide: HTTP_PARENT_HANDLER,
   useFactory: () => inject(HttpHandler, {skipSelf: true}),
 },
 {
   provide: HttpBackend,
   useExisting: HTTP_PARENT_HANDLER,
 },
];

// HttpInterceptorHandler
private readonly parentHandler = inject(HTTP_PARENT_HANDLER, {
 self: true,
 optional: true,
});

const rootInterceptorFns = this.injector.get(
 HTTP_ROOT_INTERCEPTOR_FNS,
 [],
 this.backend === this.parentHandler ? {self: true} : undefined,
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Was more think of something like:

export const ɵHTTP_CLIENT_IS_DELEGATING = new InjectionToken<boolean>('ɵHTTP_CLIENT_IS_DELEGATING');

// packages/common/http/src/provider.ts
export function withRequestsMadeViaParent(): HttpFeature<HttpFeatureKind.RequestsMadeViaParent> {
  return makeHttpFeature(HttpFeatureKind.RequestsMadeViaParent, [
    {
      provide: HttpBackend,
      useFactory: () => {
        const handlerFromParent = inject(HttpHandler, {skipSelf: true, optional: true});
        // ... error handling ...
        return handlerFromParent; // Return directly
      },
    },
    {
      provide: ɵHTTP_CLIENT_IS_DELEGATING,
      useValue: true
    }
  ]);
}

// packages/common/http/src/backend.ts (Inside HttpInterceptorHandler)
handle(initialRequest: HttpRequest<any>): Observable<HttpEvent<any>> {
  if (this.chain === null) {
    // Query with {self: true} to strictly check if the *current* client configuration is delegating
    const isDelegating = this.injector.get(ɵHTTP_CLIENT_IS_DELEGATING, false, {self: true});

    const rootInterceptorFns = this.injector.get(
      HTTP_ROOT_INTERCEPTOR_FNS,
      [],
      isDelegating ? {self: true} : undefined,
    );
    // ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I tried this approach, but the token can become stale if another feature overrides HttpBackend.

Eg : with withRequestsMadeViaParent() and withXhr(), XHR becomes the effective backend while the token remains true, so inherited root interceptors are skipped. This test reproduces it.

it('should inherit root interceptors when withXhr overrides parent delegation', () => {
      TestBed.configureTestingModule({
        providers: [
          provideHttpClient(),
          {
            provide: HTTP_ROOT_INTERCEPTOR_FNS,
            useValue: makeLiteralTagInterceptorFn('root'),
            multi: true,
          },
        ],
      });

      const child = createEnvironmentInjector(
        [
          provideHttpClient(withRequestsMadeViaParent(), withXhr()),
          {provide: XhrFactory, useClass: MockXhrFactory},
        ],
        TestBed.inject(EnvironmentInjector),
      );

      try {
        child.get(HttpClient).get('/test').subscribe();
        const factory = child.get(XhrFactory) as MockXhrFactory;
        expect(factory.mock.mockHeaders['X-Tag']).toBe('root');
        factory.mock.mockFlush(200, 'OK', '{}');
      } finally {
        child.destroy();
      }
    });

JeanMeche Jul 17, 2026
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe the issue here is that we allow this contradictory usage. Both withRequestsMadeViaParent() and withXhr() register a different backend.

SkyZeroZx Jul 17, 2026
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I thought that could be the case too; in fact, I don't know if it's intentional or not.

But I think a feature on some route might want to use withXhr while maintaining interceptors for the upload report, which isn't supported in fetch.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Both provide HttpBackend only the latest registred will effectively be injected.

SkyZeroZx Jul 17, 2026
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

My bad , you're right, we'll most likely have to throw an exception in this case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Updated, we will now throw an exception at development time, although I still think we should allow a custom HttpBackend for example in provideHttpClientTesting

SkyZeroZx requested a review from JeanMeche July 16, 2026 15:30
JeanMeche requested review from alan-agius4 and removed request for JeanMeche July 17, 2026 15:38
alan-agius4 added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 20, 2026
SkyZeroZx force-pushed the fix/leak-parent-transfercache branch 2 times, most recently from 449ce42 to 26df123 Compare July 20, 2026 16:56
SkyZeroZx requested a review from JeanMeche July 20, 2026 18:50

JeanMeche commented Jul 21, 2026
edited
Loading

Copy link
Copy Markdown
Member

Found a regression/change:
Prior to this change the interceptor would run twice, now it only runs ones.

import { TestBed } from '@angular/core/testing';
import { EnvironmentInjector, createEnvironmentInjector } from '@angular/core';
import { HttpClient, provideHttpClient, withRequestsMadeViaParent } from '@angular/common/http';
import { HttpTestingController, provideHttpClientTesting } from '@angular/common/http/testing';
import { HTTP_ROOT_INTERCEPTOR_FNS } from '@angular/common/http/src/interceptor';
describe('Delegating HttpClient root interceptors', () => {
  it('should only run root interceptors once in a delegated chain', () => {
    let rootInterceptorRunCount = 0;
    TestBed.configureTestingModule({
      providers: [
        provideHttpClient(),
        provideHttpClientTesting(),
        {
          provide: HTTP_ROOT_INTERCEPTOR_FNS,
          multi: true,
          useValue: (req, next) => {
            rootInterceptorRunCount++;
            return next(req);
          }
        }
      ],
    });
    const childInjector = createEnvironmentInjector(
      [provideHttpClient(withRequestsMadeViaParent())],
      TestBed.inject(EnvironmentInjector)
    );
    const client = childInjector.get(HttpClient);
    const httpMock = TestBed.inject(HttpTestingController);
    client.get('/api/data').subscribe();
    httpMock.expectOne('/api/data').flush({});
    // ❌ FAILS BEFORE PR: The count would be 2 because the child executed the parent's 
    // root interceptors, and then the parent executed them again.
    expect(rootInterceptorRunCount).toBe(1); 
  });
});

Copy link
Copy Markdown
Contributor Author

Found a regression/change: Prior to this change the interceptor would run twice, now it only runs ones.

import { TestBed } from '@angular/core/testing';
import { EnvironmentInjector, createEnvironmentInjector } from '@angular/core';
import { HttpClient, provideHttpClient, withRequestsMadeViaParent } from '@angular/common/http';
import { HttpTestingController, provideHttpClientTesting } from '@angular/common/http/testing';
import { HTTP_ROOT_INTERCEPTOR_FNS } from '@angular/common/http/src/interceptor';
describe('Delegating HttpClient root interceptors', () => {
  it('should only run root interceptors once in a delegated chain', () => {
    let rootInterceptorRunCount = 0;
    TestBed.configureTestingModule({
      providers: [
        provideHttpClient(),
        provideHttpClientTesting(),
        {
          provide: HTTP_ROOT_INTERCEPTOR_FNS,
          multi: true,
          useValue: (req, next) => {
            rootInterceptorRunCount++;
            return next(req);
          }
        }
      ],
    });
    const childInjector = createEnvironmentInjector(
      [provideHttpClient(withRequestsMadeViaParent())],
      TestBed.inject(EnvironmentInjector)
    );
    const client = childInjector.get(HttpClient);
    const httpMock = TestBed.inject(HttpTestingController);
    client.get('/api/data').subscribe();
    httpMock.expectOne('/api/data').flush({});
    // ❌ FAILS BEFORE PR: The count would be 2 because the child executed the parent's 
    // root interceptors, and then the parent executed them again.
    expect(rootInterceptorRunCount).toBe(1); 
  });
});

In fact, I believe it was a bug and not something expected to happen twice (or so I think), since that's why the problem described in #69777 occurs.

I doubt we would expect or that it would make any sense to expect an interceptor to execute twice (when you create an interceptor in an app, this doesn't happen).

JeanMeche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

AGENT: Great PR! The tests are very thorough and it effectively fixes the duplicate root interceptor execution. I have left a few inline suggestions for your consideration to simplify the implementation by removing the internal injection token.

Comment thread packages/common/http/src/backend.ts Outdated
Comment thread packages/common/http/src/backend.ts Outdated
Comment thread packages/common/http/src/provider.ts Outdated
SkyZeroZx force-pushed the fix/leak-parent-transfercache branch from cb9fbd6 to 51616ee Compare July 22, 2026 14:33
SkyZeroZx requested a review from JeanMeche July 23, 2026 18:25
JeanMeche removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 30, 2026

JeanMeche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

AGENT: Thank you for working on this fix for root interceptors and transfer cache! I have left a few inline feedback comments regarding dev mode bundle optimizations, runtime null safety, and test coverage for your consideration.

Comment thread packages/common/http/src/provider.ts Outdated
Represent withRequestsMadeViaParent() with an internal delegating backend so the interceptor handler can distinguish delegated clients from independent child configurations.

Delegated clients leave inherited root interceptors to the parent chain. This prevents duplicate execution and lets HttpTransferCache evaluate authentication and cache filters after parent request interceptors, while independent child clients continue to inherit framework root interceptors.

Add coverage for independent root inheritance, request and response ordering, authenticated requests, cache filters, and public cache hits.

Fixes angular#69777
SkyZeroZx force-pushed the fix/leak-parent-transfercache branch from 51616ee to 4f3f6c3 Compare July 30, 2026 19:56
SkyZeroZx requested a review from JeanMeche July 30, 2026 19:58
Comment thread packages/common/http/src/provider.ts Outdated
* "bubble up" until either reaching the root level or an `HttpClient` which was not configured with
* this option.
*
* This feature is incompatible with the `withFetch` and `withXhr` features.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We should probably rephrase that a bit. It works will with both backend if they are in the parent.

Suggested change
* This feature is incompatible with the `withFetch` and `withXhr` features.
* This feature is incompatible with the `withFetch` and `withXhr` features in the `provideHttpClient()`.

Or something else if you can come up with a better phrasing :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thinking about it (or asking GPT 5.6 Sol Ultra Max 😄), it gave me this option, wdyt?

 * This feature cannot be combined with `withFetch` or `withXhr` in the same `provideHttpClient()` call.

JeanMeche added the action: merge The PR is ready for merge by the caretaker label Jul 30, 2026
JeanMeche added the target: patch This PR is targeted for the next patch release label Jul 30, 2026
thePunderWoman merged commit bb78286 into angular:main Jul 31, 2026
27 checks passed

Copy link
Copy Markdown
Contributor

This PR was merged into the repository. The changes were merged into the following branches:

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: merge The PR is ready for merge by the caretaker area: common/http Issues related to HTTP and HTTP Client target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HttpTransferCache can cache authenticated responses when using withRequestsMadeViaParent

4 participants


Back | FazBrowse Home | New Git URL