Skip to content

fix(secrets): refuse a secret upload that would overwrite a concurrent access change - #435

Draft
PiquelChips wants to merge 2 commits into
fix/secret-access-preconditionfrom
fix/secret-upload-access-precondition
Draft

PiquelChips wants to merge 2 commits into
fix/secret-access-preconditionfrom
fix/secret-upload-access-precondition

Conversation

@PiquelChips

@PiquelChips PiquelChips commented Nov 10, 2026 •

Copy link
Copy Markdown
Member

#429 guarded the access endpoint, but a secret upload could still lose a concurrent access change. app secret set --previews, and the dashboard's Save when its form changed the access, merge their change into the access they read and send the whole list with the value. The upload was only checked against the generation, and an access change doesn't bump it, so the other change was overwritten.

  • API: the value upload (PUT …/secrets/{name}) takes expected_environments and expected_previews, the access the caller read, in the same form as environments and previews. Replacing an existing secret then fails with 409 secret_access_conflict if its access changed meanwhile, checked in the same transaction, as fix(secrets): refuse access edits made from an outdated access list #429 does. Without them, uploads behave as before.
  • CLI and dashboard: app secret set and the dashboard's Save send the access they read whenever they replace it. A value-only upload sends none, since it keeps the current access and has nothing to overwrite.
  • Store: Store::put_stored_secret_guarded takes a SecretPrecondition (expected generation, and access when read), and shares check_expected_access with the access endpoint. put_stored_secret delegates to it unguarded, so its many test callers are unchanged. The 400 for an undecodable query also names expected_environments and expected_previews.

The OpenAPI document, the generated client, api.md and cli.md are updated.

This is based on #429's branch. Once #429 merges, I'll rebase it onto main and retarget it.

Proof: a contract test checks the 409 over HTTP for both the access endpoint and the upload. A store test shows an upload made from an outdated access read fails with SecretAccessConflict, and one made from the current access applies. A CLI test checks that secret set --previews sends expected_environments=all&expected_previews=false. cargo nextest run -p piquelctl -p piqueld-client (148), the daemon's secret and promotion tests (67), the Playwright secrets spec (3), just lint, just openapi-check and fmt pass.

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

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
Pre-merge checks: Inconclusive 2

Failed Failed checks: 2 inconclusive

Check name Status Explanation Resolution
Title check Pending This assessment has not completed. The check will be retried on the next pre-merge check run.
Description check Pending This assessment has not completed. The check will be retried on the next pre-merge check run.
  • 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 force-pushed the fix/secret-access-precondition branch from d4729d1 to 87e7ebd Compare November 11, 2026 12:20
@PiquelChips
PiquelChips force-pushed the fix/secret-upload-access-precondition branch 2 times, most recently from 1548df6 to 642de7e Compare November 11, 2026 13:00
@PiquelChips
PiquelChips force-pushed the fix/secret-access-precondition branch from 476185f to 6464980 Compare November 11, 2026 13:41
@PiquelChips
PiquelChips force-pushed the fix/secret-upload-access-precondition branch from 642de7e to a6f0bfa Compare November 11, 2026 13:44
@PiquelChips
PiquelChips force-pushed the fix/secret-access-precondition branch from 6464980 to c8be3c8 Compare November 12, 2026 21:37
@PiquelChips
PiquelChips force-pushed the fix/secret-upload-access-precondition branch from a6f0bfa to c598baa Compare November 12, 2026 21:51
…t access change

- The value upload takes `expected_environments` and `expected_previews`,
  the access the caller read. Replacing an existing secret then fails with
  409 `secret_access_conflict` if its access changed meanwhile, checked in
  the same transaction, like the access endpoint.
- `app secret set` and the dashboard's Save send the access they read
  whenever they replace it.
…ncurrent access change

Share the expected-access check, name the write precondition, mention the
expected parameters in the query error, and check the upload conflict over
HTTP.
@PiquelChips
PiquelChips force-pushed the fix/secret-access-precondition branch from c8be3c8 to 1da8f1d Compare November 13, 2026 13:05
@PiquelChips
PiquelChips force-pushed the fix/secret-upload-access-precondition branch from c598baa to 3d4fa80 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