Skip to content

feat: edit existing secrets in-place for sops-nix and agenix - #658

Open
Scott McMaster (scottmcmaster) wants to merge 2 commits into
08-12-scott-age-secret-add-delfrom
08-13-scott-edit-secrets
Open

feat: edit existing secrets in-place for sops-nix and agenix#658
Scott McMaster (scottmcmaster) wants to merge 2 commits into
08-12-scott-age-secret-add-delfrom
08-13-scott-edit-secrets

Conversation

@scottmcmaster

@scottmcmaster Scott McMaster (scottmcmaster) commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hook up the edit functionality for secrets for both SOPS and agenix.

ATTENTION

  1. Please note the pecularity specific to SOPS where we need to deduce the repo path from the nix store path for the secret, and if you have any better ideas, please let me know.
  2. There is a subtle difference in how recipients are handled which you should double-check, this is reflected in the UI:
    a. For age, the updated value keeps the same recipients.
    b. For SOPS, we use what's currently in the repo's .sops.yaml.

Also did some refactoring of the YAML-related utility methods, and enhanced some of the unit tests.

Screenshots (note that I hid all of the extra controls from the "add" flow since they were distracting and not directly relevant to the "edit" flow but per prelint comments recipients are included):

Screenshot 2026-08-13 at 3 35 18 PM Screenshot 2026-08-13 at 3 35 32 PM

Test Plan

New unit tests in Rust and TypeScript, manually tested the SOPS and age paths in the UI.

  • No test plan needed

Docs

  • Docs updated (companion PR in darkmatter/nixmac-web: #___)
  • No docs update needed

Copy link
Copy Markdown
Collaborator Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@scottmcmaster Scott McMaster (scottmcmaster) changed the title scott-edit-secrets feat: edit existing secrets in-place for sops-nix and agenix Aug 13, 2026
@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor
Warnings
⚠️

No Linear issue ID found in this PR's title, description, or branch name (expected something like ENG-123). Add one so this work is traceable in Linear, or add #no-linear to the PR description to acknowledge it's intentionally untracked.

Messages
📖 No docs update needed — acknowledged.

📋 PR Overview

Lines changed 1251 (+1059 / -192)
Files 3 added, 12 modified, 0 deleted
Draft / WIP no
Has Test Plan yes
Linear issue no
No Test Plan Needed no
New UI components no
New Storybook stories no
New Rust modules yes (1)
New TS source files no
New tests yes (2)
package.json touched no
Cargo.toml touched no
Infra / CI touched no

🔬 Coverage

Report Lines Statements Functions Branches
apps/native/coverage/coverage-summary.json 36.3% 35.9% 31.4% 30.5%

Generated by 🚫 dangerJS against 6bd3a30

@darkmatter

darkmatter Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

🎨 Storybook preview

Open Storybook preview

Updated for 6bd3a30


🧭 Story changes

Compared to 08-12-scott-age-secret-add-del (snapshot diff at story level):

✏️ Changed stories (8)

💡 Update snapshots ↗ to regenerate baselines and open a PR.


⚠️ Detected UI changes (5)

These stories' HTML snapshots changed. I've added screenshots + links to the changed stories below. Review them carefully then accept the changes to regenerate baselines and include them in this PR:

Flows/Evolve › Playground

Flows/Evolve › Playground

Flows/Evolve › 1. Begin (idle)

Flows/Evolve › 1. Begin (idle)

Flows/Evolve › 2. Evolving (progress)

Flows/Evolve › 2. Evolving (progress)

Flows/Evolve › Evolving With Error Event

Flows/Evolve › Evolving With Error Event

Flows/Evolve › 3. Review (changes generated)

Flows/Evolve › 3. Review (changes generated)


Accept UI changes

  • Click here to accept these changes

Alternatively, you can run bun run test:update-snapshots locally to re-generate the baselines and then push the changes to this PR.

What does this do?

The screenshots above show UI changes detected by the Storybook
snapshot tests run on this PR. Each image is the rendered output of
a Storybook story from the code in this PR branch; the snapshot
test compared it against the committed baseline in
__snapshots__/ and flagged the difference.

Checking the box tells the darkmatter[bot] to regenerate the
baselines from this PR's current code and commit them directly to
this branch. The new baselines become the source of truth for
future runs — only accept after confirming the visual changes are
intentional.

Comparison baseline: the committed __snapshots__/ files on this
PR branch (carried forward from develop). Accept updates them in
place on this branch.

@prelint

prelint Bot commented Aug 13, 2026

Copy link
Copy Markdown

Ship with changes Secret edit flow: recipients shown and agenix edit button gated on resolution

Product decisions in this change

Agree 1. Agenix secrets hide the "Edit value" button when recipients cannot be resolved, displaying an in-place explanation instead of a late failure after the user has entered a new value.

This directly addresses the gap the prior review called out. Two distinct states now each produce their own specific message: resolution failed entirely, and resolution succeeded but returned no recipients. The tests cover both states. No extra backend query is needed at render time because the recipient resolution state already lives in the vault entry. The behavior is correct and the implementation matches the intent.

Agree 2. The edit form shows the recipients who will receive access to the new value, in read-only mode, before the user confirms the operation.

A user replacing a compromised secret needs to confirm that no extra identities have gained access. The display uses the existing recipient component. Unregistered recipients appear by their raw public key labeled "Unclassified recipient", which is an honest fallback. The asymmetry between agenix (preserves the file's recorded recipients) and SOPS (applies current .sops.yaml rules) is explicitly labeled in the copy. This addresses the recipient-display gap from the prior review.

Agree with concerns 3. Editing a SOPS secret re-encrypts every key in the shared file, not only the key being edited, so all sibling secrets in the same file also get new recipients under the current `.sops.yaml` rules.

This is inherent to SOPS file-level encryption and cannot be changed without switching the secrets storage model. The UI copy accurately states that the update uses the recipients registered in .sops.yaml. The concern is that a user who sees that copy might believe only their specific key is affected. A user editing github-token in a file that also stores slack-webhook and deploy-key may not realize all three just got re-keyed. If .sops.yaml changed recently, this silently changes access for secrets the user did not intend to touch. A one-sentence note in the recipients section ("All secrets in this file share these recipients") would close the gap without backend changes.

Agree with concerns 4. The SOPS "Edit value" button remains always visible regardless of whether the backend can locate the encrypted source file, so the failure occurs after the user has already typed a new value.

Checking source-file matchability at render time would require a new backend query. The current approach accepts a late failure to avoid that extra complexity. The concern is not the late timing itself but the quality of the error the user receives at that point: a generic message with no hint that running a rebuild will refresh the Nix store artifact and resolve the mismatch. The button behavior is acceptable; the error message at the failure point is the gap.

Agree with concerns 5. When SOPS source-file recovery fails because the repository was re-encrypted after the last Nix evaluation, the error message provides no guidance on how to recover.

The conservative refusal is correct; no wrong file is ever overwritten. The gap is user guidance. Adding one sentence to the error string -- explaining that the mismatch likely means the repository was re-encrypted since the last evaluation and that running a rebuild will resolve it -- lets the user recover without external help. This remains unaddressed from the prior review.

Agree with concerns 6. A dry build runs before the edited encrypted file is committed, with no documentation of what the check verifies or does not verify.

The check confirms that the Nix configuration evaluates cleanly. It does not verify that encryption succeeded or that the new value is correct. Future contributors may add logic based on a false belief that this check validates the encryption result. A one-line comment stating the actual scope (Nix evaluation, not encryption) removes that ambiguity. This also remains unaddressed from the prior review.

Since the last review

  • Addressed in code: SOPS edit silently adopts the current .sops.yaml recipient list, which may differ from the recipients on the file when it was originally encrypted. (The edit form now shows the current .sops.yaml recipients for SOPS edits and labels them with the copy "The updated value uses the recipients registered in the repository's .sops.yaml.")
  • Addressed by explanation: The edit UI reuses the add-secret form with fields hidden via an editing flag, serving two divergent user models in one component. (No new mode branches were added; the prior review set the threshold for splitting at a third branch, which has not been reached.)
  • Still open: When SOPS content-match recovery fails, no improved error message guides the user to run a rebuild and retry. (No changes to the error strings in resolve_sops_source_file or match_repo_file_by_name_and_contents appear in this diff.)
  • Still open: The dry build before commit is undocumented regarding what it verifies and does not verify. (No explanatory comment was added to verify_dry_build_for_secret_edit or its call sites in edit_age_secret and edit_sops_secret.)
  • Addressed in code: Should the UI compare current .sops.yaml recipients against the existing file's SOPS metadata and warn the user when they differ? (The edit form now displays the recipients that will be applied, with explicit copy stating they come from .sops.yaml, giving the user full visibility before confirming the operation.)
  • Still open: For a SOPS secret in a shared YAML file, can a concurrent remote change to a sibling key be silently overwritten when the edit decrypts, modifies, and re-encrypts? (The edit_sops_secret path still calls ensure_clean_repo, which checks only the local working tree. Remote changes that have not been pulled are not detected before the re-encrypt and commit.)

Open questions

  • Should the edit UI note that editing one key in a shared SOPS file will re-encrypt all secrets in that file, so users can anticipate the full scope before confirming?

  • Is there a plan to check SOPS source-file matchability before the user enters a value, even as a lightweight backend query, so the edit button can be conditionally gated for SOPS secrets too?

  • Has the exact error string produced when SOPS source-file matching fails been reviewed to confirm it is safe to surface to users directly? It currently references internal Nix store paths and matching logic.

Recommendation

Ship with changes
The two gaps the prior review explicitly asked for -- recipient display in the edit flow and proactive button gating for agenix secrets with unresolved recipients -- are now implemented correctly. Three items remain open: the SOPS content-match error message still provides no user guidance on recovery, the dry build scope is still undocumented, and editing a SOPS secret silently re-encrypts all sibling keys in the same file without telling the user. All three are low-effort fixes, and the SOPS error message in particular has direct user impact and should be addressed before merge.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR wires up the previously stubbed "edit secret value" flow for both the sops-nix and agenix backends. On the backend it adds an edit_secret command that decrypts the existing encrypted file, replaces the requested value in place (preserving the declaration, file location, backend, and recipients), re-encrypts, runs a dry darwin-rebuild check, and commits — with best-effort rollback on failure. On the frontend it reuses AddSecretView in an "edit" mode that hides the add-only controls, and replaces the "not implemented" edit button with a real one. It also extracts the YAML helpers (replace_yaml_path, remove_yaml_path, validate_yaml_syntax) into a new shared yaml_utils module.

Changes:

  • New Rust edit_secret backend (SOPS + agenix), including conservative recovery of a SOPS repo source file from its evaluated /nix/store path, plus the editSecret orpc route and EditSecretResult shared type.
  • Frontend edit flow: AddSecretView gains an optional secret prop / buildEditRequest, SecretDetailView exposes onEdit, and SecretsManagement routes the new edit view to editSecret.
  • Refactor: YAML utilities moved to yaml_utils.rs with expanded unit tests.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
apps/native/src-tauri/src/secrets/secrets_management.rs Adds edit_secret/edit_sops_secret/edit_age_secret, SOPS store-path source recovery, and tests
apps/native/src-tauri/src/yaml_utils.rs New module hosting YAML path replace/remove/validate helpers + tests
apps/native/src-tauri/src/evolve/file_ops.rs Moves validate_yaml_syntax (and its tests) into yaml_utils
apps/native/src-tauri/src/main.rs Declares the new yaml_utils top-level module
apps/native/src-tauri/src/orpc/secrets.rs Adds editSecret input type, handler, and route
apps/native/src-tauri/src/shared_types/secrets_management.rs Adds EditSecretResult
apps/native/src/ipc/orpc-bindings.ts Generated EditSecretInput/EditSecretResult + editSecret client
apps/native/src/components/widget/secrets/add-secret-view.tsx Edit mode support and buildEditRequest
apps/native/src/components/widget/secrets/add-secret-view.test.ts Tests for buildEditRequest
apps/native/src/components/widget/secrets/secret-detail-view.tsx Replaces stub edit button with real onEdit; removes canEdit
apps/native/src/components/widget/secrets/secrets-management.tsx Routes edit view and calls editSecret
apps/native/src/components/widget/secrets/types.ts Adds edit view kind and edit apply origin

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread apps/native/src/components/widget/secrets/secret-detail-view.tsx Outdated
Comment thread apps/native/src/components/widget/secrets/add-secret-view.tsx Outdated
@scottmcmaster

Copy link
Copy Markdown
Collaborator Author

Ship with changes Secret edit flow: in-place value replacement for SOPS and agenix

Product decisions in this change

Agree 1. Editing a secret value is a first-class operation with its own endpoint and commit, separate from adding a new secret.

Separating edit from add is correct. The two operations have different preconditions. Add creates a declaration and an encrypted file. Edit replaces only the encrypted content. Combining them would force the add path to handle cases it was not designed for, such as preserving existing recipients. A dedicated commit message also makes the history legible.

Agree with concerns 2. The "Edit value" button is always visible on the secret detail view, regardless of whether the edit can succeed for that secret.

The always-visible button keeps the UI uniform, but a user with an agenix secret that has unresolved recipients starts the edit flow and sees a failure only at the build-and-commit step. A user who has typed a new credential value and submitted the form receives a backend error. The error provides no guidance on how to fix the underlying problem.

Option What it gives users What it costs Effort to change later
Always visible (current) Uniform UI Late failure with no guidance for unresolved recipients Low
Conditionally visible Early prevention and a clear message Requires checking recipient resolution state at render time Low
The current approach works if unresolved recipients are rare. It becomes harmful if users regularly encounter this state with no explanation.

Agree with concerns 3. The edit UI reuses the add-secret form with the name, backend selector, runtime path, and recipient sections hidden when in edit mode.

The shared component avoids duplicating code for a first implementation. But it now serves two divergent models: configure a new secret versus replace an existing value. Mode branches already appear in several places inside the component. Every future change to the add flow must consider whether it also applies to the edit flow. The current approach is acceptable. A dedicated edit component becomes the right choice when a third branch appears.

Agree 4. The review screen shows a symbolic placeholder diff representing the replaced value, rather than the actual encrypted bytes.

Encrypted file diffs are ciphertext and carry no useful information for a human reviewer. The placeholder honestly describes what changed without pretending the user can verify the raw bytes. This is consistent with how secrets management tools generally handle the review problem.

Agree 5. Editing a secret preserves the exact recipients already recorded on the encrypted file, and does not fall back to the currently registered recipient list.

This is the correct security model. A fallback to the current recipient list could silently add or remove identities that were not on the original file. For agenix, each file can have a distinct recipient set, so the current-registry fallback would be a silent security change. Refusing the edit when recipients cannot be resolved is also correct. A conservative failure is safer than a guess.

Agree with concerns 6. The edit flow does not show users which recipients can decrypt the new value.

Recipients do not change during an edit, so displaying them is not strictly required. But a user who is replacing a compromised secret wants confirmation that the new value is encrypted to the right identities and that no extra recipients have gained access. The review screen currently offers no security-relevant information beyond the file path and a symbolic placeholder.

Option What it gives users What it costs Effort to change later
No recipient display (current) Simpler edit form No confirmation of access scope Low
Read-only recipient display Explicit confirmation of who can decrypt the new value Minor added UI Low
Recipient data is already available in SecretEntry. The list component already exists in the add flow. A read-only display requires no backend changes.

Agree with concerns 7. The app locates the editable SOPS file by matching its filename and encrypted contents when the Nix configuration points to an internal store location rather than the repository directly.

When Nix copies a SOPS file into the store, it severs the link to the repository source. The PR recovers it by matching the store artifact name and bytes against files in the repository. Refusing on zero or multiple matches is the correct safety boundary. No wrong guess is ever made.

The fragility is in the content match. If the repository SOPS file has been re-encrypted since the last Nix evaluation, its bytes no longer match the store artifact. Recipient rotation is one common cause of this state. The edit then fails with a message saying the source cannot be found, even though the file clearly exists. The user has no obvious path forward other than running a rebuild to refresh the store artifact first. The PR author acknowledges this limitation. A follow-up should surface a clearer error message that explains the mismatch and tells the user what to do.

Agree with concerns 8. A dry build check runs before the edited encrypted file is committed.

Editing only the encrypted content does not change any Nix declarations. A dry build cannot detect a bad SOPS encryption or a corrupted agenix file. If the intent is to confirm the Nix configuration still evaluates cleanly after any recent unrelated changes, the check provides that assurance. If the intent is to validate the edited secret itself, it does not. The check is harmless but its rationale should be documented so future contributors understand what it verifies and what it does not.

Agree 9. An agenix edit fails explicitly with an error when the existing recipient set cannot be resolved, rather than proceeding with the currently registered recipients.

The agenix model allows per-file recipient lists that differ from the global registry. Re-encrypting with the current registry would be a silent security change. Refusing and surfacing a clear error is the correct behavior. Tests cover both the unresolved and empty-recipient cases.

Open questions

  • What happens when a user tries to edit a SOPS secret whose file was re-encrypted in the repository after the last Nix evaluation? The content-matching recovery will fail with a confusing message. Is there a planned path forward, or must the user run a rebuild first?
  • Should the edit button be disabled with a tooltip when an agenix secret has unresolved recipients, rather than failing at commit time after the user has already entered a new value?
  • Is there a plan to show recipient information as read-only during the edit flow, so users can confirm who gains access to the new value before committing?
  • The dry build before committing cannot validate the encrypted output itself. Is the intent to catch unrelated configuration drift, or is a separate validation of the encrypted file planned?
  • For a SOPS secret in a shared YAML file with multiple keys: if another key in the same file was changed in the repository after the last Nix evaluation, does the edit operate on the current repository version or on the Nix store version? Could a concurrent change by another contributor be silently overwritten?

Recommendation

Ship with changes The security decisions are correct: recipients are preserved exactly, refusals are conservative, and the SOPS source recovery is honest about ambiguity. Two gaps are worth addressing before merge. The edit review screen shows no recipient information, removing the one confirmation a user replacing a compromised secret would most want. The always-visible edit button also produces a late failure for agenix secrets with unresolved recipients, with no early guidance to the user.

I addressed the two gaps:

  1. Bring back recipients view in the Edit flow.
  2. Hide the Edit button proactively when age public recipients can't be resolved on the backend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants