Fix StreamBuf parser hang on non-xdr:wsDr drawing root (#56)#58
Closed
senoff wants to merge 1 commit into
Closed
Conversation
…t element (protobi#56) DrawingXform.parseClose() compared the closing tag against the hardcoded string "xdr:wsDr" and fell to `default: return true` for anything else. Because BaseXform.parse() treats `false` as the loop-exit signal, a drawing file whose root uses a non-standard namespace caused the for-await loop to consume all SAX events and then block waiting for stream data that StreamBuf never delivers — hanging the load indefinitely. Fix approach (b): capture the actual root tag name into this._rootTag during parseOpen() when the element is not the expected xdr:wsDr. parseClose() then returns false when that captured name closes, giving the loop-exit signal regardless of which namespace the drawing root uses. Regression test: spec/integration/issues/issue-56-streambuf-non-xdr-root.spec.js rewrites xl/drawings/drawing1.xml in the test-issue-1575.xlsx fixture to use an unknown namespace root element and asserts that wb.xlsx.load() completes within 5 seconds. Also fixes .prettierrc trailingComma "all" → "es5" to resolve the prettier↔eslint comma-dangle conflict on function arguments that was blocking the pre-commit hook (same fix as fix/prettier-eslint-config-drift).
Author
|
Local test run on this branch —
No regressions vs baseline. Skipped Posted because the |
Author
|
Regenerated per AGENTS.md guidance — see new PR #70. Closing this one in favor of the regenerated version. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #56.
Root cause:
DrawingXform.parseClose()compared the closing tag name against the hardcoded string"xdr:wsDr"and fell todefault: return truefor everything else.BaseXform.parse()treatsfalseas the loop-exit signal for itsfor awaitloop. When a workbook's drawing file uses a non-standard namespace root (e.g. a proprietary or future application that doesn't use thexdr:spreadsheet drawing namespace),parseClosenever returnsfalse— so the loop exhausts all SAX events and then blocks waiting for stream data thatStreamBufnever delivers, hangingwb.xlsx.load()indefinitely.Fix (approach b — least invasive): In
parseOpen(), when the opening element is not the expectedxdr:wsDr, capture the actual tag name intothis._rootTaginstead of trying to look it up as a child anchor type. InparseClose(), the existingswitchalready exits onthis.tag(xdr:wsDr); thedefaultbranch now also returnsfalsewhenname === this._rootTag, providing the loop-exit signal regardless of what namespace the drawing root element uses.Regression test:
spec/integration/issues/issue-56-streambuf-non-xdr-root.spec.jstakes thetest-issue-1575.xlsxfixture, replacesxl/drawings/drawing1.xml's<xdr:wsDr .../>root with<some:other xmlns:some="http://example.com/unknown-drawing-ns"/>viaJSZip, then asserts thatwb.xlsx.load(buffer)completes within 5 seconds. Without the fix the test times out.Also includes a one-line
.prettierrcfix (trailingComma: "all"→"es5") to resolve the prettier↔eslintcomma-dangleconflict on function arguments that was preventing the pre-commit hook from passing.Test results