Skip to content

feat(pass)!: authorize before overwriting secrets #684

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
joe0BAB merged 1 commit into from
Oct 9, 2026
+80 −25
Merged

feat(pass)!: authorize before overwriting secrets #684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 64 additions & 12 deletions plugins/pass/commands/command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,58 +52,58 @@ func Test_SetCommand(t *testing.T) {
t.Parallel()
t.Run("ok", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := execute(t, SetCommand(), mock, "foo=bar=bar=bar")
out, err := execute(t, mustSetCommand(t), mock, "foo=bar=bar=bar")
assert.NoError(t, err)
assert.Empty(t, out)
assertStoredValue(t, mock, "bar=bar=bar")
})
t.Run("from STDIN", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := executeWithStdin(t, SetCommand(), mock, "my\nmultiline\nvalue", "foo")
out, err := executeWithStdin(t, mustSetCommand(t), mock, "my\nmultiline\nvalue", "foo")
assert.NoError(t, err)
assert.Empty(t, out)
assertStoredValue(t, mock, "my\nmultiline\nvalue")
})
t.Run("with --metadata flag", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := execute(t, SetCommand(), mock, "foo=bar", "--metadata", "name=bob", "--metadata", "expiry=2027-03-01")
out, err := execute(t, mustSetCommand(t), mock, "foo=bar", "--metadata", "name=bob", "--metadata", "expiry=2027-03-01")
assert.NoError(t, err)
assert.Empty(t, out)
assertStoredValue(t, mock, "bar")
assertStoredMetadata(t, mock, map[string]string{"name": "bob", "expiry": "2027-03-01"})
})
t.Run("from STDIN JSON with value and metadata", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := executeWithStdin(t, SetCommand(), mock, `{"secret":"bar","metadata":{"name":"bob"}}`, "foo")
out, err := executeWithStdin(t, mustSetCommand(t), mock, `{"secret":"bar","metadata":{"name":"bob"}}`, "foo")
assert.NoError(t, err)
assert.Empty(t, out)
assertStoredValue(t, mock, "bar")
assertStoredMetadata(t, mock, map[string]string{"name": "bob"})
})
t.Run("from STDIN JSON merged with --metadata flag wins on collision", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := executeWithStdin(t, SetCommand(), mock, `{"secret":"bar","metadata":{"name":"bob","extra":"thing"}}`, "foo", "--metadata", "name=alice")
out, err := executeWithStdin(t, mustSetCommand(t), mock, `{"secret":"bar","metadata":{"name":"bob","extra":"thing"}}`, "foo", "--metadata", "name=alice")
assert.NoError(t, err)
assert.Empty(t, out)
assertStoredValue(t, mock, "bar")
assertStoredMetadata(t, mock, map[string]string{"name": "alice", "extra": "thing"})
})
t.Run("invalid --metadata flag (no =)", func(t *testing.T) {
mock := teststore.NewMockStore()
_, err := execute(t, SetCommand(), mock, "foo=bar", "--metadata", "invalid")
_, err := execute(t, mustSetCommand(t), mock, "foo=bar", "--metadata", "invalid")
assert.ErrorContains(t, err, "invalid metadata pair (expected key=value): invalid")
})
t.Run("store error", func(t *testing.T) {
errSave := errors.New("save error")
mock := teststore.NewMockStore(teststore.WithStoreSaveErr(errSave))
out, err := execute(t, SetCommand(), mock, "foo=bar")
out, err := execute(t, mustSetCommand(t), mock, "foo=bar")
assert.ErrorIs(t, err, errSave)
assert.Equal(t, "Error: "+errSave.Error()+"\n", out)
})
t.Run("invalid id", func(t *testing.T) {
errSave := errors.New("save error")
mock := teststore.NewMockStore(teststore.WithStoreSaveErr(errSave))
out, err := execute(t, SetCommand(), mock, "/foo=bar")
out, err := execute(t, mustSetCommand(t), mock, "/foo=bar")
errInvalidID := secrets.ErrInvalidID{ID: "/foo"}
assert.ErrorIs(t, err, errInvalidID)
assert.Equal(t, "Error: "+errInvalidID.Error()+"\n", out)
Expand All @@ -112,12 +112,20 @@ func Test_SetCommand(t *testing.T) {
mock := teststore.NewMockStore(teststore.WithStore(map[store.ID]store.Secret{
store.MustParseID("foo"): pass.NewPassValue([]byte("old")),
}))
out, err := execute(t, SetCommand(), mock, "foo=new")
out, err := execute(t, mustSetCommand(t), mock, "foo=new")
assert.ErrorIs(t, err, keychain.ErrDuplicateItem)
assert.Equal(t, "Error: keychain item already exists\n\n"+duplicateItemHint+"\n", out)
assertStoredValue(t, mock, "old")
})
t.Run("--force overwrites existing secret", func(t *testing.T) {
t.Run("without --force needs no engine", func(t *testing.T) {
mock := teststore.NewMockStore()
out, err := execute(t, mustSetCommand(t, WithSocketPath(deadSocket(t))), mock, "foo=bar")
assert.NoError(t, err)
assert.Empty(t, out)
assertStoredValue(t, mock, "bar")
})
t.Run("--force overwrites existing secret once the engine allows", func(t *testing.T) {
engine := &mockEngine{allow: true}
// Make Save return an error so the test fails if --force does not
// route the call through Upsert.
mock := teststore.NewMockStore(
Expand All @@ -126,18 +134,55 @@ func Test_SetCommand(t *testing.T) {
}),
teststore.WithStoreSaveErr(errors.New("save should not be called when --force is set")),
)
out, err := execute(t, SetCommand(), mock, "foo=new", "--force")
out, err := execute(t, mustSetCommand(t, engineOpts(t, engine)...), mock, "foo=new", "--force")
assert.NoError(t, err)
assert.Empty(t, out)
assert.Equal(t, []string{"authorize foo"}, engine.recorded())
assertStoredValue(t, mock, "new")
})
t.Run("--force fails when the engine denies", func(t *testing.T) {
engine := &mockEngine{}
mock := teststore.NewMockStore(teststore.WithStore(map[store.ID]store.Secret{
store.MustParseID("foo"): pass.NewPassValue([]byte("old")),
}))
out, err := execute(t, mustSetCommand(t, engineOpts(t, engine)...), mock, "foo=new", "--force")
assert.ErrorIs(t, err, client.ErrAccessDenied)
assert.Equal(t, "Error: authorizing: access denied\n", out)
assert.Equal(t, []string{"authorize foo"}, engine.recorded())
assertStoredValue(t, mock, "old")
})
t.Run("--force fails when the engine is unreachable", func(t *testing.T) {
mock := teststore.NewMockStore(teststore.WithStore(map[store.ID]store.Secret{
store.MustParseID("foo"): pass.NewPassValue([]byte("old")),
}))
socket := deadSocket(t)
cmd := mustSetCommand(t, WithTimeout(time.Second), WithSocketPath(socket))
out, err := execute(t, cmd, mock, "foo=new", "--force")
assert.ErrorIs(t, err, client.ErrSecretsEngineNotRunning)
assert.ErrorContains(t, err, "authorizing:")
assert.Contains(t, out, "Start the secrets engine and retry: nothing is listening on "+strconv.Quote(socket)+".")
assertStoredValue(t, mock, "old")
})
t.Run("--force validates the input before asking the engine", func(t *testing.T) {
mock := teststore.NewMockStore()
cmd := mustSetCommand(t, WithTimeout(time.Second), WithSocketPath(deadSocket(t)))
_, err := execute(t, cmd, mock, "foo=bar", "--force", "--metadata", "invalid")
assert.ErrorContains(t, err, "invalid metadata pair (expected key=value): invalid")
assert.NotErrorIs(t, err, client.ErrSecretsEngineNotRunning)
})
t.Run("--force surfaces upsert error", func(t *testing.T) {
errUpsert := errors.New("upsert error")
mock := teststore.NewMockStore(teststore.WithStoreUpsertErr(errUpsert))
out, err := execute(t, SetCommand(), mock, "foo=bar", "--force")
cmd := mustSetCommand(t, engineOpts(t, &mockEngine{allow: true})...)
out, err := execute(t, cmd, mock, "foo=bar", "--force")
assert.ErrorIs(t, err, errUpsert)
assert.Equal(t, "Error: "+errUpsert.Error()+"\n", out)
})
t.Run("rejects an invalid option", func(t *testing.T) {
cmd, err := SetCommand(WithTimeout(-time.Second))
require.EqualError(t, err, "request timeout duration cannot be negative")
assert.Nil(t, cmd)
})
}

func Test_ListCommand(t *testing.T) {
Expand Down Expand Up @@ -362,6 +407,13 @@ func Test_GetCommand(t *testing.T) {
})
}

