fix(corroborate): escape boot() error + fail-closed meta schema gate#1500
Merged
njhensley merged 2 commits intoJun 26, 2026
Merged
Conversation
Two follow-ups from the NVIDIA#1483 review: - renderer: boot()'s catch interpolated err.message into innerHTML unescaped, the one data-adjacent string bypassing the universal esc() discipline (CSP blocks execution, so not exploitable). Wrap with esc(err.message) to match the series error path. - loadRun: a meta.json schema-version mismatch only warned and continued, asymmetric with LoadAllowlist's fail-closed stance. An incompatible major (e.g. meta/v2 repurposing a field) is now skipped via errSkipRun so it is not parsed under current assumptions; an empty version stays tolerated by design and a same-major minor still loads with a warning. Add compatibleMetaSchema/ schemaMajor with table + integration tests. Signed-off-by: Nathan Hensley <[email protected]>
njhensley
added
the
theme/community
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds schema-major compatibility handling for run loading, including helpers that accept empty schema versions and compare only major versions. Runs with incompatible majors now log a warning and are skipped, and tests cover both compatibility cases and the skip behavior. The boot-time error banner now escapes the displayed error message and updates the first line markup. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
mchmarny
approved these changes
Jun 26, 2026
mchmarny
left a comment
mchmarny
left a comment
Member
There was a problem hiding this comment.
Both follow-ups are correct and well-scoped:
esc(err.message)closes the one data-adjacent string bypassing the otherwise-universal escaping discipline — good hardening regardless of the CSP backstop.- The schema gate fails closed on an incompatible major and stays symmetric with
LoadAllowlist.schemaMajor/compatibleMetaSchemacorrectly tolerate empty (GP2) and same-major minors while skipping a future major viaerrSkipRun. Table test + themeta/v2integration test cover the behavior.
CI green, lint clean. LGTM.
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
2 participants
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
Two post-merge review follow-ups from #1483: escape the one unescaped error string in the dashboard renderer, and make the
meta.jsonschema-version gate fail closed on an incompatible major (symmetric with the allowlist loader).Motivation / Context
#1483 merged with two non-blocking review threads still open. This addresses both.
Fixes: N/A
Related: #1483, #1404
Type of Change
Component(s) Affected
pkg/corroborate)Implementation Notes
renderer/index.html):boot()'scatchinterpolatederr.messageintoinnerHTMLunescaped — the one data-adjacent string bypassing the otherwise-universalesc()discipline (some browsers embed a snippet of the offendingindex.jsonintoSyntaxErrormessages). CSP (script-src 'self' 'unsafe-inline') blocks execution so it was not exploitable, but it is now wrapped withesc(err.message)to match the series error path.loadRunschema gate: ameta.jsonschema-version mismatch previously only warned and continued, asymmetric withLoadAllowlist, which fails closed on an unsupportedschemaVersionbecause a future schema may change trust semantics. An incompatible major (e.g.meta/v2repurposingsigner/allowlisted) is now skipped viaerrSkipRunrather than parsed under v1 assumptions. An empty version stays tolerated by design (GP2 may omit it); a same-major future minor still loads with a warning. NewcompatibleMetaSchema/schemaMajorhelpers carry the logic.Testing
go test ./pkg/corroborate/... ./tools/corroborate/... golangci-lint run -c .golangci.yaml ./pkg/corroborate/... ./tools/corroborate/...All pass; lint reports 0 issues. Added a table test for
compatibleMetaSchemaand an integration test asserting ameta/v2run is skipped.Risk Assessment
Rollout notes: N/A — behavior change only affects malformed/future-major evidence (skipped instead of mis-parsed) and an error-banner code path.
Checklist
make testwith-race)make lint)git commit -S)