TEST: plant two separately-located vulns for patch-gen smoke (ENG-1972)#11
TEST: plant two separately-located vulns for patch-gen smoke (ENG-1972)#11pdtnelson wants to merge 1 commit into
Conversation
One vuln per code chunk, in two different files, for a clean 1 finding ↔ 1 patch
mapping (contrast with TEST/multi-patch-single-file):
- patients.py /lookup: f-string in conn.execute() but WITH log_audit → trips only
opengrep `fstring-execute`. Fix: parameterize.
- visits.py /{visit_id}/cancel: parameterized DB write with NO log_audit → trips
only policy "CRUD must be audited". Fix: add a log_audit() call.
Expected: 2 findings in 2 files → 2 patch-generators (different-file groups).
![console-lab[bot]](https://avatars.githubusercontent.com/in/3127777?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Amplify code check status: 
Vulnerabilities Detected
| Status | Vulnerability | Path | Details |
|---|---|---|---|
| 🔴 HIGH | Audit Logging Omission | backend/app/routers/visits.py:163 | View |
| 🚨 CRITICAL | PHI Stored Without Encryption At Rest | backend/app/routers/visits.py:153 | View |
| 🔴 HIGH | fstring-execute | backend/app/routers/patients.py:116 | View |
| 🔴 HIGH | sqlite-execute-fstring | backend/app/routers/patients.py:116 | View |
| 🟠 MEDIUM | Audit Logging Omission | backend/app/routers/visits.py:153 | View |
Last updated by commit dd88768 at 2026-06-29 20:12:15 UTC.
| conn = get_db_connection() | ||
| try: | ||
| # Parameterized — no SQL injection here; the issue is the missing audit entry. | ||
| conn.execute("UPDATE visits SET status = 'cancelled', updated_at = ? WHERE id = ?", (now, visit_id)) |
There was a problem hiding this comment.
Gist: A visit cancellation — a clinically significant PHI mutation — is silently unlogged. Any unauthorized or erroneous cancellation would be undetectable in the audit trail, which is a compliance gap under HIPAA audit-control requirements. This should be fixed before merge.
Detection Agent: Audit Logging Omission
Severity: High
Description
The cancel_visit handler mutates the PHI-bearing visits table (setting status = 'cancelled') without emitting any audit log entry. No log_audit call is present in this code path, so there is no record of who cancelled the visit, which visit was affected, or when the action occurred — a direct violation of the audit-logging requirement for PHI CRUD operations.
Remediation
Add a log_audit(user_id=current_user["id"], user_role=current_user["role"], action="CANCEL", resource_type="visit", resource_id=visit_id, ip_address=request.client.host if request.client else None, success=True) call after the conn.commit() — mirroring the pattern used in delete_visit and other handlers in this file. Ensure the audit write occurs within the same try block so failures are visible.
Detection: Policy / Audit Logging Required for Database CRUD Operations
| conn.close() | ||
|
|
||
|
|
||
| @router.post("/{visit_id}/cancel", response_model=VisitResponse) |
There was a problem hiding this comment.
Gist: The chief_complaint field is stored as unencrypted plain text in the database while all other clinical PHI fields in the same table are encrypted. The new cancel_visit endpoint exposes this gap by returning the full visit record. This is a HIPAA violation requiring immediate remediation.
Detection Agent: PHI Stored Without Encryption At Rest
Severity: Critical
Description
The cancel_visit endpoint (lines 153-172) returns the full visit response via _row_to_response, which includes the chief_complaint field. This field is stored as plain TEXT in the database (no encryption), while the sibling fields notes and diagnosis are correctly stored as encrypted BLOBs (notes_enc, diagnosis_enc). A patient's chief complaint (e.g., "chest pain, shortness of breath, history of HIV") is clinical PHI that directly links health information to an individual and must be encrypted at rest.
Remediation
Encrypt the chief_complaint field at rest by renaming the database column to chief_complaint_enc (BLOB), applying encrypt_phi() on write (in create_visit and update_visit), and applying decrypt_phi() on read (in _row_to_response). This mirrors the existing pattern used for notes_enc and diagnosis_enc. A schema migration is required to re-encrypt existing plaintext values.
Detection: Policy / PHI Must Be Encrypted At Rest and In Transit
| conn = get_db_connection() | ||
| try: | ||
| # Exact-MRN lookup for the front desk. | ||
| rows = conn.execute(f"SELECT * FROM patients WHERE mrn = '{mrn}' AND is_active = 1").fetchall() |
There was a problem hiding this comment.
Gist: f-string passed to .execute() interpolates values directly into a SQL query and is vulnerable to SQL injection. Use parameterized queries instead, e.g. execute("... %s ...", (val,)).
Detection Agent: fstring-execute
Severity: High
Description
f-string passed to .execute() interpolates values directly into a SQL query and is vulnerable to SQL injection. Use parameterized queries instead, e.g. execute("... %s ...", (val,)).
Suggested fix
| rows = conn.execute(f"SELECT * FROM patients WHERE mrn = '{mrn}' AND is_active = 1").fetchall() | |
| rows = conn.execute("SELECT * FROM patients WHERE mrn = ? AND is_active = 1", (mrn,)).fetchall() |
Detection: Opengrep / fstring-execute
| conn = get_db_connection() | ||
| try: | ||
| # Exact-MRN lookup for the front desk. | ||
| rows = conn.execute(f"SELECT * FROM patients WHERE mrn = '{mrn}' AND is_active = 1").fetchall() |
There was a problem hiding this comment.
Gist: SQL query passed to execute()/executemany()/executescript() is built via string interpolation (f-string, % formatting, +, or .format()). Use parameterized queries with ? placeholders instead.
Detection Agent: sqlite-execute-fstring
Severity: High
Description
SQL query passed to execute()/executemany()/executescript() is built via string interpolation (f-string, % formatting, +, or .format()). Use parameterized queries with ? placeholders instead.
Suggested fix
| rows = conn.execute(f"SELECT * FROM patients WHERE mrn = '{mrn}' AND is_active = 1").fetchall() | |
| rows = conn.execute("SELECT * FROM patients WHERE mrn = ? AND is_active = 1", (mrn,)).fetchall() |
Detection: Opengrep / sqlite-execute-fstring
| conn.close() | ||
|
|
||
|
|
||
| @router.post("/{visit_id}/cancel", response_model=VisitResponse) |
There was a problem hiding this comment.
Gist: Visit cancellations are silently unlogged — a clinician or attacker can cancel any visit with no audit trail. This is an immediate compliance gap in a healthcare context where all state changes to patient records must be traceable.
Detection Agent: Audit Logging Omission
Severity: Medium
Description
The cancel_visit handler (POST /{visit_id}/cancel) mutates visit state by setting status = 'cancelled' but emits no audit log entry on the success path. Every other state-changing handler in this file calls log_audit with actor, action, and resource id; this handler is the sole exception, leaving cancellation events untracked.
Remediation
Add a log_audit call on the success path (after conn.commit() and before returning the response), passing user_id=current_user["id"], user_role=current_user["role"], action="CANCEL" (or "UPDATE"), resource_type="visit", resource_id=visit_id, and success=True. This mirrors the pattern used by create_visit, delete_visit, and all patient handlers.
Detection: Policy / Audit Logging Required for State-Changing Handlers
1 participant
One vuln per code chunk, in two different files, for a clean 1 finding ↔ 1 patch mapping (contrast with TEST/multi-patch-single-file):
fstring-execute. Fix: parameterize.Expected: 2 findings in 2 files → 2 patch-generators (different-file groups).