amadan
bake: a verified pipeline clears the deletion stamp for the whole app
PR #258 re-review, IMPORTANT-1. DeletedAt was cleared per NAMED channel, so a stamp could be left standing on a channel the object did not name — and a standing stamp is never restamped, so it could expire unnoticed and then refuse to re-arm. There is no object-level replay floor, only a per-channel SetAt one, which makes the chain reachable with keyless bucket writes plus a copy of a historical signed object: DELETE (every channel stamped) → PUT back an old, still-valid pipeline naming only edge (edge cleared, production's stamp left burning) → dwell past production's cycle → DELETE again (stamp non-zero, so not restamped; window == legacy, nothing pending) → no live ratchet anywhere, and the flagless legacy verdict adopts a build minutes old. The stamp means "absence has been seen since the last VERIFIED read", which is a fact about the APP. It is now cleared by prefix over every remembered channel at the top of the p != nil path, mirroring how the p == nil branch stamps it — so a replayed subset object can only ever disarm the machinery, never leave a fuse burning behind it. Reproduced first at Pass level (live = "v2" on the fourth step, plus the stale stamp after step two); both are asserted by the new test, and the plain-delete control still returns to legacy after its cycle. Riders from the same review: DeletedAt's doc block now carries the one-pass-deep old-binary caveat and says plainly that dropping THIS field fails OPEN, unlike the rest of Holds (whose blanket closed-direction sentence is qualified to match) — with today's exposure named as rollback-within-branch, since nothing deployed carries a Holds map yet. liveRatchet's deletion-cycle justification is restated: the "never shorter" claim was false once a pending had landed, and the real reason is that the pending path already ran the full old window. visibleText names attribute-borne text as its known blind spot. currentQuorum's comment no longer says a policy edit skips the read it now unconditionally makes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 files changed,
+200
−29
internal/console/pendinghandlers.go+9 −6internal/console/pipelinepage_test.go+7 −0internal/release/adopt_test.go+100 −0internal/release/bake.go+84 −23
diff --git a/internal/console/pendinghandlers.go b/internal/console/pendinghandlers.go| index cbf38df44..21a028b58 100644 |
| --- a/internal/console/pendinghandlers.go |
| +++ b/internal/console/pendinghandlers.go |
| @@ -366,12 +366,15 @@ func (s *Server) executePending(ctx context.Context, acct *Account, app, actor s |
| // gate that a fourth kind can be added without. A gated execution is a rare, |
| // human-paced action; the read is not a cost worth trading the shape for. |
| // |
| -// A policy edit is not re-checked here and does not need to be: its quorum is |
| -// derived from the channels its own edit TOUCHES (guardedEdit), and |
| -// executePendingPolicy already refuses outright unless the stored pipeline is |
| -// byte-for-byte the one the proposal was composed against (the SetAt pin). A |
| -// rule that moved at all invalidates the proposal before any quorum question |
| -// is asked, so max(stored, current) could only ever equal stored. |
| +// Every kind pays the read, INCLUDING a policy edit — the pipeline is fetched |
| +// before the kind is looked at, so an unreadable one fails the execution |
| +// closed whatever it is. What a policy edit does not do is raise its number |
| +// from a channel rule: its quorum comes from the channels its own edit |
| +// TOUCHES (guardedEdit), and executePendingPolicy refuses outright unless the |
| +// stored pipeline is byte-for-byte the one the proposal was composed against |
| +// (the SetAt pin). A rule that moved at all invalidates the proposal before |
| +// any quorum question is asked, so for that kind requiredQuorum returns the |
| +// stored number and max(stored, current) could only ever equal it. |
| func (s *Server) currentQuorum(ctx context.Context, acct *Account, app string, pc *PendingChange) (int, error) { |
| pipe, err := release.GetPipeline(ctx, s.cfg.Releases, s.cfg.ReleasePub, acct.Sqid, app) |
| if err != nil { |
diff --git a/internal/console/pipelinepage_test.go b/internal/console/pipelinepage_test.go| index 98d4668c3..d97816abe 100644 |
| --- a/internal/console/pipelinepage_test.go |
| +++ b/internal/console/pipelinepage_test.go |
| @@ -94,6 +94,13 @@ func assertNoChannelVocabulary(t *testing.T, body string) { |
| // templates escape it), and deliberately not a parser: a dependency-free |
| // assertion helper that anybody can check by eye is worth more here than |
| // fidelity to malformed markup. |
| +// |
| +// Known blind spot, named so it is a choice rather than an oversight: text |
| +// that reaches a reader THROUGH an attribute — aria-label, alt, title, |
| +// placeholder — is dropped with the rest of the tag, so vocabulary leaking |
| +// via one of those (most seriously to a screen reader, which is a reader) is |
| +// invisible to this check. If a one-channel surface ever grows such a label, |
| +// it wants asserting directly rather than trusting this projection. |
| func visibleText(html string) string { |
| var b strings.Builder |
| inTag := false |
diff --git a/internal/release/adopt_test.go b/internal/release/adopt_test.go| index bcaf73ec0..43e94c7d7 100644 |
| --- a/internal/release/adopt_test.go |
| +++ b/internal/release/adopt_test.go |
| @@ -3252,6 +3252,106 @@ func TestPassDeletedPipelineOnAppDefinedChannelHoldsWithoutFlag(t *testing.T) { |
| } |
| } |
| +// TestPassReplayedSubsetPipelineCannotPoisonTheDeletionStamp: the re-review |
| +// probe (PR #258, IMPORTANT-1). The deletion stamp is what carries the bake |
| +// verdict across a DELETE for an app-defined channel, so anything that can |
| +// leave a STALE stamp standing — one that expires unnoticed and then blocks |
| +// its own re-arming — is a way to strip the app of every ratchet it has. |
| +// |
| +// The chain, all of it keyless bucket writes plus one historical SIGNED |
| +// object anybody who ever saw the bucket can keep a copy of: |
| +// |
| +// 1. DELETE pipeline.json — every remembered channel is stamped. |
| +// 2. PUT BACK an OLD, still-validly-signed pipeline that names only a SUBSET |
| +// of the app's channels (here: edge, not production). There is no |
| +// object-level replay floor — only a per-channel SetAt one — so this is |
| +// accepted as a verified read. |
| +// 3. Dwell until the un-named channel's stale stamp has run out. |
| +// 4. DELETE again. A stamp is never restamped while non-zero, so production |
| +// is left with window == legacy, nothing pending and a dead stamp: no |
| +// live ratchet, no evidence, and the flagless legacy verdict adopts a |
| +// build that is minutes old. |
| +// |
| +// The fix is that a verified read clears the stamp for EVERY remembered |
| +// channel of the app, not only the ones the object names — so step 2 can |
| +// only ever DISARM the machinery, never leave a fuse burning behind it, and |
| +// step 4 stamps afresh. |
| +func TestPassReplayedSubsetPipelineCannotPoisonTheDeletionStamp(t *testing.T) { |
| + s := NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + priv, pub := testKeys(t) |
| + opt := t.TempDir() |
| + reg := testRegistry(t) |
| + |
| + // The historical object: a legitimately signed, edge-only pipeline from |
| + // before production existed. Kept aside, exactly as an attacker with a |
| + // copy of yesterday's bucket would. |
| + if _, err := SetPipeline(ctx, s, priv, "bab", "myapp", []PipelineChannel{{Name: "edge"}}, "test"); err != nil { |
| + t.Fatalf("SetPipeline (historical): %v", err) |
| + } |
| + historical, _, err := s.Get(ctx, PipelineKey("bab", "myapp")) |
| + if err != nil { |
| + t.Fatalf("read the historical pipeline: %v", err) |
| + } |
| + ratchetedProductionApp(t, s, ctx, priv, pub, opt, reg) |
| + |
| + // 1. Delete: production is stamped, and its held head stays held. |
| + if err := s.Delete(ctx, PipelineKey("bab", "myapp")); err != nil { |
| + t.Fatalf("delete pipeline: %v", err) |
| + } |
| + if err := Pass(ctx, reg, s, pub, opt, func(string) error { return nil }); err != nil { |
| + t.Fatalf("Pass after delete: %v", err) |
| + } |
| + if v, _ := LiveVersion(opt, "p.example.com"); v != "v1" { |
| + t.Fatalf("live = %q, want still v1 after the delete", v) |
| + } |
| + |
| + // 2. Replay the historical edge-only object (create-only PUT: the key is |
| + // absent after the delete, which is precisely the opening). |
| + if _, err := s.Put(ctx, PipelineKey("bab", "myapp"), historical, ""); err != nil { |
| + t.Fatalf("replay the historical pipeline: %v", err) |
| + } |
| + if err := Pass(ctx, reg, s, pub, opt, func(string) error { return nil }); err != nil { |
| + t.Fatalf("Pass after the replay: %v", err) |
| + } |
| + if v, _ := LiveVersion(opt, "p.example.com"); v != "v1" { |
| + t.Fatalf("live = %q, want still v1 — a verified pipeline is the app being pipelined, not a reason to adopt", v) |
| + } |
| + // The property that makes step 4 safe: the verified read left NO stamp |
| + // standing anywhere under the app, including on the channel it never |
| + // named. |
| + st := LoadFirstSeen(FirstSeenPath(opt)) |
| + for _, name := range []string{"edge", "production"} { |
| + if mem := st.Holds[channelClockKey("bab", "myapp", name)]; mem != nil && !mem.DeletedAt.IsZero() { |
| + t.Fatalf("%s keeps a deletion stamp (%v) after a verified read — a stale stamp can expire unnoticed and then refuse to re-arm", name, mem.DeletedAt) |
| + } |
| + } |
| + |
| + // 3. Dwell. Anything still stamped ages out; v2's own first sighting is |
| + // deliberately NOT aged, so the only thing that can adopt it below is the |
| + // verdict collapsing. |
| + st = LoadFirstSeen(FirstSeenPath(opt)) |
| + for _, name := range []string{"edge", "production"} { |
| + if mem := st.Holds[channelClockKey("bab", "myapp", name)]; mem != nil && !mem.DeletedAt.IsZero() { |
| + mem.DeletedAt = mem.DeletedAt.Add(-91 * time.Hour) |
| + } |
| + } |
| + if err := st.Save(); err != nil { |
| + t.Fatalf("Save dwelt clock: %v", err) |
| + } |
| + |
| + // 4. Delete again. The stamp re-arms, so the head is still held. |
| + if err := s.Delete(ctx, PipelineKey("bab", "myapp")); err != nil { |
| + t.Fatalf("second delete: %v", err) |
| + } |
| + if err := Pass(ctx, reg, s, pub, opt, func(string) error { return nil }); err != nil { |
| + t.Fatalf("Pass after the second delete: %v", err) |
| + } |
| + if v, _ := LiveVersion(opt, "p.example.com"); v != "v1" { |
| + t.Fatalf("live = %q, want STILL v1 — a replayed subset pipeline must not be able to strip the app of its deletion ratchet", v) |
| + } |
| +} |
| + |
| // TestPassErroredPipelineHoldsWithoutProductionFlag: the memory floor is only |
| // half a defence if the app's held head is then ADOPTED anyway. The bake |
| // VERDICT ("does the window apply at all?") must ride on the same evidence |
diff --git a/internal/release/bake.go b/internal/release/bake.go| index 7331939da..4964572ac 100644 |
| --- a/internal/release/bake.go |
| +++ b/internal/release/bake.go |
| @@ -120,13 +120,19 @@ type FirstSeen struct { |
| // |
| // ADDITIVE, but only one pass deep, same as Production above: an OLD |
| // binary's Save re-marshals a struct with no Holds field, which DELETES |
| - // every app's ratchet memory from the shared file. That is not silently |
| - // unsafe: the next new-binary pass re-latches every channel from |
| - // HoldFor(DefaultHolds, name) — the fail-closed baseline — and any |
| - // off-ratchet in flight simply restarts its clock from that pass's |
| - // sighting. A restarted clock only ever DELAYS a loosening; it can |
| - // never let one through early, so a mixed-version rollout costs at most |
| - // one pass of this memory in the closed direction, same as Production. |
| + // every app's ratchet memory from the shared file. For the WINDOW that |
| + // is not silently unsafe: the next new-binary pass re-latches every |
| + // channel from HoldFor(DefaultHolds, name) — the fail-closed baseline — |
| + // and any off-ratchet in flight simply restarts its clock from that |
| + // pass's sighting. A restarted clock only ever DELAYS a loosening; it |
| + // can never let one through early. |
| + // |
| + // That closed-direction argument covers Window/Pending/PendingAt and NOT |
| + // holdMemory.DeletedAt, which fails OPEN when dropped (a deleted-pipeline |
| + // app loses the only evidence it was ever pipelined, and the flagless |
| + // legacy verdict adopts). See that field's own comment — including why |
| + // today's exposure is a rollback within this branch rather than a fleet |
| + // property. |
| Holds map[string]*holdMemory `json:"holds,omitempty"` |
| path string // where Save persists; set by LoadFirstSeen |
| @@ -550,7 +556,32 @@ type holdMemory struct { |
| // adopt a build minutes old — collapsing the very protection the ratchet |
| // exists to give (spec §3: "deletion never collapses a window |
| // instantly"). Re-observing absence must not restart it, exactly as |
| - // PendingAt must not; a verified pipeline clears it. |
| + // PendingAt must not. |
| + // |
| + // It means "absence has been seen since the last VERIFIED read", which is |
| + // a fact about the app, not about one channel: a verified pipeline clears |
| + // it for every remembered channel under the app's prefix, including the |
| + // ones that object does not name. Clearing only the named channels was |
| + // poisonable — see the sweep in ObservePipeline's p != nil path. |
| + // |
| + // ADDITIVE, and one pass deep like every other field here — but NOT in |
| + // the same direction, which is worth saying plainly. Holds's own caveat |
| + // argues that a dropped ratchet memory restarts closed; that argument |
| + // does not extend to this field. An old binary's Save drops DeletedAt |
| + // along with the rest of Holds, and a dropped stamp does not delay a |
| + // loosening — it removes the only evidence the adopt pass has that a |
| + // deleted-pipeline app was ever pipelined, so floored comes up empty and |
| + // the flagless legacy verdict adopts. That is FAIL OPEN, for the window |
| + // between an old binary's Save and the next verified read (which, for a |
| + // deleted pipeline, never comes). |
| + // |
| + // The exposure is bounded by deployment reality, not by the design: |
| + // nothing in the fleet carries a Holds map at all yet — pipelines v2 has |
| + // not shipped — so the only binary that could drop it is a rollback |
| + // WITHIN this branch. If that stops being true before this ships, the |
| + // mitigation is the same one spec §3 gives for the old-binary edge (the |
| + // production: true co-write), and it wants saying in the spec rather |
| + // than assumed here. |
| DeletedAt time.Time `json:"deleted_at,omitzero"` |
| } |
| @@ -564,12 +595,26 @@ type holdMemory struct { |
| // - the enforced window differs from the name's legacy resolution, in |
| // EITHER direction: something other than the legacy ladder set it. |
| // - a pending loosening is waiting out its old window. |
| -// - a deletion is still running its cycle: the window it re-declared has |
| -// not yet elapsed on this box's own clock since the absence was first |
| -// seen. The cycle is measured against the window now ENFORCED, which |
| -// after a deletion is never shorter than the one it replaced — fail |
| -// closed, and self-clearing: a landed deletion whose legacy window is |
| -// zero is over the moment it lands. |
| +// - a deletion is still running its cycle: the window now ENFORCED has not |
| +// yet elapsed on this box's own clock since the absence was first seen. |
| +// |
| +// Measuring that last cycle against the CURRENTLY enforced window is exact, |
| +// not an approximation, because the two deletion shapes divide cleanly: |
| +// |
| +// - the legacy default is LONGER (every app-defined name, whose legacy |
| +// resolution is HoldFor's 72h fallback): it lands immediately, stamps no |
| +// Pending, and the enforced window is now that longer one — so the cycle |
| +// served here is at least the window it replaced. |
| +// - the legacy default is SHORTER (edge, 0): it goes PENDING instead, and |
| +// the pending machinery runs the FULL old window before landing. By the |
| +// time the enforced window drops to the shorter value, the cycle this |
| +// clause would ask for has already been served in full — which is why a |
| +// landed deletion on a zero-window channel is over the moment it lands, |
| +// and why that is not a hole. |
| +// |
| +// The first two conditions cover the whole of the pending case anyway |
| +// (Window != legacy, then PendingAt non-zero), so nothing is resting on this |
| +// clause while a pending is in flight. |
| // |
| // name is the channel's own name, jitter and now the pass's (FirstSeen.Jitter |
| // and its single stamped instant), so every hold on the box answers the same |
| @@ -716,18 +761,34 @@ func (f *FirstSeen) ObservePipeline(acct, app string, p *Pipeline, now time.Time |
| } |
| return changed |
| } |
| - for _, ch := range p.Channels { |
| - key := channelClockKey(acct, app, ch.Name) |
| - mem := f.Holds[key] |
| - // A verified pipeline naming the channel ends any deletion cycle: |
| - // the app is pipelined, on the object's own word, and the stamp's |
| - // only job was to carry the verdict across the gap. Cleared for a |
| - // channel whose bake string does not parse too — the app is |
| - // verifiably pipelined either way, which is all this stamp says. |
| - if mem != nil && !mem.DeletedAt.IsZero() { |
| + // A verified pipeline ends every deletion cycle this app is carrying — |
| + // EVERY remembered channel under its prefix, not only the ones this |
| + // object happens to name. The stamp means "absence has been seen since |
| + // the last verified read", which is a fact about the APP, so it is |
| + // cleared the same way the p == nil branch stamps it: by prefix. |
| + // |
| + // Per-channel clearing was poisonable (PR #258 re-review). There is no |
| + // object-level replay floor — only a per-channel SetAt one — so a |
| + // HISTORICAL signed pipeline naming a SUBSET of the app's channels can be |
| + // PUT back: delete (every channel stamped), replay an old edge-only |
| + // object (edge cleared, production's stamp left standing while the app |
| + // reads as verifiably pipelined), dwell past production's cycle (the |
| + // stale stamp expires unnoticed), delete again (already non-zero, so |
| + // never restamped). The app then carries no live ratchet at all and its |
| + // held heads adopt with no bake — a keyless bucket write reaching exactly |
| + // the outcome the ratchet exists to prevent. |
| + if prefix := holdKeyPrefix(acct, app); len(f.Holds) > 0 { |
| + for key, mem := range f.Holds { |
| + if mem == nil || !strings.HasPrefix(key, prefix) || mem.DeletedAt.IsZero() { |
| + continue |
| + } |
| mem.DeletedAt = time.Time{} |
| changed = true |
| } |
| + } |
| + for _, ch := range p.Channels { |
| + key := channelClockKey(acct, app, ch.Name) |
| + mem := f.Holds[key] |
| if w, err := ch.BakeWindow(); err == nil { |
| firstSighting := mem == nil |
| if firstSighting { |