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

Fix newline parsing in pgpass #1791

Merged
merged 2 commits into from
Apr 13, 2022

Conversation

SebastienGllmt
Copy link
Contributor

Previous behavior

When parsing pgpass files, the BufReader's read_line function is used which does not strip \n or \r. To handle this, a substring was created that strips out the last character (&line[..line.len() - 1])

Why is this an issue

This behavior fails in two cases:

  1. If the file uses \r\n instead of just \n
  2. If the file has no trailing newline

Case (2) is subtle because it means if you modify your pgpass to add a new entry and forget to add a trailing newline to your file, [..line.len() - 1] will cut off the last letter of your password (ex: hunter12 -> hunter1) giving you an authentication error.

How can we fix it?

Two options:

  1. Similar to my PR, we manually strip the newline characters from the end of the string
  2. We switch from the read_line to the lines method which strips the newline characters for you (I don't think this is a performance concern because pgpass files are presumably not large files)

Tests

I noticed there weren't any tests for load_password_from_file -- only for load_password_from_line. I assume it's because setup for a test on file reading is tedious and any issue would get caught integration tests. However, this means I'm not sure what you suggest for a test for this specific case. Please let me know if you're okay without a test or if you have a specific way you'd like me to set it up (or push to my branch yourself)

@abonander
Copy link
Collaborator

abonander commented Apr 11, 2022

If you want to generate a file and check it in to tests/postgres/, that sounds fine. The login credentials (for testing) are hardcoded as postgres/password.

@abonander
Copy link
Collaborator

Just wrote a unit test combining the other test cases into a single "file", although it required a bit of modification.

@abonander abonander merged commit 973f3d1 into launchbadge:master Apr 13, 2022
@SebastienGllmt SebastienGllmt deleted the pgpass-newline branch April 14, 2022 00:00
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.

2 participants