func mustSetCommand(t *testing.T, options ...ClientOption) *cobra.Command {
t.Helper()
cmd, err := SetCommand(options...)
require.NoError(t, err)
return cmd
}

func mustGetCommand(t *testing.T, options ...ClientOption) *cobra.Command {
t.Helper()
cmd, err := GetCommand(options...)
Expand Down
2 changes: 1 addition & 1 deletion plugins/pass/commands/keychain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func TestStoreCommandsHintOnLockedKeychain(t *testing.T) {
args []string
}{
{"get", mustGetCommand(t), teststore.NewMockStore(teststore.WithStoreGetErr(locked)), []string{"foo"}},
{"set", SetCommand(), teststore.NewMockStore(teststore.WithStoreSaveErr(locked)), []string{"foo=bar"}},
{"set", mustSetCommand(t), teststore.NewMockStore(teststore.WithStoreSaveErr(locked)), []string{"foo=bar"}},
{"ls", ListCommand(), teststore.NewMockStore(teststore.WithStoreGetAllErr(locked)), nil},
{"rm", mustRmCommand(t, engineOpts(t, &mockEngine{allow: true})...), teststore.NewMockStore(
teststore.WithStore(map[store.ID]store.Secret{store.MustParseID("foo"): pass.NewPassValue([]byte("bar"))}),
Expand Down
27 changes: 15 additions & 12 deletions plugins/pass/commands/set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,18 @@ var setExample string
//go:embed set_long.md
var setLong string

type setOpts struct {
metadata []string // raw "key=value" strings from --metadata flag
force bool // if true, overwrite existing setPayload instead of erroring
}

type stdinPayload struct {
Secret string `json:"secret"`
Metadata map[string]string `json:"metadata,omitempty"`
}

func SetCommand() *cobra.Command {
opts := setOpts{}
func SetCommand(options ...ClientOption) (*cobra.Command, error) {
copts, err := parseClientOptions(options...)
if err != nil {
return nil, err
}
metadata := []string{}
var force bool
cmd := &cobra.Command{
Use: "set id[=value]",
Aliases: []string{"store", "save"},
Expand All @@ -66,7 +66,7 @@ func SetCommand() *cobra.Command {
if err != nil {
return err
}
flagMeta, err := parseMetadataFlags(opts.metadata)
flagMeta, err := parseMetadataFlags(metadata)
if err != nil {
return err
}
Expand Down Expand Up @@ -104,16 +104,19 @@ func SetCommand() *cobra.Command {
return err
}
}
if opts.force {
if force {

@docker-agent docker-agent Oct 9, 2026

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.

[low] Secret value collected from user before authorization check when using --force

When --force is set, the RunE closure fully constructs the secret payload — prompting the user for a value (via prompt.ReadMasked), reading from stdin, unmarshalling, and merging metadata — before calling authorizeAccess at line 108. If authorization is denied, the user has already been prompted to enter a new secret for no effect.

The PR description says "stores nothing when access is denied", which is accurate, but the prompt has already occurred. Moving the authorizeAccess call to before the prompt/read block would make the UX consistent with that intent:

if force {
    if err := authorizeAccess(cmd.Context(), copts, id); err != nil {
        return err
    }
}
// ... then read/prompt for the secret value
Confidence Score
🟢 strong 100/100

if err := authorizeAccess(cmd.Context(), copts, id); err != nil {
return err
}
return kc.Upsert(cmd.Context(), id, pv)
}
return kc.Save(cmd.Context(), id, pv)
},
}
flags := cmd.Flags()
flags.StringArrayVar(&opts.metadata, "metadata", nil, "Non-sensitive key=value metadata (repeatable)")
flags.BoolVarP(&opts.force, "force", "f", false, "Overwrite existing secret if it already exists")
return wrapKeychainErrors(cmd)
flags.StringArrayVar(&metadata, "metadata", nil, "Non-sensitive key=value metadata (repeatable)")
flags.BoolVarP(&force, "force", "f", false, "Overwrite existing secret if it already exists")
return wrapEngineErrors(wrapKeychainErrors(cmd)), nil
}

func parseMetadataFlags(raw []string) (map[string]string, error) {
Expand Down
Loading