Skip to content

Use psycopg.conninfo.make_conninfo() to create PostgreSQL connection string#209

Open
jsf9k wants to merge 3 commits intodevelopfrom
bugfix/use-raw-string-for-postgres-connection-string
Open

Use psycopg.conninfo.make_conninfo() to create PostgreSQL connection string#209
jsf9k wants to merge 3 commits intodevelopfrom
bugfix/use-raw-string-for-postgres-connection-string

Conversation

@jsf9k
Copy link
Copy Markdown
Member

@jsf9k jsf9k commented Feb 28, 2025

🗣 Description

This pull request modifies the Python code to use psycopg.conninfo.make_conninfo() to create the PostgreSQL connection string.

💭 Motivation and context

If the PostgreSQL password, for example, contains a literal backslash (\) character then we do not want it to be interpreted as the first character in a Python string escape sequence.

It is highly unlikely that anyone would choose a host name, database name, username, or password that genuinely contains special characters like \n, \t, etc.; therefore, it should be safe to treat all backslashes in the PostgreSQL connection string as literal backslashes.

🧪 Testing

All automated tests pass.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All new and existing tests pass.
  • Bump version.

✅ Pre-merge checklist

  • Finalize version.

✅ Post-merge checklist

  • Create a release.

@jsf9k jsf9k added bug This issue or pull request addresses broken functionality version bump This issue or pull request increments the version number python Pull requests that update Python code labels Feb 28, 2025
@jsf9k jsf9k self-assigned this Feb 28, 2025
@coveralls
Copy link
Copy Markdown

coveralls commented Feb 28, 2025

Pull Request Test Coverage Report for Build 21264655225

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 8 unchanged lines in 1 file lost coverage.
  • Overall coverage remained the same at 83.946%

Files with Coverage Reduction New Missed Lines %
guacscanner.py 8 82.61%
Totals Coverage Status
Change from base Build 21194023395: 0.0%
Covered Lines: 225
Relevant Lines: 272

💛 - Coveralls

@jsf9k jsf9k marked this pull request as ready for review February 28, 2025 20:08
@jsf9k jsf9k requested a review from dv4harr10 February 28, 2025 20:08
@dv4harr10
Copy link
Copy Markdown
Contributor

Hi Shane @jsf9k , just one comment, in the src/guacscanner/guacscanner.py file the instances where cursor.execute(ENTITY_COUNT_QUERY, (entity_name, entity_type)) is used these are examples of string concatenation which is a possible vulnerability for SQL injection. What's your thoughts?

@jsf9k
Copy link
Copy Markdown
Member Author

jsf9k commented Feb 28, 2025

Hi Shane @jsf9k , just one comment, in the src/guacscanner/guacscanner.py file the instances where cursor.execute(ENTITY_COUNT_QUERY, (entity_name, entity_type)) is used these are examples of string concatenation which is a possible vulnerability for SQL injection. What's your thoughts?

In this project I think it's OK. The values being concatenated do not originate from users but from the AWS API, and the latter is a trusted source.

Copy link
Copy Markdown
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

Makes sense, but we should still test that this actually works as intended.

@jsf9k
Copy link
Copy Markdown
Member Author

jsf9k commented Mar 3, 2025

Makes sense, but we should still test that this actually works as intended.

The only real way to test is to add a backslash to the PostgreSQL password in, say, our dev-a COOL environment. I will get @dav3r's assistance with that when we finish up the current cisagov/skeleton-packer Lineage wave.

cisagovbot pushed a commit that referenced this pull request May 20, 2025
@jsf9k jsf9k force-pushed the bugfix/use-raw-string-for-postgres-connection-string branch from 519f564 to d5dd863 Compare January 22, 2026 20:58
@jsf9k jsf9k requested a review from Copilot March 2, 2026 19:05
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates PostgreSQL connection string construction to use a raw f-string, aiming to avoid accidental escape-sequence interpretation when credentials contain backslashes.

Changes:

  • Switched the DSN construction from an f-string to a raw f-string (rf"")
  • Added an explanatory comment about backslashes in passwords
Comments suppressed due to low confidence (1)

src/guacscanner/guacscanner.py:772

  • This comment states the raw string is needed to avoid Python string escape handling due to backslashes in the password, but that isn’t accurate for f-string interpolation (the password is a runtime value). Consider updating the comment to describe the actual escaping/quoting concern (conninfo parsing), or remove it if you switch to passing connection parameters instead of building a DSN string.
    # We use a raw string here since the password, in particular, could contain
    # a literal backslash.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/guacscanner/guacscanner.py Outdated
@jsf9k jsf9k changed the title Use raw string for PostgreSQL connection string Use psycopg.conninfo.make_conninfo() to create PostgreSQL connection string Apr 22, 2026
@jsf9k jsf9k requested a review from Copilot April 23, 2026 13:03
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/guacscanner/guacscanner.py
Comment thread src/guacscanner/guacscanner.py
@github-actions github-actions Bot added the test This issue or pull request adds or otherwise modifies test code label Apr 23, 2026
@jsf9k jsf9k force-pushed the bugfix/use-raw-string-for-postgres-connection-string branch from dba7b67 to c9dda8e Compare April 29, 2026 18:49
jsf9k and others added 3 commits April 29, 2026 14:49
The password, in particular, may contain backslash literals.  This
will ensure that such backslashes are not treated as the first
character in a Python escape sequence.
This avoids prematurely treating backslashes as escapes.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@jsf9k jsf9k force-pushed the bugfix/use-raw-string-for-postgres-connection-string branch from 5456213 to c9dda8e Compare April 29, 2026 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This issue or pull request addresses broken functionality python Pull requests that update Python code test This issue or pull request adds or otherwise modifies test code version bump This issue or pull request increments the version number

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants