Skip to content

fix(http): distinguish repeated transfer cache params - #68571

Merged
thePunderWoman merged 1 commit into
angular:mainfrom
Hexix23:fix-transfer-cache-param-keys
Jun 11, 2026
Merged

fix(http): distinguish repeated transfer cache params#68571
thePunderWoman merged 1 commit into
angular:mainfrom
Hexix23:fix-transfer-cache-param-keys

Conversation

@Hexix23

@Hexix23 Hexix23 commented May 5, 2026

Copy link
Copy Markdown
Contributor

This fix addresses ambiguous HttpTransferCache key generation for request parameters.

sortAndConcatParams() previously interpolated params.getAll(k) directly. Since getAll() returns an array, JavaScript string coercion joined repeated values with commas. This caused semantically different requests such as role=user,admin and role=user&role=admin to produce the same transfer-cache key material during SSR.

Changes:

  • Serialize transfer-cache params as structured sorted key/value tuples instead of comma-joined strings.
  • Deduplicate repeated URLSearchParams.keys() entries before reading getAll().
  • Preserve the existing empty-params key material.
  • Add a regression test showing scalar-comma params and repeated params no longer reuse the same cached response.

This change is prepared for a security report that follows the Google OSS VRP patch-first path for product vulnerabilities in OT0 projects.

Security impact:

In an SSR application, this cache-key ambiguity can make a later security-sensitive HttpClient
request receive the response from an earlier semantically different request in the same render. For
example, an attacker-influenced scalar-comma request can be cached and then replayed as the response
for a trusted repeated-param authorization/data request to the same URL. In that shape, Angular's
server-rendered output can be based on the wrong backend decision because the trusted request is not
dispatched.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.dev application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

During SSR, HttpTransferCache can generate identical key material for distinct request params because repeated values are joined with commas:

new HttpParams().set('role', 'user,admin')
new HttpParams().append('role', 'user').append('role', 'admin')

Both requests previously serialized as role=user,admin, allowing the second request to receive the first cached response.

What is the new behavior?

Transfer-cache params are serialized as structured key/value tuples. Repeated params and scalar comma params now produce distinct cache keys, so the repeated-param request reaches the backend once and then reuses only its own cache entry.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Local validation:

bazelisk test //packages/common/http/test:test --test_filter='TransferCache withHttpTransferCache should differentiate repeated parameters from scalar comma parameters'
bazelisk test //packages/common/http/test:test

Both tests passed locally.

@pullapprove
pullapprove Bot requested a review from thePunderWoman May 5, 2026 09:13
@angular-robot angular-robot Bot added the area: common Issues related to APIs in the @angular/common package label May 5, 2026
@ngbot ngbot Bot added this to the Backlog milestone May 5, 2026
@JeanMeche

Copy link
Copy Markdown
Member

We already have #68424 that should be good to go.

@JeanMeche JeanMeche closed this May 5, 2026
@Hexix23

Hexix23 commented May 5, 2026

Copy link
Copy Markdown
Contributor Author

Relation to #68424:

#68424 fixes a related but narrower issue: duplicated search-param keys from URLSearchParams.keys() could make sortAndConcatParams() emit duplicated key material and grow unnecessarily. That PR deduplicates the keys, but it still serializes values with ${params.getAll(k)}.

Because getAll(k) returns an array, scalar comma values and repeated values still collapse to the same string after #68424:

new HttpParams().set('role', 'user,admin')
new HttpParams().append('role', 'user').append('role', 'admin')

Both still serialize as role=user,admin on #68424, so the second request can still reuse the first transfer-cache entry.

I verified this locally by applying this PR's regression test on top of #68424. It fails there with:

Expected one matching request for criteria "Match URL: /test-params?role=user&role=admin", found none.

This PR addresses that remaining ambiguity by serializing params as structured sorted key/value tuples instead of comma-joined value arrays.

Comment thread packages/common/http/src/transfer_cache.ts Outdated
@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 labels May 5, 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.

See suggestion. Also, commit should be scoped “http” instead of “common”.

@Hexix23 Hexix23 changed the title fix(common): distinguish repeated transfer cache params fix(http): distinguish repeated transfer cache params May 6, 2026

@Hexix23 Hexix23 left a comment

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.

Thanks, I checked this suggestion and it preserves the security fix.

For the vulnerable pair:

new HttpParams().set('role', 'user,admin')
new HttpParams().append('role', 'user').append('role', 'admin')

the suggested normalization produces distinct cache-key material:

  role=user%2Cadmin
  role=user&role=admin

So the scalar comma value and the repeated-param shape no longer collide.

It also naturally preserves repeated parameters after URLSearchParams.sort(), while keeping the implementation simpler than the JSON tuple serialization.

I’ve also updated the commit scope/title from common to http as requested.

@alan-agius4

Copy link
Copy Markdown
Contributor

@Hexix23, looks like the commit has not been pushed.

@Hexix23

Hexix23 commented May 6, 2026

Copy link
Copy Markdown
Contributor Author

@alan-agius4 done

@alan-agius4
alan-agius4 force-pushed the fix-transfer-cache-param-keys branch from 16cdde4 to f09efaa Compare May 6, 2026 11:30
@angular-robot angular-robot Bot added the area: common/http Issues related to HTTP and HTTP Client label May 6, 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

@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels May 6, 2026
@Hexix23

Hexix23 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Hi,

Any update about this? :D

@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

@alan-agius4

Copy link
Copy Markdown
Contributor

@Hexix23 please rebase and fix the conflicts.

@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 and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 10, 2026
Serialize transfer cache request parameters without comma-joining repeated values so distinct HttpClient requests cannot reuse the same cached response.
@Hexix23
Hexix23 force-pushed the fix-transfer-cache-param-keys branch from 372f22e to b0fb38e Compare June 10, 2026 09:40
@Hexix23

Hexix23 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Done! :D

@alan-agius4
alan-agius4 requested review from JeanMeche and removed request for thePunderWoman June 10, 2026 10:12
@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jun 10, 2026
@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 10, 2026
@thePunderWoman
thePunderWoman merged commit a6c7fc5 into angular:main Jun 11, 2026
26 checks passed
@thePunderWoman

Copy link
Copy Markdown
Contributor

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

@angular-automatic-lock-bot

Copy link
Copy Markdown

This pull request has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Jul 12, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: common/http Issues related to HTTP and HTTP Client area: common Issues related to APIs in the @angular/common package 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