Conversation
github-actions
Bot
added
scope: docs
288c02a to
b41a856
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ironclaw_auth crate, establishing product-facing authentication contracts for the Reborn project. It defines essential traits and data structures for authentication flows, credential accounts, and secure secret interactions, supported by an in-memory implementation for early integration testing. New documentation detailing the auth contract is also provided. Reviewer feedback highlighted the importance of ensuring atomicity during the lifecycle cleanup process and suggested sanitizing error messages in secret handle generation to maintain decoupling from internal implementation details.
| } | ||
|
|
||
| let is_owner = account.owner_extension.as_ref() == Some(&request.extension_id); | ||
| let had_grant = account.granted_extensions.contains(&request.extension_id); |
There was a problem hiding this comment.
The cleanup_for_lifecycle implementation iterates over all accounts and performs modifications. To ensure atomicity in a concurrent environment, the lock on state must be held across the entire read-modify-write sequence for each account to prevent race conditions.
References
- Ensure atomicity of read-modify-write operations on shared state by holding a lock across the entire sequence (read, validate, and write).
There was a problem hiding this comment.
I think this one is already covered: cleanup holds the state mutex for the whole read/modify/write loop. Leaving as-is.
| SecretHandle::new(format!("{prefix}-{id}")).map_err(|error| AuthProductError::InvalidRequest { | ||
| reason: error.to_string(), | ||
| }) |
There was a problem hiding this comment.
The generated_secret_handle function uses error.to_string() which might reference internal implementation details. Log messages and error reasons should be generic to maintain decoupling.
References
- Maintain style consistency with existing code. (link)
- Log messages should be generic and avoid referencing internal implementation details, such as specific data structure names or source code layout, to maintain decoupling.
There was a problem hiding this comment.
Fixed — switched this to a generic error message.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 947b324308
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .interactions | ||
| .remove(&request.interaction_id) | ||
| .ok_or(AuthProductError::UnknownOrExpiredFlow)?; | ||
| ensure_same_scope(scope, &interaction.scope)?; |
There was a problem hiding this comment.
submit_secret deletes the pending interaction before checking ownership and expiry, so a cross-scope caller can invalidate someone else’s manual-token flow: the unauthorized call returns CrossScopeDenied, but the real owner’s subsequent submit will now fail as UnknownOrExpiredFlow. This creates a denial-of-service path on auth setup; validate scope/expiry first and only remove the interaction after those checks pass.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — scope/expiry are checked before removing the pending interaction now.
| record.status = AuthFlowStatus::Completed; | ||
| record.error = None; | ||
| let continuation = (record.scope.clone(), record.continuation.clone()); | ||
| let completed = record.clone(); | ||
| state.continuations.push(continuation); |
There was a problem hiding this comment.
A successful callback always pushes a continuation, but the flow is never consumed or guarded against terminal states, so replaying the same authorized callback can enqueue duplicate continuations (e.g., repeated extension activation or turn resumption). Add a terminal-state/idempotency check or remove/mark-consumed flow records atomically before enqueuing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — callback replay now hits the terminal-state guard and does not enqueue another continuation.
b41a856 to
6edacca
Compare
|
|
||
| /// Product surface that initiated or renders an auth flow. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] | ||
| pub enum AuthSurface { |
There was a problem hiding this comment.
Hmm, should the auth crate know about where the request is coming from? I think it should probably know about the 'extension trust' (first party, system, thirdparty etc.)
| } | ||
|
|
||
| #[async_trait] | ||
| pub trait CredentialAccountService: Send + Sync { |
There was a problem hiding this comment.
probably should be in a separate file
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code review
Reviewed PR #3813 at commit 6edacca8512035f4cf68236f41b7970aa99911a8 with the multi-agent code-review workflow.
Result: request changes. I found 1 High, 11 Medium, and 1 Low finding after deduplication. Security, bugs, tests, and conventions all reported actionable items; performance/concurrency returned clean.
The High item is the OAuth callback/provider-exchange contract: the contract exposes only hashed callback code to the provider client while allowing callback completion/continuation enqueueing after state validation. The other findings are mostly contract hardening, state-machine consistency, and focused missing coverage for new public auth APIs.
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct OAuthProviderCallbackRequest { | ||
| pub provider: AuthProviderId, | ||
| pub authorization_code_hash: String, |
There was a problem hiding this comment.
High: OAuth exchange contract drops the raw code needed for provider validation
OAuthProviderCallbackRequest carries only authorization_code_hash, while complete_oauth_callback can mark the flow completed and enqueue the continuation after only a state-hash match. A production callback path following this contract cannot exchange the real authorization code and PKCE verifier with the provider before completing auth, so it either cannot validate provider proof or has to treat a hash as sufficient. Keep the raw callback code and PKCE verifier in a non-serializable/one-shot input to the provider client, exchange them first, and only then complete the flow while persisting hashes only.
| /// Provider/integration identifier shown in redacted product state. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] | ||
| #[serde(transparent)] | ||
| pub struct AuthProviderId(String); |
There was a problem hiding this comment.
Medium: validated newtypes deserialize without validation
These string-backed types validate through ::new(), but #[serde(transparent)] with derived Deserialize lets wire or stored records instantiate values that validate_public_text() would reject, including overlong strings or control characters. This also violates .claude/rules/types.md for newly added validated newtypes. Use #[serde(try_from = "String")]/TryFrom<String> for these wrappers and validate AuthProductScope::session_id on deserialization too.
| /// Product surface that initiated or renders an auth flow. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] | ||
| pub enum AuthSurface { | ||
| Chat, |
There was a problem hiding this comment.
Medium: public wire enums will serialize as PascalCase
The new serialized contract enums use default serde names, so values like SetupAdmin and AccountSelectionRequired will appear on the wire instead of repo-standard snake_case. .claude/rules/types.md requires #[serde(rename_all = "snake_case")] for wire-stable enums. Add that to the public/persisted enums here, using explicit tagging where externally visible payload shape needs to be fixed.
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub credential_account_id: Option<CredentialAccountId>, | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub opaque_state_hash: Option<String>, |
There was a problem hiding this comment.
Medium: distinct OAuth hash values are still stringly typed
opaque_state_hash, pkce_verifier_hash, and authorization_code_hash are different auth-domain values with the same shape, but they cross the contract as plain Strings. The typed-internals rule calls out exactly this class of auth identity confusion. Introduce separate newtypes, for example OpaqueStateHash, PkceVerifierHash, and AuthorizationCodeHash, and use them in the flow record, callback input, and provider callback request.
| record.updated_at = now(); | ||
|
|
||
| match input.provider_result { | ||
| ProviderCallbackResult::Authorized { .. } => { |
There was a problem hiding this comment.
Medium: empty OAuth callback codes are accepted as completed auth
The Authorized arm ignores authorization_code_hash, so an authorized result with an empty or missing code still marks the flow Completed and enqueues the continuation. That lets malformed callbacks advance product workflows as if authentication succeeded. Validate the authorized code hash before completing the flow and return MalformedCallback when it is empty.
| Ok(record) | ||
| } | ||
|
|
||
| async fn get_flow( |
There was a problem hiding this comment.
Medium: get_flow has no direct contract coverage
The new public AuthFlowManager::get_flow implementation is not called by the inline auth tests or adjacent/top-level auth tests, leaving the None, owner lookup, and cross-scope error behavior unpinned. Add a test such as get_flow_returns_none_and_denies_cross_scope covering missing flow, owner lookup, and cross-scope denial.
| .collect()) | ||
| } | ||
|
|
||
| async fn select_unique_configured_account( |
There was a problem hiding this comment.
Medium: multiple configured accounts are not tested
Existing account-selection coverage exercises zero configured accounts and exactly one configured account, but not the many-configured edge case that must return AccountSelectionRequired for product account selection. Add credential_account_selection_requires_user_choice_for_multiple_configured_accounts with two configured accounts for the same scope/provider.
| scope: &AuthProductScope, | ||
| request: SecretSubmitRequest, | ||
| ) -> Result<SecretSubmitResult, AuthProductError> { | ||
| if request.secret.expose_secret().trim().is_empty() { |
There was a problem hiding this comment.
Medium: manual-token invalid and expired submits lack tests
The manual-token tests cover happy path and cross-scope denial, but do not exercise whitespace-only secret rejection or the expired-interaction UnknownOrExpiredFlow branch. Add manual_token_submit_rejects_empty_and_expired_secret_inputs covering both cases.
|
|
||
| #[async_trait] | ||
| impl AuthProviderClient for InMemoryAuthProductServices { | ||
| async fn exchange_callback( |
There was a problem hiding this comment.
Medium: OAuth provider exchange fake is untested
The new AuthProviderClient::exchange_callback fake implementation has no direct test, including its malformed empty-code error path and the returned access/refresh secret-handle metadata shape. Add oauth_provider_exchange_callback_returns_handles_and_rejects_empty_code_hash covering success metadata and empty authorization_code_hash.
| report.revoked_accounts.push(account.id); | ||
| } | ||
| } | ||
| (SecretCleanupAction::Deactivate, _, true | false) |
There was a problem hiding this comment.
Medium: deactivate cleanup branch has no coverage
cleanup_for_lifecycle is only tested for Uninstall, leaving the Deactivate action variant untested even though it has distinct retention behavior for owned accounts and grants. Add cleanup_deactivate_retains_owned_accounts_and_removes_grants covering SecretCleanupAction::Deactivate for owned and granted accounts.
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Multi-agent review for PR #3813 at 6edacca.
Summary:
- 12 findings after dedupe: 3 security, 2 bugs, 6 tests, 2 conventions, with 1 security/conventions duplicate merged.
- Highest severity: Medium, so this is a COMMENT review per the code-review skill event rules.
- Local verification run by orchestrator:
cargo test -p ironclaw_authpassed in the PR worktree.
Primary themes:
- New product-auth contract types need wire/deserialization hardening before downstream adapters depend on them.
- The fake service has a couple of contract-invariant holes around malformed callback completion and configured accounts without handles.
- Several new public contract methods and edge/error paths need direct regression coverage.
|
|
||
| /// Provider/integration identifier shown in redacted product state. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] | ||
| #[serde(transparent)] |
There was a problem hiding this comment.
Medium (conventions) — Validated newtypes bypass validation during deserialization
AuthProviderId is a newly added validated String newtype, but it derives Deserialize with #[serde(transparent)], so JSON/DB/wire rehydration bypasses the same validate_public_text checks enforced by ::new(). The same pattern repeats for CredentialAccountLabel, ProductActionRef, LifecyclePackageRef, TurnRunRef, and AuthGateRef. The repo rule at .claude/rules/types.md:114 explicitly requires #[serde(try_from = "String")] and says not to use #[serde(transparent)] on newly added validated newtypes. Security also flagged this as an invariant bypass for product-surface values.
Fix: Switch each validated String newtype to #[serde(try_from = "String")], add TryFrom<String> through the shared validator, and add deserialize rejection tests.
|
|
||
| /// Product surface that initiated or renders an auth flow. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] | ||
| pub enum AuthSurface { |
There was a problem hiding this comment.
Medium (conventions) — Wire enums serialize as PascalCase instead of snake_case
AuthSurface derives serde without #[serde(rename_all = "snake_case")], so its wire form is Chat/Web/SetupAdmin instead of the repo-standard snake_case contract. The same issue applies to the other new public serialized enums in this contract crate, including AuthFlowKind, AuthFlowStatus, AuthChallenge, AuthContinuationRef, AuthErrorCode, CredentialAccountStatus, CredentialOwnership, and SecretCleanupAction. These are product contract types, so .claude/rules/types.md:159 applies.
Fix: Add #[serde(rename_all = "snake_case")] to the externally serialized enums, with tag/content attributes where needed for data-carrying variants, and cover representative round trips in tests.
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub credential_account_id: Option<CredentialAccountId>, | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub opaque_state_hash: Option<String>, |
There was a problem hiding this comment.
Medium (security) — Flow records serialize OAuth verifier hashes
AuthFlowRecord is the durable record returned by get_flow and derives Serialize/Debug while exposing opaque_state_hash and pkce_verifier_hash. The contract says OAuth state and PKCE verifier values must not be adapter-visible, but any adapter/log/API that serializes or debugs this record can learn the exact value complete_oauth_callback later accepts for state verification, turning a leaked record into callback-forgery or PKCE metadata exposure risk.
Fix: Split the internal flow record from an adapter-safe projection, or mark verifier fields non-serializable/non-debug and avoid returning them from product-facing getters.
| pub enum AuthChallenge { | ||
| OAuthUrl { | ||
| flow_id: AuthFlowId, | ||
| auth_url: String, |
There was a problem hiding this comment.
Medium (security) — Rendered OAuth challenge URL is an unchecked string
AuthChallenge::OAuthUrl is explicitly rendered by adapters but stores auth_url as an arbitrary String with no HTTPS, host, scheme, credential, or length validation. If a provider, extension, or persisted flow can influence this value, product surfaces may render a javascript:, http:, credential-bearing, or phishing URL as an auth challenge.
Fix: Replace auth_url with a validated newtype or URL wrapper that enforces HTTPS, bounded length, and provider/redirect policy before challenge construction/deserialization.
| record.updated_at = now(); | ||
|
|
||
| match input.provider_result { | ||
| ProviderCallbackResult::Authorized { .. } => { |
There was a problem hiding this comment.
Medium (bugs) — Empty OAuth authorization hashes can complete a flow
The Authorized callback arm ignores authorization_code_hash, so a caller can pass ProviderCallbackResult::Authorized with an empty hash and the fake marks the flow Completed and enqueues the continuation. The provider client rejects an empty authorization_code_hash, but this callback completion path bypasses that check, letting malformed callbacks resume product workflow without a usable credential exchange result.
Fix: Validate the authorized code hash before completing the flow, returning MalformedCallback for an empty value.
| } | ||
| } | ||
|
|
||
| async fn cancel_flow( |
There was a problem hiding this comment.
Medium (tests) — cancel_flow contract is untested
The new public AuthFlowManager::cancel_flow method is not exercised by existing tests, so the Canceled status/error transition and the UnknownOrExpiredFlow/CrossScopeDenied error paths are unverified.
Fix: Add tests::cancel_flow_marks_owned_flow_canceled_and_rejects_missing_or_cross_scope_flow covering success, missing flow, and cross-scope cancellation.
| .collect()) | ||
| } | ||
|
|
||
| async fn select_unique_configured_account( |
There was a problem hiding this comment.
Medium (tests) — Many configured accounts selection is untested
select_unique_configured_account takes a collection and current coverage verifies zero and one configured account, but not the many-configured-account case that must return AccountSelectionRequired.
Fix: Add tests::select_unique_configured_account_requires_disambiguation_for_many_configured_accounts covering two configured accounts for the same provider and scope.
| Ok(challenge) | ||
| } | ||
|
|
||
| async fn submit_secret( |
There was a problem hiding this comment.
Medium (tests) — Manual token rejection paths are untested
submit_secret has explicit rejection paths for blank submitted secrets, unknown interactions, and expired interactions, but existing manual-token coverage only exercises cross-scope rejection and success.
Fix: Add tests::manual_token_submit_rejects_empty_unknown_and_expired_interactions covering blank secret, nonexistent interaction id, and expired pending interaction.
|
|
||
| #[async_trait] | ||
| impl AuthProviderClient for InMemoryAuthProductServices { | ||
| async fn exchange_callback( |
There was a problem hiding this comment.
Medium (tests) — Provider callback exchange fake is untested
The new public AuthProviderClient::exchange_callback implementation is not called by existing tests, leaving both its successful sanitized token metadata contract and empty authorization_code_hash MalformedCallback path uncovered.
Fix: Add tests::exchange_callback_returns_sanitized_token_metadata_and_rejects_empty_code_hash covering successful exchange and empty authorization_code_hash.
| uuid_id!(AuthInteractionId); | ||
|
|
||
| /// Validate bounded product metadata that may be rendered to adapters. | ||
| fn validate_public_text( |
There was a problem hiding this comment.
Medium (tests) — Public text validation errors are untested
The public typed constructors and AuthProductScope::with_session_id all propagate validate_public_text errors for empty, oversized, and control-character input, but existing tests only construct valid values.
Fix: Add tests::bounded_public_text_rejects_empty_control_and_oversized_values covering AuthProviderId, CredentialAccountLabel, typed continuation refs, and session id validation failures.
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review Summary — PR #3813
7 findings (0 Critical, 0 High, 0 Medium, 7 Low)
Reviewer breakdown:
- Security: 2 findings (2 Low)
- Bugs: 1 finding (1 Low)
- Tests: 3 findings (3 Low)
- Conventions: 1 finding (1 Low)
Security (2 findings)
[S-1] Mutex poisoning masks previous panics — Low (conf: 75)
lock_state() swallows PoisonError and returns BackendUnavailable. If a panic occurred while holding the mutex, subsequent calls fail silently with a misleading error — the real issue (data inconsistency from panic) is hidden.
Fix: Use lock().unwrap() or properly handle PoisonError — unwrap the guard and log the poison flag.
[S-2] String equality comparison on auth state hash — Low (conf: 50)
complete_oauth_callback uses Rust PartialEq (==) for state hash comparison. Rust's &str PartialEq is NOT constant-time. For fixed-length SHA-256 hashes this is a nominal risk but the timing oracle is conceptually real.
Fix: Compare using subtle::ConstantTimeEq or a constant-time string comparison.
Bugs (1 finding)
[B-1] AuthErrorCode::UnknownOrExpiredFlow doesn't distinguish not-found from expired — Low (conf: 60)
cancel_flow and complete_oauth_callback both return UnknownOrExpiredFlow when a flow is not found in the map. Callers can't distinguish a genuinely expired flow from a non-existent one.
Fix: Add AuthErrorCode::FlowNotFound to distinguish from actual expiration.
Tests (3 findings)
[T-1] get_flow() not tested directly — Low (conf: 70)
AuthFlowManager::get_flow() is implemented but never tested. No test exercises valid scope, invalid scope, or non-existent flow.
[T-2] cancel_flow() not tested — Low (conf: 70)
AuthFlowManager::cancel_flow() is implemented but never tested. No cancellation path testing.
[T-3] AuthProviderClient::exchange_callback() not tested — Low (conf: 70)
exchange_callback() is only partially tested. Happy path with valid code hash is never exercised.
Conventions (1 finding)
[C-1] New pub traits and types lack doc comments — Low (conf: 50)
All pub traits (AuthFlowManager, CredentialAccountService, AuthInteractionService, CredentialSetupService, AuthProviderClient, ProductWorkflowContinuationSink, SecretCleanupService) and key struct types lack Rustdoc doc comments.
Status: No blocking issues. This is a clean contract-first submission. All findings are Low-severity suggestions for the fake-service implementation. No changes required before merge.
|
this is done in #3865 , closing this duplicated one now |
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review: [Reborn] Add product auth contracts and fake-service tests
Reviewers: Security · Bugs · Performance/Concurrency · Tests · Conventions
Mode: PR (diff-only)
Commit: 6edacca
Summary
Well-structured contract-first auth crate. The type system correctly prevents raw secret leakage (SecretString, redacted Debug, SecretHandle indirection, CredentialAccountProjection excludes handles). The contract doc and ownership boundaries are thorough.
2 High and 15 Medium findings should be addressed before merging, mostly around missing test coverage, unvalidated string fields in the trait API, and a partial-failure ordering bug.
Stats
| Severity | Count | Breakdown |
|---|---|---|
| High | 2 | Tests (1), Bugs (1) |
| Medium | 15 | Security (2), Bugs (2), Performance (2), Tests (4), Conventions (5) |
| Low | 5 | Conventions (3), Performance (1), Tests (1) |
High Severity
| # | Cat | Title | File |
|---|---|---|---|
| f-tes-1 | Tests | cancel_flow has zero test coverage | lib.rs:790 |
| f-bug-1 | Bugs | submit_secret: interaction removed before account creation — lost on failure | lib.rs:850 |
f-tes-1: cancel_flow has no test for any path. Add cancel_flow_pending_succeeds and cancel_flow_completed_returns_error.
f-bug-1: In submit_secret, the interaction is removed before account creation + continuation enqueue. If either fails, the interaction is permanently lost. Defer removal until after both succeed.
Medium Severity
Security:
- f-sec-2:
validate_public_textdoesn't check RTL overrides (U+202A-E) or zero-width chars. Labels could render differently in UI. - f-sec-1:
opaque_state_hashcompared with String equality — not timing-safe. Production impls should use constant-time comparison.
Bugs:
- f-bug-2:
cancel_flowreturnsUnknownOrExpiredFlowfor valid-but-non-cancellable flows. AddFlowNotCancellablevariant. - f-bug-3: State hash mismatch returns
MalformedCallback— conflated with CSRF/replay. Consider addingStateMismatcherror.
Performance:
- f-per-1:
list_accountsreturns unboundedVec— no pagination in trait API. Production impls need pagination. - f-per-2: Continuations Vec grows without bound — no prune/ack method in trait.
Tests:
- f-tes-2:
update_statuserror paths untested (CredentialMissing, CrossScopeDenied). - f-tes-3:
select_unique_configured_accountedge cases (0 accounts, 2+ accounts) untested. - f-tes-4:
CredentialSetupServicemethods never directly exercised by tests. - f-tes-5:
submit_secreterror paths untested (expired interaction, cross-scope).
Conventions:
- f-con-1: 1530-line single lib.rs. Split into domain modules for navigability.
- f-con-2:
session_iduses rawOption<String>— should use validated newtype. - f-con-4:
AuthChallenge::OAuthUrl.auth_urlunvalidated — no URL format check. - f-con-5:
opaque_state_hash/pkce_verifier_hashareOption<String>— no hash format validation. - f-con-7:
ProviderCallbackResult::Denied.error_codeis unvalidated provider string.
Low Severity
| # | Cat | Title |
|---|---|---|
| f-con-3 | Conventions | ProductActionRef missing Display impl |
| f-con-6 | Conventions | AuthProductError::InvalidRequest.code() returns BackendUnavailable |
| f-per-3 | Performance | cleanup_for_lifecycle O(n) full scan — document indexing requirement |
| f-tes-6 | Tests | validate_public_text boundary cases untested |
| f-con-8 | Conventions | Cargo.toml missing rust-version, description, license |
Positive observations
- ✅ SecretString + redacted Debug on SecretSubmitRequest — correct secure-input boundary
- ✅ CredentialAccountProjection excludes secret handles, only count — correct redaction
- ✅ Cross-scope validation on every state-mutating operation
- ✅ Ownership-aware cleanup with idempotent semantics
- ✅ Contract doc thoroughly maps V1→Reborn migration inventory
- ✅ validate_public_text enforces length, empty, NUL/control checks — good baseline
| ) -> Result<Option<AuthFlowRecord>, AuthProductError> { | ||
| let state = self.lock_state()?; | ||
| let Some(record) = state.flows.get(&flow_id) else { | ||
| return Ok(None); |
There was a problem hiding this comment.
[High] [Tests] cancel_flow has zero test coverage (f-tes-1)
cancel_flow is a public trait method with no test for any path: happy (Pending→Canceled), error (non-cancellable state), not-found.
Fix: Add tests::cancel_flow_pending_succeeds and tests::cancel_flow_completed_returns_error.
| ProviderCallbackResult::Denied { .. } => { | ||
| record.status = AuthFlowStatus::Failed; | ||
| record.error = Some(AuthErrorCode::ProviderDenied); | ||
| Err(AuthProductError::ProviderDenied) |
There was a problem hiding this comment.
[High] [Bugs] submit_secret: interaction removed before account creation — lost on failure (f-bug-1)
The pending interaction is removed from the state map before creating the CredentialAccount and enqueuing continuation. If either operation fails, the interaction is permanently lost with no rollback path.
Fix: Defer state.interactions.remove() until after account creation and continuation enqueue both succeed.
|
|
||
| uuid_id!(AuthFlowId); | ||
| uuid_id!(CredentialAccountId); | ||
| uuid_id!(AuthInteractionId); |
There was a problem hiding this comment.
[Medium] [Security] validate_public_text does not check for RTL overrides or zero-width chars (f-sec-2)
RTL override characters (U+202A-U+202E) and zero-width chars (U+200B, U+FEFF) are allowed through. Labels could render differently in UI vs stored form.
Fix: Add check for BiDi control and zero-width characters.
| pub surface: AuthSurface, | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub session_id: Option<String>, | ||
| } |
There was a problem hiding this comment.
[Medium] [Conventions] session_id uses raw Option bypassing validate_public_text (f-con-2)
All other public text fields use validated newtypes. session_id could contain control chars, NUL bytes, or be arbitrarily long. AGENTS.md: "Use strong types and enums over stringly-typed control flow".
Fix: Create AuthSessionId validated newtype.
| flow_id: AuthFlowId, | ||
| auth_url: String, | ||
| expires_at: Timestamp, | ||
| }, |
There was a problem hiding this comment.
[Medium] [Conventions] AuthChallenge::OAuthUrl.auth_url is unvalidated String (f-con-4)
No URL validation. Non-URL values like javascript:alert(1) or file:///etc/passwd would be stored and rendered to users.
Fix: Add URL format validation or use a validated URL type.
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct OAuthTokenMetadata { | ||
| pub provider: AuthProviderId, | ||
| pub access_secret: SecretHandle, |
There was a problem hiding this comment.
[Medium] [Performance] Continuations Vec grows without bound — no prune/ack method (f-per-2)
No trait method to prune or acknowledge consumed continuations. Grows linearly without bound.
Fix: Add ack_continuation or prune_continuations to the trait.
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct OAuthProviderCallbackRequest { |
There was a problem hiding this comment.
[Medium] [Bugs] cancel_flow returns UnknownOrExpiredFlow for valid but non-cancellable state (f-bug-2)
When flow exists but is in terminal state, error is UnknownOrExpiredFlow. Callers cannot distinguish expired from non-cancellable.
Fix: Add FlowNotCancellable { flow_id, status } error variant.
| impl InMemoryAuthProductServices { | ||
| pub fn new() -> Self { | ||
| Self::default() | ||
| } |
There was a problem hiding this comment.
[Medium] [Security] opaque_state_hash compared with String equality — not timing-safe (f-sec-1)
State hash compared with !=. Timing leak could reveal hash prefix/length. Production impls should use constant-time comparison.
Fix: Document requirement. Consider subtle::ConstantTimeEq.
| async fn request_secret_input( | ||
| &self, | ||
| request: SecretInputRequest, | ||
| ) -> Result<AuthChallenge, AuthProductError> { |
There was a problem hiding this comment.
[Medium] [Tests] CredentialSetupService methods never directly exercised by tests (f-tes-4)
submit_manual_token never called through the CredentialSetupService trait directly.
Fix: Add test calling CredentialSetupService::submit_manual_token.
| .flows | ||
| .get_mut(&flow_id) | ||
| .ok_or(AuthProductError::UnknownOrExpiredFlow)?; | ||
| ensure_same_scope(scope, &record.scope)?; |
There was a problem hiding this comment.
[Medium] [Tests] submit_secret error paths untested (f-tes-5)
Interaction-not-found and cross-scope submission error paths have no tests.
Fix: Add submit_secret_expired_interaction_returns_error and submit_secret_cross_scope_returns_denied.
2 participants
Summary
ironclaw_authas the contract-first Reborn product auth crate for [Reborn] Migrate secrets, OAuth, and auth setup product flows #3289 Step 1.docs/reborn/contracts/auth-product.mdwith the V1 behavior inventory and update the Reborn contract index.Closes #3810
Part of #3289
Validation
Local targeted validation only, per repo size/build-cost constraints:
cargo test -p ironclaw_authcargo clippy -p ironclaw_auth --all-targets -- -D warningscargo fmt --check --package ironclaw_authcargo fmt --all --checkcargo test -p ironclaw_architecturegit diff --checkFull workspace build/test intentionally left to CI.