Repository navigation
feat(pass)!: authorize before overwriting secrets - #684
Conversation
3470f74 to
d2d11d8
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
| 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 wrapKeychainErrors(cmd), nil |
There was a problem hiding this comment.
[medium] Missing wrapEngineErrors wrapper when --force triggers authorizeAccess
SetCommand now calls authorizeAccess inside RunE when --force is used, which can return a *client.ConnectError. The wrapEngineErrors helper wraps RunE to call withEngineHint on any error, appending an actionable message (e.g. "start Docker Desktop") when a ConnectError carries a hint. Without it, users running set --force against an unreachable engine will see a bare connection error with no guidance.
Both GetCommand (get.go:82) and RmCommand (rm.go:68) already return wrapEngineErrors(wrapKeychainErrors(cmd)). This PR introduced the authorizeAccess call into SetCommand but didn't apply the same outer wrapper.
| return wrapKeychainErrors(cmd), nil | |
| return wrapEngineErrors(wrapKeychainErrors(cmd)), nil |
| Confidence | Score |
|---|---|
| 🟢 strong | 100/100 |
| } | ||
| } | ||
| if opts.force { | ||
| if force { |
There was a problem hiding this comment.
[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 |
set --force asks the engine for the secret it is about to overwrite and stores nothing when access is denied. SetCommand takes the same client options as get, rm and run and returns an error.
d2d11d8 to
d4013b8
Compare
3 participants
set --force asks the engine for the secret it is about to overwrite and stores nothing when access is denied. SetCommand takes the same client options as get, rm and run and returns an error.
Closes docker/secrets-engine-private#729.