Skip to content

1Password: read Environments with the stable op CLI - #1241

Merged
theoephraim merged 6 commits into
dmno-dev:mainfrom
lukeredpath:1password-stable-cli-environments
Oct 28, 2026
Merged

theoephraim merged 6 commits into
dmno-dev:mainfrom
lukeredpath:1password-stable-cli-environments

Conversation

@lukeredpath

@lukeredpath lukeredpath commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

opLoadEnvironment() fails on every stable 1Password CLI when the plugin uses op (desktop app auth, or useCliWithServiceAccount). It calls op environment read, which only exists in beta builds. Stable op added Environments support in v2.41.0, but only as op run --environment (release notes: "makes the --environment flag for op run generally available").

The plugin now reads Environments only through op run --environment <id> --no-masking -- <runtime> -e <print env as JSON>. Recent betas support it too. An op that rejects the flag (older than v2.41.0, or an older beta) gets an error asking to update with op update, instead of a second code path for op environment read.

Detail

  • The child is the current JS runtime (process.execPath -e), not env -0. That keeps Windows working, for the same reason the batch reads moved to op inject. The standalone binary gets BUN_BE_BUN=1 so its -e runs as bun.
  • op gets a minimal env. It runs with the same env op inject already used (PATH, USER, HOME, XDG_CONFIG_HOME, proxy vars, OP_BIOMETRIC_UNLOCK_ENABLED, the service account token when opted in), now shared as opCliBaseEnv(), plus the Windows profile vars op uses to find its config when HOME is unset (USERPROFILE, HOMEDRIVE, HOMEPATH, LOCALAPPDATA, APPDATA). OP_ACCOUNT and OP_CONFIG_DIR are not passed, matching op inject: the account comes from the plugin's account option. Nothing else from the parent process reaches the child, so runtime config (NODE_OPTIONS, NODE_COMPILE_CACHE, BUN_OPTIONS, ...) and unrelated op:// references can't affect it.
  • Membership is marked, not inferred. op run merges the Environment over its own env. Each of those few names is also set to a per-call sentinel through --env-file, which op run ranks below Environments and applies only to the child. In the child, a name holds a non-sentinel value only if the Environment set it, even when the values are identical.
  • Platform additions are excluded. macOS adds XPC_FLAGS and __CF_USER_TEXT_ENCODING when any process starts, so a direct spawn of the runtime with the same env finds those names first. That probe runs alongside op run and is memoized per process. SystemRoot and windir keep their real values, because Windows processes need them.
  • Audit fix (unrelated): next 16.3.8 in the lockfile hit two critical advisories (GHSA-2464-fhr4-w9xw, GHSA-673q-mjm6-r5xr) and failed CI's audit. The nextjs integration now has next@^16.4.3 as an explicit devDependency (it was only an auto-installed peer).
  • Docs and changeset: the README and website note now say v2.41.0+ instead of saying a beta is required, plus a patch changeset.

Verification

  • bun run test:ci in packages/plugins/1password: 41 tests pass. The fake op implements op run --environment with real op run's precedence and inherited-reference resolution, plus a too-old mode. New cases cover a value equal to one passed to op, an unrelated inherited op:// reference, a parent NODE_OPTIONS that would break the child, Windows profile vars reaching op without being returned, and the too-old error.
  • End to end on macOS with real op (2.41.0 stable, and 2.34.0 beta) and desktop app auth: a real Environment's variables resolved. With NODE_COMPILE_CACHE, NODE_V8_COVERAGE and a decoy value for the Environment's variable set in the parent, only the Environment's value came back and no sentinel-named directories were created.

Not verified: Windows, the standalone binary's BUN_BE_BUN path, and useCliWithServiceAccount against real op.

Stable op releases (v2.41.0+) read Environments only through `op run --environment`, and `op environment read` exists only in beta builds. With desktop app auth or useCliWithServiceAccount, opLoadEnvironment() failed on every stable op.

When op has no `environment` command, the plugin now runs `op run --environment` with the current JS runtime as the child, so it also works on windows and in the standalone binary. It recovers the Environment's variables by diffing the child's env against the inherited env.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017wRXVCHSXuGwsJBugHqVuR

@pullfrog pullfrog Bot 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.

Important

The stable fallback cannot reliably distinguish selected-Environment variables from inherited variables. This can omit requested values or load unrelated secrets.

Reviewed changes
Reviewed the stable CLI fallback, its callers, tests, and documentation.

  • Stable CLI fallback: Retains op environment read and falls back to op run --environment with a JavaScript subprocess and standalone Bun handling.
  • Output and errors: Normalizes both CLI paths to JSON and adds upgrade guidance for unsupported CLI versions.
  • Tests and documentation: Adds fake stable-CLI coverage and updates the documented version requirement. The build, all 39 plugin tests, and typecheck passed; real authenticated CLI, Windows, and standalone binary behavior were not exercised.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using gpt-6.1-sol | 𝕏

Diffing the child env against process.env dropped Environment variables whose value matched an inherited one, and `op run` also resolved unrelated inherited op:// references, which could leak into the result or fail the read.

Every inherited name is now set to a per-call sentinel through --env-file, which op run ranks below Environments, so a non-sentinel value can only come from the Environment. Inherited op:// references are no longer passed to op. A direct spawn with the same sentinels finds names the platform rewrites at process start (macOS XPC_FLAGS and __CF_USER_TEXT_ENCODING), and those are ignored.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017wRXVCHSXuGwsJBugHqVuR

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes
Reviewed the membership-isolation fix since a610f3c6, including its subprocess handling and regression coverage.

  • Replaced value comparison: Marked inherited names through --env-file so selected Environment values remain present even when they match the parent environment.
  • Isolated inherited references: Removed inherited op:// references before invoking the stable CLI and added runtime rewrite detection plus temporary-file cleanup.
  • Expanded regression coverage: Updated the fake CLI's precedence and reference-resolution behavior and added tests for both prior failure cases. The build, all 41 plugin tests, and typecheck passed. Authenticated CLI, macOS, Windows, and standalone binary execution were not exercised in this review.

Pullfrog  | View workflow run | Using gpt-6.1-sol | 𝕏

@theoephraim

Copy link
Copy Markdown
Member

Thanks! Will have this out in the next release.

…ent first

op run hands its own env to the child, which is a JS runtime, so parent runtime config
(NODE_COMPILE_CACHE, BUN_OPTIONS, ...) affected it even when marked. op now gets the same
minimal env op inject uses (shared as opCliBaseEnv). Stable op and recent betas both support
op run --environment, so it goes first and only older betas fall back to op environment read.
The platform env probe runs alongside op run and is memoized.
Stable op (v2.41.0+) and recent betas support op run --environment, so older betas now get an
error asking them to update instead of a second code path.

@pullfrog pullfrog Bot 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.

Important

The new minimal environment drops CLI configuration inputs that Environment reads previously inherited, which can prevent configuration discovery or select the wrong account.

Reviewed changes
Reviewed the changes since 771b5e3e, including subprocess isolation and the removal of the beta-only command path.

  • Restricted inherited variables: Shared the existing op inject environment helper with Environment reads and blocked parent runtime configuration from reaching the child.
  • Memoized platform detection: Ran the platform-variable probe concurrently with op run and cached its result by environment shape.
  • Removed the beta fallback: Used op run --environment exclusively and updated upgrade guidance, documentation, and CLI regression tests.

The plugin build, all 40 tests, and typecheck passed. Authenticated CLI, Windows, and standalone binary execution were not exercised in this review.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using gpt-6.1-sol | 𝕏

PATH: process.env.PATH!,
...process.env.USER && { USER: process.env.USER },
...process.env.HOME && { HOME: process.env.HOME },
...process.env.XDG_CONFIG_HOME && { XDG_CONFIG_HOME: process.env.XDG_CONFIG_HOME },

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.

[P2] Keep CLI configuration inputs in the isolated environment

This allowlist drops OP_CONFIG_DIR and OP_ACCOUNT, so Environment reads that previously respected a custom configuration directory or ambient account selection now use the defaults instead. It also drops USERPROFILE and HOMEDRIVE/HOMEPATH, which 1Password's documented Windows home-discovery mechanism needs when HOME is unset; retaining only SystemRoot and windir does not preserve configuration discovery.

Technical details
# Preserve CLI configuration discovery without inheriting runtime settings

## Affected sites
- `packages/plugins/1password/src/cli-helper.ts:38-52`: `opCliBaseEnv()` excludes the configuration/account inputs.
- `packages/plugins/1password/src/cli-helper.ts:253-255`: Environment reads use this allowlist and add Windows system-directory variables, but not profile-directory variables.
- `packages/plugins/1password/src/cli-helper.ts:273-285,305-306`: the invocation supplies no `--config` replacement and supplies `--account` only when the plugin instance explicitly sets it.

