Skip to content

fix(secrets): refuse access edits made from an outdated access list - #429

Open
PiquelChips wants to merge 1 commit into
mainfrom
fix/secret-access-precondition
Open

PiquelChips wants to merge 1 commit into
mainfrom
fix/secret-access-precondition

Conversation

@PiquelChips

@PiquelChips PiquelChips commented Nov 10, 2026 •

Copy link
Copy Markdown
Member

Two people editing a stored secret's access list at once through the access endpoint (piquelctl app secret access, the dashboard's access form) silently lost one change. Both read the list, change it, and PUT the whole list back with no precondition, and an access change doesn't bump the secret's generation, so nothing else caught it either.

  • API (CLI-02, D2): the access PUT now takes a SecretAccessChange body, made of the new environments and previews plus an optional expected access the caller read. The store compares expected with the current access inside the same transaction and answers 409 secret_access_conflict on a mismatch; secret_access_changed stays the event kind of a successful change. Bodies without expected, as old clients send, behave as before. There is no migration.
  • Scope: this guards the access endpoint only. Setting a value with new access, through app secret set --previews or the dashboard's Save, goes through put_stored_secret, which is guarded by generation alone and can still overwrite a concurrent access change.
  • Older daemons: a newer CLI or dashboard gets a 400 from a daemon that predates this change, since SecretAccess denies unknown fields such as expected. That's accepted.
  • Client, CLI and dashboard send the access they loaded as expected.

SecretAccessChange repeats SecretAccess's two fields rather than flattening it, because SecretAccess denies unknown fields and the generated client doesn't handle allOf. OpenAPI, the generated client and api.md are updated.

Proof: a focused store test shows that a change made from an outdated access list fails with SecretAccessConflict and keeps the newer one, while one made from the current access applies. The piquelctl and client tests (147), the daemon's secret tests (52) and the Playwright secrets spec (3) pass, as do just lint (native and wasm), just openapi-check and fmt.

Note

Written by Claude Opus 5.5 in Claude Code and reviewed by Claude Opus 5.5 (via T3 Code)

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 35 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: eef2fa8b-903c-4c2c-a35e-4fb7a19ce66d
Commits

Reviewing files that changed from the base of the PR and between a1127cf and 1da8f1d.

Files selected for processing: 21

  • apps/piquelctl/src/secrets.rs
  • apps/piqueld-ui/src/browser/management/secrets.rs
  • apps/piqueld/src/api/http/error.rs
  • apps/piqueld/src/api/http/openapi.rs
  • apps/piqueld/src/api/http/secrets.rs
  • apps/piqueld/src/api/service/mod.rs
  • apps/piqueld/tests/api_contract/secrets.rs
  • crates/piqueld-client/src/generated.rs
  • crates/piqueld-client/src/lib.rs
  • crates/piqueld-client/src/secrets.rs
  • crates/piqueld-core/src/api.rs
  • crates/piqueld-store/src/lib.rs
  • crates/piqueld-store/src/preview/tests.rs
  • crates/piqueld-store/src/restore/tests.rs
  • crates/piqueld-store/src/secret/stored/mod.rs
  • crates/piqueld-store/src/secret/stored/tests.rs
  • crates/piqueld-store/src/secret/tests.rs
  • docs/openapi-v1.json
  • docs/user/reference/api.md
  • docs/user/reference/cli.md
  • tools/client-codegen/config.json
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@PiquelChips
PiquelChips marked this pull request as ready for review November 10, 2026 18:04
@PiquelChips
PiquelChips force-pushed the fix/secret-access-precondition branch 3 times, most recently from 6464980 to c8be3c8 Compare November 12, 2026 21:37
- `PUT …/secrets/{name}/access` takes a `SecretAccessChange` body: the new
  `environments` and `previews`, and optionally `expected`, the access the
  caller read. The store compares it in the transaction and answers 409
  `secret_access_conflict` when someone changed the access meanwhile, instead
  of overwriting their change. Old bodies, without `expected`, still work.
- piquelctl and the dashboard send the access they read.
@PiquelChips
PiquelChips force-pushed the fix/secret-access-precondition branch from c8be3c8 to 1da8f1d Compare November 13, 2026 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs Triage

Development

Successfully merging this pull request may close these issues.

1 participant