fix(oauth): reject malformed ic2.* states in decode_hosted_oauth_state (#1441)

[ilblackdragon]

Summary

• Fix decode_hosted_oauth_state so any state starting with ic2. must fully parse as a valid versioned envelope or return Err — malformed ic2.* states no longer silently fall through to legacy handling, which would use the full envelope as the flow_id and fail to match the raw nonce in pending_oauth_flows
• Update existing test to expect Err instead of successful fallback for non-envelope ic2. prefixed states
• Add regression tests: malformed envelope rejection (bad base64, bad JSON, missing separator) and encode→decode round-trip consistency

Test plan

• cargo test -p ironclaw --lib cli::oauth_defaults — 29 tests pass
• cargo clippy --all --benches --tests --examples --all-features — zero warnings
• cargo fmt --check — clean

Closes #1441

🤖 Generated with Claude Code

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[Copilot]

Pull request overview

Fixes hosted OAuth state decoding so ic2.-prefixed states must be valid versioned envelopes (and error otherwise), preventing silent legacy fallback that can break pending-flow lookup.

Changes:

• Make decode_hosted_oauth_state return Err for malformed ic2.* states instead of falling back to legacy parsing.
• Update/extend unit tests to cover malformed envelope rejection and encode→decode round-trip behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

strip_prefix(&format!("{HOSTED_STATE_PREFIX}.")) allocates a new String on every decode attempt. Consider using a constant like HOSTED_STATE_PREFIX_DOT: &str = "ic2." (or concat!) and pass that directly to strip_prefix to avoid the per-call allocation.

[ilblackdragon]

Fixed in eebb092 — added HOSTED_STATE_PREFIX_DOT constant and replaced the &format!(...) call with it.

[Copilot]

In the "Valid base64 but not JSON" case, the test uses a fake checksum, so decoding can fail at the checksum check and never exercise the JSON parse error branch. To make this a stronger regression, build a state with the correct checksum for the non-JSON payload and assert the error contains the JSON parse failure message.

Suggested change

	// Valid base64 but not JSON
	use base64::Engine;
	let not_json = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(b"not json");
	let err = decode_hosted_oauth_state(&format!("ic2.{not_json}.fakechecksum"))
	.expect_err("non-JSON payload should fail (possibly checksum)");
	assert!(
	err.contains("checksum") || err.contains("JSON"),
	"unexpected error: {err}"
	// Valid base64 but not JSON: ensure checksum is correct so we exercise JSON parsing.
	use base64::Engine;
	use sha2::Sha256;
	let not_json = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(b"not json");
	// Compute a checksum that matches the non-JSON payload so decoding reaches JSON parsing.
	let mut hasher = Sha256::new();
	hasher.update(not_json.as_bytes());
	let digest = hasher.finalize();
	// Truncate the digest in the same way as the production code and encode with URL_SAFE_NO_PAD.
	let checksum = super::URL_SAFE_NO_PAD.encode(&digest[..8]);
	let err = decode_hosted_oauth_state(&format!("ic2.{not_json}.{checksum}"))
	.expect_err("non-JSON payload should fail with a JSON parse error");
	assert!(
	err.contains("JSON"),
	"unexpected error (expected JSON parse failure): {err}"

[ilblackdragon]

Fixed in eebb092 — the test now computes the correct checksum over the raw non-JSON bytes so decoding reaches the JSON parse step. Assertion now requires the error to contain "JSON".

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

HOSTED_STATE_PREFIX and HOSTED_STATE_PREFIX_DOT duplicate the prefix string ("ic2" vs "ic2.") and can drift if the prefix ever changes. Consider defining the dotted prefix in terms of HOSTED_STATE_PREFIX (e.g., const HOSTED_STATE_PREFIX_DOT: &str = concat!(HOSTED_STATE_PREFIX, ".");) to keep them mechanically consistent.

Suggested change

	const HOSTED_STATE_PREFIX_DOT: &str = "ic2.";
	const HOSTED_STATE_PREFIX_DOT: &str = concat!(HOSTED_STATE_PREFIX, ".");

[ilblackdragon]

Fixed in e4d0601 — removed the separate HOSTED_STATE_PREFIX_DOT constant since concat\! requires literals and can't reference const items. Both encode and decode now derive the dotted prefix via format\!("{HOSTED_STATE_PREFIX}.") from the single HOSTED_STATE_PREFIX constant, so they can't drift.

[zmanian]

Code Review — reject malformed ic2.* states in decode_hosted_oauth_state

+86 / -18 across 2 files. Clean, focused bug fix. Closes #1441.

---

Summary

The bug: decode_hosted_oauth_state used chained let guards (if let ... && let ... && let ...) which meant any parse failure in an ic2.*-prefixed state silently fell through to legacy handling. The full malformed envelope string was then used as the flow_id, which never matched the original nonce stored in pending_oauth_flows, breaking the OAuth callback.

The fix: each parse step (rsplit_once, decode, from_slice, empty check) now returns an explicit Err with a descriptive message. ic2.* states must fully parse or fail — no fallthrough.

Assessment

This is correct and complete. The fix directly addresses the root cause and the test coverage is thorough.

Minor observations

1. format! allocation on every decode call (low — already addressed)

strip_prefix(&format!("{HOSTED_STATE_PREFIX}.")) allocates per call. Copilot flagged this and @ilblackdragon fixed it in e4d0601 by deriving the dotted prefix from the single HOSTED_STATE_PREFIX constant.

2. fallback_deliverable: None in job_monitor.rs (nit)

Three additions adapting to a struct change from another PR on staging. Unrelated to the OAuth fix but harmless.

3. Legacy path still accepts arbitrary strings as flow_id (pre-existing, not this PR)

The legacy fallback (bare nonce or instance:nonce format) still accepts any string as a flow_id without validation. This is fine for backward compat but worth noting — a non-ic2. prefixed garbage string will parse as a legacy flow_id and only fail later when the lookup misses.

Verdict

Approve. The fix is minimal, correct, and well-tested. All review comments have been addressed in follow-up commits.

[zmanian]

Review: reject malformed ic2.* states in decode_hosted_oauth_state

Verdict: Approve

Correctness

The fix is correct and directly addresses the root cause. The old code used chained if let ... && let ... guards, which meant any parse failure (bad base64, bad JSON, missing separator) in an ic2.*-prefixed state silently fell through to legacy handling. The full envelope string was then used as the flow_id, which never matched the nonce in pending_oauth_flows. The fix converts each guard into an explicit Err return -- no fallthrough possible.

Security

No bypass paths remain for ic2.* states. Once strip_prefix matches, the function is committed to the versioned path and will return Err on any malformation. The checksum validation still runs before JSON parsing, preventing payload tampering.

One pre-existing observation (not introduced by this PR): the legacy path accepts any non-empty string as a flow_id without structural validation. That's fine for backward compat but worth tracking.

Error handling

Follows project conventions: descriptive String errors via .map_err() and .ok_or(). No .unwrap() or .expect() in production code. Error messages are specific enough to distinguish failure modes in logs (checksum separator, base64, JSON, empty flow_id).

Tests

Comprehensive coverage:

• test_decode_hosted_oauth_state_rejects_non_envelope_ic2_prefix -- the original bug scenario
• test_decode_versioned_state_rejects_malformed_envelopes -- missing separator, bad base64, valid-base64-but-not-JSON (with correct checksum to exercise the JSON parse path)
• test_oauth_flow_key_round_trip_consistency -- encode/decode round-trip with and without instance name

The non-JSON test correctly computes the real checksum over the raw bytes so it gets past the checksum check and exercises the JSON parse error path. This was addressed in the follow-up commit per Copilot's feedback.

Minor note

The format!("{HOSTED_STATE_PREFIX}.") allocation on every decode call remains after e4d0601 reverted the HOSTED_STATE_PREFIX_DOT constant (because concat! can't reference const items). This is negligible in practice -- OAuth state decoding is not a hot path -- but could be addressed with a once_cell::sync::Lazy or by making the prefix a literal "ic2." with a comment linking it to HOSTED_STATE_PREFIX. Not blocking.

The fallback_deliverable: None additions in job_monitor.rs are unrelated struct-field adaptations from staging. Harmless.

LGTM -- clean, minimal fix with strong test coverage.

[claude]

Code review

Found 7 issues:

1. [CRITICAL:92] Timing-sensitive checksum comparison enables token forgery
   Line 590 uses string equality instead of constant-time comparison, enabling timing attacks on OAuth state tokens. Other auth paths use subtle::ConstantTimeEq.
   https://github.com/anthropics/ironclaw/blob/9d538136b5d86a1eb0a11ef469729b7304db24fb/src/cli/oauth_defaults.rs#L588-L592

2. [HIGH:95] Return type violates CLAUDE.md error design rule
   decode_hosted_oauth_state() returns Result<T, String> instead of typed error enum. CLAUDE.md requires thiserror types. Module already uses OAuthCallbackError elsewhere.
   https://github.com/anthropics/ironclaw/blob/9d538136b5d86a1eb0a11ef469729b7304db24fb/src/cli/oauth_defaults.rs#L581-L581

3. [HIGH:78] User-controlled flow_id logged to tracing
   encode_hosted_oauth_state logs flow_id from user-supplied OAuth requests. If it contains sensitive params, this leaks to logs.
   https://github.com/anthropics/ironclaw/blob/9d538136b5d86a1eb0a11ef469729b7304db24fb/src/cli/oauth_defaults.rs#L569-L572

4. [MEDIUM:80] No size limits on base64 decode allows memory DoS
   URL_SAFE_NO_PAD.decode(payload_b64) on user input has no bounds. Attacker can send large base64 to exhaust memory on public OAuth endpoint. Recommend: check payload_b64.len() before decode.
   https://github.com/anthropics/ironclaw/blob/9d538136b5d86a1eb0a11ef469729b7304db24fb/src/cli/oauth_defaults.rs#L586-L588

5. [MEDIUM:75] String allocation on every invocation (format!())
   Line 582 calls format!("{HOSTED_STATE_PREFIX}.") each time, allocating new String. HOSTED_STATE_PREFIX is const "ic2", should use const or starts_with(). Hot path: every OAuth request.
   https://github.com/anthropics/ironclaw/blob/9d538136b5d86a1eb0a11ef469729b7304db24fb/src/cli/oauth_defaults.rs#L582-L582

6. [MEDIUM:70] Detailed error messages leak validation structure
   Error messages describe envelope format ("missing checksum separator", "base64 decode failed", etc.), helping attackers target attacks. Consider generic messages.
   https://github.com/anthropics/ironclaw/blob/9d538136b5d86a1eb0a11ef469729b7304db24fb/src/cli/oauth_defaults.rs#L585-L596

7. [MEDIUM:65] JSON parsing has no size limit (complementary DoS vector)
   serde_json::from_slice() on decoded payload with no max size. Combined with feat: Sandbox jobs #4, allows memory exhaustion via oversized JSON. Recommend checking payload_json.len().
   https://github.com/anthropics/ironclaw/blob/9d538136b5d86a1eb0a11ef469729b7304db24fb/src/cli/oauth_defaults.rs#L593-L594