Closed
Conversation
Add stripUrlScheme utility and use it to normalize domains by removing http/https schemes. Apply this to createLoginMessage, generateLoginPayload, and verifyLoginPayload so payload.domain is stored and compared without a URL scheme (per EIP-4361). Update tests to cover scheme stripping and backward compatibility when verifying payloads. Also adjust biome.json formatter line ending to CRLF.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 74fde1d The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds a utility to strip http(s):// schemes and trailing paths from domain strings and integrates it into SIWE login payload generation and verification. Verification normalizes domains and retries legacy verification when needed; tests and a changeset were added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+8
to
+10
| export function stripUrlScheme(domain: string): string { | ||
| return domain.replace(/^https?:\/\//, ""); | ||
| } |
Contributor
There was a problem hiding this comment.
The regex pattern is case-sensitive and will fail to strip uppercase scheme variants like HTTP:// or HTTPS://. According to RFC 3986, URI schemes are case-insensitive. This will cause EIP-4361 non-compliance when users provide domains with uppercase schemes.
Impact: Domains like "HTTP://example.com" will not be stripped, violating EIP-4361 which requires the domain field to be an authority without scheme.
Fix: Make the regex case-insensitive:
export function stripUrlScheme(domain: string): string {
return domain.replace(/^https?:\/\//i, "");
}
Suggested change
| export function stripUrlScheme(domain: string): string { | |
| return domain.replace(/^https?:\/\//, ""); | |
| } | |
| export function stripUrlScheme(domain: string): string { | |
| return domain.replace(/^https?:\/\//i, ""); | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
Contributor
size-limit report 📦
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8706 +/- ##
==========================================
+ Coverage 52.72% 52.73% +0.01%
==========================================
Files 934 935 +1
Lines 62968 62988 +20
Branches 4135 4137 +2
==========================================
+ Hits 33199 33219 +20
Misses 29671 29671
Partials 98 98
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/thirdweb/src/auth/core/create-login-message.ts (1)
3-10: MovestripUrlSchemeinto its own utility module.
generate-login-payload.tsandverify-login-payload.tsnow depend on the message-builder file just to reuse a small normalization helper. Pulling this into a dedicated auth utility keeps the dependency direction cleaner and avoids stacking multiple stateless helpers in one file. As per coding guidelines,**/*.{ts,tsx}: Limit each file to one stateless, single-responsibility function for clarity and testability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/thirdweb/src/auth/core/create-login-message.ts` around lines 3 - 10, Extract the stripUrlScheme function into its own auth utility module that exports the same function signature (export function stripUrlScheme(domain: string): string) and remove it from the message-builder file; then update callers (generate-login-payload and verify-login-payload) to import stripUrlScheme from the new utility instead of from the message-builder, leaving behavior unchanged, and update any tests or imports that referenced the old location accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/hot-donkeys-itch.md:
- Line 5: The changelog entry in .changeset/hot-donkeys-itch.md contains a
typo/awkward phrasing ("Stripe URL scheme from SIWE domain across all
touchpoints"); update that line to a clear, grammatical release note (for
example: "Switch Stripe URL scheme to use the SIWE domain across all
touchpoints" or "Use the SIWE domain for Stripe URL scheme across all
touchpoints") so the published release notes read correctly.
In `@packages/thirdweb/src/auth/core/create-login-message.ts`:
- Around line 8-10: stripUrlScheme currently only trims the http(s) prefix which
leaves trailing slashes and path segments (e.g. "https://example.com/" ->
"example.com/") that are invalid as SIWE authorities; update stripUrlScheme to
parse the input with the URL API (new URL(domain) or fallback) and return the
normalized authority (hostname plus optional port) with no trailing slash or
path, lowercased, and validate/throw if parsing fails so functions like
createLoginMessage and SIWE payload generation use a proper authority value.
In `@packages/thirdweb/src/auth/core/verify-login-payload.test.ts`:
- Around line 149-189: The test currently signs a payload generated by
generateLoginPayload (which already normalizes domain), so it doesn't cover
legacy signatures containing schemes; modify the test to build or obtain a
payload whose domain still includes the URL scheme (e.g., "https://example.com")
before signing — for example, call generateLoginPayload or clone its result and
then set payload.domain = "https://example.com" (or construct the payload object
manually) and pass that to signLoginPayload, then call
verifyLoginPayload(options) and assert verificationResult.valid is true and
verificationResult.payload.domain is normalized to "example.com"; reference
generateLoginPayload, signLoginPayload, verifyLoginPayload, and payload.domain
to locate and change the test.
In `@packages/thirdweb/src/auth/core/verify-login-payload.ts`:
- Around line 51-52: The domain normalization fixes the equality check but
verification still rebuilds the signed message with the normalized domain via
createLoginMessage(payload), so signatures created against the legacy
scheme-bearing first line will fail; update the verification logic in
verify-login-payload.ts (the function that calls createLoginMessage and performs
signature verification) to attempt verification against both message shapes: the
normalized message (using stripUrlScheme(payload.domain) as currently done) and
the legacy/raw-domain message (using the original payload.domain including
scheme), trying each in turn before rejecting; use createLoginMessage with both
domain variants and run the existing signature check for both to preserve
backward compatibility.
---
Nitpick comments:
In `@packages/thirdweb/src/auth/core/create-login-message.ts`:
- Around line 3-10: Extract the stripUrlScheme function into its own auth
utility module that exports the same function signature (export function
stripUrlScheme(domain: string): string) and remove it from the message-builder
file; then update callers (generate-login-payload and verify-login-payload) to
import stripUrlScheme from the new utility instead of from the message-builder,
leaving behavior unchanged, and update any tests or imports that referenced the
old location accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f26a9879-e385-453a-889a-3b98f3c80568
📒 Files selected for processing (7)
.changeset/hot-donkeys-itch.mdpackages/thirdweb/src/auth/core/create-login-message.test.tspackages/thirdweb/src/auth/core/create-login-message.tspackages/thirdweb/src/auth/core/generate-login-payload.test.tspackages/thirdweb/src/auth/core/generate-login-payload.tspackages/thirdweb/src/auth/core/verify-login-payload.test.tspackages/thirdweb/src/auth/core/verify-login-payload.ts
Move stripUrlScheme into its own module and add comprehensive tests. createLoginMessage no longer strips the scheme (uses payload.domain as-is), generate-login-payload imports the new util, and verify-login-payload now normalizes the domain before building the expected message for EIP-4361 compliance. verify logic also falls back to the legacy (scheme-included) message for backward compatibility with signatures from older SDKs. Updated/added tests to cover stripUrlScheme behavior and legacy payload verification.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/thirdweb/src/auth/core/strip-url-scheme.ts (1)
6-7: Consider edge cases with query strings and fragments.The second regex
/\/.*$/only strips content starting from/. URLs with query strings (?key=val) or fragments (#section) but no path would not be fully normalized.For typical SIWE domains this is unlikely to matter, but if you want full RFC 3986 authority extraction, you could extend the second replacement:
🔧 Optional refinement
export function stripUrlScheme(domain: string): string { - return domain.replace(/^https?:\/\//, "").replace(/\/.*$/, ""); + return domain.replace(/^https?:\/\//, "").replace(/[/?#].*$/, ""); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/thirdweb/src/auth/core/strip-url-scheme.ts` around lines 6 - 7, stripUrlScheme currently removes only the path via replace(/\/.*$/, "") which misses query strings or fragments when there is no path; update the implementation in the stripUrlScheme function to strip everything after the authority by matching any of "/", "?" or "#" (e.g. replace(/[\/?#].*$/, "") or equivalently split on the first occurrence of "/", "?" or "#" and take the first segment) so domains like "example.com?x=1" and "example.com#frag" are normalized correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/thirdweb/src/auth/core/strip-url-scheme.ts`:
- Around line 6-7: stripUrlScheme currently removes only the path via
replace(/\/.*$/, "") which misses query strings or fragments when there is no
path; update the implementation in the stripUrlScheme function to strip
everything after the authority by matching any of "/", "?" or "#" (e.g.
replace(/[\/?#].*$/, "") or equivalently split on the first occurrence of "/",
"?" or "#" and take the first segment) so domains like "example.com?x=1" and
"example.com#frag" are normalized correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ec61d45-a2f8-43bc-87a2-2bb660cc3017
📒 Files selected for processing (6)
.changeset/hot-donkeys-itch.mdpackages/thirdweb/src/auth/core/generate-login-payload.tspackages/thirdweb/src/auth/core/strip-url-scheme.test.tspackages/thirdweb/src/auth/core/strip-url-scheme.tspackages/thirdweb/src/auth/core/verify-login-payload.test.tspackages/thirdweb/src/auth/core/verify-login-payload.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/hot-donkeys-itch.md
- packages/thirdweb/src/auth/core/generate-login-payload.ts
- packages/thirdweb/src/auth/core/verify-login-payload.test.ts
GenerateLoginPayload now defaults uri to a fully-qualified https://<domain> by using stripUrlScheme and prepending https://, ensuring the login payload contains a valid URL. stripUrlScheme was updated to remove query strings and fragments (in addition to scheme and trailing slashes). Tests were updated to expect the https URI and to cover query/fragment cases.
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
1 participant
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.
Add stripUrlScheme utility and use it to normalize domains by removing http/https schemes. Apply this to createLoginMessage, generateLoginPayload, and verifyLoginPayload so payload.domain is stored and compared without a URL scheme (per EIP-4361). Update tests to cover scheme stripping and backward compatibility when verifying payloads.
PR-Codex overview
This PR focuses on ensuring compliance with EIP-4361 by stripping the URL scheme from domain fields in the authentication process, enhancing backward compatibility for legacy payloads.
Detailed summary
stripUrlSchemefunction to remove URL schemes and trailing paths from domain strings.generateLoginPayloadto usestripUrlSchemefor thedomainfield.verifyLoginPayloadto normalize domains by stripping URL schemes.stripUrlSchemeto cover various scenarios.generateLoginPayloadandverifyLoginPayloadto ensure correct behavior with both scheme-stripped and legacy payloads.Summary by CodeRabbit
Bug Fixes
Tests
Chores