Wire all secret callers to scoped and user providers#4465
Merged
Conversation
When a user upgrades ToolHive, system secrets may still exist under bare keys (e.g. BEARER_TOKEN_foo) until the secret scope migration completes. If migration fails or hasn't run yet, ScopedProvider callers would be unable to find their secrets under the new scoped key (__thv_workloads_BEARER_TOKEN_foo), breaking workload auth. Add a transparent fallback in ScopedProvider.GetSecret: on a not-found response for the scoped key, also try the bare (pre- migration) key. Once migration completes and bare keys are deleted, the fallback finds nothing and becomes a natural no-op — no config check or injection needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add t.Parallel() to TestScopedProvider_GetSecret_MigrationFallback and its subtests to satisfy the paralleltest linter requirement. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Update every call site that creates a secrets provider to use the appropriate wrapper introduced in Phase 1: - System callers (workload auth tokens, registry credentials, build auth files) now use CreateScopedSecretProvider, placing secrets under the __thv_<scope>_ prefix and hiding them from user-facing commands. - User-facing callers (thv secret commands, REST API, MCP tool server, header secrets, build-env-from-secrets) now use CreateUserSecretProvider, blocking access to __thv_* reserved keys. - RunConfig.WithSecrets and ValidateSecrets now accept separate system and user providers so auth-token resolution and --secret flag resolution use the correct scope independently. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Verify that the UserProvider wiring in CLI secret commands correctly blocks access to __thv_* keys: - set with __thv_ prefix is rejected - get with __thv_ prefix is rejected with "reserved for system use" - delete with __thv_ prefix is rejected - two distinct __thv_ key names are blocked, confirming the check is not tied to a single hardcoded name Each test uses an isolated XDG_CONFIG_HOME/HOME temp directory so the user's real secrets store is never touched. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add GetUserSecretsProvider() to pkg/auth/secrets alongside the existing GetSecretsManager(), centralising the config-read + CreateUserSecretProvider boilerplate in one place - Remove getScopedWorkloadsSecretsManager() from config_buildauthfile.go; it was identical to authsecrets.GetSecretsManager() — use that directly - Replace inline boilerplate in config_buildenv.go/validateSecretExists and secret.go/getSecretsManager with calls to GetUserSecretsProvider(), eliminating duplicated setup-check + provider-create blocks - Rename local variable manager → provider in GetSecretsManager() to match the naming convention used throughout the new provider code Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
amirejaz
requested review from
ChrisJBurns,
JAORMX,
blkt,
jhrozek,
lujunsan,
rdimitrov and
yrobla
as code owners
github-actions
bot
added
size/M
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4465 +/- ##
==========================================
- Coverage 69.64% 69.64% -0.01%
==========================================
Files 491 491
Lines 50304 50327 +23
==========================================
+ Hits 35036 35050 +14
- Misses 12580 12595 +15
+ Partials 2688 2682 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Replace CreateUserSecretProvider and CreateScopedSecretProvider with the new CreateProvider(managerType, WithUserFacing()) and CreateProvider(managerType, WithScope(scope)) API introduced in the phase2-factory-helpers PR. Affected callers: - cmd/thv/app/registry_login.go - cmd/thv/app/header_flags.go - pkg/auth/secrets/secrets.go - pkg/runner/env.go - pkg/runner/runner.go - pkg/runner/protocol.go - pkg/mcp/server/set_secret.go - pkg/mcp/server/list_secrets.go - pkg/workloads/manager.go - pkg/api/v1/secrets.go Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
amirejaz
force-pushed
the
phase3-wire-callers
branch
from
d9168dc to
8dabc6a
Compare
github-actions
bot
added
size/M
reyortiz3
approved these changes
Mar 31, 2026
aponcedeleonch
approved these changes
Apr 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CreateProviderwith explicit scope or user-facing options so that system keys are never accidentally exposed via the user-facing API, and user commands cannot read or modify internal system secrets.thv secretcommands reject reserved__thv_*key names.Type of change
Changes
cmd/thv/app/registry_login.goWithScope(ScopeRegistry)for registry login credential storagecmd/thv/app/header_flags.goWithUserFacing()for user-facing header flag secret readscmd/thv/app/secret.goWithUserFacing()for user-facing secret management commandspkg/api/v1/secrets.goWithUserFacing()for REST API secret endpointspkg/auth/secrets/secrets.goWithScope(ScopeWorkloads)for workload auth tokenspkg/mcp/server/list_secrets.goWithUserFacing()for MCP list_secrets toolpkg/mcp/server/set_secret.goWithUserFacing()for MCP set_secret toolpkg/runner/env.goWithUserFacing()for env var secret injectionpkg/runner/protocol.goWithScope(ScopeWorkloads)for protocol-level workload secretspkg/runner/runner.goWithUserFacing()for runner secret accesspkg/workloads/manager.goWithScope(ScopeWorkloads)for workload manager secret accesspkg/secrets/scoped.goScopedProvider.GetSecret(from #4386)pkg/secrets/scoped_test.goDoes this introduce a user-facing change?
User-facing
thv secretcommands now reject any key name starting with__thv_with a clear error message. Internal workload secrets (registry tokens, OAuth refresh tokens) are transparently namespaced under__thv_<scope>_<name>and are invisible to user commands.Test plan
task test)thv secretcommands verify that__thv_*keys are blockedtask build)Generated with Claude Code