curo1305
150 lines
6.8 KiB
Markdown
150 lines
6.8 KiB
Markdown
---
|
||
phase: 06.1-close-v1-audit-gaps
|
||
reviewed: 2026-05-30T00:00:00Z
|
||
depth: standard
|
||
files_reviewed: 3
|
||
files_reviewed_list:
|
||
- backend/tests/test_shares.py
|
||
- backend/tests/conftest.py
|
||
- backend/tests/test_audit.py
|
||
findings:
|
||
critical: 0
|
||
warning: 3
|
||
info: 2
|
||
total: 5
|
||
status: 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:**
|
||
```python
|
||
# 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:**
|
||
```python
|
||
# 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:**
|
||
```python
|
||
@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:
|
||
```python
|
||
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:**
|
||
```python
|
||
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_
|