From cb42cbb446fe0bde995f2b915807315f115b1089 Mon Sep 17 00:00:00 2001 From: Bob Senoff Date: Wed, 2 Sep 2026 04:39:36 -0500 Subject: [PATCH] XLS-352: pin every zip entry's mtime via an optional zipEntryDate option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Zip-entry mtime (`date`) is a per-entry archiver/JSZip option that defaults to `new Date()` at append time; a module-level `zip: {date}` option is a no-op (never forwarded), so output was non-deterministic at DOS-format 2-second granularity (XLS-350's "coin flip" determinism issue). Threads a new `zipEntryDate?: Date` option through both writer paths: - streaming `stream.xlsx.WorkbookWriter`, via a new `_append(data, options)` choke point every internal `zip.append` call now goes through. - buffered `Workbook.xlsx.writeBuffer`/`writeFile`, via a new `ZipWriter._pinEntryDates()` called at `finalize()` (right before `zip.generateAsync`), which sweeps ALL `this.zip.files` — including JSZip's auto-created folder entries (`xl/`, `xl/worksheets/`, ...) that never pass through `.append` and would otherwise keep the wall clock. Left undefined (the default), every entry keeps the wall clock exactly as before — existing callers see no change. This is the durable, in-fork replacement for the server-side `PinnedWorkbookWriter` subclass, which reaches past the published exceljs surface to monkey-patch `zip.append` from outside the base constructor. Once this lands and the server's @protobi/exceljs pin is bumped, that subclass becomes deletable in favor of passing `zipEntryDate` directly. New spec (spec/integration/issues/issue-xls352-zip-entry-mtime-pin.spec.js) covers both writer paths with 3 arms each: byte-identity across a real >2s delay when pinned, per-entry mtime stamping, and a positive control proving the option is causally responsible (bytes differ / wall clock entries when omitted). 6/6 passing; full unit (886/1 pending) and integration (208) suites pass with no regressions. Also fixes two pre-existing lint-config-drift blockers this diff's lint-staged run surfaced on the touched files (unrelated to the zipEntryDate change itself, and out of scope for the standing fix/prettier-eslint-config-drift branch which reformats the whole repo): - workbook-writer.js: `require('../../xlsx/xml/theme1.js')` violated import/extensions (real, pre-existing eslint error independent of prettier). - Added `// prettier-ignore` above 7 other pre-existing statements (in workbook-writer.js, zip-stream.js, xlsx.js) that Prettier's current version reformats into a shape ESLint's `comma-dangle`/ `space-before-function-paren` rules reject — confirmed via a pristine origin/master copy that this reformat-then-reject failure is 100% pre-existing and unrelated to any line this diff touches. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01QY55Be9GCxpDat9XQhzeGZ --- index.d.ts | 8 + lib/stream/xlsx/workbook-writer.js | 49 ++++-- lib/utils/zip-stream.js | 16 ++ lib/xlsx/xlsx.js | 16 +- package.json | 2 +- .../issue-xls352-zip-entry-mtime-pin.spec.js | 139 ++++++++++++++++++ 6 files changed, 211 insertions(+), 19 deletions(-) create mode 100644 spec/integration/issues/issue-xls352-zip-entry-mtime-pin.spec.js diff --git a/index.d.ts b/index.d.ts index cae3394bd..63754c40b 100644 --- a/index.d.ts +++ b/index.d.ts @@ -1936,6 +1936,14 @@ export namespace stream { * Styles can add some performance overhead. Default is false */ useStyles: boolean; + + /** + * XLS-352: an optional Date stamped as the mtime of EVERY zip entry the writer emits, so + * the output is byte-deterministic across runs (a per-entry `date` otherwise defaults to + * `new Date()`). Applies to both the streaming WorkbookWriter and the buffered + * `xlsx.write`/`writeBuffer` path. When omitted, entries keep the wall clock. + */ + zipEntryDate?: Date; } interface ArchiverZipOptions { diff --git a/lib/stream/xlsx/workbook-writer.js b/lib/stream/xlsx/workbook-writer.js index 2d97cec88..53364171c 100644 --- a/lib/stream/xlsx/workbook-writer.js +++ b/lib/stream/xlsx/workbook-writer.js @@ -17,7 +17,7 @@ const SharedStringsXform = require('../../xlsx/xform/strings/shared-strings-xfor const WorksheetWriter = require('./worksheet-writer'); -const theme1Xml = require('../../xlsx/xml/theme1.js'); +const theme1Xml = require('../../xlsx/xml/theme1'); class WorkbookWriter { constructor(options) { @@ -44,6 +44,13 @@ class WorkbookWriter { this.zipOptions = options.zip; + // XLS-352: an optional Date stamped as the mtime of EVERY zip entry this writer appends. + // `date` is a per-entry archiver option (default `new Date()`); archiver ignores any + // module-level `zip: {date}`, so byte-deterministic output requires forwarding this value to + // each `zip.append` call — which the `_append` choke point below does. Left undefined, entries + // keep the wall clock exactly as before, so callers that do not pass it see no change. + this.zipEntryDate = options.zipEntryDate; + this.media = []; this.commentRefs = []; @@ -65,9 +72,21 @@ class WorkbookWriter { return this._definedNames; } + // XLS-352 choke point: every zip entry is appended through here, so a caller-supplied + // `zipEntryDate` (see the constructor) is stamped as the entry's mtime. When it is undefined — + // or the individual call already carries its own `date` — the options pass through untouched and + // archiver's default (the wall clock) is preserved. This is the durable, in-fork replacement for + // the server's `PinnedWorkbookWriter` subclass, which wrapped `zip.append` from outside. + _append(data, options) { + if (this.zipEntryDate !== undefined && (!options || options.date === undefined)) { + return this.zip.append(data, {...options, date: this.zipEntryDate}); + } + return this.zip.append(data, options); + } + _openStream(path) { const stream = new StreamBuf({bufSize: 65536, batch: true}); - this.zip.append(stream, {name: path}); + this._append(stream, {name: path}); stream.on('finish', () => { stream.emit('zipped'); }); @@ -75,6 +94,7 @@ class WorkbookWriter { } _commitWorksheets() { + // prettier-ignore const commitWorksheet = function(worksheet) { if (!worksheet.committed) { return new Promise(resolve => { @@ -144,6 +164,7 @@ class WorkbookWriter { if (options.tabColor) { // eslint-disable-next-line no-console console.trace('tabColor option has moved to { properties: tabColor: {...} }'); + // prettier-ignore options.properties = Object.assign( { tabColor: options.tabColor, @@ -187,14 +208,14 @@ class WorkbookWriter { addStyles() { return new Promise(resolve => { - this.zip.append(this.styles.xml, {name: 'xl/styles.xml'}); + this._append(this.styles.xml, {name: 'xl/styles.xml'}); resolve(); }); } addThemes() { return new Promise(resolve => { - this.zip.append(theme1Xml, {name: 'xl/theme/theme1.xml'}); + this._append(theme1Xml, {name: 'xl/theme/theme1.xml'}); resolve(); }); } @@ -207,7 +228,7 @@ class WorkbookWriter { {Id: 'rId2', Type: RelType.CoreProperties, Target: 'docProps/core.xml'}, {Id: 'rId3', Type: RelType.ExtenderProperties, Target: 'docProps/app.xml'}, ]); - this.zip.append(xml, {name: '/_rels/.rels'}); + this._append(xml, {name: '/_rels/.rels'}); resolve(); }); } @@ -222,12 +243,13 @@ class WorkbookWriter { }; const xform = new ContentTypesXform(); const xml = xform.toXml(model); - this.zip.append(xml, {name: '[Content_Types].xml'}); + this._append(xml, {name: '[Content_Types].xml'}); resolve(); }); } addMedia() { + // prettier-ignore return Promise.all( this.media.map(medium => { if (medium.type === 'image') { @@ -236,12 +258,12 @@ class WorkbookWriter { return this.zip.file(medium.filename, {name: filename}); } if (medium.buffer) { - return this.zip.append(medium.buffer, {name: filename}); + return this._append(medium.buffer, {name: filename}); } if (medium.base64) { const dataimg64 = medium.base64; const content = dataimg64.substring(dataimg64.indexOf(',') + 1); - return this.zip.append(content, {name: filename, base64: true}); + return this._append(content, {name: filename, base64: true}); } } throw new Error('Unsupported media'); @@ -256,7 +278,7 @@ class WorkbookWriter { }; const xform = new AppXform(); const xml = xform.toXml(model); - this.zip.append(xml, {name: 'docProps/app.xml'}); + this._append(xml, {name: 'docProps/app.xml'}); resolve(); }); } @@ -265,7 +287,7 @@ class WorkbookWriter { return new Promise(resolve => { const coreXform = new CoreXform(); const xml = coreXform.toXml(this); - this.zip.append(xml, {name: 'docProps/core.xml'}); + this._append(xml, {name: 'docProps/core.xml'}); resolve(); }); } @@ -275,7 +297,7 @@ class WorkbookWriter { return new Promise(resolve => { const sharedStringsXform = new SharedStringsXform(); const xml = sharedStringsXform.toXml(this.sharedStrings); - this.zip.append(xml, {name: '/xl/sharedStrings.xml'}); + this._append(xml, {name: '/xl/sharedStrings.xml'}); resolve(); }); } @@ -308,13 +330,12 @@ class WorkbookWriter { return new Promise(resolve => { const xform = new RelationshipsXform(); const xml = xform.toXml(relationships); - this.zip.append(xml, {name: '/xl/_rels/workbook.xml.rels'}); + this._append(xml, {name: '/xl/_rels/workbook.xml.rels'}); resolve(); }); } addWorkbook() { - const {zip} = this; const model = { worksheets: this._worksheets.filter(Boolean), definedNames: this._definedNames.model, @@ -326,7 +347,7 @@ class WorkbookWriter { return new Promise(resolve => { const xform = new WorkbookXform(); xform.prepare(model); - zip.append(xform.toXml(model), {name: '/xl/workbook.xml'}); + this._append(xform.toXml(model), {name: '/xl/workbook.xml'}); resolve(); }); } diff --git a/lib/utils/zip-stream.js b/lib/utils/zip-stream.js index 96efd8e7b..7bd278587 100644 --- a/lib/utils/zip-stream.js +++ b/lib/utils/zip-stream.js @@ -10,6 +10,7 @@ const {stringToBuffer} = require('./browser-buffer-encode'); class ZipWriter extends events.EventEmitter { constructor(options) { super(); + // prettier-ignore this.options = Object.assign( { type: 'nodebuffer', @@ -35,7 +36,22 @@ class ZipWriter extends events.EventEmitter { } } + // XLS-352: stamp a caller-supplied `entryDate` as the mtime of EVERY entry before generating, + // so the buffered writer's output is byte-deterministic. Doing it here — the single generate + // choke point — rather than per-append is deliberate: JSZip AUTO-CREATES folder entries (`xl/`, + // `xl/worksheets/`, ...) that never pass through `append`, and each defaults to `new Date()`. + // Pinning at append would leave those folder entries on the wall clock (two runs then differ in + // exactly their headers). Iterating `this.zip.files` here covers the file entries and the + // auto-created folder entries alike. Undefined leaves JSZip's default (the wall clock) untouched. + _pinEntryDates() { + if (this.options.entryDate === undefined) return; + Object.keys(this.zip.files).forEach(name => { + this.zip.files[name].date = this.options.entryDate; + }); + } + async finalize() { + this._pinEntryDates(); const content = await this.zip.generateAsync(this.options); this.stream.end(content); this.emit('finish'); diff --git a/lib/xlsx/xlsx.js b/lib/xlsx/xlsx.js index 0d66a8c73..81226b530 100644 --- a/lib/xlsx/xlsx.js +++ b/lib/xlsx/xlsx.js @@ -164,6 +164,7 @@ class XLSX { const cacheId = cacheIdMatch[1]; // Find the pivot cache definition relationship // pivotTable.rels should have a relationship to the cache definition + // prettier-ignore const cacheDefRel = pivotTable.rels.find( rel => rel.Type === RelType.PivotCacheDefinition ); @@ -371,6 +372,7 @@ class XLSX { * @deprecated since version 4.0. You should use `#read` instead. Please follow upgrade instruction: https://github.com/exceljs/exceljs/blob/master/UPGRADE-4.0.md */ createInputStream() { + // prettier-ignore throw new Error( '`XLSX#createInputStream` is deprecated. You should use `XLSX#read` instead. This method will be removed in version 5.0. Please follow upgrade instruction: https://github.com/exceljs/exceljs/blob/master/UPGRADE-4.0.md' ); @@ -571,6 +573,7 @@ class XLSX { await this._processPivotCacheDefinitionEntry(entry, model, match[1]); break; } + // prettier-ignore match = entryName.match(/xl\/pivotCache\/_rels\/(pivotCacheDefinition\d+)[.]xml[.]rels/); if (match) { await this._processPivotCacheDefinitionRelsEntry(stream, model, match[1]); @@ -617,6 +620,7 @@ class XLSX { // Write async addMedia(zip, model) { + // prettier-ignore await Promise.all( model.media.map(async medium => { if (medium.type === 'image') { @@ -702,7 +706,8 @@ class XLSX { addPivotTables(zip, model) { const hasProgrammaticPivots = model.pivotTables && model.pivotTables.length > 0; - const hasPreservedPivots = model.preservedPivotTables && + const hasPreservedPivots = + model.preservedPivotTables && Object.keys(model.preservedPivotTables.pivotTables || {}).length > 0; if (!hasProgrammaticPivots && !hasPreservedPivots) return; @@ -840,8 +845,8 @@ class XLSX { } addCharts(zip, model) { - const hasPreservedCharts = model.preservedChartsXml && - Object.keys(model.preservedChartsXml).length > 0; + const hasPreservedCharts = + model.preservedChartsXml && Object.keys(model.preservedChartsXml).length > 0; if (!hasPreservedCharts) return; @@ -1104,7 +1109,10 @@ class XLSX { async write(stream, options) { options = options || {}; const {model} = this.workbook; - const zip = new ZipStream.ZipWriter(options.zip); + // XLS-352: forward an optional `zipEntryDate` into the zip writer so every buffered entry's mtime + // is stamped with it (the buffered twin of the streaming writer's per-entry pin). Undefined leaves + // JSZip's default (the wall clock) untouched, so callers that do not pass it see no change. + const zip = new ZipStream.ZipWriter({...options.zip, entryDate: options.zipEntryDate}); zip.pipe(stream); this.prepareModel(model, options); diff --git a/package.json b/package.json index da5de1b5a..d513c900e 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@protobi/exceljs", - "version": "4.4.0-protobi.10", + "version": "4.4.0-protobi.11", "description": "Excel Workbook Manager - Temporary fork with pivot table enhancements and bug fixes pending upstream merge", "private": false, "license": "MIT", diff --git a/spec/integration/issues/issue-xls352-zip-entry-mtime-pin.spec.js b/spec/integration/issues/issue-xls352-zip-entry-mtime-pin.spec.js new file mode 100644 index 000000000..407a28ed8 --- /dev/null +++ b/spec/integration/issues/issue-xls352-zip-entry-mtime-pin.spec.js @@ -0,0 +1,139 @@ +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const JSZip = require('jszip'); + +const ExcelJS = verquire('exceljs'); + +// XLS-352 — teach the fork to pin every zip entry's mtime itself, so the server's +// `PinnedWorkbookWriter` subclass (which reached past the published surface to wrap `zip.append` +// from inside the base constructor) can be deleted. +// +// The defect: `date` is a PER-ENTRY zip option (archiver's `data.date` / JSZip's file `date`), +// defaulting to `new Date()` at append time. A module-level `zip: {date}` is a no-op — archiver +// never forwards it to an entry header. So each entry's local-file-header mtime carries the wall +// clock at the DOS format's two-second granularity, and two builds are byte-identical only when +// they happen to land in the same two-second bucket (XLS-350's coin-flip). +// +// The fix adds one option, `zipEntryDate`, threaded to EVERY `zip.append` on BOTH writer paths: +// - streaming: `stream.xlsx.WorkbookWriter` — via the `_append` choke point. +// - buffered: `Workbook.xlsx.writeBuffer/writeFile` — via `ZipWriter.append` (JSZip `.file`). +// +// This is pinned three ways per path, because each alone is a false green: +// 1. Byte-identity across a real DOS tick — the determinism property itself. +// 2. Every entry's parsed mtime equals the pin — proves the date reached the entry headers, not +// just that two runs happened to match. +// 3. POSITIVE CONTROL: WITHOUT the option the output differs from the pinned output AND the +// entries carry the wall clock (a recent year), so the option is demonstrably what pins them. +// Revert the fork fix and arms 1+2 stay green vacuously; this arm is what reddens. + +// A fixed pin on an even two-second boundary (DOS granularity) and at midday, so a CI timezone +// offset shifts the hour but never the calendar date the readback asserts. +const PIN = new Date(2000, 5, 15, 12, 0, 0); + +const sleep = ms => new Promise(resolve => setTimeout(resolve, ms)); + +let tmpCounter = 0; +function tmpFile() { + return path.join(os.tmpdir(), `xls352-stream-${process.pid}-${Date.now()}-${tmpCounter++}.xlsx`); +} + +// Build a workbook through the STREAMING writer and return its bytes. `extraOptions` carries (or +// omits) `zipEntryDate`. `created`/`modified` are pinned too so docProps/core.xml — entry CONTENT, +// the other half of byte-identity — does not itself carry the wall clock and mask the entry-mtime +// property under test. +async function buildStreaming(extraOptions) { + const file = tmpFile(); + const wb = new ExcelJS.stream.xlsx.WorkbookWriter({ + stream: fs.createWriteStream(file, {flags: 'w'}), + useStyles: true, + useSharedStrings: false, + ...extraOptions, + }); + wb.created = PIN; + wb.modified = PIN; + const ws = wb.addWorksheet('S'); + ws.addRow(['hello', 42]).commit(); + ws.commit(); + await wb.commit(); + const buffer = fs.readFileSync(file); + fs.unlinkSync(file); + return buffer; +} + +// Build the same workbook through the BUFFERED writer (`writeBuffer`) and return its bytes. +async function buildBuffered(extraOptions) { + const wb = new ExcelJS.Workbook(); + wb.created = PIN; + wb.modified = PIN; + const ws = wb.addWorksheet('S'); + ws.addRow(['hello', 42]); + return wb.xlsx.writeBuffer(extraOptions); +} + +// Every entry, INCLUDING the folder entries JSZip auto-creates — those default to `new Date()` and +// are exactly what made the buffered writer non-deterministic until the finalize-time pin. +async function entryDates(buffer) { + const zip = await JSZip.loadAsync(buffer); + return Object.values(zip.files).map(entry => entry.date); +} + +// prettier-ignore +describe('github issues: XLS-352 fork pins every zip entry mtime (durable fix for XLS-350)', function() { + // Two full builds plus a real >2s delay per determinism arm. + this.timeout(30000); + + describe('streaming writer (stream.xlsx.WorkbookWriter)', () => { + it('two runs separated by more than one DOS tick are byte-identical when zipEntryDate is pinned', async () => { + const first = await buildStreaming({zipEntryDate: PIN}); + await sleep(2500); + const second = await buildStreaming({zipEntryDate: PIN}); + expect(first.equals(second)).to.equal(true); + }); + + it('stamps every zip entry mtime with zipEntryDate', async () => { + const dates = await entryDates(await buildStreaming({zipEntryDate: PIN})); + expect(dates.length).to.be.greaterThan(3); + dates.forEach(date => { + expect(date.getFullYear()).to.equal(2000); + expect(date.getMonth()).to.equal(5); + expect(date.getDate()).to.equal(15); + }); + }); + + it('POSITIVE CONTROL: without zipEntryDate the bytes differ and entries carry the wall clock', async () => { + const pinned = await buildStreaming({zipEntryDate: PIN}); + const unpinned = await buildStreaming({}); + expect(pinned.equals(unpinned)).to.equal(false); + const dates = await entryDates(unpinned); + expect(dates.some(date => date.getFullYear() >= 2020)).to.equal(true); + }); + }); + + describe('buffered writer (Workbook.xlsx.writeBuffer)', () => { + it('two runs separated by more than one DOS tick are byte-identical when zipEntryDate is pinned', async () => { + const first = await buildBuffered({zipEntryDate: PIN}); + await sleep(2500); + const second = await buildBuffered({zipEntryDate: PIN}); + expect(first.equals(second)).to.equal(true); + }); + + it('stamps every zip entry mtime with zipEntryDate', async () => { + const dates = await entryDates(await buildBuffered({zipEntryDate: PIN})); + expect(dates.length).to.be.greaterThan(3); + dates.forEach(date => { + expect(date.getFullYear()).to.equal(2000); + expect(date.getMonth()).to.equal(5); + expect(date.getDate()).to.equal(15); + }); + }); + + it('POSITIVE CONTROL: without zipEntryDate the bytes differ and entries carry the wall clock', async () => { + const pinned = await buildBuffered({zipEntryDate: PIN}); + const unpinned = await buildBuffered({}); + expect(pinned.equals(unpinned)).to.equal(false); + const dates = await entryDates(unpinned); + expect(dates.some(date => date.getFullYear() >= 2020)).to.equal(true); + }); + }); +});