Update connection-parameters.js #326

Merged
merged 5 commits into from Feb 26, 2014

Conversation

3 participants
Contributor

za-creature commented Apr 11, 2013

The current connection url handling fails when the password contains
encoded special characters: After the encodeURI, the special
characters from the password are double encoded, and the password is
rejected by postgres.

Proposed fix handles one level of double encoding, and while it
might break compatibility with passwords like "asdfg%77fgh" (which
would've been escaped to asdfg%2577fgh before this patch), I
strongly feel that maintaining backwards compatibility is in this
case less important than following standards and discouraging bad
coding practices.

za-creature Update connection-parameters.js
The current connection url handling fails when the password contains
encoded special characters: After the encodeURI, the special
characters from the password are double encoded, and the password is
rejected by postgres.

Proposed fix handles one level of double encoding, and while it
might break compatibility with passwords like "asdfg%77fgh" (which
would've been escaped to asdfg%2577fgh before this patch), I
strongly feel that maintaining backwards compatibility is in this
case less important than following standards and discouraging bad
coding practices.
c666b20
Contributor

za-creature commented Apr 11, 2013

Whoops, I pasted the wrong line. it should be:

str = encodeURI(str).replace(/%25(\d\d)/g, "%$1");

Owner

brianc commented Apr 11, 2013

you can update the branch with more commits & it will update the pull request.

Also: can you add tests for this please?

za-creature added some commits Apr 11, 2013

za-creature Update connection-parameters.js
Different double-encode removal strategy
5493a52
za-creature Merge pull request #1 from za-creature/master
unittest
b536867
za-creature Update creation-tests.js
connection exports 'database' instead of 'path'
264839d
za-creature Update connection-parameters.js b6ef157
Contributor

za-creature commented Apr 11, 2013

Not entirely sure about the last test. Is it my fault? The failed test is non-deterministic (setTimeout), and I don't know how to schedule another test

Owner

brianc commented Jul 3, 2013

Sorry for taking so long on this! Gah! I just kicked off the test again.

What's the status of this? I think this just bit me while using Sequelize.

gabegorelick referenced this pull request in sequelize/sequelize Jan 7, 2014

Closed

Postgres connection URI not escaped #1212

Owner

brianc commented Jan 7, 2014

looks like all the tests pass - I don't see any reason not to merge this

brianc merged commit b6ef157 into brianc:master Feb 26, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment