postgres dialect panics if password contains a space #775

Closed
opened 2017-11-14 13:56:24 +00:00 by daniellee · 4 comments
daniellee commented 2017-11-14 13:56:24 +00:00 (Migrated from github.com)

With the following code (the password should be escaped but isn't):

cntStr := "postgres://badpass:test space@localhost:5432/grafana?sslmode=disable"
engine, err := xorm.NewEngine("postgres", cntStr)

The parseOpts method panics instead of returning an error.

Would you accept a PR that does the following:

Removes the errorf function that is used in the parseOpts function and that returns an error if the parsing fails.

With the following code (the password should be escaped but isn't): ```go cntStr := "postgres://badpass:test space@localhost:5432/grafana?sslmode=disable" engine, err := xorm.NewEngine("postgres", cntStr) ``` The [parseOpts](https://github.com/go-xorm/xorm/blob/master/dialect_postgres.go#L1178-L1182) method panics instead of returning an error. Would you accept a PR that does the following: Removes the [errorf function](https://github.com/go-xorm/xorm/blob/master/dialect_postgres.go#L1120) that is used in the parseOpts function and that returns an error if the parsing fails.

Please feel free to send a PR.

Please feel free to send a PR.
daniellee commented 2017-11-16 10:24:35 +00:00 (Migrated from github.com)

@lunny are the instructions in CONTRIBUTING.md up to date? Should tests be placed in https://github.com/go-xorm/tests ?

I have a working version with a dialect_postgres_test.go class that tests the Parse function but should the test go here -> https://github.com/go-xorm/tests/blob/master/postgres/postgres_test.go

Also, noticed that the code for parsing is from an old version of pq and it does more than is needed. For example, it parses out a full uri but only the db name is returned. Is it ok to remove that code (for example this)?

@lunny are the instructions in [CONTRIBUTING.md](https://github.com/go-xorm/xorm/blob/master/CONTRIBUTING.md) up to date? Should tests be placed in https://github.com/go-xorm/tests ? I have a working version with a dialect_postgres_test.go class that tests the Parse function but should the test go here -> https://github.com/go-xorm/tests/blob/master/postgres/postgres_test.go Also, noticed that the code for parsing is from an old version of pq and it does more than is needed. For example, it parses out a full uri but only the db name is returned. Is it ok to remove that code (for example [this](https://github.com/go-xorm/xorm/blob/master/dialect_postgres.go#L1142-L1156))?

It's out date. All tests now merged into this repository. Please add test with your PR.

It's out date. All tests now merged into this repository. Please add test with your PR.

It's OK to remove unused code.

It's OK to remove unused code.
Sign in to join this conversation.
No Milestone
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: xorm/xorm#775
No description provided.