Skip to content

fix(console): let channel saves replace a bound secret the caller cannot operate - #3632

Merged
freeqaz merged 7 commits into
mainfrom
perm-2444
Oct 25, 2026
Merged

freeqaz merged 7 commits into
mainfrom
perm-2444

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Since #3611, configurationSecretsOperate is false when the caller lacks Secret operate on any Secret the Agent's Configuration binds, and the console then makes the channel Secrets panel and the Channels drawer read-only. But updateConfiguration authorizes operate only on the secretBindings a save submits (or on every bound Secret when the save omits them). It does not check the Secrets a save replaces.

Scenario: the Configuration binds SLACK_BOT_TOKEN to an administrator's Secret A (the member has no operate on it) and SLACK_APP_TOKEN to Secret B (the member has operate). The member has Configuration read and update and operate on their own Secret C. Picking C for the bot token saves (the API checks C and B), but the console locked the panel, and its copy said a save needs operate on "each Secret the Configuration binds", which is not what the API checks.

The panel cannot submit only the changed keys: secretBindings in a Configuration PATCH replaces the whole map, so a partial map would drop the other bindings. The console needs a per-binding answer.

Change

  • API (additive field): GET /namespaces/{namespaceId}/agents/{agentId}/permissions returns configurationSecretsOperateDeniedBindings: the sorted binding names whose bound Secret the caller lacks Secret operate on. It uses the same check authorizeBindings makes (Secret operate on the exact source), shares IAM decisions with the existing bits (one call per distinct Secret), and writes no audit row. It is reported only to a caller with Configuration read and update, the same gate as configurationSecretsOperate; otherwise it is empty. It names bindings only, which a Configuration reader already sees; it names no Secret. configurationSecretsOperate is now "reported and the list is empty", which is the same value as before. getConfigurationSecretSources becomes getConfigurationSecretBindingSources (name to source).
  • Console: when every listed name is a Slack token name (SLACK_APP_TOKEN, SLACK_BOT_TOKEN), the channel Secrets panel and the Channels drawer stay editable. The panel shows a note that names the token to replace, and the drawer notice says the same. A save that keeps a listed token on its current Secret is refused before any request, and the message names the binding. A listed name that channel saves keep (any other key) leaves both read-only, and the note names that key. An unknown or unlisted answer keeps today's read-only rule. The Configuration editor submits no bindings, so it keeps the fix(console): say up front when a configuration save needs secret operate on its bound secrets #3611 rule. While channel saves can succeed, the draft is not called read-only. A panel save rereads the permission answer, so the drawer and the panel stop asking to replace a binding the save rebound.
  • Docs: the channels guide and the agent-editing flow describe what the API checks. The API reference and OpenAPI are regenerated. A new Storybook story, Components/Credentials → "Slack tokens, editor must replace the bot token Secret", shows the state.

Verification

  • tests/integration/console-api.test.mjs: a new test, "the per-binding Secret answer names the bindings a Configuration save must replace". Without Configuration update (Agent update only) the answer is empty. With update and partial operate, both names that bind the unoperable Secret are listed, sorted, and no denial is audited. A real PATCH that keeps a listed name answers 403. A PATCH that rebinds every listed name to the member's Secret answers 200, and the answer is then empty. An IAM failure on a Secret operate decision answers 503 for the whole answer. The exact-shape asserts include the new field.
  • tests/browser/console-agent-detail.test.mjs:
    • A new test, "a channel Secrets save may replace a bound Secret the member cannot operate", uses the scenario above against the real API and IAM. Changing only the app token is refused up front, with no write request. Picking the member's Secret for the bot token, with the app token kept on the administrator's operable Secret, saves (PATCH 200, bindings read back). The draft is not called read-only, and the Configuration editor stays read-only. After the replacement, a second panel save and an Edit Slack drawer save both PATCH.
    • A new test, "the Channels drawer save must replace a bound Secret the member cannot operate": a channel-setting-only save is refused up front, and replacing the bot token saves.
    • The finding-2437 test is now "... without Secret operate on a kept bound Secret reads the channel Secrets". A non-Slack binding the member cannot operate keeps the panel, the Configuration editor and Channels read-only, and the copy names the binding.
  • Mutants (local, targeted tests): all 15 were killed:
    • server: the list always empty, the list naming every binding, no Configuration-update gate, unsorted, an IAM failure read as a denial, an IAM failure read as allowed;
    • console: never offering replacement, offering it when a kept name is listed, the panel refusal dropped, the drawer refusal dropped, an omitted-bindings save not counted as keeping, the old draftReadOnly, the hint hidden, no reread after a panel save, the JSON editor following the channel rule.
  • Full files on a remote test VM: console-api.test.mjs and console-agent-detail.test.mjs pass (14/14 and 63/63). Pins at 2 CPUs: the 4 changed browser tests and the 3 integration tests passed 20/20.
  • tsc for the console, root and tests projects; eslint and prettier on the touched files; openapi:check; docs build check (294 pages, links); real docs word counter: agent-editing.md is 2483 words (was 2484).
  • Storybook rebuilt from this head. I inspected the new story.

