fix(backend): compare requested wallet to authenticated wallet - #1451
Merged
ogazboiz merged 6 commits intoJul 29, 2026
Conversation
…uireWalletOwnership requireWalletOwnership compared requestedWallet to itself instead of to the authenticated wallet, so the check always passed and any authenticated wallet could access another wallet's scoped resources (borrower loans/events). Fixes LabsCrypt#1363
The keyset-pagination merge (adminDisputeController, indexerController,
remittanceController, lib/pagination) had never actually been run through
lint/build/test locally:
- lib/pagination.ts: decodeCursor/parseKeysetParams passed a plain
{ code: string } object where AppError.badRequest expects an ErrorCode
enum member, failing typecheck/build.
- adminDisputeController/indexerController/remittanceController:
req.query.snapshot_seq (string | ParsedQs | ...) was passed straight into
parseKeysetParams without narrowing to string | null like the sibling
cursor/limit params already did.
- indexerController's getBorrowerEvents/getLoanEvents had an inconsistent
return (TS7030 not-all-paths-return); added the missing return.
- indexerController still imported cacheService, buildKeysetClause, and
defined buildEventsCacheKey, none of which are used anywhere — removed.
- remittanceController's getRemittances ran a redundant, unused parallel
COUNT(*) query (countResult was never read) alongside the real,
snapshot-correct total-count query a few lines below — dropped it.
- 127 Prettier formatting errors across these files — ran lint:fix.
Also rewrote adminDisputePagination.test.ts and the two indexer-events
cases in paginationFiltering.test.ts, which were still asserting the old
offset/page_info response shape (page_info, flat data, raw-string cursor)
from before the keyset-pagination migration landed on these endpoints. The
controllers now correctly implement docs/pagination-contract.md's
page.{next_cursor,snapshot_seq,total_at_snapshot,limit} + data.items shape
with opaque base64url cursors; the tests were updated to match, not the
other way around.
Merged upstream/main (77 commits) and cherry-picked the earlier
keyset-pagination CI fix; on top of that, this branch had picked up further
breakage from very recent upstream commits:
- services/defaultChecker.ts: an orphaned, syntactically invalid duplicate
of fetchOverdueLoanIds sat outside the DefaultChecker class (stray
'private' keyword at module scope). Its corrected due_ledger < $2
predicate (vs. the real method's <=, which flagged loans as overdue one
ledger early) was folded into the real class method, and the dead
duplicate was deleted.
- services/sorobanService.ts: removed an orphaned, broken
updateUserScoresBulk that referenced a nonexistent this.userRepository;
the real, used implementation already lives in services/scoresService.ts.
- middleware/pauseGuard.ts + errors/{AppError,errorCodes}.ts: pauseGuard.ts
called AppError.serviceUnavailable(), which didn't exist — added it
(503) following the existing factory-method/ErrorCode pattern. Also fixed
a generic argument on the non-generic query() call and prefixed unused
req/res middleware params.
- app.ts / routes/statusRoutes.ts: dropped an unused initializePauseState
import from app.ts (it's already correctly called at startup in
index.ts) and unused Request/Response/NextFunction type imports.
- services/authService.ts: verifyChallengeTimestamp rejected any
future-dated challenge timestamp, but its own test suite (added in a
later commit than the implementation) expects a small clock-skew
tolerance for near-future timestamps. Added a 5s tolerance constant.
- middleware/auth.ts + assorted test files: pure Prettier formatting from
the newest upstream commits, fixed via lint:fix.
- frontend PauseBanner.tsx: Prettier formatting fix.
Verified locally: backend lint/build/typecheck clean, 514/514 tests pass;
frontend lint/typecheck clean, 124/124 tests pass, build succeeds;
contracts fmt/clippy clean, 287/287 tests pass (untouched).
…p-self-compare # Conflicts: # backend/src/controllers/adminDisputeController.ts # backend/src/controllers/indexerController.ts # backend/src/controllers/remittanceController.ts # backend/src/lib/pagination.ts # backend/src/services/sorobanService.ts
…upstream sync Merged upstream/main to resolve PR LabsCrypt#1451's conflicts, applying the same resolution already used on the sibling PR LabsCrypt#1452 against the same upstream commit range: - Upstream had consolidated backend/src/lib/pagination.ts into backend/src/utils/pagination.ts, conflicting with this branch's earlier CI-fixup commits touching lib/pagination.ts and the three controllers that import it. Took upstream's consolidated version and re-applied the equivalent fixes on top: restored the DecodedCursor/KeysetPaginationParams interfaces the consolidation dropped, fixed AppError.badRequest() calls passing a plain { code } object instead of an ErrorCode, re-narrowed req.query.snapshot_seq in the three controllers, restored a missing return, and dropped dead cacheService/buildKeysetClause/ buildEventsCacheKey and a redundant unused count query. - sorobanService.ts conflicted because upstream still carries the broken updateUserScoresBulk (referencing a nonexistent this.userRepository) that this branch had already removed; kept the removal. - utils/asyncHandler.ts: reapplied the ES2020-safe workaround for setting Error.cause manually (the two-argument Error(message, { cause }) form upstream's own asyncHandler fix uses isn't available under this project's ES2020 target). - transactionController.ts: reapplied removal of the dead, unrouted getTransactions handler and duplicate MAX_LIMIT/DEFAULT_LIMIT consts, and removed its matching broken test file (import path escaped outside backend/src entirely). - __tests__/adminDisputePagination.test.ts: updated its import from the deleted lib/pagination.js to utils/pagination.js. Also found and fixed the same class of *new* upstream breakage as before — a corrected implementation pasted as a free-standing, non-compiling duplicate alongside the real, already-fixed method, plus matching orphaned tests referencing types/helpers that don't exist: - contracts/multisig_governance/src/lib.rs: removed an orphaned propose_admin_transfer(env, proposer, new_admin) outside any impl block; the real, differently-signed method already implements the reproposal-cooldown check correctly. Removed its two matching orphaned tests referencing a nonexistent MultisigGovernance type (the real type is GovernanceContract). - contracts/lending_pool/src/lib.rs: removed an orphaned deposit(env, depositor, amount) outside any impl block. This was also silently shadowing the real events::deposit(...) glob import, breaking an unrelated call site inside Self::deposit that intended to emit a Deposit event. Removed its matching orphaned test calling nonexistent setup_lending_pool/mint_tokens helpers and client.share_balance_of. - contracts/loan_manager/src/lib.rs: removed an orphaned refinance_loan(env, borrower, loan_id, new_principal_amount) outside any impl block; the real method already has the correct (non-inverted) collateral sufficiency check. Removed its two matching orphaned tests. - Prettier/rustfmt formatting fixes across touched files. Verified locally: backend lint/build/typecheck clean, 552/552 tests pass; frontend lint/typecheck clean, 148/148 tests pass, build succeeds; contracts fmt/clippy clean, 287/287 tests pass. The LabsCrypt#1363 fix in middleware/jwtAuth.ts (requestedWallet !== authenticatedWallet) is untouched and intact throughout.
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
2 participants
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
Fixes #1363:
requireWalletOwnershipinbackend/src/middleware/jwtAuth.tscompared the requested wallet to itself (requestedWallet !== requestedWallet), which is alwaysfalse, so the ownership check never rejected a request. Any authenticated wallet could access another wallet's scoped resources, includingGET /indexer/events/borrower/:borrowerandGET /loans/borrower/:borrower.requestedWallet !== authenticatedWallet, matching the pattern already used correctly byrequireWalletParamMatchesJwt.requireWalletOwnershipinbackend/src/middleware/__tests__/jwtAuth.test.ts(previously untested), covering theparams.borrower/params.wallet/body.walletsources, the 403 mismatch case, 401 unauthenticated, and 400 missing wallet.Pull Request Checklist
Please ensure your PR follows these steps, mirroring our
CONTRIBUTING.mdguidelines.CONTRIBUTING.mddocument.Test plan
npm test -- src/middleware/__tests__/jwtAuth.test.tsinbackend/— all 6 newrequireWalletOwnershiptests pass.requireScopestest failures predate this change and are untouched by it.