No confirmation prompt on any destructive operation #39

New Issue

2026-08-09T03:40:49+02:00

clawbot commented

2026-08-09 03:40:49 +02:00

From the 1.0 security survey.

Threat

There is no interactive confirmation anywhere in the tool. Searching non-test code for are you sure, [y/N], and bufio.NewReader(os.Stdin) returns zero hits.

secret rm (internal/cli/secrets.go:180-205, RemoveSecret:651) deletes a secret and all its versions with no prompt and no --force flag at all.
secret vault remove (internal/cli/vault.go:144-174, RemoveVault:579) requires --force only when the vault is non-empty and refuses to remove the last vault, but never prompts.
secret unlocker remove (internal/cli/unlockers.go:257-289) has a --force flag, no prompt. Removing the last unlocker without the mnemonic makes the vault permanently unrecoverable — the README says so in capitals.

The README currently documents this as deliberate, comparing it to Unix rm. That comparison does not hold: rm deletes a file the user can often restore from backup, whereas this deletes the only copy of key material that by design exists nowhere else. And unlike rm, there is no -i, no trash, and no undo.

This compounds the unvalidated-name bug (#33): today secret rm .. destroys the entire vault with a single keystroke sequence and no opportunity to reconsider.

Definition of done

Destructive commands prompt for confirmation on an interactive terminal before proceeding: secret rm, secret version rm, secret vault remove, secret unlocker remove.
The prompt names precisely what will be destroyed — the secret name and how many versions, the vault name and its secret count, the unlocker ID and whether it is the last one.
For unlocker remove when it is the last unlocker, the prompt states plainly that the vault becomes unrecoverable without the mnemonic.
--force / -f skips the prompt on every one of these commands, so existing scripted use keeps working with one flag added.
Non-interactive behavior is explicit and safe. When stdin is not a TTY, the command must not silently proceed and must not hang waiting for input. It fails with a clear message telling the user to pass --force. This is the part most likely to be got wrong, and getting it wrong breaks CI pipelines or, worse, makes them destructive.
Tests cover: prompt accepted, prompt declined leaves everything intact, --force bypasses the prompt, and non-TTY without --force fails cleanly without blocking.
README's DANGER sections are updated to describe the new behavior. They currently document the absence of prompts as intentional.
make check green. TODO.md updated in the same commit.

Implementation requirements

Detect TTY on stdin, which is where the answer is read from. Checking stdout is a common mistake and gives the wrong answer whenever output is piped — secret rm foo | tee log would then refuse or hang despite a perfectly interactive user.
Decline must be the default for an empty response. [y/N] means bare Enter cancels.
The existing --force semantics on vault remove and unlocker remove currently mean "override a safety check", and this adds "skip the prompt" to the same flag. Make sure the combined behavior is coherent and documented, and that --force on a last-unlocker removal still does the genuinely dangerous thing the user asked for.
Do not treat this as a substitute for #33. Input validation must reject .. outright whether or not a prompt exists; a prompt that asks "really delete ..?" is not a fix.

From the 1.0 security survey. ## Threat There is no interactive confirmation anywhere in the tool. Searching non-test code for `are you sure`, `[y/N]`, and `bufio.NewReader(os.Stdin)` returns zero hits. - `secret rm` (`internal/cli/secrets.go:180-205`, `RemoveSecret:651`) deletes a secret and **all its versions** with no prompt and **no `--force` flag at all**. - `secret vault remove` (`internal/cli/vault.go:144-174`, `RemoveVault:579`) requires `--force` only when the vault is non-empty and refuses to remove the last vault, but never prompts. - `secret unlocker remove` (`internal/cli/unlockers.go:257-289`) has a `--force` flag, no prompt. Removing the last unlocker without the mnemonic makes the vault permanently unrecoverable — the README says so in capitals. The README currently documents this as deliberate, comparing it to Unix `rm`. That comparison does not hold: `rm` deletes a file the user can often restore from backup, whereas this deletes the only copy of key material that by design exists nowhere else. And unlike `rm`, there is no `-i`, no trash, and no undo. This compounds the unvalidated-name bug (#33): today `secret rm ..` destroys the entire vault with a single keystroke sequence and no opportunity to reconsider. ## Definition of done - Destructive commands prompt for confirmation on an interactive terminal before proceeding: `secret rm`, `secret version rm`, `secret vault remove`, `secret unlocker remove`. - The prompt names precisely what will be destroyed — the secret name and how many versions, the vault name and its secret count, the unlocker ID and whether it is the last one. - For `unlocker remove` when it is the last unlocker, the prompt states plainly that the vault becomes unrecoverable without the mnemonic. - `--force` / `-f` skips the prompt on every one of these commands, so existing scripted use keeps working with one flag added. - **Non-interactive behavior is explicit and safe.** When stdin is not a TTY, the command must not silently proceed and must not hang waiting for input. It fails with a clear message telling the user to pass `--force`. This is the part most likely to be got wrong, and getting it wrong breaks CI pipelines or, worse, makes them destructive. - Tests cover: prompt accepted, prompt declined leaves everything intact, `--force` bypasses the prompt, and non-TTY without `--force` fails cleanly without blocking. - README's DANGER sections are updated to describe the new behavior. They currently document the absence of prompts as intentional. - `make check` green. `TODO.md` updated in the same commit. ## Implementation requirements - Detect TTY on **stdin**, which is where the answer is read from. Checking stdout is a common mistake and gives the wrong answer whenever output is piped — `secret rm foo | tee log` would then refuse or hang despite a perfectly interactive user. - Decline must be the default for an empty response. `[y/N]` means bare Enter cancels. - The existing `--force` semantics on `vault remove` and `unlocker remove` currently mean "override a safety check", and this adds "skip the prompt" to the same flag. Make sure the combined behavior is coherent and documented, and that `--force` on a last-unlocker removal still does the genuinely dangerous thing the user asked for. - Do not treat this as a substitute for #33. Input validation must reject `..` outright whether or not a prompt exists; a prompt that asks "really delete `..`?" is not a fix.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:40:49 +02:00

clawbot referenced this issue

2026-08-09 03:43:16 +02:00

README missing four required sections; TODO.md is stale and describes completed work as pending #46

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/secret#39