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

Escape search_path in Connection.setSchema(...) #207

Merged
merged 2 commits into from Dec 1, 2014

Conversation

Projects
None yet
6 participants
@sehrope
Contributor

sehrope commented Oct 14, 2014

The Connection.setSchema(...) method does not escape the schema name before executing SET search_path TO .... It just wraps it in single quotes as a literal.

This works fine to handle whitespace in a schema name (ex: "foo bar") but it does not work if there is a single single-quote (') in the name. For example it will fail with a schema named foo'bar.

This patch replaces the single quoted schema name in the query string with an escaped identifier. I've also added a couple tests with funky schema names.

While testing this patch out, I noticed that Connection.getSchema() and Connection.setSchema(...) don't play nice with each other. Specifically, setSchema(...) allows for unescaped input (which it then escapes/quotes) but getSchema() returns back quoted/escaped text.

This means that the following would not work:

// Cache the old schema:
String oldSchema = conn.getSchema();
conn.setSchema(newSchema);
// .. do some work in newSchema
// Revert back to oldSchema - this won't work as we'd end up double escaping the schema name:
conn.setSchema(oldSchema);

You can see this first hand in the additional schema tests I added. The assertEquals(...) comparisons test against the escaped values of the funky schema names rather than the unescaped values. Ex: \"schema '5\" vs schema '5

I don't think that's correct. getSchema() should unescape it's output on the Java side before returning it back. That would be a breaking change for anyone expecting the old behavior so I haven't added it to this patch yet.

Thoughts?

PS: For the sake of discussion, let's put aside how silly it would be to have schemas with spaces or single quotes...

@davecramer

This comment has been minimized.

Member

davecramer commented Oct 14, 2014

Does it deal with UPPER case schema names ?

Dave Cramer

On 14 October 2014 13:11, Sehrope Sarkuni notifications@github.com wrote:

The Connection.setSchema(...) method does not escape the schema name
before executing SET search_path TO .... It just wraps it in single
quotes as a literal.

This works fine to handle whitespace in a schema name (ex: "foo bar") but
it does not work if there is a single single-quote (') in the name. For
example it will fail with a schema named foo'bar.

This patch replaces the single quoted schema name in the query string with
an escaped identifier. I've also added a couple tests with funky schema
names.

While testing this patch out, I noticed that Connection.getSchema() and
Connection.setSchema(...) don't play nice with each other. Specifically,
setSchema(...) allows for unescaped input (which it then escapes/quotes)
but getSchema() returns back quoted/escaped text.

This means that the following would not work:

// Cache the old schema:
String oldSchema = conn.getSchema();
conn.setSchema(newSchema);
// .. do some work in newSchema
// Revert back to oldSchema - this won't work as we'd end up double escaping the schema name:
conn.setSchema(oldSchema);

You can see this first hand in the additional schema tests I added. The
assertEquals(...) comparisons test against the escaped values of the
funky schema names rather than the unescaped values. Ex: "schema '5" vs schema
'5

I don't think that's correct. getSchema() should unescape it's output on
the Java side before returning it back. That would be a breaking change for
anyone expecting the old behavior so I haven't added it to this patch yet.

Thoughts?

PS: For the sake of discussion, let's put aside how silly it would be to

have schemas with spaces or single quotes...

You can merge this Pull Request by running

git pull https://github.com/sehrope/pgjdbc search-path-escaping

Or view, comment on, or merge it at:

#207
Commit Summary

  • Escape schema name when setting search_path
  • Add tests for schema name containing special characters

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#207.

@sehrope sehrope force-pushed the sehrope:search-path-escaping branch from a1d87aa to 169c6df Oct 14, 2014

@sehrope

This comment has been minimized.

Contributor

sehrope commented Oct 14, 2014

No it didn't deal with it properly as they weren't escaped properly when treated as identifiers.

I've updated the patch to go back to passing the schema name as a literal escaped via Utils.appendEscapedLiteral(...).

I also added a test with a mixed-case schema name that now passes.

@sehrope

This comment has been minimized.

Contributor

sehrope commented Oct 14, 2014

Somewhat related, looking at the Travis CI output (https://travis-ci.org/pgjdbc/pgjdbc/jobs/37962958) I don't think the JDBC 4 tests are running. They run successfully on my local machine but I think something is misconfigured in either the Travis config file or whatever build properties it's using to determine the tests to run.

@alexismeneses

This comment has been minimized.

Contributor

alexismeneses commented Nov 20, 2014

Sehrope,

To put my 2 cents in concerning getSchema(), I clearly think that the unescaping thing should be added.

I do not feel there's a real compatibility break here as

  • getSchema() hasn't been released in an official version yet (till the recent backport in PR #215, it was only in the master)
  • the same idea could be applied to setSchema(): one could have relied on the fact it was not escaped before and expect the old behavior. Yet, you're fixing it and you're right.

The consistent behavior between both methods seems more important to me

@davecramer

This comment has been minimized.

Member

davecramer commented Nov 23, 2014

I would think this should also be backpatched to 9.3

ringerc added a commit that referenced this pull request Dec 1, 2014

Merge pull request #207 from sehrope/search-path-escaping
Escape search_path in Connection.setSchema(...)

@ringerc ringerc merged commit 9ed5fb1 into pgjdbc:master Dec 1, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@ringerc

This comment has been minimized.

Member

ringerc commented Dec 1, 2014

I concur, a backpatch to 9.3 is warranted. @alexismeneses, any objection to prepping a PR against the 9.3 branch? I'm struggling for time at the moment.

@ringerc

This comment has been minimized.

Member

ringerc commented Dec 1, 2014

See #215

@alexismeneses

This comment has been minimized.

Contributor

alexismeneses commented Dec 1, 2014

I'm waiting to see if you agree to merge #229. Then I'll create a backpatch PR of the whole.

On the same subject, I think #216 could be added [edit: to the backpatch I mean]

@frode-carlsen

This comment has been minimized.

frode-carlsen commented Feb 17, 2015

This patch breaks search_path entries which lists multiple schemas in a search order as it single-quotes the entire entry - e.g. #setSchema("myschema, public") gets interpreted as SET SESSION search_path TO 'myschema, public'; instead of SET SESSION search_path TO myschema, public;

@alexismeneses

This comment has been minimized.

Contributor

alexismeneses commented Feb 17, 2015

This behavior is intended as the JDBC 4.1 specification expect the string argument of Connection.setSchema(String) to be a (single) schema name. So it is completely escaped as one could have created a schema name with commas inside.

You need to send the SQL directly if you want to handle multiple schema names in the search_path.

@frode-carlsen

This comment has been minimized.

frode-carlsen commented Feb 17, 2015

In that case, would it not be better to SET SCHEMA instead of SET search_path in the implementation?

@valgog

This comment has been minimized.

Contributor

valgog commented Feb 17, 2015

Actually, it is probably too late to talk about this, but does the current
implementation of the setSchema follow the specification?

http://docs.oracle.com/javase/7/docs/api/java/sql/Connection.html#setSchema(java.lang.String)

Cheers,

-- Valentine Gogichashvili

On Tue, Feb 17, 2015 at 11:00 AM, Frode Carlsen notifications@github.com
wrote:

In that case, would it not be better to SET SCHEMA instead of SET
search_path in the implementation?


Reply to this email directly or view it on GitHub
#207 (comment).

@alexismeneses

This comment has been minimized.

Contributor

alexismeneses commented Feb 17, 2015

@frode-carlsen
SET SCHEMA is not supported on versions prior to 8.4 of the backend, so using SET search_path is more portable and allow to achieve the same goal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment