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

Validates PostgreSQL table name #19602

Merged
merged 2 commits into from
Apr 24, 2024
Merged

Conversation

ramondeklein
Copy link
Contributor

@ramondeklein ramondeklein commented Apr 24, 2024

The PostgreSQL store can be for submitting events to a table. This table can be configured, but it is inserted using Sprintf(...) to generate INSERT, UPDATE, DELETE statements. Despite it being a configuration value (and not originating from user input) it is still a good idea to validate or sanitize the value.

This PR checks the table name and only allows the use of valid PSQL identifiers. Note that PostgreSQL identifier rules also allow diacritics as valid identifier names, but they are only accepted here when used with quotes. Creating a regular expression that accepts diacritics is hard and probably not used often.

@ramondeklein ramondeklein force-pushed the sanitize-postgresql-tablename branch 2 times, most recently from e9bfb40 to dda4f2a Compare April 24, 2024 13:17
@ramondeklein ramondeklein marked this pull request as draft April 24, 2024 13:20
@ramondeklein ramondeklein self-assigned this Apr 24, 2024
@ramondeklein ramondeklein marked this pull request as ready for review April 24, 2024 13:28
Copy link
Contributor

@klauspost klauspost left a comment

Choose a reason for hiding this comment

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

A bit overly dramatic title. Unless something else is wrong this should not be available for anyone but admins to set.

I suggest "Sanitize postgres event table name" or something less "dramatic".

internal/event/target/postgresql.go Outdated Show resolved Hide resolved
@ramondeklein ramondeklein changed the title Fixes SQL injection Validates PostgreSQL table name Apr 24, 2024
@ramondeklein
Copy link
Contributor Author

A bit overly dramatic title. Unless something else is wrong this should not be available for anyone but admins to set.
I suggest "Sanitize postgres event table name" or something less "dramatic".

You're right. Updated commit message and PR title.

@klauspost
Copy link
Contributor

Good find. Could we imagine any existing table names becoming invalid now (other than the ones containing a quote)?

@ramondeklein
Copy link
Contributor Author

ramondeklein commented Apr 24, 2024

Good find. Could we imagine any existing table names becoming invalid now (other than the ones containing a quote)?

If you're using a table name with diacritics, then they may be marked as invalid (where previously they were working fine). I do think that's a rare scenario and it can be easily fixed by using quotes (although that's always tricky on command-line and/or YAML).

I did check if I could find an existing function in one of the Go packages that would validate the name, but I couldn't find one. What I could do is first normalize the name using golang.org/x/text/uniccode/norm and then check it. Let me try that...

@ramondeklein ramondeklein marked this pull request as draft April 24, 2024 14:07
@klauspost
Copy link
Contributor

@ramondeklein Yeah. It will probably be pretty annoying to upgrade to this, if valid (even if strange) names would be rejected, since they would have to build a new table.

@ramondeklein
Copy link
Contributor Author

@ramondeklein Yeah. It will probably be pretty annoying to upgrade to this, if valid (even if strange) names would be rejected, since they would have to build a new table.

They could also update the configuration to use the name with diacritics within double quotes, but I pushed a fix that also allows diacritics.

@ramondeklein ramondeklein marked this pull request as ready for review April 24, 2024 15:40
@harshavardhana
Copy link
Member

@ramondeklein please make branches on your fork, instead of the origin.

@ramondeklein
Copy link
Contributor Author

@ramondeklein please make branches on your fork, instead of the origin.

Will do that next time.

@harshavardhana harshavardhana merged commit 701da12 into master Apr 24, 2024
20 checks passed
@harshavardhana harshavardhana deleted the sanitize-postgresql-tablename branch April 24, 2024 17:51
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.

None yet

3 participants