Skip to content

Commit 9d40f8a

Browse files
arshsmith1pkozlowski-opensource
authored andcommitted
fix(http): prevent transfer cache key collisions
`makeCacheKey` joined the request fields with `|` before hashing. The url and the serialized body can contain `|` themselves, so a shifted field boundary (url `/items/a` + body `b|c` vs url `/items/a|b` + body `c`) produced the same joined string and the same key, letting two distinct requests share a transfer cache slot. Join with `\0` instead, which cannot occur in a valid url or in encoded params, so the field boundaries cannot be forged by field content. (cherry picked from commit 3192dcc)
1 parent e02a0de commit 9d40f8a

2 files changed

Lines changed: 15 additions & 3 deletions

File tree

packages/common/http/src/transfer_cache.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -395,7 +395,10 @@ function makeCacheKey(
395395
serializedBody = '';
396396
}
397397

398-
const key = [method, responseType, mappedRequestUrl, serializedBody, encodedParams].join('|');
398+
// Joining with `|` lets a shifted field boundary (url `/a` + body `b|c` vs url `/a|b` + body `c`)
399+
// collapse to the same string and thus the same hash. `\0` cannot occur in a valid url or in
400+
// encoded params, so the field boundaries can't be forged by field content.
401+
const key = [method, responseType, mappedRequestUrl, serializedBody, encodedParams].join('\0');
399402
const hash = generateHash(key);
400403

401404
return makeStateKey(hash);

packages/common/http/test/transfer_cache_spec.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -419,15 +419,15 @@ describe('TransferCache', () => {
419419

420420
const transferState = TestBed.inject(TransferState);
421421
expect(JSON.parse(transferState.toJson()) as Record<string, unknown>).toEqual({
422-
'2da5dfaf112523258ec9c26a0abe9a093b59ed7dbe5f43e4b5ee25a407ac9cf0': {
422+
'd501aa2d57b63a95df74e3b0558782b71b077974e968ed303cd30b27e4b70702': {
423423
[BODY]: 'foo',
424424
[HEADERS]: {},
425425
[STATUS]: 200,
426426
[STATUS_TEXT]: 'OK',
427427
[REQ_URL]: '/test-1',
428428
[RESPONSE_TYPE]: 'json',
429429
},
430-
'869485290d9385f3c0a9ba571918c335bbca9e03373bf8260d02f2b7dd335849': {
430+
'ceddc6689dc1f2fc3a0b8c364b6e00a79b99a149f27e84da87cec03d44c150c8': {
431431
[BODY]: 'buzz',
432432
[HEADERS]: {},
433433
[STATUS]: 200,
@@ -764,6 +764,15 @@ describe('TransferCache', () => {
764764
makeRequestAndExpectOne('/test-1', null, {method: 'POST', transferCache: true, body: 'bar'});
765765
});
766766

767+
it('should differentiate POST requests with an ambiguous url/body boundary', () => {
768+
// `/items/a` with body `b|c` and `/items/a|b` with body `c` are different requests, but a
769+
// cache key that concatenates the fields with `|` maps both to the same string. The second
770+
// request must be treated as a cache miss and hit the network.
771+
makeRequestAndExpectOne('/items/a', null, {method: 'POST', transferCache: true, body: 'b|c'});
772+
makeRequestAndExpectNone('/items/a', 'POST', {transferCache: true, body: 'b|c'});
773+
makeRequestAndExpectOne('/items/a|b', null, {method: 'POST', transferCache: true, body: 'c'});
774+
});
775+
767776
it('should cache POST with the differing body in object form', () => {
768777
makeRequestAndExpectOne('/test-1', null, {
769778
method: 'POST',

0 commit comments

Comments
 (0)