fix: URL-encode special characters in connection string passwords#597
fix: URL-encode special characters in connection string passwords#597VJ-yadav wants to merge 2 commits intoAltimateAI:mainfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 53 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic Password | d4a35d0 | packages/opencode/test/altimate/driver-normalize.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/drivers/src/normalize.ts (1)
161-163: PotentialURIErrorif username contains malformed percent sequences.
decodeURIComponent(user)will throw aURIErrorif the username contains malformed percent-encoded sequences (e.g.,user%GGname). While rare, this could cause unexpected failures.Consider wrapping in try-catch or skipping the decode/re-encode cycle for the username when it doesn't contain
%:🛡️ Proposed defensive fix
- // Re-encode both user and password to be safe - const encodedUser = encodeURIComponent(decodeURIComponent(user)) + // Re-encode both user and password to be safe. + // If user contains %, attempt decode; otherwise encode directly. + let encodedUser: string + try { + encodedUser = encodeURIComponent(decodeURIComponent(user)) + } catch { + // Malformed percent sequence — encode as-is + encodedUser = encodeURIComponent(user) + } const encodedPassword = encodeURIComponent(password)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/drivers/src/normalize.ts` around lines 161 - 163, The decode/re-encode of the username using decodeURIComponent(user) can throw a URIError for malformed percent sequences; update the logic around encodedUser (where decodeURIComponent(user) and encodeURIComponent are used) to first check if user contains '%' and only perform decode/encode when safe, or wrap decodeURIComponent(user) in a try-catch and fall back to using the original user string if decoding fails, ensuring encodedUser is always set to a valid encodeURIComponent value without throwing.packages/opencode/test/altimate/driver-normalize.test.ts (1)
974-978: Consider adding test coverage for/and?in passwords.The PR objectives mention handling
/as a special character, and the implementation's regex also includes?. Adding explicit tests would strengthen coverage:📝 Suggested additional tests
test("encodes / in password", () => { const input = "postgresql://admin:pass/word@localhost/db" const result = sanitizeConnectionString(input) expect(result).toBe("postgresql://admin:pass%2Fword@localhost/db") }) test("encodes ? in password", () => { const input = "postgresql://admin:pass?word@localhost/db" const result = sanitizeConnectionString(input) expect(result).toBe("postgresql://admin:pass%3Fword@localhost/db") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/driver-normalize.test.ts` around lines 974 - 978, Add two unit tests to the existing "encodes multiple special characters" suite that verify sanitizeConnectionString correctly percent-encodes '/' and '?' inside the password portion: create one test using input "postgresql://admin:pass/word@localhost/db" expecting "postgresql://admin:pass%2Fword@localhost/db" and another using "postgresql://admin:pass?word@localhost/db" expecting "postgresql://admin:pass%3Fword@localhost/db"; place these tests alongside the existing tests in driver-normalize.test.ts to ensure sanitizeConnectionString handles both characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/drivers/src/normalize.ts`:
- Around line 161-163: The decode/re-encode of the username using
decodeURIComponent(user) can throw a URIError for malformed percent sequences;
update the logic around encodedUser (where decodeURIComponent(user) and
encodeURIComponent are used) to first check if user contains '%' and only
perform decode/encode when safe, or wrap decodeURIComponent(user) in a try-catch
and fall back to using the original user string if decoding fails, ensuring
encodedUser is always set to a valid encodeURIComponent value without throwing.
In `@packages/opencode/test/altimate/driver-normalize.test.ts`:
- Around line 974-978: Add two unit tests to the existing "encodes multiple
special characters" suite that verify sanitizeConnectionString correctly
percent-encodes '/' and '?' inside the password portion: create one test using
input "postgresql://admin:pass/word@localhost/db" expecting
"postgresql://admin:pass%2Fword@localhost/db" and another using
"postgresql://admin:pass?word@localhost/db" expecting
"postgresql://admin:pass%3Fword@localhost/db"; place these tests alongside the
existing tests in driver-normalize.test.ts to ensure sanitizeConnectionString
handles both characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 44b3dc65-0c0f-455e-ab26-0eacd895c951
📒 Files selected for processing (3)
packages/drivers/src/index.tspackages/drivers/src/normalize.tspackages/opencode/test/altimate/driver-normalize.test.ts
Wire the existing (but uncalled) sanitizeConnectionString() into normalizeConfig() so all drivers using connection_string get automatic URL-encoding of special characters in passwords. Changes: - Fix regex to split on last @ (not first) so passwords containing @ are handled correctly - Add try-catch around decodeURIComponent for malformed percent sequences - Wire sanitizeConnectionString into normalizeConfig after alias resolution - Re-export sanitizeConnectionString from drivers package - 19 tests covering @, #, :, /, ? in passwords, already-encoded values, malformed URIs, mongodb schemes, and normalizeConfig integration Fixes AltimateAI#589 Co-Authored-By: Vijay Yadav <[email protected]>
VJ-yadav
force-pushed
the
fix/special-char-passwords
branch
from
ad39640 to
d4a35d0
Compare
1 participant
Summary
Fixes #589 — stored passwords with special characters (
@,#,:,/, etc.) cause cryptic authentication failures when used in URI-style connection strings.sanitizeConnectionString()existed innormalize.tsbut was never called — connection strings with unencoded passwords were passed directly to drivers@instead of the last, so passwords containing@were silently mis-parsed (the function returned early thinking no encoding was needed)sanitizeConnectionString()intonormalizeConfig()so every driver that accepts aconnection_stringbenefits automatically, and fix the regex to use greedy matching for the userinfo portionDrivers using individual config fields (host, user, password as separate values) are unaffected — native driver libraries handle raw passwords correctly without URI encoding.
Test Plan
sanitizeConnectionStringdirectly andnormalizeConfigintegration@,#,:are correctly percent-encoded%40,%23) are left untouchedmongodb://,mongodb+srv://,postgresql://schemes all handledconnection_string(individual fields) is not alteredconnectionStringalias resolves toconnection_stringbefore sanitizationChecklist
tsgoerror on@clickhouse/clientimport is unrelated (reproduces onmain)Summary by CodeRabbit
Release Notes