fix(report): dedup header findings by key with deterministic sort#335
fix(report): dedup header findings by key with deterministic sort#335TBX3D wants to merge 2 commits into
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
=======================================
Coverage ? 65.02%
=======================================
Files ? 88
Lines ? 7866
Branches ? 0
=======================================
Hits ? 5115
Misses ? 2359
Partials ? 392 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pr summary2 files changed (+158 -35)
|
046eebe to
befd27a
Compare
vmfunc
left a comment
vmfunc
left a comment
There was a problem hiding this comment.
the sort in Flatten plus the js env-var stability test are the right call, that map iteration needed pinning and the 200-run test is a good way to lock it.
the header key change is the wrong fix though. folding the full value into the key breaks the run-stable identity the Key field is documented to guarantee (finding.go:39, "the same finding across two runs produces the same Key"). the collapse you're actually hitting is only repeated header names (Set-Cookie), but headers.go emits every resp.Header, so you're now also destabilizing every single-valued volatile header: Date is on basically every response, plus ETag/Age/Server-with-version/CF-Ray/X-Request-Id. all of those were name-stable before and now churn a new key each run, so the diff/notify layer that keys off Finding.Key reports them added+removed on every scan of every target.
disambiguate by occurrence index within the same name instead, e.g. key("headers", h.Name+"#"+strconv.Itoa(i)). splits the two Set-Cookies, leaves Date and friends stable across runs. adjust the test's wantKey accordingly and it's in.
|
right, the value was the wrong axis and Date/ETag/Age/CF-Ray would have churned one adjustment to what you sketched: the disambiguator counts occurrences within the two Set-Cookies still split, Date and friends stay put, wantKey updated. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes nondeterministic and overly-aggressive deduplication of header findings by ensuring multi-valued headers produce distinct, run-stable keys and by making Flatten output ordering deterministic.
Changes:
- Add run-stable, per-occurrence disambiguation for multi-valued headers (e.g., repeated
Set-Cookie). - Sort
Flattenoutput byKeyto avoid nondeterministic ordering from Go map iteration (notably JS env vars). - Add regression tests for header key behavior and stable JS env-var ordering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/finding/finding.go | Adds global Flatten key sort + per-occurrence header key disambiguation. |
| internal/finding/finding_test.go | Adds tests asserting distinct header keys, run-stable keys, and deterministic ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| identifier := h.Name | ||
| if n := seen[h.Name]; n > 0 { | ||
| identifier += "#" + strconv.Itoa(n) | ||
| } | ||
| seen[h.Name]++ |
| // a multi-valued header (Set-Cookie is the canonical case) emits one | ||
| // HeaderResult per value, and keying on the name alone collapses every | ||
| // value but the first onto one dedup Key. disambiguate by how many times | ||
| // the name has been seen rather than by the value: the value would | ||
| // destabilize every volatile single-valued header (Date, ETag, Age, CF-Ray) | ||
| // against the run-stable Key contract. |
| // some flatten* funcs iterate a Go map (js env vars), which randomizes | ||
| // order per run; sort by Key here once so all report output is stable. | ||
| sort.SliceStable(out, func(i, j int) bool { return out[i].Key < out[j].Key }) | ||
| return out |
| // some flatten* funcs iterate a Go map (js env vars), which randomizes | ||
| // order per run; sort by Key here once so all report output is stable. | ||
| sort.SliceStable(out, func(i, j int) bool { return out[i].Key < out[j].Key }) | ||
| return out |
c7b0388 to
683bb4b
Compare
|
one correction on my own fix: the occurrence suffix used "#", and "#" is a valid switched the separator to ":", which rfc7230 excludes from a field-name, so the |
|
CI on this branch is red for a reason unrelated to the change: main does not internal/modules/executor.go:197:57: undefined: MaxBodySize #374 fixes it. this branch builds and tests clean with that one-line fix applied |
…istically multi-valued headers (set-cookie) emitted one finding per value but keyed on name alone, so store.Diff collapsed every value but the first onto one key. fold the value into the header key so distinct values stay distinct. several flatten funcs source findings off randomized go map iteration (js env vars, header names), leaving report output order non-deterministic across identical scans. sort the flattened slice on the stable key once in Flatten so json/sarif/markdown/-silent all render a stable order.
folding the value into the header key gave every multi-valued header a distinct key, but it also destabilized the single-valued volatile ones. Date rides on nearly every response, alongside ETag, Age, CF-Ray and X-Request-Id, so each scan minted a fresh key and the diff/notify layer reported them added+removed every run. Key is documented to be run-stable (finding.go:41), so that was the wrong axis. count occurrences of the header name instead. the two Set-Cookies still split, Date and friends stay put. the count is per name rather than the slice index: headers.go ranges over resp.Header, so a name's slice position is randomized per run while the order of values within one name is preserved.
683bb4b to
e8de5f8
Compare
header findings were deduped by name alone, so two Set-Cookie (or other multi-valued) headers with different values collapsed onto one finding.
the key now disambiguates by how many times the header name has been seen, not by the value. Key is documented to be run-stable (finding.go:41), and folding the value in would destabilize every volatile single-valued header: Date rides on nearly every response, alongside ETag, Age, CF-Ray and X-Request-Id, so each scan would mint a fresh key and the diff/notify layer would report them added+removed every run.
the count is per name rather than the slice index: headers.go ranges over resp.Header, so a name's slice position is randomized per run while the order of values within one name is preserved.
Flatten's output is also sorted by key, since several flatten paths (in particular the JS env-var map) source from randomized Go map iteration and produced a different finding order on every run.