amadan
console: service credentials are not revocable from the personal page (C49)
C49 (Important). templates/settings.html withheld the Revoke form from service rows, but that was presentation, not a control: POST /auth/cli/tokens/revoke accepts any listing id whose row carries the viewer's address, and a service credential's `address` column IS its minter. Three things make it the wrong door rather than a second one — RevokeCLIToken is a hard DELETE where the pane writes the revoked_at tombstone the store's identity rules depend on, it writes no account audit row, and it is scoped to the minter so it outlives ownership. An owner who minted a credential and was later transferred out could still hand-post its id, stop a production service, and leave the account's feed silent, which hollowed out C48's assurance that the audit row names who pressed it. Refused before the passkey (no gesture makes this the right door), with a flash naming the account pane. Scoped to service rows only: legacy kind='app' rows keep working there exactly as they do today. The classify read fails CLOSED — refusing a terminal revoke during a database hiccup is retryable, routing a credential into a hard DELETE is not. Minor 2: serviceWhat moved beside serviceAuditKey and is now SHARED with handleAPIServiceRevoke, which had the same logic inline. Two doors writing about the same event into the same feed must not drift apart on phrasing. Minor 3: create's reload sentence no longer says the secret "stays valid" — true of the credential, not of the reader's access to it. Plus a sentence on the step-up path, where the address bar is the callback and a reload re-runs a burned pending action rather than re-posting the form. Minor 4: rotate renders role and scope, gated on a new ScopeKnown — a failed findServiceCredential read-back leaves App "", which would otherwise render an app-scoped credential as "the whole account" on the screen used to check what was just re-armed. Absent beats false. Minor 5: an HTML comment above the block names the test that pins the C47 sentences, where the person deleting them will actually be.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016UimU7fAfeAoBMm6HKCPwQ
5 files changed,
+234
−44
internal/console/apiservices.go+33 −17internal/console/authcli.go+70 −0internal/console/svcpage.go+24 −25internal/console/svcpage_test.go+85 −0internal/console/templates/account_settings.html+22 −2
diff --git a/internal/console/apiservices.go b/internal/console/apiservices.go| index d906851d3..c7a6127ae 100644 |
| --- a/internal/console/apiservices.go |
| +++ b/internal/console/apiservices.go |
| @@ -371,6 +371,38 @@ func serviceAuditKey(name, app string) string { |
| return name |
| } |
| +// serviceWhat names the credential a revoke acted on: the SUBJECT an audit |
| +// row is filed under and the WORDS both doors use to describe what happened. |
| +// |
| +// Shared by handleAPIServiceRevoke and handleServiceRevokePage (svcpage.go), |
| +// which is the whole point of it being a function — the two doors write into |
| +// the same activity feed about the same event, and a phrasing improvement |
| +// made in one of them would otherwise leave the account's trail disagreeing |
| +// with itself depending on whether an operator used the browser or the CLI. |
| +// Same reason serviceHandleOf is shared rather than re-derived. |
| +// |
| +// A nameless legacy row is described by what it IS — its label and its app — |
| +// because its id says nothing to whoever reads the trail a year later. The |
| +// handle is the fallback for a row that could not be read back: not-found is |
| +// never "there was nothing to revoke", only "we could not name it". |
| +func serviceWhat(h serviceHandle, row CLIToken, found bool) (subject, what string) { |
| + subject, what = h.name, "service credential "+h.name |
| + if subject == "" { |
| + subject, what = h.id, "credential "+h.id |
| + } |
| + if !found { |
| + return subject, what |
| + } |
| + if row.Name != "" { |
| + return row.Name, "service credential " + row.Name |
| + } |
| + what = "app credential for " + row.App |
| + if row.Label != "" { |
| + what = "app credential " + row.Label + " for " + row.App |
| + } |
| + return row.App, what |
| +} |
| + |
| // serviceAuditSubject is the app column an audit row is filed under when |
| // there is no credential to name — an attempted revoke whose handle |
| // addresses nothing (C42). |
| @@ -552,23 +584,7 @@ func (s *Server) handleAPIServiceRevoke(w http.ResponseWriter, r *http.Request) |
| // Audited even when nothing matched: "someone tried to revoke X" is |
| // worth having in the trail of an account whose credentials are being |
| // argued about, and the detail says which it was. |
| - subject, what := h.name, "service credential "+h.name |
| - if subject == "" { |
| - subject, what = h.id, "credential "+h.id |
| - } |
| - if found { |
| - if row.Name != "" { |
| - subject, what = row.Name, "service credential "+row.Name |
| - } else { |
| - // A nameless legacy row is described by what it IS — its label |
| - // and its app — because its id says nothing to whoever reads |
| - // the trail a year later. |
| - subject, what = row.App, "app credential for "+row.App |
| - if row.Label != "" { |
| - what = "app credential " + row.Label + " for " + row.App |
| - } |
| - } |
| - } |
| + subject, what := serviceWhat(h, row, found) |
| detail := "revoked " + what + " (cli)" |
| if err != nil { |
| detail = "revoked " + what + " — nothing live matched (cli)" |
diff --git a/internal/console/authcli.go b/internal/console/authcli.go| index 88a269434..15d69e842 100644 |
| --- a/internal/console/authcli.go |
| +++ b/internal/console/authcli.go |
| @@ -1070,6 +1070,32 @@ func (s *Server) handleCLITokens(w http.ResponseWriter, r *http.Request) { |
| s.renderCLI(w, http.StatusOK, "settings", p) |
| } |
| +// serviceRowOf finds a live SERVICE credential of this address's by listing |
| +// id, for handleCLITokenRevoke's refusal below. It answers three ways — |
| +// found, not-a-service, and COULD NOT TELL — because the third is not the |
| +// second. |
| +// |
| +// ok=false with err!=nil means the read failed, and the caller FAILS CLOSED |
| +// on it. That is the asymmetry of the two mistakes: refusing a person's own |
| +// terminal revoke for as long as a database hiccup lasts is annoying and |
| +// retryable, while letting an unreadable row through routes a service |
| +// credential into RevokeCLIToken's hard DELETE, which is neither. |
| +func (s *Server) serviceRowOf(addr, id string) (tok CLIToken, isService bool, err error) { |
| + if id == "" { |
| + return CLIToken{}, false, nil |
| + } |
| + toks, err := s.store.CLITokens(addr) |
| + if err != nil { |
| + return CLIToken{}, false, err |
| + } |
| + for _, t := range toks { |
| + if t.ID == id { |
| + return t, t.IsService(), nil |
| + } |
| + } |
| + return CLIToken{}, false, nil |
| +} |
| + |
| func (s *Server) handleCLITokenRevoke(w http.ResponseWriter, r *http.Request) { |
| addr, raw, ok := s.session(r) |
| if !ok { |
| @@ -1080,6 +1106,50 @@ func (s *Server) handleCLITokenRevoke(w http.ResponseWriter, r *http.Request) { |
| s.renderMessage(w, r, http.StatusForbidden, "Refused", "Cross-site request blocked.") |
| return |
| } |
| + // C49: a SERVICE credential is not revocable here, and withholding the |
| + // button from the template was never the control — this door accepts any |
| + // listing id whose row carries the viewer's address, and a service |
| + // credential's `address` column IS its minter. |
| + // |
| + // Three things make that the wrong door rather than merely a second one. |
| + // RevokeCLIToken is a hard DELETE where the account pane writes the |
| + // revoked_at TOMBSTONE the store's design depends on (CLIPrincipal goes |
| + // on refusing the exact secret; a later mint of the same name gets a NEW |
| + // row rather than resurrecting this one's identity). It writes no ACCOUNT |
| + // audit row, so the feed an owner reads would say nothing — which would |
| + // hollow out the very assurance the pane's missing revoke-confirmation |
| + // leans on (see handleServiceRevokePage, C48). And it is scoped to the |
| + // MINTER, so it outlives ownership: mint a credential, be transferred out |
| + // of the account, and this door would still let you stop a production |
| + // service you no longer have any authority over. |
| + // |
| + // Refused BEFORE requireFreshAuth: no gesture could make this the right |
| + // door, so asking for one would waste it. |
| + // |
| + // SCOPED TO SERVICE ROWS ONLY. A legacy kind='app' row keeps working here |
| + // exactly as it always has (TestPersonalSettingsStillRevokesALegacyAppToken |
| + // pins it) — its minter is not necessarily an account owner, so this may |
| + // be the only door they have. The same hard-DELETE-and-no-audit |
| + // inconsistency is true of it and predates this branch; widening the |
| + // refusal would take away a working control to fix nothing here. |
| + svc, isService, err := s.serviceRowOf(addr, r.FormValue("id")) |
| + switch { |
| + case err != nil: |
| + log.Printf("console: cli token revoke: classify %s: %v", addr, err) |
| + s.setFlash(w, "err", "Could not check what that credential is, so nothing was revoked. Try again.") |
| + http.Redirect(w, r, "/settings", http.StatusSeeOther) |
| + return |
| + case isService: |
| + where := "its account's settings page" |
| + if acct, err := s.store.AccountByID(svc.AccountID); err == nil && acct != nil { |
| + where = "/a/" + acct.Sqid + "/settings" |
| + } |
| + s.setFlash(w, "err", "“"+svc.Name+"” is a service credential, not a terminal of yours — it "+ |
| + "belongs to its account, and is revoked from "+where+", where the revocation is "+ |
| + "recorded in that account's activity. Nothing was revoked.") |
| + http.Redirect(w, r, "/settings", http.StatusSeeOther) |
| + return |
| + } |
| // Step-up (spec §7a): killing a terminal is a security action, and the |
| // person doing it should be the person whose terminals they are. An |
| // attacker with a stolen cookie revoking the real owner's credentials is a |
diff --git a/internal/console/svcpage.go b/internal/console/svcpage.go| index 00c65be8c..41692fc58 100644 |
| --- a/internal/console/svcpage.go |
| +++ b/internal/console/svcpage.go |
| @@ -60,6 +60,13 @@ |
| // secret the operator has just copied. The same trade |
| // handleFleetRotateBoxPage already makes. Do not "fix" it with a flash. |
| // |
| +// The re-post hazard is REAL ONLY WHEN STEP-UP WAS ALREADY FRESH. When the |
| +// passkey was demanded, what the address bar holds is the step-up CALLBACK, |
| +// not this form, and reloading that re-runs a pending action TakePendingAction |
| +// has already burned — so it neither mints nor rotates, it reports that the |
| +// action expired. The page over-warns on that path, which is the safe |
| +// direction, and says which path the reader is on. |
| +// |
| // The trade is ACCEPTED, and the honest half of accepting it is that the |
| // PAGE SAYS SO, next to the secret and differently for each verb (C47, |
| // account_settings.html). The browser's own "confirm form resubmission" |
| @@ -110,6 +117,16 @@ type ServiceSecretView struct { |
| // App is the app the credential is bound to, "" for account-scoped, so |
| // the deliver-it line can name it rather than printing a placeholder. |
| App string |
| + // ScopeKnown says Role and Scope are FACTS rather than defaults. |
| + // |
| + // Create fills them from the form it just validated, so they are always |
| + // known there. Rotate reads them back with findServiceCredential, which |
| + // is best-effort by design — and a failed read leaves App "", which |
| + // serviceScopeWords renders as "the whole account". That would be a LIE |
| + // about an app-scoped credential, on the screen an operator is using to |
| + // check what they just re-armed. False beats absent here, so the flag |
| + // exists and the template withholds both fields rather than guessing. |
| + ScopeKnown bool |
| } |
| // DeliverCommand is the ONE command the pane tells an operator to run to get |
| @@ -301,7 +318,7 @@ func (s *Server) handleServiceCreatePage(w http.ResponseWriter, r *http.Request) |
| "minted service credential "+name+" with role "+string(role)+" over "+scope+" (console)") |
| s.renderServiceSecret(w, r, addr, raw, acct, &ServiceSecretView{ |
| Name: name, Role: string(role), Scope: scope, App: app, |
| - Token: secret, Doors: s.doorsFor(role), |
| + Token: secret, Doors: s.doorsFor(role), ScopeKnown: true, |
| }) |
| } |
| @@ -346,12 +363,16 @@ func (s *Server) handleServiceRotatePage(w http.ResponseWriter, r *http.Request) |
| // Read back for the audit's filing only (serviceAuditKey), the same |
| // best-effort lookup handleAPIServiceRotate makes: a failed read files |
| // the row under the credential's own name and costs nothing else. |
| - row, _ := s.findServiceCredential(acct.ID, serviceHandle{name: name}) |
| + row, found := s.findServiceCredential(acct.ID, serviceHandle{name: name}) |
| s.audit(acct, serviceAuditKey(name, row.App), addr, "service-rotate", |
| "rotated the secret of service credential "+name+" (console)") |
| + // found carries through to ScopeKnown (Minor 4): an operator re-arming a |
| + // credential is shown WHAT they re-armed, and a read-back that failed |
| + // says so rather than defaulting to "the whole account" — which would be |
| + // wrong about exactly the credentials it matters most to be right about. |
| s.renderServiceSecret(w, r, addr, raw, acct, &ServiceSecretView{ |
| Name: name, Role: row.Role, Scope: serviceScopeWords(row.App), App: row.App, |
| - Token: secret, Rotated: true, |
| + Token: secret, Rotated: true, ScopeKnown: found, |
| }) |
| } |
| @@ -449,28 +470,6 @@ func (s *Server) handleServiceRevokePage(w http.ResponseWriter, r *http.Request) |
| back("ok", "Revoked "+what+" — its next call fails, whatever secret it holds.") |
| } |
| -// serviceWhat names the credential a revoke acted on, for the audit trail and |
| -// for the flash. A nameless legacy row is described by what it IS — its label |
| -// and its app — because its id says nothing to whoever reads either a year |
| -// later. Shared shape with handleAPIServiceRevoke's own naming. |
| -func serviceWhat(h serviceHandle, row CLIToken, found bool) (subject, what string) { |
| - subject, what = h.name, "service credential "+h.name |
| - if subject == "" { |
| - subject, what = h.id, "credential "+h.id |
| - } |
| - if !found { |
| - return subject, what |
| - } |
| - if row.Name != "" { |
| - return row.Name, "service credential " + row.Name |
| - } |
| - what = "app credential for " + row.App |
| - if row.Label != "" { |
| - what = "app credential " + row.Label + " for " + row.App |
| - } |
| - return row.App, what |
| -} |
| - |
| // serviceHandleWords is the handle as the flash says it back. It is the |
| // PARSED handle, never the raw form value, so nothing unbounded and |
| // caller-controlled reaches the page — the same reason the audit row above |
diff --git a/internal/console/svcpage_test.go b/internal/console/svcpage_test.go| index 9ea00df50..af1faff85 100644 |
| --- a/internal/console/svcpage_test.go |
| +++ b/internal/console/svcpage_test.go |
| @@ -348,6 +348,21 @@ func TestTheFreshSecretAppearsOnceAndOnlyInItsOwnResponse(t *testing.T) { |
| if !strings.Contains(body, "Reloading this page rotates again") { |
| t.Fatalf("the rotate response does not warn that a reload retires this secret:\n%s", body) |
| } |
| + // Minor 4: an operator re-arming a credential is shown WHAT they are |
| + // re-arming. The facts come from findServiceCredential's read-back, so |
| + // rendering them is also what makes a degraded read visible rather than |
| + // silently absent. |
| + // |
| + // Asserted against the SECRET BLOCK, not the whole page: the listing and |
| + // the mint form both mention every role and the words "the whole |
| + // account", so a page-wide Contains would pass without the block saying |
| + // anything at all. |
| + block := paneSecretBlock(t, body) |
| + for _, want := range []string{"publish", "the whole account"} { |
| + if !strings.Contains(block, want) { |
| + t.Fatalf("the rotate block does not name the credential's %q:\n%s", want, block) |
| + } |
| + } |
| if _, ok := fx.st.CLIPrincipal(secret, time.Now()); ok { |
| t.Fatal("the pre-rotation secret still resolves") |
| } |
| @@ -377,6 +392,23 @@ func paneSecret(t *testing.T, body string) string { |
| return secret |
| } |
| +// paneSecretBlock slices out the banner the fresh secret is rendered in, so a |
| +// claim about what the OPERATOR IS TOLD at the moment of minting is not |
| +// satisfied by the listing or the mint form further down the same page. |
| +func paneSecretBlock(t *testing.T, body string) string { |
| + t.Helper() |
| + i := strings.Index(body, "CARLOS_SERVICE_TOKEN=") |
| + if i < 0 { |
| + t.Fatalf("no secret in the response:\n%s", body) |
| + } |
| + start := strings.LastIndex(body[:i], `<div class="banner">`) |
| + end := strings.Index(body[i:], "</div>") |
| + if start < 0 || end < 0 { |
| + t.Fatalf("the secret is not inside a banner block:\n%s", body) |
| + } |
| + return body[start : i+end] |
| +} |
| + |
| // paneCacheControl mints one more credential purely to read the response |
| // header off it — the header is the property under test, and the body is |
| // already covered above. |
| @@ -468,6 +500,59 @@ func TestPersonalSettingsStillRevokesALegacyAppToken(t *testing.T) { |
| } |
| } |
| +// TestPersonalSettingsCannotHandPostAServiceRevoke (C49) — the mirror is |
| +// read-only in FACT, not only in presentation. |
| +// |
| +// Withholding the button from the template is not a control: POST |
| +// /auth/cli/tokens/revoke accepts any listing id whose row carries the |
| +// viewer's address, and a service credential's address column IS its minter. |
| +// Worse than the pane's own revoke in three ways at once — RevokeCLIToken is |
| +// a hard DELETE rather than the revoked_at tombstone, it writes no account |
| +// audit row, and it survives the minter losing ownership. An owner who minted |
| +// a credential and was later transferred out could still hand-post its id, |
| +// stop a production service, and leave the account's activity feed silent. |
| +// |
| +// It is refused BEFORE the passkey: no gesture could make this the right |
| +// door, so asking for one would waste it. |
| +func TestPersonalSettingsCannotHandPostAServiceRevoke(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + secret, err := fx.st.NewServicePrincipal(OpsAccountID, "hub", "", svcrole.Publish, |
| + "paul@keymail.dev", time.Now()) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + toks, err := fx.st.CLITokens("paul@keymail.dev") |
| + if err != nil || len(toks) != 1 { |
| + t.Fatalf("CLITokens = %d rows %v, want the service row", len(toks), err) |
| + } |
| + id := toks[0].ID |
| + |
| + c := signInPaul(t, fx) |
| + csrf := sessionToken(t, fx, c) |
| + // postForm, not postGated: a hop into Keymail here would itself be the |
| + // bug — the refusal must cost nothing. |
| + status, loc, body := postForm(t, c, fx.ts.URL+"/auth/cli/tokens/revoke", |
| + url.Values{"csrf": {csrf}, "id": {id}}) |
| + if strings.HasPrefix(loc, fx.fk.srv.URL) { |
| + t.Fatalf("the refusal cost a passkey trip (-> %q)", loc) |
| + } |
| + if status != http.StatusSeeOther || loc != "/settings" { |
| + t.Fatalf("hand-posted service revoke = %d -> %q, want 303 back to /settings:\n%s", status, loc, body) |
| + } |
| + if _, ok := fx.st.CLIPrincipal(secret, time.Now()); !ok { |
| + t.Fatal("a hand-posted /settings revoke killed a service credential") |
| + } |
| + // And the row is still THERE — RevokeCLIToken is a hard DELETE, so a |
| + // credential that went through it would be gone rather than tombstoned. |
| + if rows, err := fx.st.ServicePrincipals(OpsAccountID); err != nil || len(rows) != 1 { |
| + t.Fatalf("ServicePrincipals = %d rows %v, want the credential untouched", len(rows), err) |
| + } |
| + _, _, page := get(t, c, fx.ts.URL+"/settings") |
| + if !strings.Contains(page, "is a service credential") { |
| + t.Fatalf("/settings does not say why the revoke was refused:\n%s", page) |
| + } |
| +} |
| + |
| // TestPaneMintRefusalsCostNoPasskey — a bad name or an unknown role is the |
| // caller's slip, and no gesture could change the answer, so it is refused |
| // BEFORE the passkey. Same reasoning handleStoreDestroy's "no backend" |
diff --git a/internal/console/templates/account_settings.html b/internal/console/templates/account_settings.html| index e9335dc76..da5fbf3e2 100644 |
| --- a/internal/console/templates/account_settings.html |
| +++ b/internal/console/templates/account_settings.html |
| @@ -18,25 +18,45 @@ |
| <p class="small">Credentials a SERVER holds — a CI runner, a router, a job — not a person. Each one |
| is bound to this account, carries exactly one role, and never expires: revoking is the control, |
| and it works from here.</p> |
| +{{/* The once-only secret block. The two "Reloading this page" sentences below |
| + are a CONTROL, not decoration (C47): this response is a 200 rather than a |
| + redirect, because a redirect would have to carry the plaintext somewhere |
| + the next request could read it — and the price of that choice is that a |
| + reload re-posts. Saying so, next to the secret, is what makes the trade |
| + honest. Both sentences are pinned by |
| + TestTheFreshSecretAppearsOnceAndOnlyInItsOwnResponse in svcpage_test.go; |
| + if you are deleting one as redundant copy, read svcpage.go's package |
| + comment first. */}} |
| {{with .ServiceSecret}} |
| <div class="banner"> |
| {{if .Rotated}} |
| + {{if .ScopeKnown}} |
| + <p><strong>New secret for {{.Name}} — role {{.Role}}, over {{.Scope}}:</strong></p> |
| + {{else}} |
| <p><strong>New secret for {{.Name}}:</strong></p> |
| + {{end}} |
| {{else}} |
| <p><strong>Minted {{.Name}} — role {{.Role}}, over {{.Scope}}:</strong></p> |
| {{end}} |
| <p><code>CARLOS_SERVICE_TOKEN={{.Token}}</code></p> |
| <p class="small">Shown once. Only its hash is stored — a lost secret means rotate, not another |
| look at this page.</p> |
| + {{if and .Rotated (not .ScopeKnown)}} |
| + <p class="small">This credential's role and scope could not be read back just now, so they are |
| + not shown. Only the secret changed — the credential is otherwise exactly what it was.</p> |
| + {{end}} |
| {{if .Rotated}} |
| <p class="small"><strong>Reloading this page rotates again</strong> and retires the secret |
| above. Copy it before you reload — if you lose it, rotate once more and redistribute; nothing |
| is destroyed, but this secret will not come back.</p> |
| {{else}} |
| <p class="small"><strong>Reloading this page</strong> tries to mint {{.Name}} a second time and |
| - is refused, because the name is taken. The secret above stays valid — but it is still shown |
| - only here, so copy it before you leave.</p> |
| + is refused, because the name is taken. The credential above keeps working — but this screen is |
| + the only place its secret is ever shown, and a reload loses it.</p> |
| {{end}} |
| + <p class="small">If you were just asked for your passkey, this page's address is the |
| + confirmation step rather than the form: reloading it re-runs a confirmation that has already |
| + been used, so it reports that the action has expired and changes nothing either way.</p> |
| <p class="small">Deliver it: <code>{{.DeliverCommand}}</code></p> |
| {{if .Rotated}} |
| <p class="small">ROTATION IS NOT REVOCATION: this refuses the credential's NEXT call with the |