Fix percent-encoded database names in connection URL paths - #977
Fix percent-encoded database names in connection URL paths#977fallintoplace wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix double-encoded database names when a database is provided via the URL path (e.g. /my%20database previously becoming database=my%2520database), by decoding the path segment during URL config parsing and adding regression tests.
Changes:
- Decode
url.pathname(minus the leading/) when mapping it toconfig.database. - Add unit regression cases for spaces, Unicode, and literal
%sequences in database names. - Add a changelog entry describing the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/client-common/src/config.ts | Decodes the database name extracted from the URL path. |
| packages/client-common/tests/unit/config.test.ts | Adds parameterized tests verifying correct decoding behavior. |
| packages/client-common/CHANGELOG.md | Documents the bug fix in the changelog. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
packages/client-common/src/config.ts:502
decodeURIComponent(url.pathname.slice(1))can throw aURIErroron malformed percent-encoding (e.g. stray%), which would currently bubble up without any ClickHouse-specific context. Consider catching decode failures and rethrowing a clear error so config parsing fails deterministically and with an actionable message.
if (url.pathname.trim().length > 1) {
config.database = decodeURIComponent(url.pathname.slice(1));
}
packages/client-common/src/config.ts:502
- This change modifies code under
packages/client-common, but the PR only updates the Node/Web package changelogs. If@clickhouse/client-commonis still published from this repo (it isprivate: false), it likely also needs a changelog entry (or a note in the PR description clarifying why it is intentionally excluded despite code changes).
if (url.pathname.trim().length > 1) {
config.database = decodeURIComponent(url.pathname.slice(1));
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
packages/client-common/src/config.ts:502
decodeURIComponent(url.pathname.slice(1))can throw aURIError(e.g. when the URL contains a bare%or otherwise-invalid percent-encoding). SinceprepareConfigWithURLdoesn’t catch that, this change can surface an opaqueURIError: URI malformedto users when the database name comes from the URL path. Consider catching decode failures here and rethrowing a ClickHouse-specific error message (with the original error ascause) so mis-encoded URLs fail with a clear diagnosis.
if (url.pathname.trim().length > 1) {
config.database = decodeURIComponent(url.pathname.slice(1));
}
b3c54c5 to
2f2b6b6
Compare
2f2b6b6 to
1d94617
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/client-common/src/config.ts:502
decodeURIComponentcan throwURIError: URI malformedif the path contains an invalid percent-escape sequence. With this change, that error would bubble up without context (unlikecreateUrl, which wraps malformed URLs). Consider wrapping the decode in a try/catch and throwing a ClickHouse-specific error message so users can diagnose bad URLs more easily.
if (url.pathname.trim().length > 1) {
config.database = decodeURIComponent(url.pathname.slice(1));
}
Problem
The WHATWG
URLAPI exposespathnamein percent-encoded form. The client copied the database path directly intoconfig.database, thenURLSearchParamsencoded its percent signs again when building a request:Spaces, Unicode characters, and literal percent signs were therefore sent with the wrong database name.
Fix
Decode the database path when converting it from its URL representation into configuration, consistent with the existing username and password handling. Request serialization then performs the single required encoding pass.
Only database names parsed from URL paths are decoded. Values supplied directly through the
databaseoption retain their existing semantics; a regression test pins that boundary.The Node.js and Web clients both use the shared source through their
src/commonsymlinks, and both unit suites run these tests. Their changelogs document the fix.Validation
npm run test:node:unit(412 passed, 4 skipped)npm run test:web:unit(261 passed)npm run buildnpm run typechecknpm run lintnpm run prettier:check