Fix critical audit findings: FORCE RLS, owner-scoped UPDATEs, canonical email#97
Conversation
…al email, auto_provision default
C1: FORCE ROW LEVEL SECURITY — new migration so table owner can't bypass RLS
C2: Add owner_id to WHERE clause in update_entry, upsert_alert_update,
upsert_preference_update SQL — prevents cross-tenant updates
C3: OAuth auto-provisioning and identity linking now use canonical_email
(handles Gmail dot/+tag variants), extract canonical_email to helpers.py
C4: AuthMiddleware.auto_provision defaults to False (was True)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cmeans
left a comment
There was a problem hiding this comment.
QA Review — PR #97: Fix critical audit findings
Code Review
All 4 critical findings addressed correctly:
C1: FORCE ROW LEVEL SECURITY (migration j5e6f7g8h9i0)
- Adds
ALTER TABLE {table} FORCE ROW LEVEL SECURITYto all 4 tables - Verified:
relforcerowsecurity = trueon all tables - Observation: FORCE works correctly with non-superuser roles (tested: 0 rows unscoped, 17 scoped). However, the Docker Compose
awarenessrole is a superuser, which bypasses RLS even with FORCE. This is a known PostgreSQL limitation — the audit recommended "use a non-owner app role." The migration is correct; the role separation is a follow-up item.
C2: owner_id in UPDATE WHERE clauses
update_entry.sql:WHERE id = %s→WHERE id = %s AND owner_id = %s✅upsert_alert_update.sql: same fix ✅upsert_preference_update.sql: same fix ✅- postgres_store.py params updated to include
owner_idin all 3 call sites ✅ - Verified manually: cross-owner UPDATE returns
UPDATE 0
C3: canonical_email in OAuth provisioning
canonical_email()extracted from cli.py tohelpers.pyfor shared usecreate_user_auto.sql: now includescanonical_emailcolumn ✅link_oauth_identity.sql: matches oncanonical_emailinstead of rawemail✅postgres_store.py:create_user_if_not_exists()andlink_oauth_identity()compute canonical before SQL ✅- Verified:
a.lice+work@Gmail.com→ canonicalalice@gmail.com
C4: auto_provision default
AuthMiddleware.__init__:auto_provision: bool = False(wasTrue) ✅- Verified:
AuthMiddleware(None, jwt_secret='x').auto_provision == False
CI Gate Check
| Check | Conclusion |
|---|---|
| lint | ✅ SUCCESS |
| typecheck | ✅ SUCCESS |
| test (3.10/3.11/3.12) | ✅ SUCCESS |
| codecov/patch | ✅ SUCCESS |
Test Results
| Check | Result |
|---|---|
| pytest (490 tests) | ✅ 490/490 pass |
| ruff, mypy | ✅ Clean |
| CI | ✅ All green |
| Manual #1: FORCE RLS | ✅ Applied (relforcerowsecurity=t); verified with non-superuser role (0 unscoped, 17 scoped). Note: default Docker superuser bypasses — role separation is a follow-up |
| Manual #2: UPDATE scoped | ✅ Cross-owner UPDATE returns UPDATE 0, entry unchanged |
| Manual #3: canonical_email | ✅ a.lice+work@Gmail.com → alice@gmail.com |
| Manual #4: auto_provision default | ✅ False |
Observation
The awareness Docker role is a superuser, which bypasses all RLS (even FORCE). The FORCE migration is correct and effective for non-superuser roles, but production deployments should create a dedicated non-superuser app role for the connection pool. This was noted in the original audit (S-level recommendation). Consider a follow-up PR that creates a awareness_app role in Docker Compose and updates the DSN.
Verdict
Zero blockers. All 4 critical findings fixed correctly. One observation (superuser role bypass — follow-up item, not this PR's scope). Ready for QA Signoff.
|
Adding Ready for QA Signoff — all 4 critical audit findings verified. FORCE RLS applied (works with non-superuser role), owner-scoped UPDATEs confirmed, canonical_email matching verified, auto_provision defaults False. CI green, 490/490 pytest. |
Summary
Addresses 4 critical findings from the multi-tenant/OAuth code audit (main@52ed457):
FORCEso the table owner role can't bypass RLS policies. PreviouslyENABLEalone let the connection pool role (table owner) skip all policies.update_entry.sql,upsert_alert_update.sql,upsert_preference_update.sqlnow includeAND owner_id = %s. Prevents cross-tenant updates if any code path misses Python-side ownership check.create_user_auto.sqlnow storescanonical_email,link_oauth_identity.sqlmatches oncanonical_emailinstead of raw email. Handles Gmail dot/+tag variants (j.doe+work@gmail.com→jdoe@gmail.com).canonical_email()extracted fromcli.pytohelpers.pyfor shared use.AuthMiddlewareparameter defaults toFalse(wasTrue). Server.py already overrides this, but direct instantiation was dangerous.QA
Prerequisites
pip install -e ".[dev]"AWARENESS_PORT=8421)Manual tests (via MCP tools)
Connect as the DB owner role, set
app.current_userto one user, verify you can't see another user's entries even without the SET:Expected: empty result set when no
app.current_useris setExpected: as user B, this should fail or return no match (entry belongs to user A)
Expected: both email variants resolve to the same canonical_email, user is linked
Instantiate
AuthMiddleware(app, jwt_secret="x")without passingauto_provision— verify it defaults to False🤖 Generated with Claude Code