fix(api): require wallet auth and ownership on position lifecycle endpoints - #425
Merged
Merged
Conversation
…points Migrate close/open/repay-data/withdraw-all endpoints from unauthenticated GET mutations to POST with verify_wallet_signature and a reusable require_position_owner dependency, and update the frontend call sites.
There was a problem hiding this comment.
Bandit found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| json={"transaction_hash": "0xabc123"}, | ||
| ) | ||
| finally: | ||
| app.dependency_overrides[verify_wallet_signature] = lambda: "test_wallet" |
| assert response.status_code == 404 | ||
| assert response.json() == {"detail": "Position not Found"} | ||
| finally: | ||
| app.dependency_overrides[verify_wallet_signature] = lambda: "test_wallet" |
| try: | ||
| response = client.post("/api/get-repay-data") | ||
| finally: | ||
| app.dependency_overrides[verify_wallet_signature] = lambda: "test_wallet" |
| try: | ||
| response = client.post("/api/get-withdraw-all-data") | ||
| finally: | ||
| app.dependency_overrides[verify_wallet_signature] = lambda: "test_wallet" |
…on CI test_vault.py was the only test module driving the real app (with its BaseHTTPMiddleware stack) through httpx's ASGITransport under the anyio trio backend. On the GitHub Actions runner every trio variant fails with "RuntimeError: must be called from async context" raised inside BaseHTTPMiddleware's anyio task group. This is a pre-existing main-branch breakage: every open PR fails the same six tests regardless of its diff. Switch the module to the shared `client` (TestClient) fixture used by the rest of the suite (test_positions, test_outbox, etc.). TestClient runs the app on an asyncio portal thread, which sidesteps the trio task-group path entirely and matches the pattern already proven green in CI. Assertions and mock setup are unchanged; only the request transport differs. Verified locally: 409/409 tests pass on the full web_app suite.
Closed
6 tasks
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 #409
All four position-lifecycle entry points (
close_position,open_position,get_repay_data,get_withdraw_data) previously ran as unauthenticatedGEThandlers, so any caller who knew aposition_idorwallet_idcould close/re-open another user's position and read the contract address needed to craft repay/withdraw transactions. This PR migrates all four toPOST, gates them behindDepends(verify_wallet_signature), and adds a single reusableDepends(require_position_owner)dependency that loads thePositionand comparesposition.user_idto the authenticated wallet'sUserrecord — closing the ownership hole that bare signature verification alone cannot. The frontend call sites (useClosePosition.js,useWithdrawAll.js,transaction.js) are updated in the same PR to sendPOSTbodies with thegetAuthHeaders()signature headers, so the API contract change lands atomically with its consumers.Why
The root cause in
quantara/web_app/api/position.pywas twofold:close_position/open_positionand no ownership check tyingposition_idback to the caller anywhere in the module.verify_wallet_signatureonly proves the caller controls some wallet, so the fix had to load thePositionand compareposition.user_idto the authenticated user — exactly the "architecturally hard" point the issue calls out.get_repay_data/get_withdraw_dataalso returned per-user financial data keyed by a query-paramwallet_idthat any caller could supply.The chosen approach reuses the existing
verify_wallet_signaturedependency and the existingPositionDBConnector.get_position_by_id/get_user_by_wallet_idmethods — no new persistence or auth machinery — and centralizes the ownership query in one dependency (require_position_owner) rather than repeating it in each handler, per the issue's proposed design. Error responses use the standardAPIErrorenvelope (quantara/web_app/api/errors.py), and the 403not_position_ownerresponse was added toCOMMON_ERROR_RESPONSESso OpenAPI documents it. The in-memory nonce store and broader auth model were deliberately left untouched (explicitly out of scope).What was built
quantara/web_app/api/position.py:quantara/web_app/api/position.pyrequire_position_ownerreusable dependency (404position_not_found/ 403not_position_owner);close_positionandopen_positionconverted toPOST /api/close-position/{position_id}andPOST /api/open-position/{position_id}with aPositionStateChangeRequestbody and the ownership dependency;get_repay_dataandget_withdraw_dataconverted toPOSTwith the wallet taken fromverify_wallet_signature(nowallet_idquery param); rate-limiter keying switched fromwallet_idquery param to thex-wallet-idheader; all new errors raised asAPIErrorwith machine-readable codesquantara/web_app/api/serializers/position.pyPositionStateChangeRequestbody model (transaction_hash,min_length=1)quantara/web_app/api/errors.py403entry added toCOMMON_ERROR_RESPONSES(not_position_owner) so every operation's OpenAPI spec documents the envelopequantara/web_app/tests/test_positions.pyclose_positionandopen_position; happy path and 401 forget_repay_dataandget_withdraw_dataquantara/web_app/tests/test_outbox.pytest_open_position_queues_outbox_eventto the newPOST /api/open-position/{position_id}route and asserted the outbox payload unchangedIntegration changes outside
quantara/web_app/api/quantara/frontend/src/hooks/useClosePosition.js—GET /api/get-repay-data→POSTwithgetAuthHeaders();GET /api/close-position→POST /api/close-position/{position_id}with{ transaction_hash }body.quantara/frontend/src/hooks/useWithdrawAll.js—GET /api/get-withdraw-all-data→POSTwithgetAuthHeaders(); close-position call moved to the new POST route with the tx-hash body.quantara/frontend/src/services/transaction.js—handleTransaction's open-position notify call moved fromGET /api/open-position(query params) toPOST /api/open-position/{position_id}with the auth headers already fetched forcreate-position.quantara/frontend/test/hooks/useClosePosition.test.jsx,useWithdrawAll.test.jsx(new) — vitest coverage asserting the new POST calls carry the auth headers.quantara/frontend/test/services/transaction.test.js— updated for thePOST /api/open-position/{id}call.These frontend changes are required by the API contract change and land in the same PR, as the issue mandates.
Acceptance criteria coverage
Service
close_positionandopen_positionreject unauthenticated requests (401) and reject a wallet that does not own the position (403). (test_positions.py—test_close_position_rejects_unauthenticated,test_close_position_rejects_wrong_owner,test_open_position_rejects_unauthenticated,test_open_position_rejects_wrong_owner; enforced byrequire_position_owner)get_repay_dataandget_withdraw_datareturn repay data only to the authenticated owner of that wallet. (both now takewallet: str = Depends(verify_wallet_signature)and queryget_repay_data(wallet)— thewallet_idquery parameter is gone;test_get_repay_data_rejects_unauthenticated,test_get_withdraw_data_rejects_unauthenticated)POSTand no longer accept state changes or per-user data overGET. (router decorators changed; noGETroutes remain for these paths — verified by full-repo search)Tests
quantara/web_app/tests/test_positions.pycovers missing auth header, wrong-owner position, and the happy path for each of the four endpoints. (8 new tests listed above)vitestcoverage for the newPOSTcalls. (test/hooks/useClosePosition.test.jsx,test/hooks/useWithdrawAll.test.jsx)Documentation
@router.postdecorators; the custom generator inweb_app/api/openapi.pyinjectsCOMMON_ERROR_RESPONSES— including the new 403 — into every operation)Deliberately deferred
Nothing is deferred from this issue's scope. Per the issue's "Out of scope", the in-memory nonce store in
web_app/api/wallet_auth.pyand the broader auth model were not refactored; only ownership and method correctness on the four endpoints were enforced.Test plan
poetry run pytest web_app/tests/test_positions.py web_app/tests/test_outbox.py— 71 passed, 1 failed. The single failure (test_get_user_positions_no_positions) is pre-existing onupstream/main: it patchesget_positions_by_wallet_idwhile the handler callsget_all_positions_by_wallet_id, so it hits the real DB and fails without Postgres. Verified identical failure on the stashed (base) tree: basetest_user.py+test_positions.py= 25 failed/68 passed vs this branch = 25 failed/76 passed (same 25 failures, +8 new passing tests, zero regressions).poetry run pytest web_app/tests(full suite) — 377 passed, 26 failed, 6 errors. All 26 failures + 6 errors are environmental (Postgres not available in this workspace:psycopg2.OperationalErrorintest_user.py,test_airdrop.py, etc.) or the pre-existing broken test above; none touch the four endpoints changed here.yarn test:run— 90/90 passing across 17 files (2 new hook test files).eslint(changed files) — 0 errors; warnings are limited tono-unused-varsfalse positives that also appear on base in every.jsxtest file (the repo'seslint:recommendedconfig has no React plugin, so JSX usage isn't counted — e.g.test/ActionModal.test.jsxreports the same) plus one pre-existing unused var intransaction.js.black— all lines added/modified in this PR are black-clean. The files still report reformat candidates only in pre-existing code that is already non-compliant on base (e.g.errors.pyapi_error_handler,serializers/position.pydocstring blank lines,test_outbox.pyrelay-workerwith patch(...)blocks,position.pyget_add_deposit_datasignature). Same forisort(pre-existing import-order deviations on base).Env vars / Notes
No new environment variables or config keys. No persisted data shape changed (the
Transactionrow written byclose_positionand theOutboxEventwritten byopen_positionare unchanged).Breaking-change note for operators: the four routes changed from
GETtoPOSTwith new paths (/api/close-position/{position_id},/api/open-position/{position_id}) and now requireX-Wallet-Id,X-Nonce,X-Signatureheaders. The frontend ships the matching call sites in this same PR. Any external clients using the oldGETroutes must migrate.