ebc668d28d
The hub copies the reviewer string a client sends straight into decided_by (DecideApproval, ClearEventLog); nothing on the wire ties it to the authenticated caller. Studio filled it from the OS account, so an export read like non-repudiation while being an arbitrary client claim — the legal finding of the 2026-07-26 usertest panel. The real fix is hub-side (derive decided_by from CALLER_IDENTITY); that contract is written down in docs/reviewer-identity.md and needs a hub release. Until then Studio does the one thing it can do honestly and marks its own claim as a claim, inside the record: - data/reviewer_identity.dart is the single place that produces and reads the value; wire() is idempotent, so page and HubService may both normalise. Every write path funnels through HubService, so no surface can send a bare handle. - The inbox states before the decision who will be recorded, what that attribution is worth on this hub (from AuthStatus), and the literal string that lands in decided_by. An unreadable auth policy stays unreadable — never optimistic. - Reading back: a marked value shows its plain name plus an unchecked flag; an unmarked one (legacy row, CLI decision, or a future hub-derived identity) is not classified either way. - The audit wipe seeds the same kind of marked attribution into its chain.reset marker. When the hub starts deriving the value it overwrites the field and the prefix disappears by itself — no Studio release needed. Guards: reviewer_identity_test (the value) and approvals_reviewer_identity_test (every surface that writes or renders it, against the hermetic fake hub). Visual proof for both themes via the dialog-shot harness. Signed-off-by: flemming-it <stefan.a.flemming@googlemail.com>
89 lines
3.8 KiB
Markdown
89 lines
3.8 KiB
Markdown
# Reviewer identity — what `decided_by` is worth
|
||
|
||
## The problem
|
||
|
||
The hub copies the `reviewer` string a client sends straight into
|
||
`decided_by`:
|
||
|
||
- `DecideApproval` — who approved or rejected a paused flow step
|
||
- `ClearEventLog` — who wiped the audit log, recorded in the seeded
|
||
`chain.reset` marker
|
||
|
||
Nothing on the wire ties that string to the authenticated caller.
|
||
Studio fills it from the OS account (`$USER@studio`); any other
|
||
client can send any string at all, including someone else's name.
|
||
|
||
The legal review of the approvals page (usertest panel, 2026-07-26)
|
||
graded this HIGH: a value that reads like an identity but is an
|
||
unchecked client claim lends the audit trail a non-repudiation it
|
||
does not have. An export, a court-facing report or an auditor
|
||
reading the database sees `stefan@studio` and has no way to tell
|
||
whether the hub checked anything.
|
||
|
||
This is not a leak and not remote-exploitable on its own — the
|
||
finding is about what the record *proves*, not about access.
|
||
|
||
## What Studio does today (0.81.0)
|
||
|
||
Studio marks its own claim as a claim, **inside the recorded
|
||
value**:
|
||
|
||
```
|
||
decided_by = "unverified:stefan@studio"
|
||
```
|
||
|
||
- `lib/data/reviewer_identity.dart` is the single place that
|
||
produces and reads that value. `ReviewerIdentity.wire()` is
|
||
idempotent, so both the page and `HubService` may normalise.
|
||
- Every write path funnels through `HubService.approve` /
|
||
`reject` / `clearEventLog`, so no surface can send a bare handle.
|
||
- The approvals inbox states before the decision who will be
|
||
recorded, what the attribution is worth on *this* hub (derived
|
||
from `AuthStatus.anonymous_allowed`), and the literal string that
|
||
will be stored.
|
||
- Reading back: a marked value is shown by its name with an
|
||
"unchecked" flag; an **unmarked** value (legacy row, a CLI
|
||
decision, or a future hub-derived identity) gets no badge at all
|
||
— Studio does not know its provenance and must not classify it.
|
||
|
||
Guards: `test/reviewer_identity_test.dart` (the value itself) and
|
||
`test/approvals_reviewer_identity_test.dart` (every surface that
|
||
writes or renders it, against the hermetic fake hub).
|
||
|
||
This is honest, but it is a *label*, not a fix. It does not stop a
|
||
client from sending `unverified:someone.else@studio`.
|
||
|
||
## The actual fix — hub side
|
||
|
||
`decided_by` must be derived server-side from the verified caller
|
||
and the client-supplied `reviewer` ignored:
|
||
|
||
1. In the `DecideApproval` and `ClearEventLog` handlers, take the
|
||
identity from the same source `_caller` comes from
|
||
(`CALLER_IDENTITY`) rather than from the request message.
|
||
- static validator: the configured token's name
|
||
- `jwt-rs256` validator: the JWT subject
|
||
- anonymous call: no identity — record `anonymous` explicitly
|
||
(never a client-supplied name), or refuse the decision on
|
||
channels where the operator requires attribution.
|
||
2. Keep accepting the request field for one release (ignored) so
|
||
older Studios and CLIs keep working; log a deprecation when it
|
||
is present and differs from the derived value.
|
||
3. Once the hub derives the value, it overwrites the field and the
|
||
`unverified:` prefix disappears from new records by itself — no
|
||
Studio release is needed to stop labelling. Studio's reader
|
||
already treats an unmarked value as "provenance unknown", so a
|
||
hub-derived value renders cleanly.
|
||
4. Old rows keep the prefix. That is correct: they *were* unchecked.
|
||
|
||
Open product decision for step 1: whether an anonymous hub may
|
||
decide approvals at all, or whether approvals require an
|
||
authenticated caller. Studio surfaces the anonymous case today; it
|
||
does not block it.
|
||
|
||
## Related
|
||
|
||
- `lib/data/reviewer_identity.dart` — the Studio-side contract
|
||
- Backlog T014, point 1 (usertest 2026-07-26, legal persona)
|
||
- Points 2–7 of that finding (hash-chain-backed history, deeplink,
|
||
sealed slugs, UTC offsets, auto-refresh) are untouched by this.
|