fix(js): escape dbname in lib/pq URL - #7479
Conversation
`PGClient.ExecuteQuery` built the connection URL by interpolating the dbname directly into the path. A value containing '?' could start the query string and inject lib/pq options such as `sslrootcert` before the appended `sslmode=disable`. Build the URL from escaped userinfo, path and query values so dbname stays part of the dbname. Signed-off-by: Dwi Siswanto <git@dw1.io>
Walkthrough
ChangesPostgres connection URL encoding
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/js/libs/postgres/postgres.go (1)
149-160: ⚡ Quick winConsider using
url.URLstruct for safer URL construction.While the current implementation correctly escapes credentials, dbName, and query parameters, building the URL via
fmt.Sprintfis less idiomatic and potentially error-prone. Thetargetparameter is inserted directly without validation, which could cause URL parsing issues if thehostcomponent contains reserved URL characters like@,/, or?.Using the
url.URLstruct would be cleaner and more robust:♻️ Proposed refactor using url.URL struct
func buildPostgresConnURL(username, password, target, dbName, executionId string) string { - values := url.Values{} - values.Set("sslmode", "disable") - values.Set("executionId", executionId) - - return fmt.Sprintf("postgres://%s@%s/%s?%s", - url.UserPassword(username, password).String(), - target, - url.PathEscape(dbName), - values.Encode(), - ) + u := &url.URL{ + Scheme: "postgres", + User: url.UserPassword(username, password), + Host: target, + Path: "/" + dbName, + } + q := u.Query() + q.Set("sslmode", "disable") + q.Set("executionId", executionId) + u.RawQuery = q.Encode() + return u.String() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/js/libs/postgres/postgres.go` around lines 149 - 160, The buildPostgresConnURL function currently uses fmt.Sprintf to construct the PostgreSQL connection URL, which leaves the target parameter unvalidated and could cause URL parsing issues if it contains reserved characters like @, /, or ?. Refactor this function to use the url.URL struct instead of string formatting. Create a url.URL with the appropriate scheme (postgres), user credentials using url.UserPassword, host set to the target parameter, path set to the database name, and raw query containing the query parameters. This approach leverages Go's standard library for safer and more idiomatic URL construction.pkg/js/libs/postgres/postgres_test.go (1)
1-74: ⚡ Quick winConsider adding test coverage for edge cases in the
hostparameter.The existing tests thoroughly validate dbName injection prevention and credential escaping. Consider adding a test that verifies the behavior when the
hostparameter contains URL-reserved characters (e.g.,@,/,?,#) to ensure such inputs are handled safely or produce clear errors.📝 Example test case
func TestBuildPostgresConnectionURLWithMaliciousHost(t *testing.T) { maliciousHosts := []string{ "evil@real.com:5432", "host/path:5432", "host?query:5432", } for _, target := range maliciousHosts { connStr := buildPostgresConnURL("user", "pass", target, "db", "exec-1") u, err := url.Parse(connStr) if err != nil { t.Logf("Malicious target %q correctly rejected: %v", target, err) continue } // Verify the host is interpreted as expected if u.Host != target { t.Errorf("Host mismatch: got %q, want %q", u.Host, target) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/js/libs/postgres/postgres_test.go` around lines 1 - 74, Add a new test function after TestBuildPostgresConnectionURLEscapesCredentials that validates the behavior of buildPostgresConnURL when the host parameter contains URL-reserved characters such as @, /, ?, or #. The test should create multiple test cases with malicious host inputs (e.g., evil@real.com:5432, host/path:5432, host?query:5432) and verify that either the resulting connection URL safely handles these characters or the parsing fails gracefully. Use url.Parse to validate the generated connection string and verify that the host is either properly escaped or that an appropriate error is returned to prevent host injection attacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/js/libs/postgres/postgres.go`:
- Around line 149-160: The buildPostgresConnURL function inserts the target
parameter directly into the connection URL string without escaping or validating
it for URL-reserved characters like @, /, or ?. These characters in the hostname
could break the URL structure and create security issues. Apply proper URL
escaping to the target parameter (similar to how dbName is escaped with
url.PathEscape) or add validation to ensure the target parameter does not
contain URL-reserved characters that would compromise URL parsing. Additionally,
add a test case that verifies the function properly handles or rejects inputs
with URL-reserved characters in the target parameter to prevent regression.
---
Nitpick comments:
In `@pkg/js/libs/postgres/postgres_test.go`:
- Around line 1-74: Add a new test function after
TestBuildPostgresConnectionURLEscapesCredentials that validates the behavior of
buildPostgresConnURL when the host parameter contains URL-reserved characters
such as @, /, ?, or #. The test should create multiple test cases with malicious
host inputs (e.g., evil@real.com:5432, host/path:5432, host?query:5432) and
verify that either the resulting connection URL safely handles these characters
or the parsing fails gracefully. Use url.Parse to validate the generated
connection string and verify that the host is either properly escaped or that an
appropriate error is returned to prevent host injection attacks.
In `@pkg/js/libs/postgres/postgres.go`:
- Around line 149-160: The buildPostgresConnURL function currently uses
fmt.Sprintf to construct the PostgreSQL connection URL, which leaves the target
parameter unvalidated and could cause URL parsing issues if it contains reserved
characters like @, /, or ?. Refactor this function to use the url.URL struct
instead of string formatting. Create a url.URL with the appropriate scheme
(postgres), user credentials using url.UserPassword, host set to the target
parameter, path set to the database name, and raw query containing the query
parameters. This approach leverages Go's standard library for safer and more
idiomatic URL construction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c95abcf-c4b3-4b39-85ae-76785964747d
📒 Files selected for processing (2)
pkg/js/libs/postgres/postgres.gopkg/js/libs/postgres/postgres_test.go
| func buildPostgresConnURL(username, password, target, dbName, executionId string) string { | ||
| values := url.Values{} | ||
| values.Set("sslmode", "disable") | ||
| values.Set("executionId", executionId) | ||
|
|
||
| return fmt.Sprintf("postgres://%s@%s/%s?%s", | ||
| url.UserPassword(username, password).String(), | ||
| target, | ||
| url.PathEscape(dbName), | ||
| values.Encode(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for host validation in callers and related code
# Search for host validation logic
rg -nP -C3 '\bIsHostAllowed\b' --type=go
# Find callers of ExecuteQuery to see where host comes from
rg -nP -C3 '\bExecuteQuery\s*\(' --type=go -g '!*_test.go'
# Check if there's any host validation/sanitization
rg -nP '\bhost\b.*\bvalidat' --type=go
rg -nP '\bsanitize.*\bhost\b' --type=goRepository: projectdiscovery/nuclei
Length of output: 27435
🏁 Script executed:
sed -n '100,165p' pkg/js/libs/postgres/postgres.go | cat -nRepository: projectdiscovery/nuclei
Length of output: 2644
🏁 Script executed:
# Find all callers of buildPostgresConnURL
rg -n 'buildPostgresConnURL' pkg/js/libs/postgres/postgres.go --type=go
# Get the full IsHostAllowed implementation
sed -n '120,140p' pkg/protocols/common/protocolstate/headless.go | cat -nRepository: projectdiscovery/nuclei
Length of output: 856
🏁 Script executed:
sed -n '123,180p' pkg/protocols/common/protocolstate/headless.go | cat -nRepository: projectdiscovery/nuclei
Length of output: 923
🏁 Script executed:
rg -n 'buildPostgresConnURL' --type=goRepository: projectdiscovery/nuclei
Length of output: 575
🏁 Script executed:
# Check the test cases
cat -n pkg/js/libs/postgres/postgres_test.go
# Search for ValidateHost implementation
rg -n 'func.*ValidateHost' --type=go -A 10Repository: projectdiscovery/nuclei
Length of output: 2814
🏁 Script executed:
# Search for ValidateHost implementation
rg -n 'ValidateHost' --type=go -B 2 -A 15 | head -60Repository: projectdiscovery/nuclei
Length of output: 2132
🏁 Script executed:
# Search for the ValidateHost method definition
rg -n 'func.*ValidateHost' --type=goRepository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
# Find the NetworkPolicy type definition
rg -n 'type NetworkPolicy' --type=go -A 20Repository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
# Search for ValidateHost across the codebase
rg 'ValidateHost' --type=go -lRepository: projectdiscovery/nuclei
Length of output: 114
🏁 Script executed:
# Check imports in headless.go to find where ValidateHost comes from
head -30 pkg/protocols/common/protocolstate/headless.go | cat -n
# Search for ValidateHost in all files (not just Go)
rg 'ValidateHost' -B 5 -A 15 | head -100Repository: projectdiscovery/nuclei
Length of output: 3437
🏁 Script executed:
# Check the networkpolicy package to understand what it validates
# Search for any documentation or tests about URL-reserved characters
rg 'url.*reserved|reserved.*char' --type=go -i
# Check if there are any other tests for the postgres URL building
cat -n pkg/js/libs/postgres/postgres_test.go | tail -40Repository: projectdiscovery/nuclei
Length of output: 1585
🏁 Script executed:
# Look for any test that validates host with special characters
rg 'host.*[`@/`?#]|[`@/`?#].*host' pkg/js/libs/postgres/ --type=go
# Check if there's a test case for URL-reserved characters in host
rg 'TestBuild.*Host|host.*special|special.*host' pkg/js/libs/postgres/ --type=go -i
# Let's verify that the issue exists by checking if host could contain problematic chars
# Check the exact flow once more - what does net.JoinHostPort do with special chars
python3 << 'PY'
import subprocess
import json
# Check Go documentation behavior - let's create a simple test
test_code = '''package main
import (
"fmt"
"net"
)
func main() {
// Test with @ in host
target := net.JoinHostPort("user@host", "5432")
fmt.Println("JoinHostPort result:", target)
}
'''
with open('/tmp/test_joinhost.go', 'w') as f:
f.write(test_code)
result = subprocess.run(['go', 'run', '/tmp/test_joinhost.go'], capture_output=True, text=True, cwd='/tmp')
print("Output:", result.stdout)
print("Error:", result.stderr)
PYRepository: projectdiscovery/nuclei
Length of output: 400
🏁 Script executed:
python3 << 'PY'
from urllib.parse import urlparse
# Test 1: @ in host
url1 = "postgres://user:pass@user@host:5432/dbname?sslmode=disable"
parsed1 = urlparse(url1)
print("Test 1: @ in host")
print(f" URL: {url1}")
print(f" Parsed netloc: {parsed1.netloc}")
print(f" Parsed hostname: {parsed1.hostname}")
print(f" Parsed port: {parsed1.port}")
print()
# Test 2: / in host
url2 = "postgres://user:pass@host/evil:5432/dbname?sslmode=disable"
parsed2 = urlparse(url2)
print("Test 2: / in host")
print(f" URL: {url2}")
print(f" Parsed netloc: {parsed2.netloc}")
print(f" Parsed path: {parsed2.path}")
print()
# Test 3: ? in host
url3 = "postgres://user:pass@host?rogue:5432/dbname?sslmode=disable"
parsed3 = urlparse(url3)
print("Test 3: ? in host")
print(f" URL: {url3}")
print(f" Parsed netloc: {parsed3.netloc}")
print(f" Parsed query: {parsed3.query}")
PYRepository: projectdiscovery/nuclei
Length of output: 555
🏁 Script executed:
# Let's check one more thing - whether there are any other safeguards
# Check if there's any URL validation before the connection
rg -n 'url\.Parse|ParseURL' pkg/js/libs/postgres/postgres.go --type=go -B 2 -A 2Repository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
# Let's verify if there's any hostname validation that rejects URL-reserved characters
# Check what networkpolicy ValidateHost actually does - see if it's documented in imports
grep -r 'ValidateHost' --include='*.go' | grep -v node_modules | head -20Repository: projectdiscovery/nuclei
Length of output: 323
Add validation or escaping for target parameter containing URL-reserved characters.
The target parameter is not protected against URL-reserved characters like @, /, or ?. While IsHostAllowed provides network policy validation, it does not validate URL safety. If a host contains these characters (e.g., attacker@malicious.com), they will be inserted into the PostgreSQL connection URL without escaping, potentially breaking the URL structure. For example, @ in the host would confuse URL parsing regarding credentials vs. host identity.
No test case currently validates behavior with URL-reserved characters in the host parameter. Add a test case verifying that such inputs are either rejected or safely handled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/js/libs/postgres/postgres.go` around lines 149 - 160, The
buildPostgresConnURL function inserts the target parameter directly into the
connection URL string without escaping or validating it for URL-reserved
characters like @, /, or ?. These characters in the hostname could break the URL
structure and create security issues. Apply proper URL escaping to the target
parameter (similar to how dbName is escaped with url.PathEscape) or add
validation to ensure the target parameter does not contain URL-reserved
characters that would compromise URL parsing. Additionally, add a test case that
verifies the function properly handles or rejects inputs with URL-reserved
characters in the target parameter to prevent regression.
|
@coderabbitai create an issue for this, prefixed with |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh issue create \
--repo "projectdiscovery/nuclei" \
--title "[BUG] PostgreSQL JS Library: DBName Injection via Unescaped URL Path in PGClient.ExecuteQuery" \
--assignee "dwisiswant0" \
--body "## Description
The \`PGClient.ExecuteQuery\` method in the JavaScript library for PostgreSQL connections (\`pkg/js/libs/postgres/postgres.go\`) is vulnerable to connection-string injection via an unsanitized \`dbname\` parameter.
## Root Cause
The database name (\`dbname\`) was directly interpolated into the \`postgres://\` connection URL path using \`fmt.Sprintf\` without any URL-path escaping. A \`dbname\` value containing a \`?\` character would be treated as the start of a query string by the URL parser, allowing an attacker-controlled value to inject arbitrary \`lib/pq\` DSN options (e.g., \`sslrootcert\`, \`sslcert\`, \`sslkey\`) ahead of the intended \`sslmode=disable\` parameter.
## Impact
An attacker who can influence the \`dbname\` value passed to \`PGClient.ExecuteQuery\` could override TLS/SSL-related connection options, potentially bypassing certificate validation or redirecting TLS verification to an attacker-controlled certificate authority.
## References
- Fix: `#7479`
- Reported by: `@dwisiswant0`"Length of output: 1366
|
2 participants
Proposed changes
PGClient.ExecuteQuerybuilt the connection URLby interpolating the dbname directly into the path.
A value containing '?' could start the query string
and inject lib/pq options such as
sslrootcertbefore the appended
sslmode=disable.Build the URL from escaped userinfo, path and query
values so dbname stays part of the dbname.
Proof
Checklist
Summary by CodeRabbit
Refactor
Tests