curo1305
6.8 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 06.1-close-v1-audit-gaps | 2026-05-30T00:00:00Z | standard | 3 |
|
|
issues_found |
Phase 06.1: Code Review Report
Reviewed: 2026-05-30 Depth: standard Files Reviewed: 3 Status: issues_found
Summary
Reviewed the three test-only files promoted in Phase 6.1: test_shares.py (7 xfail stubs promoted to real tests covering SHARE-01 through SHARE-05), conftest.py (new second_auth_user fixture), and test_audit.py (4 xfail stubs promoted to real tests covering ADMIN-06).
The share tests are structurally sound. Fixture isolation is correct: db_session is function-scoped, so each test gets a fresh in-memory SQLite database; expire_on_commit=False ensures ORM objects remain accessible after the fixture's commit() calls; JSONB columns round-trip correctly through SQLite's TEXT storage because SQLAlchemy's JSONB result_processor fires regardless of dialect. The IDOR revocation test (test_share_revoke_wrong_owner_404) correctly exercises the 404-not-403 invariant.
Three defects were found, all in test_audit.py: one incomplete security invariant assertion and two coverage gaps that leave the CSV export untested for data leakage.
Warnings
WR-01: test_audit_log_no_doc_content — metadata_ nested check omits password_hash and credentials_enc
File: backend/tests/test_audit.py:101
Issue: The docstring for test_audit_log_no_doc_content explicitly states it checks "including nested inside metadata_" for all four forbidden keys (filename, extracted_text, password_hash, credentials_enc). However the inner loop at line 101 only iterates over ("filename", "extracted_text"). A future audit entry that stored a password_hash or credentials_enc value inside metadata_ would silently pass this test.
The four-key forbidden_keys set is defined at line 89 and used for the top-level check — but is not reused for the nested check, creating divergence that will not be caught by a reader who trusts the docstring.
Fix:
# Replace the inner loop at line 101 with the full forbidden set:
meta = item.get("metadata_")
if isinstance(meta, dict):
for key in forbidden_keys: # was: ("filename", "extracted_text")
assert key not in meta, (
f"forbidden key '{key}' found inside metadata_ of audit item"
)
WR-02: test_audit_log_export_csv — no assertion that forbidden fields are absent from the CSV body
File: backend/tests/test_audit.py:116
Issue: The CSV export test verifies the Content-Type header, Content-Disposition filename, and the presence of the correct CSV header row. It does not verify that the CSV body does not contain filename, extracted_text, password_hash, or credentials_enc. The test would pass even if the export endpoint switched from _audit_to_dict() to a direct vars(entry) serialisation that exposed all ORM columns.
This is a security-invariant test (ADMIN-06, D-15) and the gap is meaningful: the JSON viewer test (test_audit_log_no_doc_content) asserts the whitelist, but the CSV path has no equivalent assertion.
Fix:
# After the existing header assertions, add:
csv_body = response.text
forbidden_in_csv = {"filename", "extracted_text", "password_hash", "credentials_enc"}
for forbidden in forbidden_in_csv:
assert forbidden not in csv_body, (
f"forbidden field '{forbidden}' found in CSV export body"
)
WR-03: async_client fixture — dependency_overrides not guarded by try/finally
File: backend/tests/conftest.py:150
Issue: app.dependency_overrides[get_db] is set at line 150, before the async with AsyncClient(...) context manager at line 152. If ASGITransport or AsyncClient.__aenter__ raises (e.g. app startup failure), the generator terminates without reaching the yield, so pytest never runs the teardown code at line 155 (app.dependency_overrides.clear()). The global app object would then have a stale override pointing at a closed session, potentially corrupting subsequent tests in the same process.
This is low probability in practice (app construction is deterministic), but it is a correctness gap in a shared-object fixture.
Fix:
@pytest_asyncio.fixture
async def async_client(db_session: AsyncSession):
from deps.db import get_db
from main import app
app.dependency_overrides[get_db] = lambda: db_session
try:
async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as c:
yield c
finally:
app.dependency_overrides.clear()
Info
IN-01: Missing test for unauthenticated access to share endpoints
File: backend/tests/test_shares.py (no specific line — absent test)
Issue: None of the seven share tests verify that an unauthenticated request (no Authorization header) to POST /api/shares, GET /api/shares/received, or DELETE /api/shares/{id} returns 401/403. The get_regular_user dependency chain passes through HTTPBearer(auto_error=True) which raises 403 on a missing header — but this is not exercised in the test suite for the shares router, leaving an untested code path in the dependency chain for this specific router.
Fix: Add a parametrized negative test:
async def test_shares_unauthenticated(async_client):
"""All share endpoints reject requests with no auth token."""
r1 = await async_client.post("/api/shares", json={"document_id": "x", "recipient_handle": "y"})
assert r1.status_code in (401, 403)
r2 = await async_client.get("/api/shares/received")
assert r2.status_code in (401, 403)
r3 = await async_client.delete(f"/api/shares/{uuid.uuid4()}")
assert r3.status_code in (401, 403)
IN-02: Missing test for self-share rejection (400)
File: backend/tests/test_shares.py (no specific line — absent test)
Issue: api/shares.py line 89–90 explicitly rejects a share where the recipient is the same as the owner with a 400 Bad Request. This branch has no corresponding test. A refactor that removes the self-share guard would not be caught by the current suite.
Fix:
async def test_share_self(async_client, auth_user, db_session):
"""POST /api/shares where recipient is the owner returns 400."""
doc_id = await _make_doc(db_session, auth_user)
resp = await async_client.post(
"/api/shares",
json={"document_id": doc_id, "recipient_handle": auth_user["user"].handle},
headers=auth_user["headers"],
)
assert resp.status_code == 400
Reviewed: 2026-05-30 Reviewer: Claude (gsd-code-reviewer) Depth: standard