Skip to content

fix(api): require wallet auth and ownership on position lifecycle endpoints - #425

Merged
YaronZaki merged 2 commits into
Quantarq:mainfrom
1Judah:fix/issue-409-position-authz
Aug 20, 2026
Merged

fix(api): require wallet auth and ownership on position lifecycle endpoints#425
YaronZaki merged 2 commits into
Quantarq:mainfrom
1Judah:fix/issue-409-position-authz

Conversation

@1Judah

@1Judah 1Judah commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #409

All four position-lifecycle entry points (close_position, open_position, get_repay_data, get_withdraw_data) previously ran as unauthenticated GET handlers, so any caller who knew a position_id or wallet_id could close/re-open another user's position and read the contract address needed to craft repay/withdraw transactions. This PR migrates all four to POST, gates them behind Depends(verify_wallet_signature), and adds a single reusable Depends(require_position_owner) dependency that loads the Position and compares position.user_id to the authenticated wallet's User record — 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 send POST bodies with the getAuthHeaders() signature headers, so the API contract change lands atomically with its consumers.

Why

The root cause in quantara/web_app/api/position.py was twofold:

  1. No authentication at all on close_position / open_position and no ownership check tying position_id back to the caller anywhere in the module. verify_wallet_signature only proves the caller controls some wallet, so the fix had to load the Position and compare position.user_id to the authenticated user — exactly the "architecturally hard" point the issue calls out.
  2. Mutation via GET, exposing the endpoints to CSRF, prefetch, and browser caching. get_repay_data / get_withdraw_data also returned per-user financial data keyed by a query-param wallet_id that any caller could supply.

The chosen approach reuses the existing verify_wallet_signature dependency and the existing PositionDBConnector.get_position_by_id / get_user_by_wallet_id methods — 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 standard APIError envelope (quantara/web_app/api/errors.py), and the 403 not_position_owner response was added to COMMON_ERROR_RESPONSES so 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:

File What it contains
quantara/web_app/api/position.py require_position_owner reusable dependency (404 position_not_found / 403 not_position_owner); close_position and open_position converted to POST /api/close-position/{position_id} and POST /api/open-position/{position_id} with a PositionStateChangeRequest body and the ownership dependency; get_repay_data and get_withdraw_data converted to POST with the wallet taken from verify_wallet_signature (no wallet_id query param); rate-limiter keying switched from wallet_id query param to the x-wallet-id header; all new errors raised as APIError with machine-readable codes
quantara/web_app/api/serializers/position.py PositionStateChangeRequest body model (transaction_hash, min_length=1)
quantara/web_app/api/errors.py 403 entry added to COMMON_ERROR_RESPONSES (not_position_owner) so every operation's OpenAPI spec documents the envelope
quantara/web_app/tests/test_positions.py New suites: happy path, 401 missing-auth, and 403 wrong-owner for close_position and open_position; happy path and 401 for get_repay_data and get_withdraw_data
quantara/web_app/tests/test_outbox.py Updated test_open_position_queues_outbox_event to the new POST /api/open-position/{position_id} route and asserted the outbox payload unchanged

Integration changes outside quantara/web_app/api/

  • quantara/frontend/src/hooks/useClosePosition.jsGET /api/get-repay-dataPOST with getAuthHeaders(); GET /api/close-positionPOST /api/close-position/{position_id} with { transaction_hash } body.
  • quantara/frontend/src/hooks/useWithdrawAll.jsGET /api/get-withdraw-all-dataPOST with getAuthHeaders(); close-position call moved to the new POST route with the tx-hash body.
  • quantara/frontend/src/services/transaction.jshandleTransaction's open-position notify call moved from GET /api/open-position (query params) to POST /api/open-position/{position_id} with the auth headers already fetched for create-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 the POST /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_position and open_position reject unauthenticated requests (401) and reject a wallet that does not own the position (403). (test_positions.pytest_close_position_rejects_unauthenticated, test_close_position_rejects_wrong_owner, test_open_position_rejects_unauthenticated, test_open_position_rejects_wrong_owner; enforced by require_position_owner)
  • get_repay_data and get_withdraw_data return repay data only to the authenticated owner of that wallet. (both now take wallet: str = Depends(verify_wallet_signature) and query get_repay_data(wallet) — the wallet_id query parameter is gone; test_get_repay_data_rejects_unauthenticated, test_get_withdraw_data_rejects_unauthenticated)
  • The four endpoints use POST and no longer accept state changes or per-user data over GET. (router decorators changed; no GET routes remain for these paths — verified by full-repo search)

Tests

  • quantara/web_app/tests/test_positions.py covers missing auth header, wrong-owner position, and the happy path for each of the four endpoints. (8 new tests listed above)
  • Frontend hooks updated, with vitest coverage for the new POST calls. (test/hooks/useClosePosition.test.jsx, test/hooks/useWithdrawAll.test.jsx)

Documentation

  • OpenAPI docs reflect the new methods and 401/403 responses. (methods come from the @router.post decorators; the custom generator in web_app/api/openapi.py injects COMMON_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.py and 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.py71 passed, 1 failed. The single failure (test_get_user_positions_no_positions) is pre-existing on upstream/main: it patches get_positions_by_wallet_id while the handler calls get_all_positions_by_wallet_id, so it hits the real DB and fails without Postgres. Verified identical failure on the stashed (base) tree: base test_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.OperationalError in test_user.py, test_airdrop.py, etc.) or the pre-existing broken test above; none touch the four endpoints changed here.
  • yarn test:run90/90 passing across 17 files (2 new hook test files).
  • eslint (changed files) — 0 errors; warnings are limited to no-unused-vars false positives that also appear on base in every .jsx test file (the repo's eslint:recommended config has no React plugin, so JSX usage isn't counted — e.g. test/ActionModal.test.jsx reports the same) plus one pre-existing unused var in transaction.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.py api_error_handler, serializers/position.py docstring blank lines, test_outbox.py relay-worker with patch(...) blocks, position.py get_add_deposit_data signature). Same for isort (pre-existing import-order deviations on base).
  • Manual: run the app with a wallet and Freighter to exercise close/withdraw-all end-to-end (requires a live Stellar testnet contract, not possible in this workspace).

Env vars / Notes

No new environment variables or config keys. No persisted data shape changed (the Transaction row written by close_position and the OutboxEvent written by open_position are unchanged).

Breaking-change note for operators: the four routes changed from GET to POST with new paths (/api/close-position/{position_id}, /api/open-position/{position_id}) and now require X-Wallet-Id, X-Nonce, X-Signature headers. The frontend ships the matching call sites in this same PR. Any external clients using the old GET routes must migrate.

…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.

@github-advanced-security github-advanced-security AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

@YaronZaki YaronZaki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1Judah please fix the CI

…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.

@YaronZaki YaronZaki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@YaronZaki
YaronZaki merged commit 2192f40 into Quantarq:main Aug 20, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Position lifecycle endpoints accept unauthenticated GET mutations: arbitrary position close

3 participants