Skip to content

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