Skip to content
This repository has been archived by the owner on Feb 10, 2024. It is now read-only.

fix: parseCookies error when cookie value has '=' #339

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 33 additions & 7 deletions src/parse-cookies.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,39 @@
const parseCookies = (s: string): { [key: string]: string } => {
if (s.length === 0) return {};
const parsed: { [key: string]: string } = {};
const pattern = new RegExp("\\s*;\\s*");
s.split(pattern).forEach((i) => {
const [encodedKey, encodedValue] = i.split("=");
const key = decodeURIComponent(encodedKey);
const value = decodeURIComponent(encodedValue);
parsed[key] = value;
});
const pairs = s.split(';')

Choose a reason for hiding this comment

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

Doesn't this just change the bug from breaking with '=' values to breaking with ';' values?


for (let i = 0; i < pairs.length; i++) {

Choose a reason for hiding this comment

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

you are only using i to get the pair. Why not use forEach here?

s.split(';').forEach((pair) => {
  const index = pair.indexOf('='):
  // rest of the logic
});

Copy link
Author

@yes1am yes1am Jan 9, 2023

Choose a reason for hiding this comment

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

In fact, I was copying the code from here and thought it would solve my problem and also pass the all tests: https://github.com/jshttp/cookie/blob/04be428b438605b48ad6af503227b817c07b9b52/index.js#L48-L82

const pair = pairs[i];
const index = pair.indexOf('=')
Copy link

@Akolyte01 Akolyte01 Jan 6, 2023

Choose a reason for hiding this comment

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

this logic could be simplified using split with a limit

const [key, val] = pair.split('=', 2);
if (val === undefined) {
  continue;
}


// skip things that don't look like key=value
if (index < 0) {
continue;
}

const key = pair.substring(0, index).trim()

// only assign once
if (undefined == parsed[key]) {

Choose a reason for hiding this comment

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

I'm curious why you chose to have undefined to be on the left hand side of this statement. Also are you sure == is the correct operator?

image

You could use === instead. This would ensure that your parsed key is actually undefined

let val = pair.substring(index + 1, pair.length).trim()

// quoted values
if (val[0] === '"') {

Choose a reason for hiding this comment

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

doesn't look like the previous implementation does this. What use case is this for?

val = val.slice(1, -1)
}

// catch error: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/decodeURIComponent#catching_errors
try {
const _key = decodeURIComponent(key);
const _val = decodeURIComponent(val);
parsed[_key] = decodeURIComponent(_val);
} catch (error) {
parsed[key] = val
}
}
}

return parsed;
};

Expand Down
1 change: 1 addition & 0 deletions test/parse-cookies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const tests1: Test[] = group("parse-cookies > ", [
assert.deepEqual(parseCookies("a=1;b=2"), { a: "1", b: "2" });
assert.deepEqual(parseCookies("a=1;b=2;c=3"), { a: "1", b: "2", c: "3" });
assert.deepEqual(parseCookies("%3D=%3D"), { "=": "=" });
assert.deepEqual(parseCookies("a=b=c;"), { a: "b=c"});
}),
]);

Expand Down