MF3-L12: validate session-key scope ID formats at request boundary#826
MF3-L12: validate session-key scope ID formats at request boundary#826philanton wants to merge 1 commit into
Conversation
…dary submit_session_key_state only checked array length and lowercasing for ApplicationIDs and AppSessionIDs, letting malformed-but-lowercase IDs reach signature verification and persist as inert, unusable scopes. Validate each application_id with app.IsValidApplicationID (subsumes the lowercase check) and each app_session_id with core.IsValidHash plus the lowercase canonical check, rejecting both at the request boundary — matching the validation already applied in create_app_session. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 163 files, which is 13 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (163)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nksazonov
left a comment
nksazonov
left a comment
There was a problem hiding this comment.
Good job — the fix correctly addresses the audit finding by moving both IsValidApplicationID and IsValidHash checks ahead of signature verification, closing the malformed-but-lowercase bypass. Two minor polish items below.
| if !core.IsValidHash(id) { | ||
| c.Fail(rpc.Errorf("invalid_session_key_state: app_session_ids must be 0x-prefixed 32-byte hashes, got: %s", id), "") | ||
| return | ||
| } | ||
| if id != strings.ToLower(id) { | ||
| c.Fail(rpc.Errorf("invalid_session_key_state: app_session_ids must be lowercase, got: %s", id), "") | ||
| return |
There was a problem hiding this comment.
The two guards produce two different error messages for what the protocol defines as a single rule: "must be a 0x-prefixed 32-byte lowercase hex hash." A caller with an uppercase-hex input (e.g. 0xABCD…) passes the first check and hits the second, receiving a different error code than a caller with a non-hash string. API consumers have to handle two error surfaces for one conceptual constraint.
Suggestion: add core.IsValidLowercaseHash() with regex ^0x[0-9a-f]{64}$ (the exact canonical form), replace both guards with a single call and a single error message, and keep the test coverage for both uppercase-hex and non-hash inputs confirming they both fail via the same path.
| if id != strings.ToLower(id) { | ||
| c.Fail(rpc.Errorf("invalid_session_key_state: application_ids must be lowercase, got: %s", id), "") | ||
| if !app.IsValidApplicationID(id) { | ||
| c.Fail(rpc.Errorf("invalid_session_key_state: application_ids must match %s, got: %s", app.ApplicationIDRegex.String(), id), "") |
There was a problem hiding this comment.
The regex string (^[a-z0-9_-]{1,66}$) in the error message is an internal implementation detail that callers shouldn't need to parse to understand what went wrong. If the regex ever changes, existing clients matching on the error text will silently break.
Suggestion: replace the dynamic app.ApplicationIDRegex.String() with a stable human-readable description, for example: "application_ids must contain only lowercase letters, digits, dashes, and underscores (max 66 chars), got: %s".
ihsraham
left a comment
ihsraham
left a comment
There was a problem hiding this comment.
No blockers from me. Approving for L-12 closure.
The fix moves both scope-ID format checks to the request boundary before signature verification and storage. application_ids now use app.IsValidApplicationID, and app_session_ids require a 32-byte hash shape plus lowercase canonical form. The new tests also cover malformed app IDs and app-session IDs and assert those requests do not reach LockSessionKeyState, which is the right proof for this finding.
What is still worth cleaning up is not a closure blocker: the public docs/API wording still describes these arrays as loose lowercase strings rather than the stricter formats now enforced. I would update that in the release/docs pass so clients do not copy the old shape.
3 participants
What
app_sessions.v1.submit_session_key_stateonly validated array length and lowercasing forapplication_idsandapp_session_ids. It did not validate each ID's format. As a result malformed-but-lowercase IDs passed request validation, reached signature verification, and could persist as inert, unusable scopes (never matching a real application/app-session).This aligns the handler with the validation already applied in
create_app_sessionand the RPCapplication_idquery-param path.Changes
application_ids: replaced the lowercase-only loop withapp.IsValidApplicationID()(regex^[a-z0-9_-]{1,66}$, which already enforces lowercase).app_session_ids: addedcore.IsValidHash()(^0x[0-9a-fA-F]{64}$) before the lowercase canonical check.IsValidHashis case-insensitive, so the lowercase check is kept.submitStateExpectingErrorhelper; added format-rejection cases (uppercase / illegal char / over-length application IDs; non-hash / short / no-0x/ uppercase-hex app-session IDs).Severity
Low — hardening/consistency, not an exploitable vuln. The request is self-authenticated (both
user_sigandsession_key_siggate persistence, so no injection into another user's state), and DB column constraints (VARCHAR(66)/CHAR(66)) already reject oversized values. Worst real case is a user signing their own malformed scope that silently never authorizes anything; this rejects it up front instead.Test
All green.
🤖 Generated with Claude Code