Skip to content

Bound flatCols to MaxColumns when flattening worksheet columns - #2370

Merged
xuri merged 2 commits into
qax-os:masterfrom
arpitjain099:fix/col-max-bound
Aug 7, 2026
Merged

Bound flatCols to MaxColumns when flattening worksheet columns#2370
xuri merged 2 commits into
qax-os:masterfrom
arpitjain099:fix/col-max-bound

Conversation

@arpitjain099

Copy link
Copy Markdown
Contributor

flatCols materializes one xlsxCol per column in the range a <col> element declares. Those min and max attributes come straight from the worksheet XML and are not validated anywhere on the way in, so the size of that loop is whatever the file says it is.

A worksheet cannot hold more than MaxColumns columns, so flattening past that bound cannot produce anything usable. This clamps both loops to MinColumns..MaxColumns.

For any range a worksheet can genuinely contain the result is unchanged, since the clamp only takes effect outside the representable range. The four column mutators that call flatCols (setColVisible, setColOutlineLevel, setColStyle, setColWidth) keep their signatures, so nothing changes for callers.

I went with clamping rather than returning ErrColumnNumber to keep this to flatCols. Returning an error would mean threading it through those four helpers and their nine call sites across col.go and stream.go. Happy to redo it that way if you would rather reject such a file outright, and equally happy to move the check to parse time instead.

Verified:

  • go build ./... clean, gofmt clean
  • full go test ./... passes (71s)
  • added TestFlatColsBounded, which sets a <col> range of 1..2147483647 on a sheet, calls SetColWidth, and asserts the flattened set stays within MaxColumns while a real column is still flattened and readable

Filed at your request from the report on the advisory.

arpitjain099 and others added 2 commits August 7, 2026 05:18
The min and max attributes of a col element are read from the worksheet XML
without validation, and flatCols materializes one xlsxCol per column in that
range. A worksheet cannot hold more than MaxColumns columns, so a file declaring
a wider range makes the function allocate for columns that cannot exist.

Clamping both loops to MinColumns..MaxColumns keeps the flattening result the
same for any range a worksheet can actually contain, and bounds the work for
ranges it cannot.

Adds a regression test asserting the flattened set stays within MaxColumns and
that a real column is still flattened and readable afterwards.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Comment thread col_test.go Outdated
Comment thread col.go Outdated

@xuri xuri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. I've made some changes based on your branch.

@xuri xuri added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 7, 2026
@xuri xuri added this to v2.11.1 Aug 7, 2026
@xuri xuri moved this to Security in v2.11.1 Aug 7, 2026
@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.62%. Comparing base (db93f8d) to head (bac5891).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2370   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files          32       32           
  Lines       26994    26994           
=======================================
  Hits        26892    26892           
  Misses         53       53           
  Partials       49       49           
Flag Coverage Δ
unittests 99.62% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@xuri
xuri merged commit a54c578 into qax-os:master Aug 7, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

Status: Security

Development

Successfully merging this pull request may close these issues.

2 participants