Media (real app with a local fixture: real API and IAM, simulated runtime). The Secrets are disposable.

Before (main): the panel is read-only and names a grant the API does not check.
before: Credentials tab, panel read-only
before: Channels tab read-only

After: the panel is editable and names the token to replace. A save that keeps the token is refused before any request. Replacing the token saves. The drawer notice says the same. The 390-wide view and the Storybook story follow.
after: panel editable, note names the bot token
after: save keeping the bot token refused up front
after: bot token replaced and saved
after: Edit Slack drawer notice
after: 390 wide
Storybook: Slack tokens, editor must replace the bot token Secret

Walkthrough video (refusal, then replacement and save): after-walkthrough.webm

Not verified: deployment of the saved bindings and a live Slack connection. They are outside this change.

Risks

  • This adds a public API field. It is additive, and existing clients ignore it.
  • Disclosure: the list names bindings the caller can already read on the Configuration. It reports only what a Configuration save would decide for each binding: a caller can learn the same by saving with that binding kept or replaced. No Secret ID or name is added, and no audit row is written.
  • The answer is advisory and taken at page load. A grant or revocation since then applies on the next permission reread. The API still authorizes every save, and a member who picks a Secret they cannot operate still gets the API's 403.
  • The rule covers only the two Slack token names, the only bindings the channel editors write. Named Slack accounts keep their Configuration API path.

…replace one

The caller-permissions answer gains configurationSecretsOperateDeniedBindings:
the Configuration binding names whose bound Secret the caller may not operate.
A Configuration save is authorized on the Secrets it submits, so the channel
Secrets panel and the Channels drawer stay editable when every such name is a
Slack token name, and a save that keeps one is refused before any request.
…cret

Integration: the per-binding answer is unreported without Configuration update,
names each binding whose Secret the caller may not operate, and matches the
real PATCH (keeping one is refused, replacing them saves). Browser: the channel
Secrets panel and the Channels drawer refuse a save that keeps the Secret up
front and save one that replaces it; a binding channel saves keep leaves them
read-only. The draft is not called read-only while channel saves can succeed.
The permission answer describes the bindings the panel was built with. After
the panel saves a listed name to another Secret, neither the hint nor the
up-front refusal applies to it until the page reads the answer again.
A Configuration save needs operate on each Secret it keeps bound. The guide
and flow say that channel saves stay editable when each unoperable Secret is a
Slack token, which the save must replace, and that a save keeping one is
refused before the request. Storybook gains the replace-the-bot-token state.
@freeqaz
freeqaz requested a review from a team as a code owner October 25, 2026 13:03
@clawsweeper

clawsweeper Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

…3630

Those two stories change in the recovery fix; an omitted per-binding list is an
unlisted answer, which keeps today's rule. Keeps the two PRs mergeable in
either order.
@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 25, 2026
Review follow-ups (findings 2457, 2458). A panel save rereads the answer, so
the Channels drawer no longer refuses a save after the panel rebound a listed
token; the panel's local workaround goes. Tests: the drawer saves after a panel
replacement, the Configuration editor stays read-only for the replace persona,
and an IAM failure on a Secret operate decision answers 503. The flow doc
splits the update and operate cases; the recovery stories from #3630 carry the
new field.
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 25, 2026
@clawsweeper

clawsweeper Bot commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge.

What this changes

This PR adds per-binding permission hints so channel editors can replace bound Secrets the caller cannot operate.

Example: SLACK_BOT_TOKEN binds Secret A, while the member can operate replacement Secret C and the existing app-token Secret B.

  • Before: The Console makes the channel editors read-only even though the API permits replacing A with C.
  • After: The Console permits replacement and refuses saves that retain A, but switching from a successful Credentials save to Channels can incorrectly demand another replacement.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Useful, supported repair with real UI evidence and meaningful integration coverage, but the cross-tab stale denial needs correction.
Proof confidence 🐚 platinum hermit (4/6) ✨ media proof bonus Sufficient (screenshot): Supplied real-Console screenshots show the changed Credentials owner enabling replacement and refusing a retained forbidden binding; the body reports successful API/IAM-backed replacement and repeat saves and links a walkthrough. Additional media inspection was blocked by the review environment, not absent from the contribution. Deployment and live Slack execution are explicitly outside the demonstrated scope.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: A member cannot replace an inaccessible bound Slack Secret with an authorized Secret through the Console, although the API permits that save.
Reason: The maintainer-authored direction restores an existing authorized operation and retains the exact-resource IAM boundary.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

