[SDK] Fix: Route in-app wallet OTP login through getClientFetch - #8894
[SDK] Fix: Route in-app wallet OTP login through getClientFetch#8894blockgroot wants to merge 2 commits into
Conversation
sendOtp and verifyOtp built request headers by hand and called the global fetch directly, instead of going through getClientFetch like every other in-app wallet call. getClientFetch is the only place that attaches platform headers, including x-bundle-id on React Native, so OTP login never sent x-bundle-id even when it was available. Since the backend enforces the Bundle ID access restriction on this endpoint, any client ID with that restriction enabled rejected all email/phone OTP login attempts with a 401. Swap both functions to getClientFetch(client, ecosystem), matching the pattern already used in siwe.ts, and drop the now-redundant manual x-client-id/ecosystem headers since getClientFetch sets those itself. Fixes thirdweb-dev#8774
🦋 Changeset detectedLatest commit: abb7e20 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 |
|
@blockgroot is attempting to deploy a commit to the thirdweb Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughOTP email and phone requests now use ChangesOTP transport
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change routes OTP requests through the shared client fetch path and adds focused tests; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
This PR has been inactive for 7 days. It is now marked as stale and will be closed in 2 days if no further activity occurs. |
Fixes #8774
Notes for the reviewer
sendOtp/verifyOtp(packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts) built request headers by hand and called the globalfetchdirectly, instead of going throughgetClientFetchlike every other in-app wallet call (rg -l "getClientFetch" packages/thirdweb/src/wallets/in-app/→ 18 files;otp.tswas the only file in that tree calling barefetch).getClientFetchis the only place that attaches platform headers, includingx-bundle-idon React Native (viagetPlatformHeaders()inutils/fetch.ts). Because OTP login skipped it,x-bundle-idwas never sent — even when it was available fromglobalThis.Application.applicationId. Since the backend enforces the Bundle ID access restriction on this endpoint, any client ID with that restriction configured rejected every email/phone OTP login with a 401UNAUTHORIZED, and the only workaround was disabling the restriction ("Allow all bundle IDs") entirely.Fix: route both functions through
getClientFetch(client, ecosystem), matching the existing pattern insiwe.ts(the closest analog — another login endpoint on the same host). Dropped the now-redundant manualx-client-id/x-ecosystem-id/x-ecosystem-partner-idheaders sincegetClientFetchsets those itself; kept onlyContent-Type, which it doesn't set.Confirmed the swap is safe: both URLs resolve to
embedded-wallet.thirdweb.com, whichgetClientFetch'sisInAppWalletUrlbranch already excludes from the Bearer-token/auth-token path — so this only adds the missing headers, it doesn't change which credential gets sent.How to test
Added
otp.test.ts, following thevi.mock/vi.mocked(getClientFetch).mockReturnValue(...)pattern already used inbackend.test.ts:sendOtp/verifyOtpcallgetClientFetch(client, ecosystem)and invoke the returned fetch.getClientFetchdid nothing (the realfetchstill fired), so the tests hit the live API and failed (KEY_NOT_FOUND/Failed to verify verification code) — that failure is the proof the bug existed.ecosystemargument is forwarded, since that's exactly the branch the old manual-header code depended on.Both green (10/10).
biome checkon the changed files is clean.PR-Codex overview
This PR focuses on improving the OTP login functionality by ensuring that the
sendOtpandverifyOtpfunctions usegetClientFetchinstead of the globalfetch. This change allows the necessary platform headers to be attached, enabling proper authentication with Bundle ID access restrictions.Detailed summary
sendOtpandverifyOtpto usegetClientFetchinstead of globalfetch.x-client-id,x-ecosystem-id,x-ecosystem-partner-id) fromsendOtpandverifyOtp.getClientFetchand attach necessary headers.Summary by CodeRabbit
Bug Fixes
Tests