Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add notify method #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add notify method #2

wants to merge 3 commits into from

Conversation

leodaher
Copy link

This patch adds a method for sending a PostgreSQL notify with a payload
to a specific channel.

@leodaher leodaher force-pushed the add-notify branch 2 times, most recently from ebd9f0c to 35246b3 Compare August 30, 2019 16:21
"""
connection = get_dbapi_connection(connection)
cursor = connection.cursor()
cursor.execute("NOTIFY %s, '%s';" % (channel, payload))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is vulnerable to SQL-injection (correct me if I'm wrong 😉). Is there a way to fix this? If not, it should be explicitly stated in the documentation that untrusted payloads / channels aren't supported.

Copy link

@matthewwo matthewwo Sep 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know Psycopg2’s execute method will sanitizes the arguments if passed as a tuple to the method.

For example

conn.execute(“NOTIFY %s, %s”, (channel, payload))

I’m not sure about sqlalchemy though :)

Look forward to this PR be merged! Looks neat and the library can be more complete with this change.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also it seems to be better to use the pg_notify function in Postgres for sending notifications to non-constant channel names and payloads?

Refer to the Notes section in the doc: https://www.postgresql.org/docs/9.0/sql-notify.html

@djrobstep
Copy link
Owner

Thank you kindly for this PR! Definitely a much-needed addition to this library.

Some things will need changing before we merge:

  • The current implementation is vulnerable to SQL injection (as mentioned by @madsmtm)
  • The cursor needs proper cleanup (probably with a with as per http://initd.org/psycopg/docs/usage.html)
  • As the docs and @prankymat mention, the pg_notify( function is probably nicer than the keyword style so let's change to that.
  • I'd rather the tests did a real test with a temporary database rather than mocking (this isn't essential, so we can probably change this in the future/a separate PR/whatever).

This patch adds a method for sending a PostgreSQL notify with a payload
to a specific channel.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants