fix: domain-bind wallet signature challenges (#118) - #124
Conversation
The nonce challenge was not domain-bound: verifySignature accepted raw signatures over the bare nonce hex and a "Stellar Signing Key:" fallback, so a (nonce, signature) pair captured from any other context could be replayed against StepFi. Every accepted signature now signs a canonical StepFi challenge envelope (domain, address, uri, version, nonce, issuedAt, expirationTime, networkPassphrase); the nonce row stores a SHA-256 digest of the exact message and verification only ever runs against that message. Browser wallets verify per SEP-53; the legacy raw-nonce scheme is deprecated behind AUTH_ALLOW_LEGACY_RAW_SIGNATURES with a 2026-10-31 sunset. Migration: 20260825000000_add_nonce_message_binding.sql. Generated with Codebuff 🤖 Co-Authored-By: Codebuff <[email protected]>
EmeditWeb
left a comment
EmeditWeb
left a comment
There was a problem hiding this comment.
✅ Automated Audit: solves
The PR targets the root cause rather than symptoms: nonce rows now persist issued_at plus a SHA-256 message_hash of the exact canonical envelope, verification selects exactly one scheme per request (removing the weakest-format fallback chain), and signatures are validated against the stored-hash-bound message with strict domain/URI/network-passphrase/expiry checks, making cross-service replay impossible for the canonical schemes. Both unit and e2e regression tests were added covering the issue's mandated cases (legacy rejected once flag off, sep0043/envelope happy paths, foreign-domain, tampered-message, expired envelope), satisfying the testing standard. The significant caveat is that AUTH_ALLOW_LEGACY_RAW_SIGNATURES defaults to true, so the replayable raw-nonce path remains open out-of-the-box until an operator flips the flag; this is expressly permitted by the issue's ground rule ('preserve mobile-client compatibility OR provide an explicit, documented migration window') and is documented with a sunset date, but the exported LEGACY_RAW_SIGNATURES_SUNSET constant appears to have no runtime enforcement. Additionally, the auth.service.ts patch is truncated mid-file, so verifyLegacyRawSignature/assertChallengeBinding internals and null-message_hash row handling could not be fully audited.
Gaps identified:
- Legacy raw-nonce scheme is enabled by default (AUTH_ALLOW_LEGACY_RAW_SIGNATURES=true), so the exact replay vulnerability in issue #118 remains exploitable in default deployments during the migration window; consider defaulting to false
- No runtime enforcement of the 2026-10-31 sunset date — LEGACY_RAW_SIGNATURES_SUNSET appears informational only; a hard time-based reject would remove reliance on manual ops action
- Truncated diff prevented full audit of resolveChallengeMessage()/assertChallengeBinding() and behavior for pre-migration nonce rows with null message_hash (unit test exists but implementation unverifiable)
- Replay-across-environments isolation relies on networkPassphrase/domain equality checks; a dedicated cross-environment test vector (same keys, different API_URL) was not shown
Audited by stepfi-audit-bot 🤖
EmeditWeb
left a comment
EmeditWeb
left a comment
There was a problem hiding this comment.
⚠️ Automated Audit: partial
@Marvelg256 Good start — please look into the gaps identified below.
The code changes genuinely target the root cause: a canonical StepFi-bound challenge envelope, SHA-256 message hashes stored on nonce rows so verification only runs against the issued challenge, strict domain/URI/network/expiry checks, removal of the multi-format fallback, plus unit/e2e regression tests and documented migration window — matching the issue's requirements. However, the PR cannot be approved because independent verification reports merge conflicts with the base branch, and the sandbox test run failed entirely ('[WinError 2]'), so the claimed '353 tests passing' is unverified. Additionally, the legacy raw-nonce scheme (the actual vulnerable path) remains accepted by default (AUTH_ALLOW_LEGACY_RAW_SIGNATURES=true), so cross-service replay stays exploitable out-of-the-box until the flag is flipped; this is permitted by the issue's migration-window clause but means the deployed fix is incomplete by default.
Gaps identified:
- Merge conflicts with base branch must be resolved before approval
- Independent test execution failed; PR-claimed results (353 passing) are unverified
- Cross-service replay of legacy raw-nonce signatures remains possible by default until AUTH_ALLOW_LEGACY_RAW_SIGNATURES=false; no enforcement tied to the 2026-10-31 sunset (date is documentation-only)
- Truncated diffs prevent confirming every claimed regression test (legacy-disabled rejection, wrong-network, expired-envelope, rows-without-hash) actually exists
CI checks: none configured
Merge conflicts:
Sandbox error: [WinError 2] The system cannot find the file specified
Audited by stepfi-audit-bot 🤖
EmeditWeb
left a comment
EmeditWeb
left a comment
There was a problem hiding this comment.
⚠️ Automated Audit: partial
@Marvelg256 Good start — please look into the gaps identified below.
The PR substantively addresses the root cause of #118: nonce rows now persist a SHA-256 digest of the exact canonical domain-bound challenge, verification runs against exactly one client-selected scheme with strict domain/URI/network-passphrase/expiry checks, the multi-format fallback ('try raw, then Stellar Signing Key') is removed, and regression tests were added for legacy-disabled, wrong-domain, tampered-message, expired-envelope, and replay paths. The independent sandbox run (29 suites / 353 tests passed) is consistent with the PR's claimed results, so no discrepancy needs flagging. However, the PR has unresolved merge conflicts with the base branch and per policy cannot be approved as-is, and because AUTH_ALLOW_LEGACY_RAW_SIGNATURES defaults to true, the exact cross-service replay attack described in the issue still succeeds in any default deployment until an operator manually flips the flag - the 2026-10-31 sunset is documentation-only with no server-side enforcement.
Gaps identified:
- Unresolved merge conflicts with base branch - PR cannot be merged/approved until rebased
- Legacy raw-nonce scheme remains accepted by default (AUTH_ALLOW_LEGACY_RAW_SIGNATURES=true), so the issue's core replay vector is still live out-of-the-box; the sunset date has no enforcement mechanism (no runtime cutoff after 2026-10-31)
- Strongest regression coverage lives in the e2e suite, which requires Supabase credentials and is excluded from both CI (none configured) and the independent verification run
- Acceptance criterion 'every accepted signature provably signs a StepFi-bound challenge' is only conditionally met (flag-dependent), though this follows the migration window the issue itself permits
CI checks: none configured
Merge conflicts:
Independent test run: PASSED
actions.controller.spec.ts (12.833 s)
PASS test/unit/modules/auth/auth.controller.spec.ts (7.894 s)
PASS test/unit/modules/users/users.controller.spec.ts (17.977 s)
[Nest] 6840 - 08/26/2026, 8:54:46 AM ERROR [LoansService] Failed to build create_loan XDR for pending-1787730886826-nhuroyao: Soroban unavailable
[Nest] 6840 - 08/26/2026, 8:54:46 AM ERROR [LoansService] Failed to persist pending loan pending-1787730886842-3el0g8ju: insert failed
[Nest] 6840 - 08/26/2026, 8:54:46 AM ERROR [LoansService] Failed to fetch reputation score for GABCDEFGHIJKLMNOPQRSTUVWXYZ234567ABCDEFGHIJKLMNOPQRSTUVW: rpc timeout
[Nest] 6840 - 08/26/2026, 8:54:47 AM ERROR [LoansService] Failed to update loan 11111111-2222-3333-4444-555555555555 status: update failed
[Nest] 6840 - 08/26/2026, 8:54:47 AM ERROR [LoansService] Failed to fetch loans for GABCDEFGHIJKLMNOPQRSTUVWXYZ234567ABCDEFGHIJKLMNOPQRSTUVW: db offline
PASS test/unit/modules/loans/loans.service.spec.ts (144.051 s)
PASS test/unit/modules/auth/jwt-auth.guard.spec.ts
PASS test/unit/modules/loans/loans.controller.spec.ts (22.426 s)
PASS test/unit/modules/health/health.controller.spec.ts
PASS test/unit/stellar/contracts/clients/creditline.client.spec.ts
PASS test/unit/modules/vendors/vendors.service.spec.ts (26.8 s)
PASS test/unit/modules/learners/learners.controller.spec.ts
PASS test/unit/modules/learners/learner-profile.dto.spec.ts
PASS test/unit/modules/liquidity/liquidity.controller.spec.ts (5.182 s)
PASS test/unit/modules/vouching/vouching.service.spec.ts (5.866 s)
PASS test/unit/stellar/stellar.service.spec.ts (5.24 s)
A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them.
Test Suites: 29 passed, 29 total
Tests: 353 passed, 353 total
Snapshots: 0 total
Time: 155.802 s
Ran all test suites.
Audited by stepfi-audit-bot 🤖
EmeditWeb
left a comment
EmeditWeb
left a comment
There was a problem hiding this comment.
⚠️ Automated Audit: partial
@Marvelg256 Good start — please look into the gaps identified below.
The code changes genuinely implement domain-bound challenge envelopes (canonical envelope issuance, SHA-256 hash binding on nonce rows, strict SEP-53 verification, explicit signatureType routing replacing the multi-format fallback). The root cause — cross-service signature replay via unbound nonce challenges — is structurally addressed. However, AUTH_ALLOW_LEGACY_RAW_SIGNATURES defaults to true, meaning the old replayable raw-nonce scheme remains the default accepted format, so the vulnerability persists for any client not yet migrated. The unit test that supposedly validates legacy rejection when the flag is off appears to pass the signed canonical message to the legacy verifier (which expects a signature over raw nonce bytes), so it may not be a valid regression test of the intended guard. Additionally, the PR has merge conflicts and cannot be merged.
Gaps identified:
- Merge conflicts must be resolved before merge
- Negative regression test: explicitly test that legacy raw-nonce signatures are rejected when AUTH_ALLOW_LEGACY_RAW_SIGNATURES=false
- Default of AUTH_ALLOW_LEGACY_RAW_SIGNATURES=true means the vulnerable path remains open by default — document a concrete timeline/trigger for flipping it
- No test for expired envelope rejection in the unit suite (only covered in e2e if present)
- PR description does not clarify whether the legacy flag defaults to true or false in the verification claim of '353 tests passing' — CI only ran build-test, not the full e2e suite
CI checks: ✅ PASSED: build-test
Merge conflicts:
Audited by stepfi-audit-bot 🤖
…pp#118) The 2026-10-31 legacy migration window was documentation-only, so the replayable cross-service vector from StepFi-app#118 stayed open in default deployments until an operator manually flipped AUTH_ALLOW_LEGACY_RAW_SIGNATURES. Legacy raw-nonce signatures are now hard-rejected after AUTH_LEGACY_SIGNATURES_SUNSET (default 2026-10-31) even while the flag is true, closing the window automatically at the sunset. Also closes the StepFi-app#124 audit gaps: the legacy-disabled regression test now proves rejection before any signature verification, sunset-cutoff unit tests were added, and the unresolved conflict markers in context/progress-tracker.md were resolved. Generated with Codebuff 🤖 Co-Authored-By: Codebuff <[email protected]>
|
@EmeditWeb - All the audit gaps from the review are addressed, and the merge
Verification: Please review when you get a chance 🙏 |
2 participants
🔗 Related Issue
Closes #118
🔖 Title
Domain-bind wallet signature challenges — eliminate cross-service signature replay
📝 Description
AuthService.verifySignature()previously accepted two ad-hoc message formats — raw Ed25519 over the bare nonce hex, and the'Stellar Signing Key: ' + noncefallback — neither of which binds a signature to StepFi. An attacker who captured a (nonce, signature) pair from any other dApp, phishing prompt, or service using an identical raw scheme could self-issue the same nonce viaPOST /auth/nonceand authenticate as the victim. Because the server tried formats in order, the weakest accepted format defined the security floor.This PR makes every accepted signature provably sign a StepFi-bound challenge:
generateNonce()now issues a deterministic envelope —domain,address(wallet),statement,uri,version,nonce,issuedAt,expirationTime,networkPassphrase— and returns it as a newmessagefield on the nonce response.issued_atandmessage_hash(SHA-256 of the exact challenge text). Verification only ever runs against a message whose digest matches the stored hash — never against client-supplied alternatives (AUTH_CHALLENGE_MISMATCHotherwise).signatureType: 'sep0043'verifies per SEP-53 (signature overSHA-256("Stellar Signed Message:\n" + envelope), which is what Freighter'ssignMessage()produces) with strict checks: envelopedomain== our host,urimatches, network passphrase matches, and the envelope is not expired.signatureType: 'envelope'verifies raw Ed25519 over the envelope UTF-8 text.AUTH_ALLOW_LEGACY_RAW_SIGNATURES(defaulttruefor mobile-client compatibility) with a documented sunset of 2026-10-31. When disabled, legacy requests fail withAUTH_LEGACY_SIGNATURE_DISABLED.signatureType; the old multi-format fallback is removed.Compatibility choice (per issue ground rules): documented migration window. The legacy scheme stays accepted by default so current StepFi-App builds keep working, and the window is documented in
docs/setup/environment-variables.md,.env.example, and the progress tracker.🔄 Changes Made
supabase/migrations/20260825000000_add_nonce_message_binding.sql— addissued_at,message_hashtononcessrc/modules/auth/auth.service.ts— canonical envelope issuance, hash-bound verification, SEP-53 browser verification, flag-gated legacy pathsrc/modules/auth/dto/verify-request.dto.ts—signatureTypegains'envelope'; optionalmessagefieldsrc/modules/auth/dto/nonce-response.dto.ts— nonce response gainsmessageAUTH_CHALLENGE_DOMAIN,AUTH_ALLOW_LEGACY_RAW_SIGNATURESread viaConfigService; documented indocs/setup/environment-variables.md+.env.examplesep0043/envelopehappy paths, wrong-domain / wrong-network rejection, tampered message, expired envelope, rows without a stored hash, single-use replaydocs/api/endpoints.md,docs/setup/environment-variables.md,.env.example,context/progress-tracker.md,context/architecture-context.md,SECURITY.md🧪 Verification
npm run build✅npm test✅ — 353 tests passing (up from 344; none removed)no-explicit-anyerrors inaudit.interceptor.ts/transaction-status-checker.processor.tsare unrelated)npm run test:e2e) requires Supabase credentials and is not part of CI🗒️ Additional Notes
"Stellar Signed Message:\n"prefix + SHA-256), and the envelope structure follows the EIP-4361-style fields the issue describes (domain, URI, version, issued-at, expiration). The SEP-53 test vector was verified empirically againststellar-sdk(hash-then-sign) before implementation. Thesep0043string is kept in the API contract for backward compatibility.ConfigServiceinAuthService(same pattern asSTELLAR_NETWORK_PASSPHRASEelsewhere) rather thansrc/config/env.ts, which is purpose-built for the admin-wallet allowlist.