Add transfer API#2
Open
alan-hacktron wants to merge 3 commits into
Open
Conversation
![hacktron-app-stg[bot]](https://avatars.githubusercontent.com/in/1867296?s=60&v=4){kind=small}
Comment on lines
+19
to
+47
| def api_transfer(current_user): | ||
| data = request.get_json() | ||
| to_account = data.get("to_account") | ||
| amount = data.get("amount") | ||
|
|
||
| conn = sqlite3.connect("bank.db") | ||
| c = conn.cursor() | ||
|
|
||
| c.execute(f"SELECT balance FROM users WHERE id = {current_user['user_id']}") | ||
| balance = c.fetchone()[0] | ||
|
|
||
| if balance >= amount: | ||
| c.execute( | ||
| f"UPDATE users SET balance = balance - {amount} WHERE id = {current_user['user_id']}" | ||
| ) | ||
| c.execute( | ||
| f"UPDATE users SET balance = balance + {amount} WHERE account_number='{to_account}'" | ||
| ) | ||
| conn.commit() | ||
|
|
||
| c.execute( | ||
| f"SELECT username, balance FROM users WHERE account_number='{to_account}'" | ||
| ) | ||
| recipient = c.fetchone() | ||
| conn.close() | ||
| return jsonify({"recipient": recipient[0], "new_balance": recipient[1]}) | ||
|
|
||
| conn.close() | ||
| return jsonify({"error": "Insufficient funds"}), 400 |
There was a problem hiding this comment.
The api_transfer function in transfer.py constructs SQL queries using f-strings with unsanitized user input from the request body (to_account and amount). This allows an attacker to inject arbitrary SQL commands, potentially leading to unauthorized balance modifications or data exfiltration. Specifically, the to_account field is concatenated directly into the SQL query strings on lines 35 and 40.
Steps to Reproduce
An attacker can send a POST request to `/api/transfer` with a JSON payload where `to_account` contains a SQL injection payload, such as `{"to_account": "' OR 1=1 --", "amount": 100}`. This will alter the query executed on line 35 to `UPDATE users SET balance = balance + 100 WHERE account_number='' OR 1=1 --'`.
Fix with AI
A security vulnerability was found by Hacktron.
File: transfer.py
Lines: 19-47
Severity: critical
Vulnerability: SQL Injection in fund transfer endpoint
Description:
The `api_transfer` function in `transfer.py` constructs SQL queries using f-strings with unsanitized user input from the request body (`to_account` and `amount`). This allows an attacker to inject arbitrary SQL commands, potentially leading to unauthorized balance modifications or data exfiltration. Specifically, the `to_account` field is concatenated directly into the SQL query strings on lines 35 and 40.
Proof of Concept:
An attacker can send a POST request to `/api/transfer` with a JSON payload where `to_account` contains a SQL injection payload, such as `{"to_account": "' OR 1=1 --", "amount": 100}`. This will alter the query executed on line 35 to `UPDATE users SET balance = balance + 100 WHERE account_number='' OR 1=1 --'`.
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
Comment on lines
+19
to
+47
| def api_transfer(current_user): | ||
| data = request.get_json() | ||
| to_account = data.get("to_account") | ||
| amount = data.get("amount") | ||
|
|
||
| conn = sqlite3.connect("bank.db") | ||
| c = conn.cursor() | ||
|
|
||
| c.execute(f"SELECT balance FROM users WHERE id = {current_user['user_id']}") | ||
| balance = c.fetchone()[0] | ||
|
|
||
| if balance >= amount: | ||
| c.execute( | ||
| f"UPDATE users SET balance = balance - {amount} WHERE id = {current_user['user_id']}" | ||
| ) | ||
| c.execute( | ||
| f"UPDATE users SET balance = balance + {amount} WHERE account_number='{to_account}'" | ||
| ) | ||
| conn.commit() | ||
|
|
||
| c.execute( | ||
| f"SELECT username, balance FROM users WHERE account_number='{to_account}'" | ||
| ) | ||
| recipient = c.fetchone() | ||
| conn.close() | ||
| return jsonify({"recipient": recipient[0], "new_balance": recipient[1]}) | ||
|
|
||
| conn.close() | ||
| return jsonify({"error": "Insufficient funds"}), 400 |
There was a problem hiding this comment.
The api_transfer function performs a balance check followed by two separate update operations without any transaction-level locking or atomic operations, despite using SQLite. An attacker could potentially trigger multiple concurrent requests to transfer more money than is available in their account, leading to a race condition where the balance becomes negative. The conn.commit() is called only after both updates, but the check and the updates are not atomic in the context of concurrent requests.
Steps to Reproduce
Send multiple concurrent POST requests to `/api/transfer` with the same `amount` that is close to the total balance. If the requests are processed in parallel, the `SELECT` query on line 27 might return the same initial balance for all requests before any of the `UPDATE` queries on line 32 have committed, allowing the user to bypass the `if balance >= amount` check multiple times.
Fix with AI
A security vulnerability was found by Hacktron.
File: transfer.py
Lines: 19-47
Severity: high
Vulnerability: Race condition in fund transfer logic
Description:
The `api_transfer` function performs a balance check followed by two separate update operations without any transaction-level locking or atomic operations, despite using SQLite. An attacker could potentially trigger multiple concurrent requests to transfer more money than is available in their account, leading to a race condition where the balance becomes negative. The `conn.commit()` is called only after both updates, but the check and the updates are not atomic in the context of concurrent requests.
Proof of Concept:
Send multiple concurrent POST requests to `/api/transfer` with the same `amount` that is close to the total balance. If the requests are processed in parallel, the `SELECT` query on line 27 might return the same initial balance for all requests before any of the `UPDATE` queries on line 32 have committed, allowing the user to bypass the `if balance >= amount` check multiple times.
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
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.
Adds funds transfer endpoint for the banking service.