-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(host-runtime): auth-gate fingerprint includes setup; + #6299 CodeRabbit cleanups #6303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
cf363d0
refactor(reborn): address CodeRabbit review nits on #6299 (test-only …
ilblackdragon f7fc2f6
fix(host-runtime): include credential setup in the auth-gate fingerpr…
ilblackdragon 7c63e05
fix(host-runtime): encode gate-fingerprint scope lists injectively (#…
ilblackdragon b27540c
test(host-runtime): cover the provider_scopes-only injectivity collis…
ilblackdragon 963f47e
Merge remote-tracking branch 'origin/main' into refactor/reborn-coder…
ilblackdragon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2119,13 +2119,20 @@ fn stable_auth_gate_id( | |
| let mut requirements = credential_requirements | ||
| .iter() | ||
| .map(|requirement| { | ||
| let mut scopes = requirement.provider_scopes.clone(); | ||
| scopes.sort(); | ||
| // `setup` MUST be part of the fingerprint (#6299 IronLoop): two | ||
| // requirements that agree on provider/extension/provider_scopes but | ||
| // differ in `setup` (e.g. a ManualToken record vs a later OAuth or | ||
| // Pairing record, or differing OAuth setup scopes) are DIFFERENT auth | ||
| // requirements. Omitting it lets them derive the same deterministic | ||
| // `for_auth_gate` key; the write-once store then reports | ||
| // `GateRecordAlreadyExists` and silently keeps the stale record, so | ||
| // the runner reloads and renders the wrong authentication flow. | ||
| format!( | ||
| "credential={}:{}:{}", | ||
| "credential={}:{}:setup={}:{}", | ||
| requirement.provider.as_str(), | ||
| requirement.requester_extension.as_str(), | ||
| scopes.join(",") | ||
| stable_setup_token(&requirement.setup), | ||
| canonical_scope_list(&requirement.provider_scopes), | ||
| ) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
@@ -2138,6 +2145,38 @@ fn stable_auth_gate_id( | |
| .unwrap_or_else(|_| RuntimeGateId::new()) | ||
| } | ||
|
|
||
| /// Canonical, deterministic fingerprint token for a credential-account setup, | ||
| /// so [`stable_auth_gate_id`] distinguishes auth requirements that differ only | ||
| /// in their setup flow (#6299 IronLoop). Exhaustive by design: a new | ||
| /// `RuntimeCredentialAccountSetup` variant fails the build here rather than | ||
| /// silently hashing to an existing token. OAuth setup scopes use the same | ||
| /// injective [`canonical_scope_list`] encoding as `provider_scopes`. | ||
| fn stable_setup_token(setup: &ironclaw_host_api::RuntimeCredentialAccountSetup) -> String { | ||
| use ironclaw_host_api::RuntimeCredentialAccountSetup as Setup; | ||
| match setup { | ||
| Setup::ManualToken => "manual_token".to_string(), | ||
| Setup::OAuth { scopes } => format!("oauth:{}", canonical_scope_list(scopes)), | ||
| Setup::Pairing => "pairing".to_string(), | ||
| Setup::Retired => "retired".to_string(), | ||
| } | ||
| } | ||
|
|
||
| /// Injective canonical encoding of a scope list for the auth-gate fingerprint | ||
| /// (#6299 IronLoop). Scopes are not validated to exclude a join delimiter, so a | ||
| /// plain `join(",")` is ambiguous — `["a,b"]` and `["a", "b"]` would collide and | ||
| /// derive the same write-once gate key. Sort (a scope set is order-independent), | ||
| /// then length-prefix each element (`<byte_len>:<scope>`) so distinct sets can | ||
| /// never share an encoding regardless of which characters the scopes contain. | ||
| fn canonical_scope_list(scopes: &[String]) -> String { | ||
| let mut sorted = scopes.to_vec(); | ||
| sorted.sort(); | ||
| sorted | ||
| .iter() | ||
| .map(|scope| format!("{}:{scope}", scope.len())) | ||
| .collect::<Vec<_>>() | ||
| .join("|") | ||
| } | ||
|
|
||
| fn spawned_process_outcome_from( | ||
| result: CapabilitySpawnResult, | ||
| capability_id: CapabilityId, | ||
|
|
@@ -2607,6 +2646,93 @@ output_schema_ref = "schemas/test.output.json" | |
| assert_ne!(first_gate.gate_id, second_gate.gate_id); | ||
| } | ||
|
|
||
| #[test] | ||
| fn auth_required_outcome_changes_gate_when_only_setup_changes() { | ||
| // Regression (#6299 IronLoop): two requirements identical in provider, | ||
| // requester, and `provider_scopes` but differing ONLY in `setup` are | ||
| // DIFFERENT auth flows and must NOT collide on the deterministic | ||
| // `for_auth_gate` key. Before the fix `setup` was omitted from the | ||
| // fingerprint, so e.g. a ManualToken record and a later OAuth/Pairing | ||
| // record produced the same gate id; the write-once gate-record store | ||
| // then reported `GateRecordAlreadyExists`, kept the stale record, and | ||
| // the runner reloaded and rendered the wrong authentication flow. | ||
| use ironclaw_host_api::RuntimeCredentialAccountSetup as Setup; | ||
| let requirement_with = |setup: Setup| RuntimeCredentialAuthRequirement { | ||
| provider: RuntimeCredentialAccountProviderId::new("notion").unwrap(), | ||
| setup, | ||
| requester_extension: ExtensionId::new("notion").unwrap(), | ||
| provider_scopes: vec!["read".to_string()], | ||
| }; | ||
| let gate_id = |setup: Setup| { | ||
| let RuntimeCapabilityOutcome::AuthRequired(gate) = | ||
| auth_required_outcome(cap(), Vec::new(), vec![requirement_with(setup)]) | ||
| else { | ||
| panic!("expected auth gate"); | ||
| }; | ||
| gate.gate_id | ||
| }; | ||
|
|
||
| let manual = gate_id(Setup::ManualToken); | ||
| let oauth = gate_id(Setup::OAuth { | ||
| scopes: vec!["read".to_string()], | ||
| }); | ||
| let pairing = gate_id(Setup::Pairing); | ||
| // Distinct setup KINDS never collide (all `provider_scopes` equal). | ||
| assert_ne!(manual, oauth, "ManualToken vs OAuth must not collide"); | ||
| assert_ne!(manual, pairing, "ManualToken vs Pairing must not collide"); | ||
| assert_ne!(oauth, pairing, "OAuth vs Pairing must not collide"); | ||
|
|
||
| // OAuth setups differing ONLY in their setup scopes are distinct flows | ||
| // too (`provider_scopes` held fixed at ["read"] above and here). | ||
| let oauth_readwrite = gate_id(Setup::OAuth { | ||
| scopes: vec!["read".to_string(), "write".to_string()], | ||
| }); | ||
| assert_ne!( | ||
| oauth, oauth_readwrite, | ||
| "OAuth setups with different setup scopes must not collide" | ||
| ); | ||
|
|
||
| // Injective encoding: a single scope containing the old `,` join | ||
| // delimiter must not collide with two scopes that join to the same | ||
| // string — `["a,b"]` and `["a", "b"]` are DIFFERENT scope sets. Before | ||
| // the length-prefixed `canonical_scope_list`, both encoded to "a,b". | ||
| let one_comma_scope = gate_id(Setup::OAuth { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This only exercises the OAuth setup-scope encoding; |
||
| scopes: vec!["a,b".to_string()], | ||
| }); | ||
| let two_scopes = gate_id(Setup::OAuth { | ||
| scopes: vec!["a".to_string(), "b".to_string()], | ||
| }); | ||
| assert_ne!( | ||
| one_comma_scope, two_scopes, | ||
| "OAuth setup scopes must encode injectively: [\"a,b\"] != [\"a\", \"b\"]", | ||
| ); | ||
|
|
||
| // The same injective guarantee must hold for the per-requirement | ||
| // `provider_scopes` list, not only OAuth setup scopes — otherwise a | ||
| // revert of the `provider_scopes` encoding alone would go uncaught (the | ||
| // cases above hold `provider_scopes` fixed). Fixed ManualToken setup, | ||
| // `provider_scopes` `["a,b"]` vs `["a", "b"]`. | ||
| let provider_scopes_gate = |scopes: Vec<String>| { | ||
| let requirement = RuntimeCredentialAuthRequirement { | ||
| provider: RuntimeCredentialAccountProviderId::new("notion").unwrap(), | ||
| setup: Setup::ManualToken, | ||
| requester_extension: ExtensionId::new("notion").unwrap(), | ||
| provider_scopes: scopes, | ||
| }; | ||
| let RuntimeCapabilityOutcome::AuthRequired(gate) = | ||
| auth_required_outcome(cap(), Vec::new(), vec![requirement]) | ||
| else { | ||
| panic!("expected auth gate"); | ||
| }; | ||
| gate.gate_id | ||
| }; | ||
| assert_ne!( | ||
| provider_scopes_gate(vec!["a,b".to_string()]), | ||
| provider_scopes_gate(vec!["a".to_string(), "b".to_string()]), | ||
| "provider_scopes must encode injectively: [\"a,b\"] != [\"a\", \"b\"]", | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn dispatch_kind_to_failure_pins_every_runtime_dispatch_error_kind() { | ||
| // Every RuntimeDispatchErrorKind variant must map to a non-Unknown | ||
|
|
||
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid unnecessary heap allocations for static variants (like
ManualToken,Pairing, andRetired), we can returnstd::borrow::Cow<'static, str>instead ofString. This keeps the code efficient and avoids allocating memory for static strings while still allowing dynamic formatting for theOAuthvariant.