Skip to content

[security] fix(payment): enforce ownership on Stripe account refresh#6840

Closed
Hinotoi-agent wants to merge 2 commits intoBasedHardware:mainfrom
Hinotoi-agent:fix/stripe-connect-refresh-authz
Closed

[security] fix(payment): enforce ownership on Stripe account refresh#6840
Hinotoi-agent wants to merge 2 commits intoBasedHardware:mainfrom
Hinotoi-agent:fix/stripe-connect-refresh-authz

Conversation

@Hinotoi-agent
Copy link
Copy Markdown

Summary

This PR fixes an authenticated authorization flaw in the Stripe Connect account-link refresh flow.

  • /v1/stripe/refresh/{account_id} previously accepted any path-supplied Stripe Connect account id after user authentication
  • the endpoint did not verify that the supplied account_id belonged to the authenticated user
  • this patch now derives the expected Stripe account from the caller's own user record and rejects missing or mismatched ownership before creating a fresh Stripe account link
  • adds targeted regression coverage for the allow, deny, and missing-account cases

Security issue covered

issue impact severity
Authenticated IDOR on Stripe Connect account-link refresh An authenticated user who knows another creator's Stripe Connect account id could mint a fresh onboarding/account link for that victim account High

Before this PR

  • POST /v1/stripe/refresh/{account_id} trusted the caller-supplied account_id
  • after authentication, the handler directly called refresh_connect_account_link(account_id)
  • no ownership check tied the requested Stripe account to the authenticated uid

After this PR

  • the handler loads the caller's stored Stripe Connect account id with get_stripe_connect_account_id(uid)
  • requests with no stored Stripe account now fail closed with 404
  • requests for a different account id now fail closed with 403
  • the Stripe refresh call now uses the server-derived account id instead of the path parameter
  • regression tests lock in the ownership boundary

Why this matters

The vulnerable route was a broken object-level authorization path in a payment control plane.

A normal authenticated user should only ever be able to refresh the onboarding link for the Stripe Connect account attached to their own user record. Before this patch, the route accepted an arbitrary path account id and created a fresh Stripe account link for it.

Attack flow

authenticated attacker request
    -> POST /v1/stripe/refresh/{victim_account_id}
        -> backend trusted path-supplied Stripe account id
            -> fresh Stripe onboarding/account link for victim account

Affected code

file role
backend/routers/payment.py vulnerable refresh route and fix
backend/database/users.py canonical per-user Stripe account lookup used for authorization
backend/tests/unit/test_stripe_connect_account_refresh_authz.py regression coverage for ownership enforcement

Root cause

  • direct cause: the route authenticated the caller but never enforced ownership of the requested Stripe account id
  • trust-boundary failure: a path parameter was treated as authority for a payment control-plane action instead of deriving the target object from server-side user state

CVSS assessment

issue CVSS v3.1 vector
Stripe Connect account-link refresh IDOR 6.5 Medium CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:L/I:L/A:N

Rationale:

  • requires a normal authenticated user
  • no user interaction is required after the attacker submits the request
  • impact is primarily unauthorized access to another user's Stripe onboarding/control-plane flow rather than full platform compromise

Safe reproduction steps

  1. Create or use two users:
    • attacker user A
    • victim user B with a stored Stripe Connect account id
  2. Authenticate as user A
  3. Send a request to:
    • POST /v1/stripe/refresh/{victim_account_id}
  4. Observe pre-fix behavior:
    • the backend returns a fresh link for the victim account
  5. Observe fixed behavior:
    • the backend compares {victim_account_id} to get_stripe_connect_account_id(attacker_uid)
    • the request is rejected with 403

Expected vulnerable behavior

Before this PR, the route would accept a caller-supplied Stripe Connect account id and return:

  • account_id: <victim account id>
  • url: <fresh Stripe account link>

without verifying that the account belonged to the authenticated caller.

Changes in this PR

  • add server-side ownership lookup in refresh_account_link_endpoint
  • reject callers without a stored Stripe Connect account
  • reject mismatched account ids with 403
  • pass only the server-derived account id into refresh_connect_account_link
  • add regression tests for:
    • mismatched account id rejection
    • successful refresh for the caller's own account id
    • missing stored account rejection

Files changed

category files change
fix backend/routers/payment.py enforce ownership before refreshing Stripe account link
tests backend/tests/unit/test_stripe_connect_account_refresh_authz.py add regression coverage for the authz boundary

Maintainer impact

  • narrow patch surface limited to the Stripe Connect refresh route
  • no change to the existing happy path for refreshing the authenticated user's own account link
  • no schema or migration changes
  • no change to webhook or subscription logic

Fix rationale

The backend already has a canonical ownership source for Stripe Connect accounts: get_stripe_connect_account_id(uid).

The safest pattern is to treat that server-side value as authoritative and fail closed on any mismatch, rather than trusting a client-controlled path parameter for a payment control-plane action.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Security fix
  • Tests added/updated
  • New feature
  • Breaking change
  • Documentation-only change

Test plan

  • Added targeted regression tests for ownership enforcement
  • Ran targeted test file successfully
  • Checked diff formatting with git diff --check

Executed:

cd backend
/Users/lennon/.hermes/hermes-agent/venv/bin/python3.11 -m pytest tests/unit/test_stripe_connect_account_refresh_authz.py -q

Observed result:

3 passed in 0.10s

Disclosure notes

  • this PR is intentionally scoped to the verified authorization flaw in the Stripe Connect refresh route
  • no unrelated backend routes were changed
  • the issue was reproduced as a direct current-head code-path proof before preparing this patch

@Hinotoi-agent Hinotoi-agent changed the title fix(payment): enforce ownership on Stripe account refresh [security] fix(payment): enforce ownership on Stripe account refresh Apr 19, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 19, 2026

Greptile Summary

This PR fixes a broken object-level authorization (IDOR) on POST /v1/stripe/refresh/{account_id} by deriving the Stripe Connect account ID from the authenticated user's own record rather than trusting the caller-supplied path parameter. The fix is minimal and correct — failing closed on missing or mismatched accounts, and passing only the server-derived ID to Stripe.

Confidence Score: 5/5

Safe to merge — the security fix is correct and the only remaining findings are P2 style suggestions in the test file.

The core change is a narrow, correct IDOR fix with no schema, migration, or webhook changes. All open findings are P2 (test cleanup pattern and hardcoded test secret), neither of which affects runtime correctness or the security boundary being enforced.

backend/tests/unit/test_stripe_connect_account_refresh_authz.py — minor test hygiene issues (no-cleanup patching, hardcoded secret value)

Important Files Changed

Filename Overview
backend/routers/payment.py Adds server-side ownership check to the Stripe Connect refresh endpoint: derives the caller's account ID via get_stripe_connect_account_id(uid), returns 404 if absent and 403 on mismatch, and uses the server-derived ID for the Stripe call — correctly closing the IDOR.
backend/tests/unit/test_stripe_connect_account_refresh_authz.py New regression test file covering the three ownership cases (mismatch → 403, match → 200, missing account → 404); works correctly in isolation but uses direct module-attribute patching without cleanup, which can leak state to other tests in the same session.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Endpoint as POST /v1/stripe/refresh/{account_id}
    participant DB as get_stripe_connect_account_id(uid)
    participant Stripe as refresh_connect_account_link

    Note over Client,Stripe: After fix (this PR)

    Client->>Endpoint: uid (from auth token) + account_id (path param)
    Endpoint->>DB: lookup stored account for uid
    DB-->>Endpoint: expected_account_id

    alt No stored account
        Endpoint-->>Client: 404 Connect account not found
    else account_id != expected_account_id
        Endpoint-->>Client: 403 Not authorized
    else account_id == expected_account_id
        Endpoint->>Stripe: refresh_connect_account_link(expected_account_id)
        Stripe-->>Endpoint: account link
        Endpoint-->>Client: 200 account link
    end
Loading

Reviews (1): Last reviewed commit: "fix(payment): enforce ownership on Strip..." | Re-trigger Greptile

Comment thread backend/tests/unit/test_stripe_connect_account_refresh_authz.py Outdated
Comment thread backend/tests/unit/test_stripe_connect_account_refresh_authz.py
@Hinotoi-agent
Copy link
Copy Markdown
Author

Checked the Greptile findings against current head 30b4cb585c11b419736a5e4851ff9ca1267218f9.

No further code change appears needed:

  • backend/tests/unit/test_stripe_connect_account_refresh_authz.py now uses scoped patch.object(...) context managers instead of assigning mocks directly onto the payment_router module.
  • The hardcoded secret-looking test fallback was replaced with test_fake_encryption_secret_not_used_in_production.
  • Targeted local verification for the Stripe refresh authz unit tests passed: 3 passed in 0.15s.

The Greptile summary looks stale relative to the current branch head, and the individual review threads appear resolved. Please re-review/re-run if anything still looks open.

@beastoin beastoin closed this Apr 28, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Hey @Hinotoi-agent 👋

Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request.

After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:

  • Project standards — Ensuring consistency across the codebase
  • User needs — Making sure changes align with what our users need
  • Code best practices — Maintaining code quality and maintainability
  • Project direction — Keeping aligned with our roadmap and vision

Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out.

Thank you for being part of the Omi community! 💜

@beastoin
Copy link
Copy Markdown
Collaborator

thanks for your compute and tokens btw unfortunately bots are not welcome here yet

things might change but not now :)

@Hinotoi-agent
Copy link
Copy Markdown
Author

Thanks for the update and information.

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.

2 participants