Skip to content

Commit af74840

Browse files
KhafraDevmcollina
KhafraDev
authored and
mcollina
committed
fix: harden cookie domain, path, and unparsed attribute validation
Signed-off-by: Matteo Collina <hello@matteocollina.com>
1 parent 551138c commit af74840

4 files changed

Lines changed: 162 additions & 9 deletions

File tree

‎lib/web/cookies/util.js‎

Lines changed: 79 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ function validateCookiePath (path) {
105105

106106
if (
107107
code < 0x20 || // exclude CTLs (0-31)
108-
code === 0x7F || // DEL
108+
code > 0x7E || // exclude DEL and non-ascii
109109
code === 0x3B // ;
110110
) {
111111
throw new Error('Invalid cookie path')
@@ -114,16 +114,80 @@ function validateCookiePath (path) {
114114
}
115115

116116
/**
117-
* I have no idea why these values aren't allowed to be honest,
118-
* but Deno tests these. - Khafra
117+
* <let-dig> ::= <letter> | <digit>
118+
*
119+
* <letter> ::= any one of the 52 alphabetic characters A through Z in
120+
* upper case and a through z in lower case
121+
*
122+
* <digit> ::= any one of the ten digits 0 through 9r
123+
*
124+
* @see https://www.rfc-editor.org/rfc/rfc1034#section-3.5
125+
* @param {number} code
126+
*/
127+
function isLetterOrDigit (code) {
128+
return (
129+
(code >= 0x30 && code <= 0x39) || // 0-9
130+
(code >= 0x41 && code <= 0x5A) || // A-Z
131+
(code >= 0x61 && code <= 0x7A) // a-z
132+
)
133+
}
134+
135+
/**
136+
* Validates a cookie domain against the "preferred name syntax".
137+
*
138+
* <domain> ::= <subdomain> | " "
139+
* <subdomain> ::= <label> | <subdomain> "." <label>
140+
* <label> ::= <let-dig> [ [ <ldh-str> ] <let-dig> ]
141+
* <ldh-str> ::= <let-dig-hyp> | <let-dig-hyp> <ldh-str>
142+
* <let-dig-hyp> ::= <let-dig> | "-"
143+
*
144+
* @see https://www.rfc-editor.org/rfc/rfc1034#section-3.5
145+
* @see https://www.rfc-editor.org/rfc/rfc1123#section-2.1
146+
* @see https://www.rfc-editor.org/rfc/rfc1035#section-2.3.4
119147
* @param {string} domain
120148
*/
121149
function validateCookieDomain (domain) {
122-
if (
123-
domain.startsWith('-') ||
124-
domain.endsWith('.') ||
125-
domain.endsWith('-')
126-
) {
150+
// <domain> ::= <subdomain> | " "
151+
if (domain === ' ') {
152+
return
153+
}
154+
155+
if (domain.length > 255) {
156+
throw new Error('Invalid cookie domain')
157+
}
158+
159+
let labelLength = 0
160+
161+
for (let i = 0; i < domain.length; ++i) {
162+
const code = domain.charCodeAt(i)
163+
164+
if (code === 0x2E) {
165+
if (labelLength === 0) {
166+
throw new Error('Invalid cookie domain')
167+
}
168+
169+
if (domain.charCodeAt(i - 1) === 0x2D) { // "-"
170+
throw new Error('Invalid cookie domain')
171+
}
172+
173+
labelLength = 0
174+
continue
175+
}
176+
177+
if (labelLength === 0 && !isLetterOrDigit(code)) {
178+
throw new Error('Invalid cookie domain')
179+
}
180+
181+
if (!isLetterOrDigit(code) && code !== 0x2D) { // "-"
182+
throw new Error('Invalid cookie domain')
183+
}
184+
185+
if (++labelLength > 63) {
186+
throw new Error('Invalid cookie domain')
187+
}
188+
}
189+
190+
if (labelLength === 0 || domain.charCodeAt(domain.length - 1) === 0x2D) { // "-"
127191
throw new Error('Invalid cookie domain')
128192
}
129193
}
@@ -266,7 +330,13 @@ function stringify (cookie) {
266330

267331
const [key, ...value] = part.split('=')
268332

269-
out.push(`${key.trim()}=${value.join('=')}`)
333+
const trimmedKey = key.trim()
334+
const joinedValue = value.join('=')
335+
336+
validateCookieName(trimmedKey)
337+
validateCookieValue(joinedValue)
338+
339+
out.push(`${trimmedKey}=${joinedValue}`)
270340
}
271341

272342
return out.join('; ')

‎test/cookie/cookies.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,22 @@ test('Cookie Domain Validation', () => {
173173
})
174174
})
175175

176+
test('Cookie Unparsed Validation', () => {
177+
const parts = [
178+
'X-Custom=val; HttpOnly',
179+
'Purpose=tracking; SameSite=None; Secure',
180+
'HttpOnly; X-Custom=val'
181+
]
182+
183+
for (const part of parts) {
184+
assert.throws(() => setCookie(new Headers(), {
185+
name: 'Space',
186+
value: 'Cat',
187+
unparsed: [part]
188+
}))
189+
}
190+
})
191+
176192
test('Cookie Delete', () => {
177193
let headers = new Headers()
178194
deleteCookie(headers, 'deno')

‎test/cookie/validate-cookie-domain.js‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
'use strict'
2+
3+
const { test, describe } = require('node:test')
4+
const { throws, doesNotThrow } = require('node:assert')
5+
6+
const { setCookie, Headers } = require('../..')
7+
8+
function set (domain) {
9+
setCookie(new Headers(), { name: 'Space', value: 'Cat', domain })
10+
}
11+
12+
const invalidDomain = new Error('Invalid cookie domain')
13+
14+
describe('cookie domain validation', () => {
15+
test('does not throw for the root domain " "', () => {
16+
doesNotThrow(() => set(' '))
17+
})
18+
19+
test('throws when the name is longer than 255 octets', () => {
20+
throws(() => set('a'.repeat(256)), invalidDomain)
21+
})
22+
23+
test('throws for an empty label', () => {
24+
throws(() => set('.example.com'), invalidDomain)
25+
throws(() => set('example..com'), invalidDomain)
26+
})
27+
28+
test('throws when a label ends with a hyphen before a separator', () => {
29+
throws(() => set('example-.com'), invalidDomain)
30+
})
31+
32+
test('throws when a label starts with a non-letter/digit', () => {
33+
throws(() => set('-example.com'), invalidDomain)
34+
throws(() => set('example.-com'), invalidDomain)
35+
})
36+
37+
test('throws for an interior character that is not a letter, digit, or hyphen', () => {
38+
throws(() => set('exa_mple.com'), invalidDomain)
39+
throws(() => set('example.c*m'), invalidDomain)
40+
})
41+
42+
test('throws when a label is longer than 63 octets', () => {
43+
throws(() => set('a'.repeat(64)), invalidDomain)
44+
throws(() => set('a'.repeat(64) + '.com'), invalidDomain)
45+
})
46+
47+
test('throws for a trailing dot', () => {
48+
throws(() => set('example.com.'), invalidDomain)
49+
})
50+
51+
test('throws for a trailing hyphen', () => {
52+
throws(() => set('example-'), invalidDomain)
53+
throws(() => set('example.com-'), invalidDomain)
54+
})
55+
56+
test('throws when the domain injects an attribute via ";"', () => {
57+
throws(() => set('example.com; SameSite=None'), invalidDomain)
58+
})
59+
})

‎test/cookie/validate-cookie-path.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,14 @@ describe('validateCookiePath', () => {
4848
throws(() => validateCookiePath(';'))
4949
})
5050

51+
test('should throw for non-ascii characters', () => {
52+
throws(() => validateCookiePath('/a\xE9')) // é, 0xE9
53+
throws(() => validateCookiePath('/\x80')) // first C1 control
54+
throws(() => validateCookiePath('/\x9F')) // last C1 control
55+
throws(() => validateCookiePath('/\xFF')) // 0xFF
56+
throws(() => validateCookiePath('\u{1F600}'))
57+
})
58+
5159
test('should pass for a printable character', t => {
5260
strictEqual(validateCookiePath('A'), undefined)
5361
strictEqual(validateCookiePath('Z'), undefined)

0 commit comments

Comments
 (0)