diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 8a36398..bd2f250 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -16,7 +16,7 @@ jobs: if: ${{ !contains(github.head_ref, 'all-contributors') }} strategy: matrix: - node: [12, 14, 16] + node: [14, 16] runs-on: ubuntu-latest steps: - name: 🛑 Cancel Previous Runs diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 0000000..512ff90 --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,3 @@ +{ + "js/ts.tsdk.path": "node_modules/typescript/lib" +} diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..08de915 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,41 @@ +# Security Policy + +## Reporting a vulnerability + +If you believe you've found a security vulnerability in +`parse-nested-form-data`, please report it privately. **Do not open a public +issue.** + +Preferred channel: +[open a private vulnerability report](https://github.com/milamer/parse-nested-form-data/security/advisories/new) +on this repository. GitHub's private reporting flow keeps the discussion private +until a fix is ready and supports CVE assignment. + +If you cannot use GitHub's private reporting, email **chris@schurr.dev** with +`[security] parse-nested-form-data` in the subject line. + +When reporting, please include: + +- A description of the issue and its impact +- Affected version(s) +- A minimal reproduction (PoC code, input that triggers the bug, expected vs. + actual behavior) +- Any suggested mitigation, if you have one + +You will receive an acknowledgement within a few business days. I'll keep you +updated as the fix progresses and credit you in the published advisory unless +you'd prefer to remain anonymous. + +## Disclosure + +Coordinated disclosure is preferred. The default window is 90 days from initial +report to public disclosure, which can be shortened if a fix ships sooner or +extended by mutual agreement. + +Once a patched release is available on npm, the corresponding GitHub Security +Advisory is published so that downstream users are notified through Dependabot +and `npm audit`. + +## Supported versions + +Only the latest published version on npm receives security fixes. diff --git a/package.json b/package.json index 0a928ce..ca66d39 100644 --- a/package.json +++ b/package.json @@ -44,6 +44,7 @@ "devDependencies": { "@remix-run/web-file": "^3.0.2", "@remix-run/web-form-data": "^3.0.3", + "@types/minimatch": "^3.0.5", "kcd-scripts": "^12.3.0", "rimraf": "^3.0.2", "typescript": "^4.8.4" diff --git a/src/__tests__/index.ts b/src/__tests__/index.ts index e5bbe08..c9d59ff 100644 --- a/src/__tests__/index.ts +++ b/src/__tests__/index.ts @@ -1,6 +1,11 @@ import {FormData} from '@remix-run/web-form-data' import {File} from '@remix-run/web-file' -import {DuplicateKeyError, MixedArrayError, parseFormData} from '../' +import { + DuplicateKeyError, + ForbiddenKeyError, + MixedArrayError, + parseFormData, +} from '../' describe('basic functionality', () => { describe('transform value', () => { @@ -409,3 +414,80 @@ describe('complex examples', () => { }) }) }) + +describe('prototype pollution protection', () => { + const proto = Object.prototype as unknown as {[key: string]: unknown} + afterEach(() => { + // Defensive: clean any test-leaked properties off Object.prototype so a + // failure can't silently affect later tests. + delete proto.polluted + }) + + it('rejects `__proto__` as a top-level key', () => { + const formData = new FormData() + formData.append('__proto__.polluted', 'yes') + expect(() => parseFormData(formData)).toThrowError( + new ForbiddenKeyError('__proto__'), + ) + expect(proto.polluted).toBeUndefined() + }) + + it('rejects `__proto__` as a nested key', () => { + const formData = new FormData() + formData.append('a.__proto__.polluted', 'yes') + expect(() => parseFormData(formData)).toThrowError( + new ForbiddenKeyError('a.__proto__'), + ) + expect(proto.polluted).toBeUndefined() + }) + + it('rejects `__proto__` reached through an array element', () => { + const formData = new FormData() + formData.append('a[0].__proto__.polluted', 'yes') + expect(() => parseFormData(formData)).toThrowError( + new ForbiddenKeyError('a[0].__proto__'), + ) + expect(proto.polluted).toBeUndefined() + }) + + it('rejects `constructor` as a key', () => { + const formData = new FormData() + formData.append('constructor.prototype.polluted', 'yes') + expect(() => parseFormData(formData)).toThrowError( + new ForbiddenKeyError('constructor'), + ) + }) + + it('rejects `prototype` as a key', () => { + const formData = new FormData() + formData.append('prototype.polluted', 'yes') + expect(() => parseFormData(formData)).toThrowError( + new ForbiddenKeyError('prototype'), + ) + }) + + it('rejects `__proto__` as a leaf assignment', () => { + const formData = new FormData() + formData.append('a.__proto__', 'yes') + expect(() => parseFormData(formData)).toThrowError( + new ForbiddenKeyError('a.__proto__'), + ) + }) + + // The cases below pin the invariant for the array branch. Today the regex + // restricts array-index segments to digit characters, so a forbidden key + // can't reach the array branch (the cases parse as an object pathPart with + // a trailing `]` and trip the array/object duplicate-key guard). If the + // regex is ever loosened to accept arbitrary content inside `[...]`, these + // tests will start failing and the array branch will need its own + // forbidden-key check. + it.each(['__proto__', 'constructor', 'prototype'])( + 'does not pollute via `a[%s]`', + key => { + const formData = new FormData() + formData.append(`a[${key}]`, 'yes') + expect(() => parseFormData(formData)).toThrow() + expect(proto.polluted).toBeUndefined() + }, + ) +}) diff --git a/src/index.ts b/src/index.ts index 6c20a77..8f9e7c3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -73,6 +73,29 @@ export class MixedArrayError extends Error { this.key = key } } +/** + * Thrown when a path part would access or assign a property on + * `Object.prototype` (`__proto__`, `constructor`, `prototype`). Rejected + * regardless of whether pollution would actually occur, to keep input + * unambiguous. + * + * @example + * ```ts + * const formData = new FormData() + * formData.append('__proto__.polluted', 'yes') + * parseFormData(formData) + * // throws ForbiddenKeyError('__proto__') + * ``` + */ +export class ForbiddenKeyError extends Error { + key: string + constructor(key: string) { + super(`Forbidden key at path part ${key}`) + this.key = key + } +} + +const FORBIDDEN_OBJECT_KEYS = new Set(['__proto__', 'constructor', 'prototype']) type JsonObject = {[Key in string]?: JsonValue} type JsonArray = Array @@ -333,6 +356,9 @@ function handlePathPart( nextPathValue: JsonValue | undefined, setNextPathValue: (value: JsonValue) => void, ] { + if (FORBIDDEN_OBJECT_KEYS.has(pathPart.path)) { + throw new ForbiddenKeyError(pathPart.pathToPart) + } if (pathPart.type === 'object') { if (Array.isArray(currentPathObject)) { throw new DuplicateKeyError(pathPart.pathToPart)