From 74c8ff0886e9c2b98a44a57a7623bc79f8d48197 Mon Sep 17 00:00:00 2001 From: ghiscoding Date: Sun, 4 Oct 2026 14:43:38 -0400 Subject: [PATCH] fix: harden ZIP paths, XML names, and property copying --- .../SECURITY-AUDIT-2026-10-04.md | 62 +++++++++++++++++++ docs/alignment.md | 2 + docs/inserting-pictures.md | 2 + packages/excel-builder-vanilla/README.md | 6 ++ .../excel-builder-vanilla/src/Excel/XMLDOM.ts | 12 ++++ .../src/Excel/__tests__/StyleSheet.spec.ts | 24 ++++++- .../Excel/__tests__/Workbook.security.spec.ts | 7 +++ .../src/Excel/__tests__/Worksheet.spec.ts | 2 +- .../src/Excel/__tests__/XMLDOM.spec.ts | 36 ++++++++++- .../src/__tests__/factory.spec.ts | 9 +++ .../src/__tests__/streaming.spec.ts | 15 +++++ .../src/utilities/__tests__/pick.spec.ts | 28 +++++++++ .../src/utilities/__tests__/zip.spec.ts | 26 ++++++++ .../src/utilities/pick.ts | 10 +-- .../src/utilities/zip.ts | 9 ++- 15 files changed, 239 insertions(+), 11 deletions(-) create mode 100644 .agents/audits/excel-builder-vanilla/SECURITY-AUDIT-2026-10-04.md create mode 100644 packages/excel-builder-vanilla/src/utilities/__tests__/pick.spec.ts create mode 100644 packages/excel-builder-vanilla/src/utilities/__tests__/zip.spec.ts diff --git a/.agents/audits/excel-builder-vanilla/SECURITY-AUDIT-2026-10-04.md b/.agents/audits/excel-builder-vanilla/SECURITY-AUDIT-2026-10-04.md new file mode 100644 index 0000000..c7098ff --- /dev/null +++ b/.agents/audits/excel-builder-vanilla/SECURITY-AUDIT-2026-10-04.md @@ -0,0 +1,62 @@ +# Security audit: `packages/excel-builder-vanilla` + +Date: 2026-10-04 +Audited revision: `8123ba6b277bc18f2a1d835f2a837154cb3515b2` +Scope: library production source and production dependencies. Demo and companion packages excluded. + +## Reassessment + +The findings justify small boundary checks, but the original blanket “Moderate” ratings and remediation breadth overstated what was demonstrated. These are conditional hardening issues: an attacker must control filenames, XML/style names, or selected object keys. No host filesystem write, spreadsheet code execution, or global prototype pollution was demonstrated. + +The first patch duplicated ZIP checks in `addMedia()`, checked XML names at construction and serialization, allocated a Set per XML node with WeakMap tracking, and added style allowlists. Those allowlists omitted `indent`, previously supported by direct/differential alignment export. That approach was unnecessarily broad and introduced compatibility risk. + +## Findings and final fixes + +### ZIP paths: conditional risk during downstream extraction + +`addMedia()` feeds filenames into `/xl/media/${fileName}`. The original ZIP helper only removed the leading slash. The initial probe produced `xl/media/../../escape.png`: this escapes the media directory but resolves inside the extraction root. An additional parent segment (`xl/media/../../../escape.png`) is required to escape that root. Filesystem impact requires a downstream extractor that fails to constrain paths; this library does not extract archives. + +**Fix:** validate paths once in `utilities/zip.ts`, shared by normal and streaming ZIP exports. Reject empty, dot, and parent segments, backslashes, colons, and control characters. Retain the accepted single leading package slash. Tests cover actual media export and custom exporter overrides through both export APIs. + +**Behavior change:** unsafe paths throw during ZIP export, not in `addMedia()`. Safe relative subpaths remain accepted; there is no additional basename-only restriction. + +### XML names: conditional document-structure injection + +Attribute values and ordinary cell text are escaped, but names were interpolated verbatim. The protection key `a="1"/>`. This demonstrated XML structure injection through caller-controlled names, not execution in a spreadsheet application. + +**Fix:** a small shared guard checks element and attribute names during `XMLNode.toString()`, including directly modified attribute dictionaries. It rejects markup delimiters, XML whitespace, and control characters. It intentionally does not implement the full XML Name or OOXML schema grammar. No style allowlists are added, preserving existing alignment attributes and namespaced/Unicode names. + +`setAttribute()` also rejects collisions with node members before assigning or deleting a mirrored attribute, including `__proto__`, `children`, and `toString`. Ordinary attribute aliases, updates, and removal with `null` remain supported. No WeakMap, per-node tracking Set, or repeated construction-time XML validation is needed. + +**Behavior change:** unsafe XML names throw during serialization; reserved attribute names throw in `setAttribute()`. The first patch silently omitted unsupported style keys; the final patch preserves safe names and rejects injection attempts. This is a deliberate validation change. Custom XML strings or custom serializers remain caller-controlled and are not sanitized. + +### `pick()`: local prototype manipulation + +Selecting a JSON object's own `__proto__` key previously changed the returned object's prototype. Global `Object.prototype` was not modified. Direct `object.hasOwnProperty()` calls also failed on null-prototype objects or shadowed methods. + +**Fix:** filter requested own keys with `Object.prototype.hasOwnProperty.call()` and construct the result using `Object.fromEntries()`. This preserves a normal result prototype and copies `__proto__` as an own data property. Missing, inherited, and absent-input properties are omitted. + +## LOC and scope + +Physical line deltas against the audited revision, including comments and blank lines: + +| Category | Initial security patch | Simplified patch | +| --- | ---: | ---: | +| Production source | +68 | +15 | +| Tests (including new files) | +110 | +141 | + +Production growth is reduced by 53 lines (78%); only `XMLDOM.ts`, `pick.ts`, and `zip.ts` change at runtime. Additional test lines exercise reserved names, direct attribute mutation, Unicode/namespaced names, attribute updates/removal, legitimate alignment attributes, and export boundary behavior. Documentation is excluded from these counts. + +No API removal, deprecation, dependency change, or generated declaration change is introduced. Validation changes are documented in the package README and relevant user guides. + +## Validation + +- Original audit baseline: 23 unit test files / 299 tests passed. +- Initial remediation: 25 files / 314 tests passed. +- Simplified patch: 25 files / 328 tests passed; 100% lines (1610/1610), statements, and functions; 93.77% branches. +- Library TypeScript check, changed-file Biome check, JavaScript build, and patch whitespace checks passed. +- The original audit's production dependency advisory check reported no known vulnerabilities; direct production dependency `fflate` was resolved to 0.8.3. This is historical audit evidence, not a guarantee of vulnerability absence. + +## Limits + +No fresh performance benchmark, Excel/LibreOffice application test, downstream extraction test, fuzzing, or dedicated SAST run was performed for this simplification. Removing per-node tracking allocations and duplicate checks reduces work introduced by the first patch, but timing improvements are not claimed. XML checks do not sanitize arbitrary custom exporter output or make all workbook configuration safe for untrusted input. diff --git a/docs/alignment.md b/docs/alignment.md index de873a2..4649fe0 100644 --- a/docs/alignment.md +++ b/docs/alignment.md @@ -40,3 +40,5 @@ artistWorkbook.addWorksheet(albumList); const data = createExcelFile(artistWorkbook); downloader('Artist WB.xlsx', data); ``` + +Alignment and protection keys become XML attribute names. Names containing markup delimiters or control characters cause serialization to fail; names that would overwrite XML node members are rejected when setting the attribute. Existing alignment attributes are preserved without an additional allowlist. diff --git a/docs/inserting-pictures.md b/docs/inserting-pictures.md index 0fb71d5..0dd00e2 100644 --- a/docs/inserting-pictures.md +++ b/docs/inserting-pictures.md @@ -67,6 +67,8 @@ const data = createExcelFile(fruitWorkbook); downloader('Fruit WB.xlsx', data); ``` +Use a plain media filename such as `strawberry.jpg`. Normal and streaming exports reject ZIP paths containing parent/current-directory segments, empty segments, backslashes, colons, or control characters. Unsafe names are rejected during export, not by `addMedia()`. + ### Vite `base64` loader plugin For loading an image as `base64` with ViteJS, you could do it easily with a custom Vite loader plugin. diff --git a/packages/excel-builder-vanilla/README.md b/packages/excel-builder-vanilla/README.md index 242682d..7663427 100644 --- a/packages/excel-builder-vanilla/README.md +++ b/packages/excel-builder-vanilla/README.md @@ -85,6 +85,12 @@ albumList.setData([ ]); ``` +### Export validation + +Normal and streaming ZIP exports reject entry paths containing `.` or `..` segments, empty segments, backslashes, colons, or control characters. A single leading package slash is accepted. This also applies to media filenames and custom `generateFiles()` output; validation happens when exporting, not in `addMedia()`. + +XML serialization rejects element and attribute names containing markup delimiters or control characters, including malformed style keys. `XMLNode.setAttribute()` rejects names that would overwrite node members, such as `__proto__`, `children`, or `toString`. Ordinary attribute updates and removal with `null` remain supported. These checks do not validate the full XML/OOXML schema or sanitize custom XML strings supplied by exporters. + ## Changelog [CHANGELOG](https://github.com/ghiscoding/excel-builder-vanilla/blob/main/packages/excel-builder-vanilla/CHANGELOG.md) diff --git a/packages/excel-builder-vanilla/src/Excel/XMLDOM.ts b/packages/excel-builder-vanilla/src/Excel/XMLDOM.ts index b72c90e..a2fe2b9 100644 --- a/packages/excel-builder-vanilla/src/Excel/XMLDOM.ts +++ b/packages/excel-builder-vanilla/src/Excel/XMLDOM.ts @@ -10,6 +10,13 @@ type XMLNodeOption = { type?: string; }; +// Block markup delimiters; this is not a full XML Name grammar check. +function assertSafeXMLName(name: string) { + if (!name || /[ <>&"'=/\p{Cc}]/u.test(name)) { + throw new Error(`Unsafe XML name: ${name}`); + } +} + export class XMLDOM { static readonly declaration = ''; documentElement: XMLNode; @@ -99,9 +106,11 @@ export class XMLNode { /** Serializes this element, its attributes, and children to XML. */ toString() { + assertSafeXMLName(this.nodeName); let string = `<${this.nodeName}`; for (const attr in this.attributes) { if (Object.prototype.hasOwnProperty.call(this.attributes, attr)) { + assertSafeXMLName(attr); string = `${string} ${attr}="${htmlEscape(this.attributes[attr])}"`; } } @@ -137,6 +146,9 @@ export class XMLNode { /** Sets an attribute, or removes it when the value is `null`. */ setAttribute(name: string, val: any) { + if (name in this && !Object.prototype.hasOwnProperty.call(this.attributes, name)) { + throw new Error(`Reserved XML attribute: ${name}`); + } if (val === null) { delete this.attributes[name]; delete (this as any)[name]; diff --git a/packages/excel-builder-vanilla/src/Excel/__tests__/StyleSheet.spec.ts b/packages/excel-builder-vanilla/src/Excel/__tests__/StyleSheet.spec.ts index 00abcdf..e2170a6 100644 --- a/packages/excel-builder-vanilla/src/Excel/__tests__/StyleSheet.spec.ts +++ b/packages/excel-builder-vanilla/src/Excel/__tests__/StyleSheet.spec.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from 'vitest'; import { StyleSheet } from '../StyleSheet.js'; -import { XMLNode } from '../XMLDOM.js'; +import { XMLDOM, XMLNode } from '../XMLDOM.js'; describe('StyleSheet', () => { test('createFormat with empty object', () => { @@ -55,6 +55,28 @@ describe('StyleSheet', () => { expect(protection).toBeDefined(); }); + test('rejects markup in alignment and protection keys during XML export', () => { + const ss = new StyleSheet(); + const doc = new XMLDOM(null, 'root'); + const alignment = ss.exportAlignment(doc, { + horizontal: 'center', + 'id="1"/> alignment.toString()).toThrow('Unsafe XML name'); + expect(() => protection.toString()).toThrow('Unsafe XML name'); + }); + + test('preserves alignment attributes without an additional allowlist', () => { + const ss = new StyleSheet(); + const doc = new XMLDOM(null, 'root'); + expect(ss.exportAlignment(doc, { indent: 2, horizontal: 'center' }).toString()).toBe(''); + }); + describe('StyleSheet.createFontStyle()', () => { test('createFontStyle superscript', () => { const ss = new StyleSheet(); diff --git a/packages/excel-builder-vanilla/src/Excel/__tests__/Workbook.security.spec.ts b/packages/excel-builder-vanilla/src/Excel/__tests__/Workbook.security.spec.ts index ad32e5e..5c40055 100644 --- a/packages/excel-builder-vanilla/src/Excel/__tests__/Workbook.security.spec.ts +++ b/packages/excel-builder-vanilla/src/Excel/__tests__/Workbook.security.spec.ts @@ -1,5 +1,6 @@ import { afterEach, describe, expect, it } from 'vitest'; +import { createExcelFile } from '../../factory.js'; import { Workbook } from '../Workbook.js'; describe('Workbook prototype pollution protection', () => { @@ -33,4 +34,10 @@ describe('Workbook prototype pollution protection', () => { expect(Object.getOwnPropertyDescriptor(workbook.printTitles!, 'constructor')?.value).toEqual({ top: 3 }); expect(Object.getOwnPropertyDescriptor(workbook.printTitles!, 'toString')?.value).toEqual({ left: 'B' }); }); + + it('rejects unsafe media paths at ZIP export', async () => { + const workbook = new Workbook(); + workbook.addMedia('image', '../../../outside.png', 'AA=='); + await expect(createExcelFile(workbook, 'Uint8Array')).rejects.toThrow('Invalid ZIP entry path'); + }); }); diff --git a/packages/excel-builder-vanilla/src/Excel/__tests__/Worksheet.spec.ts b/packages/excel-builder-vanilla/src/Excel/__tests__/Worksheet.spec.ts index 1304313..e35c7e2 100644 --- a/packages/excel-builder-vanilla/src/Excel/__tests__/Worksheet.spec.ts +++ b/packages/excel-builder-vanilla/src/Excel/__tests__/Worksheet.spec.ts @@ -198,7 +198,7 @@ describe('Excel/Worksheet', () => { ws.setPageMargin({ bottom: 120, footer: 21, header: 22, left: 0, right: 33, top: 8 }); const xmlDom = new XMLDOM('something', 'root'); - const xmlNode = new XMLNode({ nodeName: 'some name' }); + const xmlNode = new XMLNode({ nodeName: 'some_name' }); ws.exportPageSettings(xmlDom, xmlNode); expect(ws._margin).toEqual({ bottom: 120, footer: 21, header: 22, left: 0, right: 33, top: 8 }); }); diff --git a/packages/excel-builder-vanilla/src/Excel/__tests__/XMLDOM.spec.ts b/packages/excel-builder-vanilla/src/Excel/__tests__/XMLDOM.spec.ts index dc7bb5c..b2f60b2 100644 --- a/packages/excel-builder-vanilla/src/Excel/__tests__/XMLDOM.spec.ts +++ b/packages/excel-builder-vanilla/src/Excel/__tests__/XMLDOM.spec.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest'; -import { XMLDOM } from '../XMLDOM.js'; +import { XMLDOM, XMLNode } from '../XMLDOM.js'; describe('basic DOM simulator for web workers', () => { describe('XMLDOM', () => { @@ -48,6 +48,40 @@ describe('basic DOM simulator for web workers', () => { const node = XMLDOM.Node.Create({ type: 'XML', nodeName: 'item', attributes: { id: 'sample' } }); expect(node?.toString()).toBe(''); }); + + it('rejects markup in element and attribute names at serialization', () => { + const doc = new XMLDOM(ns, nodeName); + expect(() => doc.createElement('item/> node.toString()).toThrow('Unsafe XML name'); + node.attributes = { 'direct injection="': 'value' }; + expect(() => node.toString()).toThrow('Unsafe XML name'); + expect(() => doc.createElement('').toString()).toThrow('Unsafe XML name'); + }); + + it.each(['__proto__', 'constructor', 'attributes', 'nodeName', 'toString', 'children', 'appendChild'])( + 'rejects attribute %s before it can overwrite node internals', + name => { + const node = new XMLNode({ nodeName: 'item' }); + expect(() => node.setAttribute(name, { polluted: true })).toThrow('Reserved XML attribute'); + expect(() => node.setAttribute(name, null)).toThrow('Reserved XML attribute'); + expect(Object.getPrototypeOf(node)).toBe(XMLNode.prototype); + expect(Object.getPrototypeOf(node.attributes)).toBe(Object.prototype); + expect(node.toString()).toBe(''); + }, + ); + + it('preserves Unicode names, namespaced attributes, and attribute aliases', () => { + const node = new XMLNode({ nodeName: 'étiquette' }); + node.setAttribute('r:id', 'one'); + node.setAttribute('r:id', 'two'); + expect((node as any)['r:id']).toBe('two'); + expect(node.toString()).toBe('<étiquette r:id="two"/>'); + node.setAttribute('r:id', null); + expect((node as any)['r:id']).toBeUndefined(); + expect(node.toString()).toBe('<étiquette/>'); + }); }); describe('XMLDOM.XMLNode', () => { diff --git a/packages/excel-builder-vanilla/src/__tests__/factory.spec.ts b/packages/excel-builder-vanilla/src/__tests__/factory.spec.ts index fd9d076..2a1a41b 100644 --- a/packages/excel-builder-vanilla/src/__tests__/factory.spec.ts +++ b/packages/excel-builder-vanilla/src/__tests__/factory.spec.ts @@ -68,6 +68,15 @@ describe('ExcelExportService', () => { expect(output).includes('workbook.xml'); }); + it('rejects unsafe ZIP entry paths from custom workbook exporters', async () => { + const workbook = createWorkbook(); + vi.spyOn(workbook, 'generateFiles').mockResolvedValueOnce({ + '/xl/media/../../../outside.png': 'AA==', + }); + + await expect(createExcelFile(workbook, 'Uint8Array')).rejects.toThrow('Invalid ZIP entry path'); + }); + it('should return an image as an Uint8Array instance when calling the method that includes an image', async () => { const blueSquareBase64 = 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg=='; diff --git a/packages/excel-builder-vanilla/src/__tests__/streaming.spec.ts b/packages/excel-builder-vanilla/src/__tests__/streaming.spec.ts index 8ceb0a1..1c15ad5 100644 --- a/packages/excel-builder-vanilla/src/__tests__/streaming.spec.ts +++ b/packages/excel-builder-vanilla/src/__tests__/streaming.spec.ts @@ -79,6 +79,21 @@ describe('Streaming API', () => { expect(Object.keys(files)).toContain('xl/worksheet.xml'); expect(Object.keys(files)).toContain('xl/media/image.png'); }); + + it('rejects unsafe ZIP entry paths from custom workbook exporters', async () => { + const workbook: any = { + async generateFiles() { + return { '/xl/media/../../../outside.png': 'AA==' }; + }, + }; + const { nodeExcelStream } = await import('../streaming.js'); + + await expect(async () => { + for await (const _chunk of nodeExcelStream(workbook)) { + // Consume the stream to trigger ZIP entry validation. + } + }).rejects.toThrow('Invalid ZIP entry path'); + }); }); it('throws on unsupported environment', () => { diff --git a/packages/excel-builder-vanilla/src/utilities/__tests__/pick.spec.ts b/packages/excel-builder-vanilla/src/utilities/__tests__/pick.spec.ts new file mode 100644 index 0000000..faa132e --- /dev/null +++ b/packages/excel-builder-vanilla/src/utilities/__tests__/pick.spec.ts @@ -0,0 +1,28 @@ +import { describe, expect, it } from 'vitest'; + +import { pick } from '../pick.js'; + +describe('pick', () => { + it('copies an own __proto__ property without changing the result prototype', () => { + const input = JSON.parse('{"__proto__":{"polluted":true},"value":1}'); + const result = pick(input, ['__proto__', 'value']); + + expect(Object.getPrototypeOf(result)).toBe(Object.prototype); + expect(Object.prototype.hasOwnProperty.call(result, '__proto__')).toBe(true); + expect(Object.getOwnPropertyDescriptor(result, '__proto__')?.value).toEqual({ polluted: true }); + expect(result.value).toBe(1); + }); + + it('reads own properties from null-prototype objects and objects with shadowed hasOwnProperty', () => { + const nullPrototypeInput = Object.assign(Object.create(null), { value: 1 }); + const shadowedMethodInput = { hasOwnProperty: false, value: 2 }; + + expect(pick(nullPrototypeInput, ['value'])).toEqual({ value: 1 }); + expect(pick(shadowedMethodInput, ['value'])).toEqual({ value: 2 }); + }); + it('omits missing and inherited properties and accepts absent input', () => { + expect(pick({ value: 1 }, ['missing', 'toString'])).toEqual({}); + expect(pick(null, ['value'])).toEqual({}); + expect(pick(undefined, ['value'])).toEqual({}); + }); +}); diff --git a/packages/excel-builder-vanilla/src/utilities/__tests__/zip.spec.ts b/packages/excel-builder-vanilla/src/utilities/__tests__/zip.spec.ts new file mode 100644 index 0000000..2f1ca68 --- /dev/null +++ b/packages/excel-builder-vanilla/src/utilities/__tests__/zip.spec.ts @@ -0,0 +1,26 @@ +import { describe, expect, it } from 'vitest'; + +import { zipPath } from '../zip.js'; + +describe('zipPath', () => { + it('removes a leading package slash from a valid path', () => { + expect(zipPath('/xl/workbook.xml')).toBe('xl/workbook.xml'); + expect(zipPath('xl/media/é image.png')).toBe('xl/media/é image.png'); + }); + + it.each([ + '', + '/', + '//absolute.txt', + './workbook.xml', + '../outside.txt', + '/xl/media/../../../outside.png', + '/xl/media/../../outside.png', + 'xl\\media\\outside.png', + 'C:/outside.txt', + 'xl//workbook.xml', + 'xl/media/\u0000outside.png', + ])('rejects unsafe ZIP entry path %j', path => { + expect(() => zipPath(path)).toThrow('Invalid ZIP entry path'); + }); +}); diff --git a/packages/excel-builder-vanilla/src/utilities/pick.ts b/packages/excel-builder-vanilla/src/utilities/pick.ts index ee0c88e..7ed4dd5 100644 --- a/packages/excel-builder-vanilla/src/utilities/pick.ts +++ b/packages/excel-builder-vanilla/src/utilities/pick.ts @@ -1,9 +1,5 @@ /** Copies the requested own properties from an object into a new object. */ -export function pick(object: any, keys: string[]) { - return keys.reduce((obj: any, key: string) => { - if (object?.hasOwnProperty(key)) { - obj[key] = object[key]; - } - return obj; - }, {}); +export function pick(object: any, keys: string[]): any { + const ownKeys = keys.filter(key => object != null && Object.prototype.hasOwnProperty.call(object, key)); + return Object.fromEntries(ownKeys.map(key => [key, object[key]])); } diff --git a/packages/excel-builder-vanilla/src/utilities/zip.ts b/packages/excel-builder-vanilla/src/utilities/zip.ts index 4a61cc0..c062ba8 100644 --- a/packages/excel-builder-vanilla/src/utilities/zip.ts +++ b/packages/excel-builder-vanilla/src/utilities/zip.ts @@ -3,5 +3,12 @@ import { strToU8 } from 'fflate'; import { base64ToUint8Array } from './base64.js'; export const isXmlPath = (path: string) => /\.(?:xml|rels)$/.test(path); -export const zipPath = (path: string) => path.replace(/^\//, ''); +export const zipPath = (path: string) => { + const entryPath = path.startsWith('/') ? path.slice(1) : path; + const invalidSegment = entryPath.split('/').some(segment => segment === '' || segment === '.' || segment === '..'); + if (invalidSegment || /[\\:\p{Cc}]/u.test(entryPath)) { + throw new Error(`Invalid ZIP entry path: ${path}`); + } + return entryPath; +}; export const toZipData = (path: string, content: string) => (isXmlPath(path) ? strToU8(content) : base64ToUint8Array(content));