Skip to content

Fix panics in GetConditionalFormats on malformed conditional formatting rules - #2375

Merged
xuri merged 2 commits into
qax-os:masterfrom
arpitjain099:fix/conditional-format-bounds
Aug 12, 2026
Merged

Fix panics in GetConditionalFormats on malformed conditional formatting rules#2375
xuri merged 2 commits into
qax-os:masterfrom
arpitjain099:fix/conditional-format-bounds

Conversation

@arpitjain099

Copy link
Copy Markdown
Contributor

Fix for GHSA-rxcj-4pj5-74gr. Thanks for asking for the PR.

What panics

GetConditionalFormats reaches three extractors that index cfRule sub-elements straight out of the worksheet XML, without checking length and in one case without checking for nil. A workbook whose rule is missing a child that Excel would always write panics the call, so a service that opens an untrusted spreadsheet and reads its conditional formats can be crashed by a small file.

Four sinks, all in styles.go:

where guard before what reaches it
extractCondFmtCellIs none cellIs rule with no <formula>, indexing Formula[0] on an empty slice
extractCondFmtColorScale none colorScale rule with no <colorScale> child, so c.ColorScale is nil
same, three-colour branch colors == 3 three <color> elements and one <cfvo>, indexing Cfvo[1] and Cfvo[2]
extractCondFmtDataBar c.DataBar != nil empty <dataBar></dataBar>, indexing Cfvo[0], Cfvo[1] and Color[0]

The third one is not in the advisory. It turned up while writing the tests: that branch is gated on the colour count and then indexes the cfvo slice, so the two counts can disagree. Worth fixing in the same pass.

The change

Four guards, eleven lines in styles.go. Each one leaves the surrounding behaviour alone: a rule missing its children now yields the zero value for those fields rather than stopping the caller. No signature changes and no new error returns, so this is not a breaking change for anyone whose files are well formed.

Tests

Four cases appended to styles_test.go. Each builds a minimal workbook in memory with archive/zip, opens it with the public OpenReader and calls GetConditionalFormats, so they drive the same path a caller does rather than reaching into internals.

Verified individually, since a panic takes the test binary down and would otherwise mask the later cases:

cellIs_without_formula         panic: index out of range [0] with length 0
colorScale_element_absent      panic: invalid memory address or nil pointer dereference
dataBar_element_empty          panic: index out of range [0] with length 0
three_colors_but_one_cfvo      panic: index out of range [1] with length 1

With the change, all four pass and the full suite is green:

ok  github.com/xuri/excelize/v2  72.413s

gofmt and go vet are clean.

Happy to split the third fix into its own commit or PR if you would rather keep the advisory scope exact.

The cellIs, colorScale and dataBar extractors read sub-elements of a cfRule
straight out of the worksheet XML and index them without checking length, and in
one case without checking for nil. A workbook whose rule is missing a child that
a file written by Excel would always have panics the call, so any service that
opens an untrusted spreadsheet and reads its conditional formats can be crashed
by a small file.

Four guards, one per reachable sink:

- extractCondFmtCellIs indexed Formula[0] in the fallback branch. The branch
  above handles exactly two formulas; a rule with none reached an empty slice.
- extractCondFmtColorScale took len(c.ColorScale.Cfvo) with ColorScale nil when
  the cfRule has no colorScale child.
- The three-colour branch of the same function was gated on the colour count
  alone, then indexed Cfvo[1] and Cfvo[2], so three colours with one cfvo
  panicked. This one is not in the report that prompted the fix; it turned up
  while writing the tests.
- extractCondFmtDataBar guarded only c.DataBar != nil, which says nothing about
  the length of Cfvo or Color, so an empty dataBar element reached all three
  indexes.

Each guard leaves the surrounding behaviour alone: a rule missing its children
now yields the zero value for those fields rather than stopping the caller.

Tests build four minimal workbooks and drive them through the public OpenReader
and GetConditionalFormats, so they exercise the same path a caller would. All
four panic without this change.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.62%. Comparing base (ab49f6f) to head (2239a97).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2375   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files          32       32           
  Lines       27001    27004    +3     
=======================================
+ Hits        26899    26902    +3     
  Misses         53       53           
  Partials       49       49           
Flag Coverage Δ
unittests 99.62% <100.00%> (+<0.01%) ⬆️

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

@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 for simplify unit tests code.

@xuri
xuri merged commit be7a163 into qax-os:master Aug 12, 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