Skip to content

fix: avoid hash collision in caveat context#3065

Merged
miparnisari merged 3 commits into
mainfrom
tstirrat/avoid-hash-collision-in-caveat-context
Apr 22, 2026
Merged

fix: avoid hash collision in caveat context#3065
miparnisari merged 3 commits into
mainfrom
tstirrat/avoid-hash-collision-in-caveat-context

Conversation

@tstirrat15

Copy link
Copy Markdown
Contributor

Description

Someone pointed out to us that if you use a nested list of lists in a caveat context, it was possible under our current serialization and hashing logic to have a serialization collision and therefore a dispatch cache hash collision.

This fixes that by using deterministic proto marshalling of the caveat context Struct object.

Changes

  • Update context_hash.go to use deterministic marshalling
  • Propagate the error signature change through the stack
  • change hashableContext to hashableContextString to satisfy the interface

Testing

Review

@tstirrat15 tstirrat15 requested a review from a team as a code owner April 22, 2026 15:33
@github-actions github-actions Bot added area/api v1 Affects the v1 API area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) area/dispatch Affects dispatching of requests labels Apr 22, 2026
@codecov

codecov Bot commented Apr 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.22222% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.01%. Comparing base (312216f) to head (23b7171).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/dispatch/keys/computed.go 57.15% 4 Missing and 2 partials ⚠️
internal/dispatch/keys/keys.go 50.00% 4 Missing and 2 partials ⚠️
internal/services/v1/hash.go 40.00% 2 Missing and 1 partial ⚠️
pkg/caveats/context_hash.go 83.34% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3065      +/-   ##
==========================================
- Coverage   76.05%   76.01%   -0.03%     
==========================================
  Files         486      486              
  Lines       59456    59440      -16     
==========================================
- Hits        45211    45178      -33     
- Misses      10972    10981       +9     
- Partials     3273     3281       +8     

☔ View full report in Codecov by Sentry.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tstirrat15 tstirrat15 force-pushed the tstirrat/avoid-hash-collision-in-caveat-context branch 8 times, most recently from 378ffad to cef36b0 Compare April 22, 2026 16:50
Comment thread CHANGELOG.md Outdated
@tstirrat15 tstirrat15 force-pushed the tstirrat/avoid-hash-collision-in-caveat-context branch from 41d0057 to d38c5f4 Compare April 22, 2026 17:07
@tstirrat15 tstirrat15 force-pushed the tstirrat/avoid-hash-collision-in-caveat-context branch 2 times, most recently from a688077 to 1402616 Compare April 22, 2026 19:32

// TestBulkCheckCaveatContextCollision is a regression test for an issue with
// caveat hash collision.
func TestBulkCheckCaveatContextCollision(t *testing.T) {

@miparnisari miparnisari Apr 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we need an end to end test for something that could easily be encoded at a unit test level - compare the hashing of {"x": ["a", [], "b"]} and {"x": [["a"], "b"]} and ensures they are not the same?

the test you wrote, if it fails, tells us very little about where the problem could be.

tstirrat15 and others added 2 commits April 22, 2026 14:43
Co-authored-by: Maria Ines Parnisari <maria.ines.parnisari@authzed.com>
@tstirrat15 tstirrat15 force-pushed the tstirrat/avoid-hash-collision-in-caveat-context branch from 1402616 to 23b7171 Compare April 22, 2026 20:43
@miparnisari miparnisari merged commit cd1e2b9 into main Apr 22, 2026
44 of 45 checks passed
@miparnisari miparnisari deleted the tstirrat/avoid-hash-collision-in-caveat-context branch April 22, 2026 21:07
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 22, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area/api v1 Affects the v1 API area/dispatch Affects dispatching of requests area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants