fix: harden SEP-10 challenge and JWT token validation - #133
Merged
Conversation
Reject SEP-10 challenge replay by tracking consumed challenge transaction hashes for the duration of their validity window, and stop leaking underlying SDK/validation error details in auth failure responses so all challenge failures resolve to a single generic unauthorized response. Enforce JWT algorithm, issuer, and audience on both sign and verify so a token minted for a different environment or signed with a different algorithm is rejected outright, and validate the decoded claim shape before trusting it. Fail fast at startup if SEP10_SIGNING_SECRET is missing in production, since the previous silent per-process fallback key breaks verification across restarts and multi-instance deployments. Closes mergepay#111
|
@AdaBebe0 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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.
Summary
Closes #111
SEP-10 authentication already delegated most structural validation (server signing key, home domain, web_auth_domain, network passphrase, basic timebounds) to the Stellar SDK's
WebAuth.readChallengeTx/verifyChallengeTxThreshold/verifyChallengeTxSignershelpers. This PR closes the remaining gaps called out in the issue:/auth/verifymore than once within its validity window, minting a fresh token each time.verifyChallengenow records the consumed challenge transaction's hash and rejects a second use of the same signed transaction. The guard is in-memory and pruned by expiry; a multi-instance deployment would need a shared store (e.g. Redis) for the same guarantee across nodes — noted inline insrc/services/sep10.ts.invalid_challenge: <sdk internal message>). All challenge failures — malformed XDR, wrong signer, wrong domain, wrong network, expired timebounds, replay — now resolve to a single generic 401UNAUTHORIZEDresponse with no validation detail attached, and nothing is logged.signToken/verifyTokennow set and enforcealg(pinned toHS256),iss, andaudexplicitly, backed by newJWT_ISSUER/JWT_AUDIENCEconfig values (defaulting to the API's own host, like the existing SEP-10 domain config). A token minted for a different environment/audience, with a different algorithm, or with a malformed claim shape is rejected before itssub/pkare ever trusted.SEP10_SIGNING_SECRETsilently fell back to a random per-process keypair. In production this would mean challenges minted by one instance (or before a restart) can never be verified by another. The server now refuses to start in production without a configured signing key (dev/test behavior is unchanged).No route authorization behavior changed beyond correcting these authentication gaps —
requireMembership/requireAdminand route-level checks are untouched.Testing
tests/sep10-hardening.test.ts(new): expired challenge, wrong network, wrong home domain, wrong signer, malformed XDR, challenge replay, and that rejection messages don't leak SDK internals.tests/plugins/auth.test.ts(new): issuer/audience embedding, rejection on wrong secret/issuer/audience/algorithm/expiration, missing claims, and that a tampered payload (swapped account) fails signature verification.tests/sep10.test.tsand full route test suite remain green.npm run test:integrationrequires a live server + database/testnet connection that isn't available in this sandbox, so it wasn't run end-to-end here; it depends only onsrc/routes/auth.tsandsrc/services/sep10.ts, which keep the same request/response contracts.