Skip to content

fix(http): skip TransferCache for cookie-bearing requests by default#67964

Merged
alxhub merged 2 commits into
angular:mainfrom
Yenya030:patch/transfercache-cookie-isolation
May 27, 2026
Merged

fix(http): skip TransferCache for cookie-bearing requests by default#67964
alxhub merged 2 commits into
angular:mainfrom
Yenya030:patch/transfercache-cookie-isolation

Conversation

@Yenya030

Copy link
Copy Markdown

What does this PR do?

This change makes TransferCache skip requests with a Cookie header by default.

Today, TransferCache already avoids caching requests with authorization headers unless the application explicitly opts in with includeRequestsWithAuthHeaders. This change treats cookie-bearing requests the same way, since those responses can also vary by user context.

The explicit opt-in path is preserved.

Why is this change needed?

Requests that carry cookies can produce user-specific responses on the server. Reusing those responses from TransferCache under the default key can return an incorrect cached response for another request to the same URL.

What changes are included?

  • skip TransferCache caching for requests that include a Cookie header by default
  • preserve the existing explicit opt-in behavior
  • add regression coverage for cookie-bearing requests
  • update the SSR guide to document the behavior

@pullapprove pullapprove Bot requested a review from kirjs March 31, 2026 21:14
@angular-robot angular-robot Bot added the area: common/http Issues related to HTTP and HTTP Client label Mar 31, 2026
@ngbot ngbot Bot added this to the Backlog milestone Mar 31, 2026
@JeanMeche JeanMeche requested review from JeanMeche and alan-agius4 and removed request for kirjs March 31, 2026 21:40
@pullapprove pullapprove Bot requested a review from kirjs March 31, 2026 21:40
Treat requests with a Cookie header like other auth-bearing requests and skip TransferCache caching them by default.

This preserves the explicit opt-in path via includeRequestsWithAuthHeaders, adds regression coverage for cookie-bearing requests, and updates the SSR guide to document the behavior.
@Yenya030

Copy link
Copy Markdown
Author

@JeanMeche requesting review from approvers on this one. This change addresses a security-relevant issue in SSR TransferCache: requests that differ only by Cookie currently alias to the same cache entry, which can cause reuse of user-specific responses across cookie contexts. The patch is intentionally narrow, preserves the existing opt-in path, and is aimed at Google's patch reward program. A review when convenient would be appreciated.

@Yenya030

Copy link
Copy Markdown
Author

@JeanMeche @alan-agius4 .

@Yenya030

Copy link
Copy Markdown
Author

@JeanMeche @alan-agius4 , Its been over a month since I submitted this PR. Has anyone taken a look at it? Not really sure what is happening.

@Yenya030

Yenya030 commented May 5, 2026

Copy link
Copy Markdown
Author

Hi, @JeanMeche @alan-agius4, I’m not sure what is happening here, but this PR has been open for over a month. Is there something wrong with the PR?

@Yenya030

Yenya030 commented May 6, 2026

Copy link
Copy Markdown
Author

Hi @JeanMeche @alan-agius4, I believe this fix is security-relevant and should be reviewed and merged by the team, as again, it is a security-relevant fix.

I also want to add that another one of my PRs, which was merged, has not received payment. That fix was merged more than a month ago, and I still have not received payment for it. I have had similar issues in the past with the Google bug bounty program, and your team helped me remediate the situation. I am hoping you can help with this again. Thanks!

@alan-agius4

Copy link
Copy Markdown
Contributor

@Yenya030 thanks for this. So I missed the request for review.

@alan-agius4 alan-agius4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this, can you also exclude request withCredentials from cache? Thanks.

@alan-agius4 alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label May 26, 2026
@alan-agius4 alan-agius4 self-assigned this May 26, 2026
@alan-agius4 alan-agius4 added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release security Issues that generally impact framework or application security and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 26, 2026
@Yenya030

Yenya030 commented May 26, 2026

Copy link
Copy Markdown
Author

No worries @alan-agius4 it happens. Done. I updated the patch to also exclude requests sent with withCredentials from TransferCache and added regression coverage. I kept this PR scoped to the maintainer-requested credential-related cases. Broader credential-context handling, such as credentials mode and more general request-context keying, looks like part of a larger TransferCache key-integrity problem and felt out of scope for this PR.

@alan-agius4 alan-agius4 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 May 27, 2026

@alan-agius4 alan-agius4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks

@alan-agius4 alan-agius4 removed the request for review from kirjs May 27, 2026 06:50
@alan-agius4 alan-agius4 removed their assignment May 27, 2026
Update the transfer cache check to safely exclude all requests sent with the `withCredentials` flag.

By default, the HTTP transfer cache avoids caching user-specific responses to prevent sensitive data exposure or incorrect caching. While requests with explicit headers like `Cookie` or `Authorization` are excluded by default, requests can also be sent with credentials via the `withCredentials` flag without having those headers explicitly declared on the request object.

To keep user-specific responses from being cached, exclude `withCredentials` requests unconditionally, even when the `includeRequestsWithAuthHeaders` option is set to true.
@alan-agius4 alan-agius4 force-pushed the patch/transfercache-cookie-isolation branch from ac445ed to fbad713 Compare May 27, 2026 06:54
@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label May 27, 2026
@alxhub alxhub merged commit 34090cb into angular:main May 27, 2026
22 of 24 checks passed
@alxhub

alxhub commented May 27, 2026

Copy link
Copy Markdown
Member

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

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 security Issues that generally impact framework or application security target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants