Fix secret precedence in Oz CLI.#10157
Conversation
|
I'm starting a first review of this pull request. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR centralizes managed-secret env-var construction, gives worker env vars precedence, and adds tests for typed-vs-generic secret precedence.
Concerns
- Typed auth secrets that emit overlapping env vars can still overwrite each other in nondeterministic
HashMapiteration order.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| } | ||
|
|
||
| for (env_name, env_value) in entries { | ||
| env_vars.insert(OsString::from(env_name), OsString::from(env_value)); |
There was a problem hiding this comment.
HashMap iteration overwrite the earlier one, so Bedrock API-key vs access-key auth can produce nondeterministic credentials. Detect typed-secret env collisions and skip or fail deterministically before inserting.
There was a problem hiding this comment.
This is fine because we should only be resolving to one of these from the server, right? Might be good to document that invariant somewhere
There was a problem hiding this comment.
Non-blocking for this PR, but it might be cleaner to set the auth env vars within the resolve_anthropic_api_key method (and same for codex)? At that point we actually know which one we're using, so we don't have to do the conflict detection 2x
liliwilson
left a comment
liliwilson
left a comment
There was a problem hiding this comment.
Nice find - the precedence order you set LGTM!!
| } | ||
|
|
||
| for (env_name, env_value) in entries { | ||
| env_vars.insert(OsString::from(env_name), OsString::from(env_value)); |
There was a problem hiding this comment.
This is fine because we should only be resolving to one of these from the server, right? Might be good to document that invariant somewhere
| } | ||
| // 2. Typed AnthropicApiKey secret. The secret name does not have to match the env var. | ||
| for secret in secrets.values() { | ||
| if let ManagedSecretValue::AnthropicApiKey { api_key } = secret { |
There was a problem hiding this comment.
Do we need to be handling the other auth forms here (e.g. bedrock/otherwise) to prevent the auth popup from showing up? Or does claude just detect them automatically
| } | ||
|
|
||
| for (env_name, env_value) in entries { | ||
| env_vars.insert(OsString::from(env_name), OsString::from(env_value)); |
There was a problem hiding this comment.
Non-blocking for this PR, but it might be cleaner to set the auth env vars within the resolve_anthropic_api_key method (and same for codex)? At that point we actually know which one we're using, so we don't have to do the conflict detection 2x
2 participants
Description
Fixes an issue where secrets could collide if specified both as an auth secret and a generic secret. As part of that, we now enforce the following precedence order:
We insert typed secrets atomically. If any env var that is part of the auth secret is present in the environment, we skip setting any part of that secret.
Testing
Tested locally that (2) takes precedence over (3).
Agent Mode