## Evidence
- https://www.1password.dev/cli/config-directories.md documents `OP_CONFIG_DIR` immediately after `--config` in configuration-directory precedence and specifies go-homedir for home discovery.
- https://developer.1password.com/docs/cli/environment-variables/ documents `OP_ACCOUNT` as the default account, overridden by an explicit `--account`.
- https://raw.githubusercontent.com/mitchellh/go-homedir/main/homedir.go shows Windows discovery checking `HOME`, then `USERPROFILE`, then `HOMEDRIVE` plus `HOMEPATH`, and returning an error when those are absent.
- An isolated helper probe confirmed all five names are absent from `opCliBaseEnv()` even when set in the parent. The fake CLI tests do not model configuration discovery or ambient account selection.
- Concrete affected cases: a custom CLI configuration selected through `OP_CONFIG_DIR`; a multi-account desktop setup selecting account B through `OP_ACCOUNT` while account A is the CLI default; Windows with `HOME` unset and configuration under the user profile.

## Required outcome
- Preserve the supported directory/account inputs needed by `op`, including Windows profile discovery, while continuing to exclude unrelated runtime configuration and secrets.
- Keep any added inherited names covered by the existing sentinel membership isolation so they do not appear in the returned Environment unless the Environment defines them.
- Add focused coverage for these configuration inputs at the CLI invocation boundary.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Windows profile vars: fixed in da482fc. opCliBaseEnv() now passes USERPROFILE, HOMEDRIVE, HOMEPATH, LOCALAPPDATA and APPDATA when set, so op can find its config when HOME is unset. The same helper is used by op inject, so item reads get this too. The child still sees these names as sentinels, so they are never returned, and a new test checks they reach op.

OP_ACCOUNT and OP_CONFIG_DIR are left out on purpose. Item reads through op inject have never passed them, so Environment reads now behave the same way. The account to read from belongs in the schema (the plugin's account option) rather than depending on whatever the user's shell has set.

HOME is usually unset on windows, so op falls back to USERPROFILE (or HOMEDRIVE + HOMEPATH)
and the AppData dirs. This also applies to op inject, which shares opCliBaseEnv.

@pullfrog pullfrog Bot 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.

ℹ️ No new issues found in this delta. The earlier configuration-input thread remains open because its Windows concern is fixed, while OP_ACCOUNT and OP_CONFIG_DIR remain intentionally excluded.

Reviewed changes
Reviewed the Windows profile-variable fix since 5439e1d3.

  • Restored Windows profile inputs: Passed USERPROFILE, HOMEDRIVE, HOMEPATH, LOCALAPPDATA, and APPDATA through the shared CLI environment helper, retaining sentinel isolation for Environment results.
  • Added boundary coverage: Extended the fake CLI to require inherited variables and verified that profile inputs reach op without appearing in the returned Environment.

The plugin build, all 41 tests, and typecheck passed. Native Windows, authenticated CLI, and standalone binary execution were not exercised.

Pullfrog  | Fix it ➔ | View workflow run | Using gpt-6.1-sol | 𝕏

next@16.3.8 was only in the lockfile as an auto-installed peer of the nextjs integration and is
affected by GHSA-2464-fhr4-w9xw and GHSA-673q-mjm6-r5xr (fixed in 16.4.1). It is now an explicit
devDependency so its version is pinned on purpose.

@pullfrog pullfrog Bot 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.

ℹ️ No new issues found in this delta. The earlier configuration-input thread remains partially addressed and open, so this is a non-approving review.

Reviewed changes
Reviewed the dependency update since da482fc8; the 1Password implementation is unchanged.

  • Updated Next.js: Added next@^16.4.3 as an explicit development dependency while retaining the integration's next >=14 peer requirement.
  • Refreshed dependency resolution: Updated Next.js, its matching environment and SWC packages, and the associated Sharp tree; deduplicated Lightning CSS lockfile entries.

The Next.js integration build, all 80 tests, and typecheck passed. bun audit reports no Next.js advisories, but exits nonzero on vulnerabilities in unchanged dependencies. An end-to-end Next.js application build was not exercised.

Pullfrog  | Fix it ➔ | View workflow run | Using gpt-6.1-sol | 𝕏

@theoephraim
theoephraim merged commit 5b50804 into dmno-dev:main Oct 28, 2026
30 of 31 checks passed
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