Skip to content

fix(secrets): scope BWS-injected provider keys - #58111

Closed
izumi0uu wants to merge 1 commit into
NousResearch:mainfrom
izumi0uu:fix/bws-secret-scope-58100
Closed

fix(secrets): scope BWS-injected provider keys#58111
izumi0uu wants to merge 1 commit into
NousResearch:mainfrom
izumi0uu:fix/bws-secret-scope-58100

Conversation

@izumi0uu

@izumi0uu izumi0uu commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes provider credential resolution when Bitwarden/BSM injects API keys into os.environ during load_hermes_dotenv(), but cron/profile execution later runs under an authoritative profile secret scope.

The fix records which HERMES_HOME an external secret source was applied for, then includes only those same-home external secrets when building that profile's secret_scope. This lets BWS-backed provider keys resolve in cron/profile-scoped paths without adding an unsafe raw os.environ fallback for scoped-missing keys.

It also preserves fail-closed behavior for unscoped multiplex credential reads: UnscopedSecretError is no longer swallowed into a raw-env fallback by get_env_value_prefer_dotenv().

Related Issue

Fixes #58100

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • Track external secret-source values by profile home in hermes_cli/env_loader.py.
  • Merge same-home external secrets into build_profile_secret_scope().
  • Preserve fail-closed behavior for unscoped multiplex credential reads.
  • Add regression coverage for BWS-scoped Xiaomi credentials and non-leak behavior.

How to Test

  1. .venv/bin/python -m pytest tests/test_env_loader_secret_sources.py tests/agent/test_secret_scope.py tests/hermes_cli/test_xiaomi_provider.py tests/cron/test_run_one_job.py -q
  2. .venv/bin/python -m pytest tests/hermes_cli/test_api_key_providers.py tests/gateway/test_multiplex_credential_isolation.py -q
  3. .venv/bin/python -m ruff check hermes_cli/env_loader.py agent/secret_scope.py hermes_cli/config.py tests/test_env_loader_secret_sources.py tests/agent/test_secret_scope.py tests/hermes_cli/test_xiaomi_provider.py

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes
  • I've tested on my platform: macOS local worktree, Python venv

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — N/A, pure Python credential resolution
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

Before the fix, an active profile scope with no XIAOMI_API_KEY returned an empty provider credential even when BWS had injected XIAOMI_API_KEY into os.environ.

After the fix, a same-home external secret resolves:

fixed_api_key= 'xiaomi-bws-secret'
fixed_source= 'XIAOMI_API_KEY'

A scoped-missing raw env var still does not leak across profiles:

empty_scope_api_key= ''

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/auth Authentication, OAuth, credential pools area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jul 4, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved (read-only token - formal approval deferred)

Security fix: scope BWS-injected provider keys so they don't leak across profile boundaries. Adds tracking to associate Bitwarden-sourced secrets with their profile home, and filters by home in . Also fixes to handle explicitly. No concerns.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved (read-only token - formal approval deferred)

Security fix: scope BWS-injected provider keys so they don't leak across profile boundaries. Adds _SECRET_SOURCE_HOMES tracking to associate Bitwarden-sourced secrets with their profile home, and filters by home in build_profile_secret_scope. Also fixes get_env_value_prefer_dotenv to handle UnscopedSecretError explicitly. No concerns.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved (LGTM)

Scopes BWS-injected provider keys to the correct namespace. Security-relevant fix.

What Looks Good

  • Security-relevant fix
  • Well-scoped
  • No debug artifacts

Reviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: LGTM

Fix (+189/-2) scoping BWS-injected provider keys to the secrets system. Security-focused fix with proper secret scope handling.

Looks Good

  • Proper secret scoping design
  • No security or performance concerns

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for pursuing the safer scoped-secret direction; the current-main cron/profile failure is real.

Problems

  • At dea78a4df37e, the proposed home map stores only variable names, then reads the current os.environ value. Current main permits applies for multiple resolved homes in hermes_cli/env_loader.py:328-331, while agent/secret_sources/registry.py:360 writes each applied value into that shared environment. Two homes with different values for one key could therefore make the first home resolve the second home's value.
  • The patch targets the pre-refactor Bitwarden result shape. Current main uses generic report.provenance in hermes_cli/env_loader.py:359-360, so this requires a deliberate port.

Suggested changes

  • Snapshot applied values by resolved home at apply time, then merge that snapshot into build_profile_secret_scope; do not reread mutable os.environ later.
  • Add a two-home/same-key/different-value isolation regression test. Preserve the explicit UnscopedSecretError re-raise proposed for hermes_cli/config.py.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@izumi0uu
izumi0uu force-pushed the fix/bws-secret-scope-58100 branch from dea78a4 to 5fa4edb Compare July 19, 2026 12:11
Snapshot values applied by external secret sources per resolved HERMES_HOME so a later profile cannot replace an earlier profile scope through shared os.environ.

Keep provider and credential-pool fallback reads on the active secret scope, and fail closed on unscoped multiplex reads.

Tests: scripts/run_tests.sh tests/test_env_loader_secret_sources.py tests/test_env_loader_op_bootstrap.py tests/agent/test_secret_scope.py tests/agent/test_credential_pool.py tests/tools/test_credential_pool_env_fallback.py tests/hermes_cli/test_xiaomi_provider.py tests/cron/test_run_one_job.py tests/hermes_cli/test_api_key_providers.py tests/gateway/test_multiplex_credential_isolation.py -q (395 passed)
@izumi0uu
izumi0uu force-pushed the fix/bws-secret-scope-58100 branch from 5fa4edb to a292f78 Compare July 19, 2026 12:16
@izumi0uu

Copy link
Copy Markdown
Contributor Author

Addressed in a292f78. I ported the fix to current main's generic report.provenance API and now snapshot the sanitized secret values per resolved HERMES_HOME at apply time, so a later home cannot change an earlier profile's scope through shared os.environ.
I also routed credential-pool environment seeding through the active secret scope, preserved the explicit UnscopedSecretError re-raise, and added the requested two-home/same-key/different-value isolation regression.
The branch is rebased onto current main. The targeted suite passes with 395 tests, and Ruff, py_compile, and diff checks pass.

@teknium1

Copy link
Copy Markdown
Contributor

Merged via #69250 — your commit landed as-is (rebase-merged, authorship preserved). The per-home value snapshot was exactly the right design: values recorded at the only moment they're unambiguous, so a later profile's apply can never mutate an earlier profile's scope through shared os.environ. Only fixups from us: a test-file cherry-pick conflict and one fake ApplyReport needing a SourceReport after the #69056 guard landed. Fixes #58100. Thanks @izumi0uu — third landed contribution in this secrets sweep!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools area/config Config system, migrations, profiles comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: get_env_value_prefer_dotenv returns None when get_secret returns None instead of falling through to os.environ

4 participants