Conversation
Signed-off-by: ken-morel <engonken8@gmail.com> Change-Id: I5f27a4f2720da75dc9234a258d74f6c96a6a6964
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
DCT-Berinyuy
left a comment
DCT-Berinyuy
left a comment
There was a problem hiding this comment.
Requesting changes: security-critical, exploitable
To be clear up front: this is not style feedback. Several of the issues below are exploitable today. Together they allow account takeover and fraudulent escrow release. This PR should not merge, and this backend should not handle real money until they're fixed.
The code compiles (cargo check passes, 6 warnings). The problems are in the logic, not the build.
How the issues chain into an attack
- Take over an account.
SignInWithGooglenever verifies theid_token, so any non-empty string signs you in as the first user in the database. Separately, a user created with an empty password can then be signed into with any password, by anyone who knows the phone number. - Create a fake sale. List a book from one account and "buy" it from another.
CreateBookPurchasemarks the escrowHeldas soon as the USSD prompt is sent. Nothing ever checks that the buyer paid:get_payment_statusis dead code (the compiler says so) and there is no webhook. - Release it.
ConfirmReceiptlets any authenticated user release any transaction. It checks neither that the caller is the buyer nor what state the transaction is in.
Result: Released escrow for money that was never collected. There's no payout code yet, but the moment payouts are wired to Released, this turns directly into the platform paying out money it never received. The current bookbridge-rust-core service is imperfect, but it never had these properties. That's the bar a replacement has to clear.
Must fix before merge
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | 🔴 CRITICAL | auth.rs:203 |
Google sign-in ignores the token and returns the first user in the DB |
| 2 | 🔴 CRITICAL | auth.rs:104, :173 |
Empty password accepted at sign-up, and then any password is accepted at sign-in |
| 3 | 🔴 CRITICAL | purchases.rs:327 |
ConfirmReceipt has no ownership or state check |
| 4 | 🔴 CRITICAL | purchases.rs:115 |
Escrow is marked Held before payment is confirmed, and payment is never verified |
| 5 | 🟠 HIGH | config.rs:45 |
Hard-coded fallback JWT_SECRET, so tokens can be forged if the env var is missing |
| 6 | 🟠 HIGH | purchases.rs:386, :156 |
DisputePurchase has no ownership check (anyone can freeze funds). GetPurchase is an IDOR |
| 7 | 🟠 HIGH | auth.rs:107 |
Passwords stored as unsalted SHA-256. Use argon2id |
| 8 | 🟡 MEDIUM | purchases.rs:79 |
The four inserts aren't in one DB transaction. No guard against double purchase or buying your own book |
Also needed
- An explicit escrow state machine with tests:
Pending → Heldonly after Fapshi confirms payment (webhook with signature check, plus polling as a fallback);Held → Releasedonly by the buyer;Held → Disputed/Refunded; every other transition rejected. The currentbookbridge-rust-corewebhook/escrow logic is the reference behaviour to match. rust.ymlstill usesworking-directory: bookbridge-rust-core, which this stack deletes. That's why CI fails.
Inline comments are on each location. Happy to pair on any of these.
| })?; | ||
|
|
||
| // For local development or token verification, find or create user placeholder | ||
| let user: User = match users::table.filter(users::deleted_at.is_null()).first(&mut conn) { |
There was a problem hiding this comment.
🔴 CRITICAL: account takeover. id_token is never verified, and this returns the first non-deleted user in the table, whoever that is. Any non-empty string logs in as that user.
Verify the token against Google (JWKS / tokeninfo): check aud equals our OAuth client ID, iss, exp, and email_verified. Then look up or create the user by the token's sub, never by "first row". Until then, this endpoint should return unimplemented, not a session.
| return Err(ConnectError::invalid_argument("name fields are required")); | ||
| } | ||
|
|
||
| let password_hash = if req.password.is_empty() { |
There was a problem hiding this comment.
🔴 CRITICAL. An empty password creates a user with password_hash = None. Combined with line 173, that account can then be signed into with any password by anyone who knows the phone number. Require a password here, or require OTP verification for passwordless accounts.
| } | ||
| }; | ||
|
|
||
| if let Some(ref expected_hash) = user.password_hash { |
There was a problem hiding this comment.
🔴 CRITICAL. If password_hash is None, this block is skipped entirely and a session is issued for any password. It should fail closed: no hash means password sign-in is refused. Also, comparing with != isn't constant-time. Argon2's verify handles that for you.
| let password_hash = if req.password.is_empty() { | ||
| None | ||
| } else { | ||
| Some(String::from_utf8_lossy(&hash_sha256(req.password.as_bytes())).to_string()) |
There was a problem hiding this comment.
🟠 HIGH. Unsalted SHA-256 is fast to brute-force and makes identical passwords share a hash. Use argon2id (the argon2 crate) with a per-user salt. from_utf8_lossy on raw digest bytes is also lossy: different digests can map to the same string.
| let new_tx = NewTransaction { | ||
| payment_id: intent_id, | ||
| recipient: book.seller_id, | ||
| transaction_status: TransactionStatus::Held, |
There was a problem hiding this comment.
🔴 CRITICAL. The escrow is marked Held right after direct_pay initiates the USSD prompt, before the buyer has approved anything. The response's status is never read (compiler warning), and get_payment_status is never called (dead code). Nothing ever moves a payment to Completed.
The transaction should start as Pending and only become Held after Fapshi confirms payment, through a signature-verified webhook with polling as a fallback, as the current bookbridge-rust-core does.
| })) | ||
| } | ||
|
|
||
| async fn confirm_receipt( |
There was a problem hiding this comment.
🔴 CRITICAL: exploitable. _user_id is read but never checked. Any authenticated user can release any transaction, and there's no state check, so unpaid, disputed or already-refunded transactions can be released too.
Required: caller == payment_intents.user_id (the buyer), and the transition is only allowed from Held. Do it as one atomic UPDATE … WHERE id = $1 AND status = 'held' AND buyer = $2, then check the affected row count, so two concurrent calls can't both succeed.
| })) | ||
| } | ||
|
|
||
| async fn dispute_purchase( |
There was a problem hiding this comment.
🟠 HIGH. Same as confirm_receipt: there's no ownership check, so any user can mark any transaction Disputed and freeze a stranger's funds. Restrict it to the buyer or seller of this purchase, and only allow it from Held.
| })) | ||
| } | ||
|
|
||
| async fn get_purchase( |
There was a problem hiding this comment.
🟠 HIGH: IDOR. _user_id is unused, so any authenticated user can fetch any purchase by ID. Restrict it to the buyer or the seller.
| status: PaymentStatus::Pending, | ||
| }; | ||
|
|
||
| diesel::insert_into(payment_intents::table) |
There was a problem hiding this comment.
🟡 MEDIUM. The payment_intent, payment, transaction and purchase inserts run as separate statements with a network call to Fapshi in the middle. A failure after the charge leaves half-written records. Wrap the DB writes in conn.transaction(...), and don't hold a pooled connection across the HTTP call.
Also missing: rejecting purchases of your own listing, and rejecting books that are already sold or have a purchase in flight (use a unique constraint or row lock to prevent double-buy races).
| let database_url = env::var("DATABASE_URL") | ||
| .unwrap_or_else(|_| "postgresql://postgres:postgres@localhost:5432/bookbridge".to_string()); | ||
| let jwt_secret = env::var("JWT_SECRET") | ||
| .unwrap_or_else(|_| "development_jwt_secret_must_change_in_production_32bytes".to_string()); |
There was a problem hiding this comment.
🟠 HIGH. If JWT_SECRET is unset in production, this silently falls back to a secret that is now public in the repo, and anyone can mint tokens for any user_id. Fail at startup when it's missing, and require at least 32 bytes. The same fail-fast rule should apply to DATABASE_URL.
This branch had an error being deployed
2 participants
Created backend services. Still to be reviewed, but working