fix: enforce RLS by downgrading DB connection role to non-superuser#571
fix: enforce RLS by downgrading DB connection role to non-superuser#571OlivierTrudeau wants to merge 6 commits into
Conversation
The aurora PostgreSQL role is a superuser, which bypasses all RLS policies regardless of FORCE ROW LEVEL SECURITY. This caused cross-org data leakage on any table that relied solely on RLS without an explicit WHERE org_id filter (e.g. the actions table). - Create aurora_app role (NOSUPERUSER NOBYPASSRLS) during DB init - get_connection() now issues SET ROLE aurora_app so RLS is enforced - get_admin_connection() retains superuser for migrations/cross-org tasks - Graceful fallback if role doesn't exist yet (first startup) Co-authored-by: Cursor <cursoragent@cursor.com>
WalkthroughThis PR adds an ChangesRLS role enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![arvo-ai-staging[bot]](https://avatars.githubusercontent.com/in/4088260?s=60&v=4){kind=small}
There was a problem hiding this comment.
Aurora Risk Review
Verdict: RISKY
This PR fixes a real cross-org data leakage bug by enforcing RLS via SET ROLE aurora_app on every connection checkout. The core logic is sound, but _downgrade_role() silently swallows SET ROLE failures at DEBUG level and continues — meaning if a Celery worker pod starts before the server pod's initialize_app() has created and granted the aurora_app role, the RLS fix silently does not apply to that worker for its entire lifetime. Since this PR is specifically remediating an active data-leakage incident, a silent non-application of the fix on the Celery path is a meaningful deployment risk that should be surfaced as an error, not swallowed.
Findings
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | HIGH | server/utils/db/connection_pool.py:227 |
SET ROLE failure silently swallowed — RLS fix can be silently bypassed on Celery workers |
| 2 | MEDIUM | server/utils/db/connection_pool.py:227 |
Celery workers have no guarantee aurora_app role exists before first connection checkout |
Aurora reviews PRs for incident prevention. This is advisory only and does not block merge.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/utils/db/connection_pool.py`:
- Around line 173-181: Reset logic in the connection pool is incomplete and can
leak session bypass state. Update the reset path in the connection return flow
inside the pool methods that handle connections, including get_connection() and
get_admin_connection(), so it clears the full RLS/bypass GUC set used by
initialize_tables(), specifically including myapp.mcp_token_resolve and
myapp.kubectl_token_resolve alongside the existing role/user/org resets. Also
change the failure handling in this reset block so a reset exception causes the
connection to be discarded/closed rather than returned to the pool, and keep the
cleanup behavior consistent in the _putconn_notify path.
- Line 155: The role-switch path in ConnectionPool must fail closed instead of
continuing with the original database role when SET ROLE aurora_app fails.
Update the logic around _downgrade_role and the connection acquisition flow it
feeds (including the same handling in the later 227-239 block) so that any
failure to switch roles raises an error, aborts the request, and prevents reuse
of the higher-privileged connection.
In `@server/utils/db/db_utils.py`:
- Around line 3075-3079: The privilege setup in the database grant block is too
broad for aurora_app because `GRANT ALL ON ALL TABLES` and the matching default
privileges include dangerous table-level rights like TRUNCATE. Update the grant
logic in the db utility code around the schema/public privilege setup to grant
only the minimal table permissions the app needs (for example, read/write DML),
and keep sequence/schema grants as needed. Make the same least-privilege
adjustment for the `ALTER DEFAULT PRIVILEGES` statements so future tables do not
inherit non-RLS-protected rights.
- Around line 3068-3074: The role setup block for aurora_app only creates the
role when missing, so existing roles with elevated privileges are left
unchanged. Update the DO $$ BEGIN block in the aurora_app role initialization
logic to explicitly enforce NOSUPERUSER and NOBYPASSRLS for both new and
pre-existing roles, using the same pg_roles/CREATE ROLE area so SET ROLE
aurora_app cannot bypass RLS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc8c8506-445c-499d-9cf5-c76fa56e12cb
📒 Files selected for processing (2)
server/utils/db/connection_pool.pyserver/utils/db/db_utils.py
…GUC reset - _downgrade_role() now raises RuntimeError on failure instead of silently continuing with superuser (fixes cross-org leakage on Celery workers that start before initialize_tables) - Reset all RLS/bypass GUCs on pool return (mcp_token_resolve, kubectl_token_resolve) to prevent stale bypass state leaking - Discard (close) connection on reset failure instead of returning dirty connection to pool - ALTER ROLE aurora_app NOSUPERUSER NOBYPASSRLS on every startup to enforce attributes even if role was previously modified - Use least-privilege grants (SELECT/INSERT/UPDATE/DELETE) instead of ALL to prevent TRUNCATE bypassing RLS - Replace empty except:pass blocks with debug logging Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/utils/db/connection_pool.py`:
- Around line 182-190: The cleanup path in connection_pool.get_connection
currently returns from the finally-like reset block, which can swallow any
exception already propagating from the caller or from _downgrade_role. Remove
the return from the failure handling in get_connection so cleanup only
closes/notifies the pool and then lets the original exception continue; apply
the same change in get_admin_connection as well. Keep the exception logging and
pool.putconn(..., close=True) behavior, but ensure no control flow in the reset
cleanup can suppress an in-flight error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c39a375a-d723-4b7d-b350-12725b19f62f
📒 Files selected for processing (2)
server/utils/db/connection_pool.pyserver/utils/db/db_utils.py
damianloch
left a comment
damianloch
left a comment
There was a problem hiding this comment.
Address bot comments they are valid
There was a problem hiding this comment.
when a postmortem gets generated automatically (from an incident rather than someone clicking the button), this path can't tell which org it belongs to anymore once RLS is enforced, so it quietly falls back to the default instructions and skips logging the run. Its not setup to error out so it will just silently ignore any custom postmortem instructions a team has set up
There was a problem hiding this comment.
Good catch — addressed. _get_action_instructions() now calls set_rls_context(cur, conn, user_id) before the query, so the org is resolved from the user_id even when there's no Flask request context (i.e. Celery-triggered postmortems). The function was already in the latest commit.
There was a problem hiding this comment.
mcp_server.py uses its own superuser pool and never downgrades, so RLS is still fully bypassed for all MCP requests. Nothing in the PR makes it worse but just wanted to make you aware
There was a problem hiding this comment.
Looked into this — it's actually fine by design. The MCP server's only direct DB access is token resolution on mcp_tokens (which uses the mcp_token_resolve GUC bypass policy specifically for that) and a SELECT 1 health check. All actual data queries go through HTTP to aurora-server, which enforces RLS. So the superuser pool here is intentional — it's the authentication layer itself, not a data access path.
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/utils/db/db_utils.py (1)
3085-3088: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail startup when
aurora_appsetup fails.Line 3086 logs and continues, but
get_connection()now raises whenSET ROLE aurora_appfails. If role creation/grants fail here, the server reports DB initialization success and then request-handling queries fail at checkout.Proposed fix
except Exception as e: - logging.warning(f"Error creating aurora_app role: {e}") + logging.error(f"FATAL: Error creating aurora_app role: {e}") conn.rollback() + raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/utils/db/db_utils.py` around lines 3085 - 3088, The aurora_app setup failure is only being logged inside the get_connection()/role initialization path, allowing startup to continue even though later SET ROLE aurora_app checks will fail. Update the exception handling around the aurora_app role/grant setup to re-raise after rollback (or otherwise propagate the failure) so initialization aborts instead of reporting success; use the existing get_connection and aurora_app role setup block to locate the change.
♻️ Duplicate comments (1)
server/utils/db/connection_pool.py (1)
182-190: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove
returnfrom cleanupfinallyblocks.Line 190 and Line 243 can suppress an exception already propagating from the caller or
_downgrade_role(). Keep closing dirty connections, but avoid returning fromfinally. This also replaces the remaining emptyexceptat Lines 239-240.Proposed fix
if connection: + reset_failed = False try: connection.rollback() with connection.cursor() as cur: cur.execute( "RESET ROLE; " @@ connection.commit() except Exception as e: logger.warning("Failed to reset session vars on pool return: %s", e) + reset_failed = True try: pool.putconn(connection, close=True) except Exception as close_exc: logger.debug("Failed to close broken connection during cleanup: %s", close_exc) with self._pool_available: self._pool_available.notify() - return - try: - self._putconn_notify(pool, connection) - except Exception as e: - logger.error("Error returning connection to pool: %s", e) + if not reset_failed: + try: + self._putconn_notify(pool, connection) + except Exception as e: + logger.error("Error returning connection to pool: %s", e) @@ if connection: + reset_failed = False try: connection.rollback() with connection.cursor() as cur: cur.execute( "RESET ROLE; " @@ connection.commit() except Exception as e: logger.warning("Failed to reset session vars on pool return: %s", e) + reset_failed = True try: pool.putconn(connection, close=True) - except Exception: - pass + except Exception as close_exc: + logger.debug("Failed to close broken admin connection during cleanup: %s", close_exc) with self._pool_available: self._pool_available.notify() - return - try: - self._putconn_notify(pool, connection) - except Exception as e: - logger.error("Error returning connection to pool: %s", e) + if not reset_failed: + try: + self._putconn_notify(pool, connection) + except Exception as e: + logger.error("Error returning connection to pool: %s", e)Also applies to: 235-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/utils/db/connection_pool.py` around lines 182 - 190, The cleanup path in connection_pool handling is suppressing errors by returning from a finally block and still has an empty except around the cleanup close path. Update the pool-return cleanup near the session reset logic and the _downgrade_role-related cleanup so they still attempt to close dirty/broken connections, but never use return inside finally; instead let any caller exception continue propagating after cleanup. Also replace the remaining empty except around the close/cleanup block with explicit logging or targeted handling using the existing logger and pool.putconn flow.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/utils/db/db_utils.py`:
- Around line 3085-3088: The aurora_app setup failure is only being logged
inside the get_connection()/role initialization path, allowing startup to
continue even though later SET ROLE aurora_app checks will fail. Update the
exception handling around the aurora_app role/grant setup to re-raise after
rollback (or otherwise propagate the failure) so initialization aborts instead
of reporting success; use the existing get_connection and aurora_app role setup
block to locate the change.
---
Duplicate comments:
In `@server/utils/db/connection_pool.py`:
- Around line 182-190: The cleanup path in connection_pool handling is
suppressing errors by returning from a finally block and still has an empty
except around the cleanup close path. Update the pool-return cleanup near the
session reset logic and the _downgrade_role-related cleanup so they still
attempt to close dirty/broken connections, but never use return inside finally;
instead let any caller exception continue propagating after cleanup. Also
replace the remaining empty except around the close/cleanup block with explicit
logging or targeted handling using the existing logger and pool.putconn flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3208d996-e590-4cc2-b049-fbf7269f20db
📒 Files selected for processing (2)
server/utils/db/connection_pool.pyserver/utils/db/db_utils.py
- Remove `return` from `finally` blocks in get_connection() and get_admin_connection() that could swallow in-flight exceptions (SonarCloud/CodeRabbit). Uses reset_failed flag instead. - Replace empty `except Exception: pass` in admin cleanup with logged debug message (github-code-quality). - Fail startup on aurora_app role setup failure instead of logging a warning and continuing (CodeRabbit). - Add set_rls_context to _get_action_instructions so auto-generated postmortems correctly resolve org-specific custom instructions when running from Celery workers (damianloch). Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/utils/db/db_utils.py (1)
3077-3081: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRevoke prior aurora_app grants before re-applying the hardened set.
TheseGRANTs are additive, so an existing deployment can keep older table/sequence privileges unless you explicitlyREVOKEfirst; add matchingALTER DEFAULT PRIVILEGES ... REVOKEstatements too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/utils/db/db_utils.py` around lines 3077 - 3081, The privilege setup in the db utility is only granting the hardened permissions and does not remove any older aurora_app privileges. Update the same grant block in db_utils to first REVOKE the prior table, sequence, and schema privileges from aurora_app before re-applying the desired set, and add matching ALTER DEFAULT PRIVILEGES ... REVOKE statements alongside the existing ALTER DEFAULT PRIVILEGES grants. Keep the changes grouped with the current GRANT/ALTER DEFAULT PRIVILEGES logic so the privilege reset is applied consistently during setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/utils/db/db_utils.py`:
- Around line 3086-3088: The fatal startup failure in the aurora_app role
creation path is using logging.error in the exception handler, which drops the
traceback and triggers Ruff warnings. Update the error handling in the db_utils
role-creation logic to use logging.exception in the same catch block that rolls
back and re-raises, so the failure is logged with full traceback while
preserving the existing rollback and raise behavior.
---
Outside diff comments:
In `@server/utils/db/db_utils.py`:
- Around line 3077-3081: The privilege setup in the db utility is only granting
the hardened permissions and does not remove any older aurora_app privileges.
Update the same grant block in db_utils to first REVOKE the prior table,
sequence, and schema privileges from aurora_app before re-applying the desired
set, and add matching ALTER DEFAULT PRIVILEGES ... REVOKE statements alongside
the existing ALTER DEFAULT PRIVILEGES grants. Keep the changes grouped with the
current GRANT/ALTER DEFAULT PRIVILEGES logic so the privilege reset is applied
consistently during setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0277d4da-9403-4de5-9f7c-1445f2f7b66a
📒 Files selected for processing (3)
server/services/actions/postmortem_action.pyserver/utils/db/connection_pool.pyserver/utils/db/db_utils.py
Existing deployments could retain broader privileges from earlier runs. REVOKE ALL before GRANTing the hardened DML-only set ensures no stale TRUNCATE/TRIGGER/REFERENCES rights persist. Also uses logging.exception for full traceback on role creation failure. Co-authored-by: Cursor <cursoragent@cursor.com>
Tests now account for SET ROLE aurora_app as the first execute call and expect the full 5-GUC reset string. Also updates the failed-reset test to verify connections are discarded (close=True) rather than returned to pool. Co-authored-by: Cursor <cursoragent@cursor.com>
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/tests/db/test_connection_pool.py (1)
237-252: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale docstring after behavior change.
The docstring at Line 238 still says "the connection is still returned," but the test body (Lines 242-252) now verifies the opposite: a failed
RESETcauses the connection to be discarded viaputconn(connection, close=True). Given this test specifically guards the security-relevant "don't return a tainted RLS-context connection to the pool" behavior, an accurate docstring matters for future maintainers.📝 Proposed docstring fix
def test_failed_reset_does_not_block_putconn(self, fresh_pool, flask_app): - """If the RESET execute raises, the connection is still returned.""" + """If the RESET execute raises, the connection is discarded (close=True) + rather than returned to the pool with stale session state."""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/tests/db/test_connection_pool.py` around lines 237 - 252, The test docstring in test_failed_reset_does_not_block_putconn is stale and contradicts the assertion now being made. Update the docstring to reflect that when RESET fails, the connection is not returned to the pool and is discarded via putconn(..., close=True). Keep the wording aligned with the behavior exercised in pool.get_connection and the mocked cursor.execute failure so future readers understand the intended RLS cleanup path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/tests/db/test_connection_pool.py`:
- Around line 245-248: The nested context managers in the test setup should be
combined to satisfy Ruff SIM117. Update the test around
flask_app.test_request_context and pool.get_connection so both are entered in a
single with statement, keeping the same request headers and connection
acquisition behavior while removing the extra nesting.
---
Outside diff comments:
In `@server/tests/db/test_connection_pool.py`:
- Around line 237-252: The test docstring in
test_failed_reset_does_not_block_putconn is stale and contradicts the assertion
now being made. Update the docstring to reflect that when RESET fails, the
connection is not returned to the pool and is discarded via putconn(...,
close=True). Keep the wording aligned with the behavior exercised in
pool.get_connection and the mocked cursor.execute failure so future readers
understand the intended RLS cleanup path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dbfb5885-fb96-48b1-a00f-8eaa7763061a
📒 Files selected for processing (2)
server/tests/db/test_connection_pool.pyserver/utils/db/db_utils.py
| with flask_app.test_request_context( | ||
| "/api/x", headers={"X-User-ID": "u-1", "X-Org-ID": "org-7"}, | ||
| ): | ||
| with pool.get_connection() as conn: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Combine nested with statements.
Ruff SIM117 flags this nested with as combinable.
♻️ Proposed fix
- with flask_app.test_request_context(
- "/api/x", headers={"X-User-ID": "u-1", "X-Org-ID": "org-7"},
- ):
- with pool.get_connection() as conn:
- assert conn is connection
+ with (
+ flask_app.test_request_context(
+ "/api/x", headers={"X-User-ID": "u-1", "X-Org-ID": "org-7"},
+ ),
+ pool.get_connection() as conn,
+ ):
+ assert conn is connection📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with flask_app.test_request_context( | |
| "/api/x", headers={"X-User-ID": "u-1", "X-Org-ID": "org-7"}, | |
| ): | |
| with pool.get_connection() as conn: | |
| with ( | |
| flask_app.test_request_context( | |
| "/api/x", headers={"X-User-ID": "u-1", "X-Org-ID": "org-7"}, | |
| ), | |
| pool.get_connection() as conn, | |
| ): | |
| assert conn is connection |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 245-248: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/tests/db/test_connection_pool.py` around lines 245 - 248, The nested
context managers in the test setup should be combined to satisfy Ruff SIM117.
Update the test around flask_app.test_request_context and pool.get_connection so
both are entered in a single with statement, keeping the same request headers
and connection acquisition behavior while removing the extra nesting.
Source: Linters/SAST tools
2 participants
Summary
auroraPostgreSQL role is a superuser which bypasses all RLS policies, even withFORCE ROW LEVEL SECURITY. This caused cross-org data leakage on tables that relied solely on RLS without an explicitWHERE org_idfilter (specifically the actions table, which auto-seeds system actions per-org — new accounts would see other orgs' actions and run history).aurora_approle (NOSUPERUSER NOBYPASSRLS NOLOGIN) during DB initialization and grants it full table/sequence access.get_connection()now issuesSET ROLE aurora_appso RLS policies are actually enforced for all request-handling queries.get_admin_connection()is a real separate code path that retains superuser privileges — used only for DDL/migrations, auth registration, and cross-org background tasks that explicitly callset_rls_context()per-org.Test plan
get_connection()(incidents, connectors, chat, etc.)Made with Cursor
Summary by CodeRabbit
New Features
aurora_appPostgreSQL role and initialized consistent permissions forpublictables, sequences, and future objects.Bug Fixes
Tests