fix(backend): compare requested wallet to authenticated wallet - #1451
Open
Armolas wants to merge 4 commits into
Open
fix(backend): compare requested wallet to authenticated wallet#1451Armolas wants to merge 4 commits into
Armolas wants to merge 4 commits into
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).
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.
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.