fix(http): distinguish repeated transfer cache params - #68571
Conversation
|
We already have #68424 that should be good to go. |
|
Relation to #68424: #68424 fixes a related but narrower issue: duplicated search-param keys from Because new HttpParams().set('role', 'user,admin')
new HttpParams().append('role', 'user').append('role', 'admin')Both still serialize as I verified this locally by applying this PR's regression test on top of #68424. It fails there with: This PR addresses that remaining ambiguity by serializing params as structured sorted key/value tuples instead of comma-joined value arrays. |
There was a problem hiding this comment.
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=adminSo 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.
|
@Hexix23, looks like the commit has not been pushed. |
|
@alan-agius4 done |
16cdde4 to
f09efaa
Compare
|
Hi, Any update about this? :D |
|
@Hexix23 please rebase and fix the conflicts. |
Serialize transfer cache request parameters without comma-joining repeated values so distinct HttpClient requests cannot reuse the same cached response.
372f22e to
b0fb38e
Compare
|
Done! :D |
|
This pull request has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This fix addresses ambiguous
HttpTransferCachekey generation for request parameters.sortAndConcatParams()previously interpolatedparams.getAll(k)directly. SincegetAll()returns an array, JavaScript string coercion joined repeated values with commas. This caused semantically different requests such asrole=user,adminandrole=user&role=adminto produce the same transfer-cache key material during SSR.Changes:
URLSearchParams.keys()entries before readinggetAll().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
HttpClientrequest 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?
What is the current behavior?
Issue Number: N/A
During SSR,
HttpTransferCachecan generate identical key material for distinct request params because repeated values are joined with commas: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?
Other information
Local validation:
Both tests passed locally.