fix: use bound parameters in pg_notify to survive dollar-tokens in payloads#123
Conversation
…yloads
``_notify_postgres`` in ``src/realtime.py`` was f-string-interpolating the
JSON payload directly into the SQL string passed to ``text()``:
NOTIFY telegram_updates, '<json>'
asyncpg scans the raw SQL for ``$N`` positional placeholders before
dispatching to Postgres, so any message body containing tokens like
``$1`` or ``$D`` tripped the driver's parameter parser and the
LISTEN/NOTIFY fanout to the viewer UI stopped working. Real example
from production: a chat message containing "sshhhh $1 will take LONGER!
$O" silently killed realtime updates for that chat until restart, with
only a generic warning in the log.
Fix: use ``pg_notify(:channel, :payload)`` with SQLAlchemy bound
parameters. Payload is opaque string data to the driver — no SQL-level
escaping needed, and the driver doesn't scan it for placeholders.
Regression tests in ``tests/test_realtime_notify.py``:
- Payload with ``$1``/``$D`` tokens is never embedded in the SQL string.
- ``.notify()`` with dollar-tokens does not emit the
"Failed to send realtime notification" warning.
📝 WalkthroughWalkthroughModified PostgreSQL notification in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
Hey @tondeaf, great catch on this one! The Here's the review: Security (APPROVE)This is actually a security improvement. The old code did Code Quality (1 HIGH, 1 MEDIUM, 1 LOW)HIGH — Existing test
MEDIUM — Duplicate mock setup LOW — Opaque ticket ID in docstring Test Adequacy (2 must-fix, 2 should-fix)Must-fix:
Should-fix:
CodeRabbitCodeRabbit was rate-limited on this PR. I'll retrigger it once the cooldown expires. Thanks again for the contribution! 🙏 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_realtime_notify.py (2)
40-59: Add coverage for single quotes in payloads.The old path manually escaped
'; the new path relies entirely on bound params. Add a regression case with text likedon't breakto ensure quote-heavy payloads still execute without SQL embedding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_realtime_notify.py` around lines 40 - 59, Add a regression case to tests/test_realtime_notify.py that sends a message containing single quotes (e.g., "don't break") through notifier.notify (NotificationType.NEW_MESSAGE) and then assert that session.execute was called once and that the SQL (from session.execute.await_args.args[0] / stmt) does NOT contain the raw payload or any $-tokens and DOES contain "pg_notify", while the bound params (from call_args.args[1] or call_args.kwargs) include the full payload with the single quote intact (params["payload"] contains "don't break"); this verifies notifier.notify relies on bound parameters (not SQL embedding) and handles quote-heavy payloads safely.
19-36: Align the new tests with the repository test style and add signatures.These new functions miss return annotations, and the module-level async pytest style conflicts with the project’s preferred
unittestpattern. Also, if the project uses asyncio auto mode, the explicit@pytest.mark.asynciodecorators can be dropped.As per coding guidelines,
**/*.py: “Use type hints for function signatures”;**/test_*.py: “Useunittest.TestCasewithMagicMock/AsyncMockpattern for consistency with existing tests”.Also applies to: 62-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_realtime_notify.py` around lines 19 - 36, Convert the new pytest-style functions into the project's unittest style: move _make_notifier_with_fake_session() and the async test test_notify_does_not_interpolate_payload_into_sql() into a unittest.TestCase subclass as methods (e.g., class TestRealtimeNotifier(unittest.IsolatedAsyncioTestCase) if async is needed), add explicit type hints for return values (e.g., -> Tuple[RealtimeNotifier, AsyncMock]) on _make_notifier_with_fake_session, and make the test an async test method on the TestCase instead of using `@pytest.mark.asyncio`; also remove the module-level pytest async decorator if the test runner uses asyncio auto mode and ensure you keep using MagicMock/AsyncMock for session_factory and db_manager to match existing patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_realtime_notify.py`:
- Line 1: Replace the opaque tracker ID in the module docstring of
tests/test_realtime_notify.py (the first-line string containing "(yrru-ks3d)")
with the actual PR or issue identifier (e.g., "PR `#123`" or "issue `#456`" or a
short issue URL) so future readers can locate the discussion; update only the
docstring text at the top of this file (the module-level string) to include the
actionable reference.
- Around line 46-59: The test currently only asserts session.execute was
awaited, which can pass without a transaction commit; update the test for
_notify_postgres() to also assert a commit was performed: check
session.commit.await_count (or the mock transaction object's commit await_count)
is 1 and optionally verify the commit call occurs after the session.execute call
by inspecting session.commit.await_args_list or the combined await call order;
ensure you reference the existing session.execute and session.commit mocks (and
_notify_postgres() behavior) when adding these assertions.
---
Nitpick comments:
In `@tests/test_realtime_notify.py`:
- Around line 40-59: Add a regression case to tests/test_realtime_notify.py that
sends a message containing single quotes (e.g., "don't break") through
notifier.notify (NotificationType.NEW_MESSAGE) and then assert that
session.execute was called once and that the SQL (from
session.execute.await_args.args[0] / stmt) does NOT contain the raw payload or
any $-tokens and DOES contain "pg_notify", while the bound params (from
call_args.args[1] or call_args.kwargs) include the full payload with the single
quote intact (params["payload"] contains "don't break"); this verifies
notifier.notify relies on bound parameters (not SQL embedding) and handles
quote-heavy payloads safely.
- Around line 19-36: Convert the new pytest-style functions into the project's
unittest style: move _make_notifier_with_fake_session() and the async test
test_notify_does_not_interpolate_payload_into_sql() into a unittest.TestCase
subclass as methods (e.g., class
TestRealtimeNotifier(unittest.IsolatedAsyncioTestCase) if async is needed), add
explicit type hints for return values (e.g., -> Tuple[RealtimeNotifier,
AsyncMock]) on _make_notifier_with_fake_session, and make the test an async test
method on the TestCase instead of using `@pytest.mark.asyncio`; also remove the
module-level pytest async decorator if the test runner uses asyncio auto mode
and ensure you keep using MagicMock/AsyncMock for session_factory and db_manager
to match existing patterns.
🪄 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: CHILL
Plan: Pro
Run ID: 9695f5e5-94f5-4b08-b750-db3c91916896
📒 Files selected for processing (2)
src/realtime.pytests/test_realtime_notify.py
| @@ -0,0 +1,80 @@ | |||
| """Regression tests for the NOTIFY bind-parameter bug (yrru-ks3d). | |||
There was a problem hiding this comment.
Replace the opaque tracker ID with the PR or issue number.
(yrru-ks3d) is not actionable for future readers; use PR #123`` or the linked issue instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_realtime_notify.py` at line 1, Replace the opaque tracker ID in
the module docstring of tests/test_realtime_notify.py (the first-line string
containing "(yrru-ks3d)") with the actual PR or issue identifier (e.g., "PR
`#123`" or "issue `#456`" or a short issue URL) so future readers can locate the
discussion; update only the docstring text at the top of this file (the
module-level string) to include the actionable reference.
| assert session.execute.await_count == 1 | ||
| call_args = session.execute.await_args | ||
| stmt = call_args.args[0] | ||
| sql = str(stmt) | ||
|
|
||
| # The payload (and any $-tokens it contains) must not be in the SQL. | ||
| assert "$1 break" not in sql | ||
| assert "$D" not in sql | ||
| # The SQL must use pg_notify with bound parameter names. | ||
| assert "pg_notify" in sql.lower() | ||
|
|
||
| params = call_args.args[1] if len(call_args.args) > 1 else call_args.kwargs | ||
| assert params["channel"] == "telegram_updates" | ||
| assert "$1 break $D asyncpg" in params["payload"] |
There was a problem hiding this comment.
Assert the commit, not just the execute.
PostgreSQL NOTIFY is delivered on transaction commit, so this regression test can pass even if _notify_postgres() stops committing.
Proposed test assertion
assert session.execute.await_count == 1
+ assert session.commit.await_count == 1
call_args = session.execute.await_args📝 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.
| assert session.execute.await_count == 1 | |
| call_args = session.execute.await_args | |
| stmt = call_args.args[0] | |
| sql = str(stmt) | |
| # The payload (and any $-tokens it contains) must not be in the SQL. | |
| assert "$1 break" not in sql | |
| assert "$D" not in sql | |
| # The SQL must use pg_notify with bound parameter names. | |
| assert "pg_notify" in sql.lower() | |
| params = call_args.args[1] if len(call_args.args) > 1 else call_args.kwargs | |
| assert params["channel"] == "telegram_updates" | |
| assert "$1 break $D asyncpg" in params["payload"] | |
| assert session.execute.await_count == 1 | |
| assert session.commit.await_count == 1 | |
| call_args = session.execute.await_args | |
| stmt = call_args.args[0] | |
| sql = str(stmt) | |
| # The payload (and any $-tokens it contains) must not be in the SQL. | |
| assert "$1 break" not in sql | |
| assert "$D" not in sql | |
| # The SQL must use pg_notify with bound parameter names. | |
| assert "pg_notify" in sql.lower() | |
| params = call_args.args[1] if len(call_args.args) > 1 else call_args.kwargs | |
| assert params["channel"] == "telegram_updates" | |
| assert "$1 break $D asyncpg" in params["payload"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_realtime_notify.py` around lines 46 - 59, The test currently only
asserts session.execute was awaited, which can pass without a transaction
commit; update the test for _notify_postgres() to also assert a commit was
performed: check session.commit.await_count (or the mock transaction object's
commit await_count) is 1 and optionally verify the commit call occurs after the
session.execute call by inspecting session.commit.await_args_list or the
combined await call order; ensure you reference the existing session.execute and
session.commit mocks (and _notify_postgres() behavior) when adding these
assertions.
…rPostgres Address PR GeiserX#123 review feedback: - Merged the dollar-token / bind-param regression tests from the separate test_realtime_notify.py into the existing TestRealtimeNotifierPostgres class to eliminate duplicated mock setup. - Replaced the stale test_notify_postgres_executes_notify_command (which only asserted that some SQL ran) with test_notify_postgres_uses_pg_notify_with_bound_params, which validates the new contract: SELECT pg_notify in the SQL plus channel/payload bound params. - Added test_notify_handles_single_quotes_in_payload so the removal of the old manual .replace("'", "''") escaping is documented. - Added explicit session.commit.await_count assertions on the dollar-token regression test; NOTIFY only delivers on commit so this matters. - Dropped redundant @pytest.mark.asyncio decorators (the project uses asyncio_mode = "auto" in pyproject.toml). - Removed the opaque (yrru-ks3d) ticket reference; the docstring now cites PR GeiserX#123 instead. - Kept the explicit session.__aenter__.return_value = session: dropping it (as the review suggested) breaks the tests, because AsyncMock's default __aenter__ returns a fresh child mock rather than the same object, so `async with session as s:` would bind s to a different mock and the assertions would miss the call. Documented this in the helper's docstring. Deleted tests/test_realtime_notify.py. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed your feedback in the latest commit. Summary: HIGH — stale
MEDIUM — duplicate mock setup
LOW — opaque ticket id
Must-fix #1 —
Must-fix #2 —
Should-fix #3 — single-quote payload test
Should-fix #4 — explicit
Ready for another look. |
|
Thanks for the contribution, @tondeaf! Merged and shipped a follow-up in #131 (v7.6.3) addressing edge cases found during review:
Released as v7.6.3. |
Summary
_notify_postgresfrom f-string-built SQL topg_notify(:channel, :payload)with SQLAlchemy bound parameters, so message bodies that happen to contain$N/$<letter>tokens stop tripping asyncpg's positional-placeholder parser and breaking realtime fanout.TestRealtimeNotifierPostgrescovering payload-not-in-SQL, dollar-token end-to-end (no warning), single-quote payload (replaces the old manual.replace("'", "''")escaping), and a tightened pg_notify-shape assertion to replace the staleexecute-was-calledtest.Type of Change
Database Changes
scripts/Data Consistency Checklist
chat_idvalues use marked format (via_get_marked_id()) — N/A, no chat_id construction in this path_strip_tz()before DB operations — N/A, no datetime handling in this pathTesting
python -m pytest tests/ -v)ruff check .)ruff format --check .)Security Checklist
Deployment Notes
The bug
Realtime LISTEN/NOTIFY fanout to the viewer silently breaks for any chat when a single message body contains a
$Nor$<letter>token (e.g."sshhhh $1 will take LONGER!"or"paid $D today"). The viewer stops receiving updates for that chat until the service is restarted. The only visible symptom is a vague "Failed to send realtime notification" warning in the log.Root cause
_notify_postgresinsrc/realtime.pyf-string-interpolates the JSON payload directly into the SQL string passed totext():asyncpg scans the raw SQL for
$Npositional placeholders before dispatching to Postgres, so any message text containing tokens that look like parameters trips its parser:The fix
Use
pg_notify(:channel, :payload)with SQLAlchemy bound parameters. Payload becomes opaque string data to the driver — no SQL-level quoting, and the placeholder scanner never sees the payload content.Regression tests live alongside the existing
TestRealtimeNotifierPostgresclass:$1/$Dtokens is never embedded in the final SQL..notify()with those tokens does not emit the "Failed to send realtime notification" warning..replace("'", "''")path).channel/payloadbound-param keys are explicitly asserted (replacing the staleexecute-was-calledtest).Test plan
pytest tests/test_realtime.py::TestRealtimeNotifierPostgres— 5/5 pass; fulltests/test_realtime.py52/52 pass