fix(siwe): isUri rejects authority-only URIs with an empty path - #434
Merged
Conversation
Siwe.isUri required scheme?.length && path && path.length >= 0 - the `path &&` guard short-circuited before the tautological length check ever ran, so any URI with an empty path (e.g. a bare origin like https://example.com) was rejected. The comment directly above states the opposite intent: "scheme and path are required, though the path can be empty". Per RFC 3986 an authority-only URI legitimately has an empty path component, and this is also the single most common SIWE uri value - Siwe.createMessage threw for https://login.xyz, the EIP-4361 reference example, and for any pathless entry in resources. viem's identical sibling function (utils/siwe/utils.ts) never had this guard and has always accepted these URIs correctly. Fix: drop the redundant `path &&`, matching viem exactly. path can never be undefined here - splitUri's third capture group is a mandatory (non-optional) regex group, always at least an empty string. Also corrects an existing pinned test ('behavior: invalid resources') that was unknowingly asserting the bug: with resources set to ['https://example.com', 'foo'], the old code rejected the FIRST entry (a legitimate bare-origin URI) instead of the actually-invalid second one. Fixed code correctly flags 'foo'.
|
@gomesalexandre is attempting to deploy a commit to the Wevm Team on Vercel. A member of the Team first needs to authorize it. |
gomesalexandre
marked this pull request as ready for review
September 1, 2026 14:44
commit: |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Siwe.isUrirejects every authority-only URI -https://login.xyz, the reference example from EIP-4361 itself, cannot be used as a SIWEuri.The bug
src/core/Siwe.ts:333:pathis a plain string; for an authority-only URI (no path segment)splitUriyields'', which is falsy, sopath &&short-circuits beforepath.length >= 0- the clause whose entire purpose is to permit an empty path - ever runs. The comment on the line above states the opposite of what the code does.Second tell: two lines down,
if (!(path.length === 0 || path.startsWith('/'))) return falsewas already provably dead code -path.length === 0could never be reached past line 333's bug.Third: viem's identical sibling function (
utils/siwe/utils.ts) never had this guard:RFC 3986
An authority-only URI legitimately has an empty path component (
URI = scheme ":" hier-part ...,hier-part = "//" authority path-abempty, wherepath-abemptyexplicitly allows zero segments - RFC 3986 §3).https://example.comis a valid, common URI.Repro (real run against current
main)Also confirmed live in the currently published
ox@1.7.4npm package, bothsrc/core/Siwe.tsand the compileddist/core/Siwe.js- this isn't a working-tree-only bug.Fix
Drop the redundant
path &&, matching viem exactly:Safe by construction:
splitUri's path capture group([^?#]*)is a mandatory (non-optional) regex group, sopathis always at least'', neverundefined- confirmed both by reading the regex and by Codex's independent review (see below).Bonus catch: an existing test was unknowingly pinning the bug
behavior: invalid resourcespassedresources: ['https://example.com', 'foo']expecting'foo'to be flagged, but the old buggy code rejected'https://example.com'first (a legitimate bare-origin URI) and the pinned snapshot had silently encoded that wrong behavior as "expected". Fixed code correctly reportsfooas the invalid entry. Updated the snapshot accordingly - flagging explicitly so it doesn't read as unexplained test churn.Testing
Real command output,
82/82passing:Confirmed genuine red-before/green-after: reverted only
Siwe.ts(kept the new tests), re-ran -8 failed | 74 passed (82), exactly the 7 new tests plus the corrected pinned snapshot. Restored the fix - back to82/82.Full
coreproject suite:189/193files pass; the 4 failing files (AbiConstructor,AbiError,Log,Provider,RpcResponse,RpcTransport,TransactionEnvelope*,TransactionRequest- all namedbehavior: network) are live-RPC-dependent tests unrelated to Siwe, hittingoxlib.sh/live infra 400s - none touchSiwe.tsorSiwe.test.ts.tsc --noEmit -p .: clean.vp check: 0 errors (2 pre-existing unrelated warnings insite/src/components/Landing.tsx).New test cases added to the
isUriandcreateMessagedescribe blocks: bare origin, with port, with query, with fragment, non-http scheme (all authority + empty path), pluscreateMessagewith a pathlessuriand a pathlessresourcesentry.Codex adversarial review
Ran synchronously, verdict SHIP, no blocking findings. Independently confirmed: RFC 3986 correctness, that
pathcannot beundefined/nullgiven the mandatory regex capture, and that the corrected pinned snapshot is right. One non-blocking style note (thepath.length >= 0check is tautological given the invariant) - kept as-is to match viem's exact wording for cross-repo consistency, which Codex agreed was a reasonable call.Scope
This fails closed (rejects/throws) - a functional/availability bug, not an authentication bypass or security issue.