Repository navigation
Conversation
When `op account get` fails and the adapter falls back to `op signin`, the resulting session token was passed to each `op item get` as `--session <token>`, so it appeared in the process's command line (and in a `sh -c` command line whenever the backtick string needed a shell). Sign in without `--raw` and read the `OP_SESSION_<id>` variable `op` prints for `eval`, then run `op item get` with that variable in its environment, as an argv array with no shell. Item, vault and field values are now passed to `op` literally rather than through shell quoting. Adds Base#capture_command, which runs an argv array via IO.popen with an env hash and sets $? like backticks, so existing error handling is unchanged.
`BW_SESSION=<token> bw ...` was built as a single backtick string. Ruby handles a leading env assignment itself only when the string has no shell metacharacters; an escaped item name (e.g. one containing a space) made it spawn `sh -c` with the session key in the shell's command line. Run `bw` via capture_command with BW_SESSION in the environment and the arguments as an array.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Small correction to the description: the Bitwarden command reaches |
The 1Password and Bitwarden secrets adapters pass a CLI session token to each command they run. Both put it somewhere it can end up in a process's command line:
op account getfails and the adapter falls back toop signin --raw, the token is passed to everyop item getas--session <token>.BW_SESSION=<token> bw ...and run with backticks. Ruby applies the leading assignment itself only when the string has no shell metacharacters; an escaped item name (one with a space, say) makes it runsh -c "<whole string>"instead.This passes the token through the environment and runs the CLI with an argv array, no shell:
Base#capture_command(*argv, env:), a thinIO.popen(env, argv, &:read)that sets$?like backticks, so the adapters' existing$?.success?checks and error messages are unchanged.op signin --account … --force(without--raw) and reads theOP_SESSION_<id>variable it prints foreval, then runsop item getwith that variable in its environment. Desktop app integration and service account setups don't sign in this way and are unaffected.run_commandtakes its arguments as an array and setsBW_SESSIONin the environment.Values (item, vault, field names) now reach the CLI literally rather than through shell quoting.
The other adapters don't pass credentials on the command line (AWS, GCP, Doppler, Bitwarden Secrets Manager and Passbolt rely on the CLI's own stored login or env token; LastPass and Enpass pass none), so they're left as they are.
Tests
capture_commandwith exact argv and env (newstub_commandhelper), so a token in argv, or a command routed back through backticks, fails the stub.test/secrets/base_adapter_test.rbruns a real process to check env is passed, arguments aren't shell-interpreted, and$?is set.