⛔ Needs changes before merge - 1 item remains

Keep this PR open: it repairs a real Console restriction, but one stale-permission bug still blocks valid channel saves after replacing a Secret in Credentials.

Priority: P2
Reviewed head: dfc9045d5ab78009bcf090b5f9b126bdd3ea7933

Before merge

  • Refresh the denied-binding answer after a Credentials replacement (P2) - After the member replaces forbidden Secret A with permitted Secret C in Credentials, onConfigurationChange updates the shared Configuration snapshot but does not refresh callerPermissions. The panel suppresses the old denial locally through toReplace(), while Channels still receives SLACK_BOT_TOKEN from channelBindingsReplace(). Switching to Channels and changing only a setting therefore compares C with the current C and refuses the save before any request, incorrectly saying the member cannot operate C. Reconcile the shared denied-binding answer after the successful replacement, and cover this cross-tab sequence.

Findings

  • [P2] Refresh the denied-binding answer after a Credentials replacement — apps/controller/src/console/agents/detail.mts:3680-3683

Tests

  • Missing end-to-end proof: The added browser coverage does not exercise replacing the bot Secret in Credentials and then saving a channel-setting-only edit in Channels.
Agent review details

How this fits together

The Console reads advisory Agent permissions from the Controller, stages Configuration bindings, and submits saves through OCC, which authorizes each submitted Secret before writing.

flowchart LR
  Member[Namespace member] --> Console[Console channel editors]
  Console --> Permissions[Controller permission summary]
  Permissions --> IAM[Selected IAM Driver]
  Console --> Save[Configuration PATCH]
  Save --> OCC[OCC authorization and mutation]
  OCC --> IAM
  OCC --> Storage[Configuration storage]
Loading

Technical review

Best possible solution:

Keep denied bindings associated with the Configuration snapshot they describe, and refresh or reconcile that shared answer after a successful Credentials replacement.

Do we have a high-confidence way to reproduce the issue?

Source establishes both the original overly broad restriction and the introduced cross-tab stale-list failure; supplied screenshots corroborate the original restriction and replacement UI, but no target code was run during this review.

Is this the best way to solve the issue?

Per-binding hints fit the existing permission-summary owner and preserve server authorization, but their lifetime must follow successful binding changes across both editors.

Full review comments:

  • [P2] Refresh the denied-binding answer after a Credentials replacement — apps/controller/src/console/agents/detail.mts:3680-3683
    After the member replaces forbidden Secret A with permitted Secret C in Credentials, onConfigurationChange updates the shared Configuration snapshot but does not refresh callerPermissions. The panel suppresses the old denial locally through toReplace(), while Channels still receives SLACK_BOT_TOKEN from channelBindingsReplace(). Switching to Channels and changing only a setting therefore compares C with the current C and refuses the save before any request, incorrectly saying the member cannot operate C. Reconcile the shared denied-binding answer after the successful replacement, and cover this cross-tab sequence.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against d959564d0b9b.

Provenance checked

  • Aggregate Configuration Secret permission hint and Console read-only rule changes intended behavior with a stated reason (1065fce: Prevent enabled saves that would retain inaccessible Secrets and receive 403; the original PR acknowledged that replacement saves were also blocked.)
  • Shared advisory source decisions for Agent saves keeps the original intent (b5dd0b2: Agent saves require grants on all currently bound sources; this PR preserves that aggregate answer while sharing exact-resource decisions with the new list.)
  • Reference-based credential replacement and partial-save handling keeps the original intent (fix(console): use Secret pickers when creating new revisions #420: Switch Secret references without overwriting shared values, and preserve pending Agent access grants after partial saves.)

Testing

Proof path: shipped entry point.

Security

None.

Evidence

Acceptance criteria:

  • node --test --test-name-pattern='a channel Secrets save may replace|the Channels drawer save must replace' tests/browser/console-agent-detail.test.mjs
  • node --test tests/integration/console-api.test.mjs
  • pnpm lint
  • pnpm format:check
  • pnpm check:workspace

What I checked:

Labels

Label changes:

  • add P2: A limited-permission member is blocked from an API-authorized Console editing workflow, with a further valid-save refusal remaining in the patch.
  • add proof: sufficient: Contributor real behavior proof is sufficient.
  • add proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Label justifications:

  • P2: A limited-permission member is blocked from an API-authorized Console editing workflow, with a further valid-save refusal remaining in the patch.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.
  • proof: sufficient: Contributor real behavior proof is sufficient.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 25, 2026, 9:38 AM ET / 13:38 UTC.

@freeqaz
freeqaz merged commit 5428bd4 into main Oct 25, 2026
42 checks passed
@freeqaz
freeqaz deleted the perm-2444 branch October 25, 2026 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant