Skip to content

Commit 34a4e06

Browse files
Copilottnorling
andauthored
fix(msal-browser): CookieStorage tolerates malformed percent-encoded cookies from unrelated third parties (#8549)
- [x] Fix `CookieStorage.getItem` to compare raw (encoded) key before decoding, avoiding `decodeURIComponent` on unrelated cookies - [x] Fix `CookieStorage.getKeys` to split on `=` first and wrap `decodeURIComponent` of the key in try-catch to skip malformed cookies - [x] Add tests for `getItem` and `getKeys` with malformed/invalid percent-encoded cookies - [x] Add test: `getItem` returns raw value when matching MSAL cookie has a malformed percent-encoded value (fallback branch) - [x] Add test: `getKeys` still includes keys when the cookie value (not key) has a malformed percent-encoded sequence - [x] Create changefile for the fix - [x] Fix prettier formatting in source and test files --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tnorling <5307810+tnorling@users.noreply.github.com> Co-authored-by: Thomas Norling <thomas.norling@microsoft.com>
1 parent a800fd2 commit 34a4e06

4 files changed

Lines changed: 121 additions & 12 deletions

File tree

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"type": "patch",
3+
"comment": "Fix CookieStorage.getItem and getKeys throwing URIError when unrelated cookies contain invalid percent-encoded sequences [#7531](https://github.com/AzureAD/microsoft-authentication-library-for-js/pull/7531)",
4+
"packageName": "@azure/msal-browser",
5+
"email": "198982749+Copilot@users.noreply.github.com",
6+
"dependentChangeType": "patch"
7+
}

lib/msal-browser/src/cache/CookieStorage.ts

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,15 +25,21 @@ export class CookieStorage implements IWindowStorage<string> {
2525
}
2626

2727
getItem(key: string): string | null {
28-
const name = `${encodeURIComponent(key)}`;
28+
const name = encodeURIComponent(key);
2929
const cookieList = document.cookie.split(";");
3030
for (let i = 0; i < cookieList.length; i++) {
31-
const cookie = cookieList[i];
32-
const [key, ...rest] = decodeURIComponent(cookie).trim().split("=");
33-
const value = rest.join("=");
34-
35-
if (key === name) {
36-
return value;
31+
const cookie = cookieList[i].trim();
32+
const eqIndex = cookie.indexOf("=");
33+
const rawKey =
34+
eqIndex === -1 ? cookie : cookie.substring(0, eqIndex);
35+
if (rawKey === name) {
36+
const rawValue =
37+
eqIndex === -1 ? "" : cookie.substring(eqIndex + 1);
38+
try {
39+
return decodeURIComponent(rawValue);
40+
} catch {
41+
return rawValue;
42+
}
3743
}
3844
}
3945
return "";
@@ -82,8 +88,15 @@ export class CookieStorage implements IWindowStorage<string> {
8288
const cookieList = document.cookie.split(";");
8389
const keys: Array<string> = [];
8490
cookieList.forEach((cookie) => {
85-
const cookieParts = decodeURIComponent(cookie).trim().split("=");
86-
keys.push(cookieParts[0]);
91+
const trimmed = cookie.trim();
92+
const eqIndex = trimmed.indexOf("=");
93+
const rawKey =
94+
eqIndex === -1 ? trimmed : trimmed.substring(0, eqIndex);
95+
try {
96+
keys.push(decodeURIComponent(rawKey));
97+
} catch {
98+
// Skip cookies with malformed percent-encoded sequences in the key
99+
}
87100
});
88101

89102
return keys;

lib/msal-browser/test/cache/CookieStorage.spec.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,34 @@ describe("CookieStorage tests", () => {
7070
expect(cookieStorage.getItem(msalCacheKey)).toBe(cacheVal);
7171
});
7272

73+
it("getItem returns correct value when unrelated cookie has malformed percent-encoded sequence", () => {
74+
cookieStorage.setItem(msalCacheKey, cacheVal);
75+
// Simulate an unrelated cookie with an invalid UTF-8 percent sequence (e.g. latin1 %E4)
76+
jest.spyOn(document, "cookie", "get").mockReturnValue(
77+
`someTracker=Kraftr%E4ume; ${msalCacheKey}=${encodeURIComponent(
78+
cacheVal
79+
)}`
80+
);
81+
expect(cookieStorage.getItem(msalCacheKey)).toBe(cacheVal);
82+
});
83+
84+
it("getItem does not throw when all unrelated cookies have malformed percent-encoded sequences", () => {
85+
jest.spyOn(document, "cookie", "get").mockReturnValue(
86+
`badCookie1=val%E4ue; badCookie2=%GGinvalid`
87+
);
88+
expect(() => cookieStorage.getItem(msalCacheKey)).not.toThrow();
89+
expect(cookieStorage.getItem(msalCacheKey)).toBe("");
90+
});
91+
92+
it("getItem returns raw value when matching MSAL cookie has malformed percent-encoded value", () => {
93+
const malformedValue = "Kraftr%E4ume";
94+
jest.spyOn(document, "cookie", "get").mockReturnValue(
95+
`${encodeURIComponent(msalCacheKey)}=${malformedValue}`
96+
);
97+
expect(() => cookieStorage.getItem(msalCacheKey)).not.toThrow();
98+
expect(cookieStorage.getItem(msalCacheKey)).toBe(malformedValue);
99+
});
100+
73101
it("removeItem", () => {
74102
cookieStorage.setItem(msalCacheKey, cacheVal);
75103
expect(cookieStorage.getItem(msalCacheKey)).toEqual(cacheVal);
@@ -88,6 +116,29 @@ describe("CookieStorage tests", () => {
88116
]);
89117
});
90118

119+
it("getKeys skips cookies with malformed percent-encoded key sequences", () => {
120+
// Simulate cookies where one key has an invalid percent-encoded sequence
121+
jest.spyOn(document, "cookie", "get").mockReturnValue(
122+
`testKey1=testVal1; bad%E4Key=someVal; testKey2=testVal2`
123+
);
124+
expect(() => cookieStorage.getKeys()).not.toThrow();
125+
const keys = cookieStorage.getKeys();
126+
expect(keys).toContain("testKey1");
127+
expect(keys).toContain("testKey2");
128+
// bad%E4Key has a malformed percent sequence in the key and should be skipped
129+
expect(keys.some((k) => k.includes("bad"))).toBe(false);
130+
});
131+
132+
it("getKeys includes keys when cookie value has malformed percent-encoded sequence", () => {
133+
jest.spyOn(document, "cookie", "get").mockReturnValue(
134+
`testKey1=Kraftr%E4ume; testKey2=testVal2`
135+
);
136+
expect(() => cookieStorage.getKeys()).not.toThrow();
137+
const keys = cookieStorage.getKeys();
138+
expect(keys).toContain("testKey1");
139+
expect(keys).toContain("testKey2");
140+
});
141+
91142
it("containsKey", () => {
92143
cookieStorage.setItem("testKey1", "testVal1");
93144
cookieStorage.setItem("testKey2", "testVal2", 5);

0 commit comments

Comments
 (0)