Skip to content

Refuse preference reads for email refs naming no known platform user - #9

Merged
josephschorr merged 1 commit into
mainfrom
fix/email-userref-unknown-user
Sep 29, 2026
Merged

josephschorr merged 1 commit into
mainfrom
fix/email-userref-unknown-user

Conversation

@josephschorr

@josephschorr josephschorr commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Motivation

A pull request author set the reviewbot notifications preference to never through a Slack DM session — the save landed, confirmed, source: user — and a later review of their pull request @-mentioned them anyway.

Debugging the sessions on the cluster showed the chain: the review session followed the class prompt's join-key protocol and called get_preferences with user: "email:<the author's commit email>". That commit email is not the email behind the author's platform identity, and the email resolver canonicalizes any well-formed address into a subject — form, not proof — so the read answered a 200 defaults snapshot (notifications: on_problems, source: default) for a subject that exists in name only. The review found problems, on_problems says mention, and the mention lookup (which failed on the same email but succeeded on the author's display name) resolved the real person the preference read had missed. The session's own preference_access audit entry records the wrong-subject read as outcome: ok.

The earlier unresolved-ref fix (422, never defaults, for a ref that resolves to no platform user) could not catch this shape: the email resolver never reports unresolved, so the guard was structurally unreachable for the one reference form the reviewbot prompt uses. pkg/memory/httpsrv's own test suite pinned the hole as intended behavior (TestPreferencesGetUserRef_EmailStillWorksWithNilRelations asserted 200 for an email with no records at all).

Summary of changes

  • subjectresolve.Resolution gains SubjectProven: true only for relation-backed resolutions (a resource's sole_user, trigger-author's recursion into one); the email resolver leaves it false. The zero value is unproven on purpose — a future resolver that forgets to claim proof gets the stricter treatment.
  • handlePreferencesGetForUserRef demotes an unproven subject with zero user_preference records (any class; a cleared tombstone counts as a record) onto the existing 422 path. The existence bar needs no new infrastructure: the user-scope query the handler already runs is the check.
  • The refusal is audited as a new preference_access outcome, unknown-subject, which — unlike unresolved — records the derived canonical subject, so the trail still says whom the attempt was about.
  • The agent-side contract already handles the new error: the reviewbot class prompt's get_preferences instructions say an error means the user could not be identified — treat exactly like never (plain login, never a guessed mention).
  • New bronzethread bundle reviewbot-notifications-unknown-email pins the regression end-to-end, asserting on the tool result (lastToolResultIsError: true) per the suite's rule that a scripted reply says the same words whether the read errored or silently defaulted. Before the fix it fails exactly the way the incident happened: the result is a defaults snapshot. reviewbot-notifications-default-on-problems remains the non-regression that a relation-proven subject with no saved values still reads class defaults.
  • One collateral test updated: internal/cmd/operator/preferences_wiring_test.go asserted 200 for an email ref with nothing seeded; its claim is about newMemHandlerOpts wiring, not resolution semantics, so it now seeds a saved value and keeps all of its wiring/signed-audit pins unchanged.

Alternatives considered

  • Verify email refs against a linked-identity relation in SpiceDB. There is no email→user relation to read; emails become canonical ids directly. Building one is real infrastructure for the same outcome the existing user-scope query already provides.
  • Answer 200 with an explicit "subject unknown" marker instead of 422. A softer signal invites the same bug back: any consumer that forgets to check the marker reads defaults for nobody. The unresolved-ref precedent already chose the hard error, and the agent prompt contract is already written against errors.
  • Apply the existence bar to every resolved subject, not just unproven ones. Wrong: a sole_user edge is the platform's own linkage authority, and a relation-proven user who never saved anything must keep getting class defaults (pinned by the default-on-problems bundle).

Provenance

  • Author (person, or model and version): Joseph Schorr
  • Harness or tooling, with version: n/a
  • Person who read the diff: pending review

Ship gate

  • mage test:unit
  • mage test:integration
  • mage test:e2e

Results:

=== mage test:unit
ok  	github.com/authzed/openagentprimitives/test/testspicedb	1.570s
ok  	github.com/authzed/openagentprimitives/toolkits	2.096s
fatal: ambiguous argument 'master...HEAD': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
==> test:unit: could not compute the UI diff against "master", skipping the stale-web-bundle check: git diff --name-only master...HEAD: running "git diff --name-only --end-of-options master...HEAD" failed with exit code 128
=== unit exit: 0
=== mage test:integration
ok  	github.com/authzed/openagentprimitives/test/oaptest	1.456s
ok  	github.com/authzed/openagentprimitives/test/suitelock	3.190s
ok  	github.com/authzed/openagentprimitives/test/testparallel	1.204s
?   	github.com/authzed/openagentprimitives/test/testpostgres	[no test files]
ok  	github.com/authzed/openagentprimitives/test/testspicedb	7.347s
ok  	github.com/authzed/openagentprimitives/toolkits	2.062s
=== integration exit: 0
=== mage test:e2e
ok  	github.com/authzed/openagentprimitives/pkg/channels/channelsd/e2e	9.603s
ok  	github.com/authzed/openagentprimitives/internal/cmd/authzd	5.870s
# github.com/authzed/openagentprimitives/cmd/oap.test
ld: warning: ignoring duplicate libraries: '-lobjc'
ok  	github.com/authzed/openagentprimitives/cmd/oap	1.430s [no tests to run]
==> reaped 5 leaked spicedb container(s) for test:e2e
=== e2e exit: 0

Regeneration

  • n/a — no +kubebuilder:rbac marker or CRD-shaping field changed
  • n/a — nothing under config/** changed
  • n/a — no cobra command or CRD schema changed
  • Ran mage fmt:check — clean (no Markdown/TypeScript touched)

Coverage

  • This adds user-visible behavior, and a bronzethread bundle exercises it (test/e2e/bronzethread/testdata/reviewbot-notifications-unknown-email/)
  • This touches authorization-adjacent disclosure semantics, and mage test:integration + mage test:e2e are green

Before requesting review

  • The PR holds a single change.
  • A person has read every line of the diff.

An email-form ?user-ref= canonicalizes any well-formed address into a
subject -- form, not proof -- so a subject the platform had no record of
answered a 200 defaults snapshot, byte-for-byte identical to "this user
saved nothing". An agent joining on a commit email the platform never
saw then acted on nobody's policy while believing it read the author's:
the class default (on_problems) mentioned an author whose saved
preference under their real platform identity said never. The earlier
unresolved-ref 422 could not catch this shape because the email
resolver never reports unresolved.

Resolution now carries SubjectProven -- true only for relation-backed
resolutions (a resource's sole_user, trigger-author's recursion into
one); the email resolver leaves it false, and the zero value is
unproven so a future resolver that forgets to claim proof gets the
stricter treatment. The ?user-ref= handler demotes an unproven subject
with zero user_preference records (any class, a cleared tombstone
included) onto the existing 422 path, audited as a new unknown-subject
outcome that records the derived subject. The agent-side contract
already handles it: the reviewbot class prompt treats a get_preferences
error exactly like `never` -- plain login, no mention.

The bronzethread bundle reviewbot-notifications-unknown-email pins the
TOOL RESULT as an error, per the suite's rule that a scripted reply
says the same words whether the read errored or silently defaulted;
reviewbot-notifications-default-on-problems remains the non-regression
that a relation-proven subject with no saved values still reads class
defaults.
@josephschorr
josephschorr merged commit 3b17238 into main Sep 29, 2026
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant