fix(cli): skip URL validation for sqlserver protocol in Studio - #29623
Conversation
MSSQL connection strings use semicolons as parameter delimiters instead of standard WHATWG URL query parameters, causing URL.canParse() to reject valid MSSQL URLs with a misleading 'not valid' error. Extract the protocol before URL validation and skip the check for sqlserver:// URLs, so they reach the existing 'not supported' error message instead. Fixes prisma#29620
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
Walkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
daltino
left a comment
daltino
left a comment
There was a problem hiding this comment.
Review: fix(cli): skip URL validation for sqlserver protocol in Studio
Nice fix for a real pain point — MSSQL connection strings like sqlserver://server;database=mydb;user=sa;password=secret are genuinely not valid WHATWG URLs, so skipping URL.canParse for them makes sense. The linked issue and inline comment are appreciated. Here are some suggestions to tighten it up before merging.
🐛 Correctness / Edge Cases
1. Protocol extraction is duplicated and brittle
The patch shows the protocol variable is extracted early (before validation), but the original code also had const protocol = new URL(connec... (truncated in the diff). This means there are now likely two protocol declarations in the function, which would be a compile/runtime error. Please verify the full diff removes or consolidates the second one.
2. split('://') edge cases
const protocol = connectionString.split('://')[0]?.toLowerCase() ?? ''- If
connectionStringis an empty string or contains no://,splitstill returns a one-element array, so[0]is''— that's fine. - If
connectionStringis something malformed like://foo,[0]is''— also fine, falls through toURL.canParse. - However, if the input is
nullorundefined(possible if the env var is unset), this will throw before the existing guard. Double-check thatconnectionStringis guaranteed to be a string at this point.
3. Consider sqlserver variants
The MSSQL connector in Prisma also accepts jdbc:sqlserver://... in some contexts. It's worth confirming whether that variant can ever reach this code path, or adding a note that it's intentionally excluded.
🧪 Test Coverage
There don't appear to be any new tests added. At minimum, consider adding a unit/integration test in the Studio test suite for:
- A valid
sqlserver://string → should not returnUserFacingError - An invalid non-sqlserver URL → should still return
UserFacingError - A valid
postgresql://URL → unchanged behaviour
Even a simple snapshot or assertion test would prevent regression here.
💅 Code Style
4. Minor: ?? '' is redundant
const protocol = connectionString.split('://')[0]?.toLowerCase() ?? ''String.prototype.split always returns a non-empty array, so [0] is never undefined. The optional chaining (?.) and nullish coalescing (?? '') are unnecessary noise. This works fine:
const protocol = connectionString.split('://')[0].toLowerCase()5. Consider a named constant or helper
If other protocols are ever added to this bypass list (e.g., some future connector), a small set or array would be cleaner and more readable:
const PROTOCOLS_SKIPPING_URL_VALIDATION = new Set(['sqlserver'])
const protocol = connectionString.split('://')[0].toLowerCase()
if (!PROTOCOLS_SKIPPING_URL_VALIDATION.has(protocol) && !URL.canParse(connectionString)) {
return new UserFacingError('The provided database URL is not valid.')
}Not required for this PR, but worth a thought.
⚡ Performance
No concerns — this is a one-time startup check, not a hot path.
Summary
| Area | Status |
|---|---|
| Core logic | ✅ Correct approach |
Duplicate protocol var |
|
| Test coverage | ❌ Missing |
| Code style | 🟡 Minor cleanup suggested |
The fix direction is right. The main asks before merging are: (1) confirm there's no duplicate protocol variable in the final file, and (2) add at least one test case for the sqlserver:// path. Great work tracking this down!
- Add tests for sqlserver://, invalid, and valid URLs - Use module-level Set for protocols that skip URL validation - Remove redundant optional chaining and nullish coalescing
|
Thanks for the review @daltino! I've pushed a follow-up commit addressing the feedback:
Thanks again for the suggestions! |
…tests UserFacingError wraps its message with a leading newline, a bold red '!' prefix, and ANSI color codes, so the exact toBe() assertions on the plain message never matched and the two Studio URL-validation tests failed in CI. Assert on error name plus a substring of the message instead, and collapse the multi-line expect() and mysql arg array to single lines to satisfy prettier.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/__tests__/Studio.vitest.ts (1)
129-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the MySQL URL is forwarded unchanged.
The test name promises unchanged forwarding, but checking only the call count would still pass if
Studio.parserewrote or replaced the connection string. Assert the mock’s URL argument equals the exact value supplied at Line 125.🤖 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 `@packages/cli/src/__tests__/Studio.vitest.ts` around lines 129 - 130, Update the test around Studio.parse to assert that createPoolMock received the exact MySQL URL supplied in the test setup, preserving it unchanged rather than only verifying invocation count. Keep the existing call-count assertion and add an argument-level assertion against the mock’s URL parameter.
🤖 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.
Outside diff comments:
In `@packages/cli/src/__tests__/Studio.vitest.ts`:
- Around line 129-130: Update the test around Studio.parse to assert that
createPoolMock received the exact MySQL URL supplied in the test setup,
preserving it unchanged rather than only verifying invocation count. Keep the
existing call-count assertion and add an argument-level assertion against the
mock’s URL parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 964091c8-d708-4015-aa6b-f3dec387fc7c
📒 Files selected for processing (1)
packages/cli/src/__tests__/Studio.vitest.ts
|
Pushed a fix for the failing The two Studio URL-validation tests failed because Verified locally (targeted, no full build):
Also merged the latest |
|
Thanks for the reviews! Here's what was addressed: @daltino's review:
@aqrln's "Tests are failing on CI":
@coderabbitai's suggestion (line 129-130):
All tests pass: |
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
…a#29623) Fixes prisma#29620 I traced the Prisma Studio URL validation issue on the current `main` branch: In `packages/cli/src/Studio.ts` (Line 281), `URL.canParse(connectionString)` rejects MSSQL URLs because they use semicolons (`;`) as parameter delimiters instead of the standard URL query parameters. The fix extracts the protocol before performing the URL check and skips validation for `sqlserver://`. This ensures that users see the correct "not supported" message instead of a misleading "not valid" error. --------- Co-authored-by: Oleksii Orlenko <alex@aqrln.net> Co-authored-by: Alexey Orlenko's AI Agent <robot@aqrln.net> Co-authored-by: sijie-Z <liuweiqing147@gmail.com>
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@prisma/adapter-pg](https://github.com/prisma/prisma) ([source](https://github.com/prisma/prisma/tree/HEAD/packages/adapter-pg)) | imports | minor | [`7.9.1` -> `7.10.0`](https://renovatebot.com/diffs/npm/@prisma%2fadapter-pg/7.9.1/7.10.0) | | [@prisma/client](https://www.prisma.io) ([source](https://github.com/prisma/prisma/tree/HEAD/packages/client)) | imports | minor | [`7.9.1` -> `7.10.0`](https://renovatebot.com/diffs/npm/@prisma%2fclient/7.9.1/7.10.0) | --- ### Release Notes <details> <summary>prisma/prisma (@​prisma/adapter-pg)</summary> ### [`v7.10.0`](https://github.com/prisma/orm/releases/tag/7.10.0) [Compare Source](prisma/orm@7.9.1...7.10.0) ##### Prisma ORM 7.10.0 Prisma ORM 7.10.0 introduces a compatibility package for running Prisma 7 alongside newer Prisma versions, secures Prisma Studio's local server, and includes fixes across Prisma Client and the PostgreSQL, MariaDB, Neon, SQLite, and Prisma Postgres Serverless adapters. ##### Highlights ##### Run Prisma 7 alongside Prisma 8 This release introduces `@prisma/prisma7`, a compatibility package that lets you retain a matching Prisma 7 CLI and configuration while installing Prisma 8 in the same project. Once 7.10.0 is released, a side-by-side installation can use: ```sh npm install --save-dev prisma@8 @prisma/prisma7@7.10.0 npm install @prisma/client@7.10.0 ``` Use `prisma` for the directly installed Prisma 8 CLI and `prisma7` for Prisma 7: ```sh npx prisma --version npx prisma7 --version npx prisma7 generate npx prisma7 migrate dev npx prisma7 db push ``` Prisma 7 now prefers version-specific configuration files, allowing its configuration to coexist with Prisma 8's `prisma.config.*` files: ```ts // prisma7.config.ts import { defineConfig } from '@prisma/prisma7/config' export default defineConfig({ schema: 'prisma/schema.prisma', migrations: { path: 'prisma/migrations', }, }) ``` Without an explicit `--config` option, Prisma 7 searches for: 1. Root-level `prisma7.config.*` files. 2. `.config/prisma7.*` files. 3. Existing `prisma.config.*` files as a backwards-compatible fallback. The supported extensions are `.js`, `.ts`, `.mjs`, `.cjs`, `.mts`, and `.cts`. An explicit config path always takes precedence: ```sh npx prisma7 generate --config ./custom/prisma7.config.ts ``` New projects initialized by the Prisma 7 CLI use `prisma7.config.ts`. Existing projects containing only `prisma.config.*` continue to work without migration or additional warnings. If a `prisma7.config.*` file exists but cannot be loaded, Prisma reports the error rather than silently falling back to another configuration. The `prisma7` identity is carried through CLI help, version output, shell completion, initialization, migration, database, and generation guidance. Stable Prisma concepts such as `schema.prisma`, Prisma Migrate, `@prisma/client`, and `PRISMA_*` environment variables remain unchanged. Together, the separate executable and configuration namespace make it possible to operate Prisma 7 and Prisma 8 side by side without command or config-file collisions. [#​29949](prisma/orm#29949), [#​29969](prisma/orm#29969), [#​29994](prisma/orm#29994), [#​30000](prisma/orm#30000), [#​30002](prisma/orm#30002), [#​30020](prisma/orm#30020) ##### Prisma Studio security hardening Prisma Studio's local HTTP server now: - Binds explicitly to `127.0.0.1` instead of all network interfaces. - Rejects browser requests from origins other than the active `localhost` or `127.0.0.1` Studio URL. - No longer returns wildcard CORS headers. - Applies the same protections across Node.js, Bun, and Deno. This prevents network clients or malicious websites from accessing Studio's database endpoints while Studio is running. [#​29890](prisma/orm#29890) ##### Prisma Client - Fixed `P2002` errors from nested writes so `meta.modelName` identifies the model where the unique constraint violation occurred, including models using `@@map` and `@@schema`. [#​29628](prisma/orm#29628) - Fixed automatically batched `findUniqueOrThrow()` calls so every missing record rejects with `P2025`; later misses no longer resolve to `undefined`. [#​29654](prisma/orm#29654) - Parameter-chunked statements are now executed atomically in a transaction and rolled back if a later chunk fails. [#​29771](prisma/orm#29771) - Improved interactive transaction cleanup during `$disconnect()`, including transactions whose driver-level startup is still in progress. [#​28768](prisma/orm#28768) - Prevented transaction cleanup failures after a timeout or backend termination from becoming unhandled promise rejections. [#​29611](prisma/orm#29611) - Fixed fluent relation queries when relation fields are literally named `select` or `include`. [#​29683](prisma/orm#29683) - Fixed handling of `Date` and `Uint8Array` values created in other JavaScript realms, such as iframes, jsdom, and Node.js `vm` contexts. [#​29177](prisma/orm#29177) - Invalid `Date` values passed to `$queryRaw` or `$executeRaw` now throw `PrismaClientValidationError` instead of a generic error. [#​29718](prisma/orm#29718) - Fixed `moduleFormat` inference for the `prisma-client` generator in TypeScript projects using `module: "node16"` or `"nodenext"`. Generated output now follows the nearest `package.json` `type`, defaulting to CommonJS when absent. [#​29712](prisma/orm#29712) - Deserialized `Bytes` values now own standalone `ArrayBuffer`s rather than exposing unrelated contents from Node.js's shared `Buffer` pool. This applies to both regular and raw query results. [#​29701](prisma/orm#29701) - Fixed an incorrect logging context in the remote executor, including Accelerate-backed query execution. [#​28892](prisma/orm#28892) ##### Client extensions and observability - Result-extension `compute` callbacks now receive the current model name as a typed second argument: ```ts compute(data, modelName) { // ... } ``` The model name is also preserved when multiple extensions compose the same computed field. [#​29782](prisma/orm#29782) - Improved OpenTelemetry context for remotely executed queries: - `$on('query')` callbacks run within the matching `db_query` span. - Events from one operation share the same trace. - Error events are recorded as span exceptions. - Log events continue to be emitted when tracing is disabled or their reported span is unavailable. [#​28892](prisma/orm#28892) ##### Driver adapters ##### MariaDB - `@prisma/adapter-mariadb` now accepts an existing `mariadb` pool. External pools remain caller-owned unless `disposeExternalPool: true` is supplied. [#​27992](prisma/orm#27992) - Fixed pooled connection leaks during commit, rollback, and failed transaction startup. Connections are now returned with `release()` and transaction-specific listeners are removed before reuse. [#​29612](prisma/orm#29612) - Added support for bracketed IPv6 addresses in both `mysql://` and `mariadb://` connection strings. [#​29026](prisma/orm#29026) - Prevented malformed connection strings from exposing embedded passwords in retained debug output and diagnostic reports. [#​27992](prisma/orm#27992) ##### PostgreSQL, Neon, and Prisma Postgres Serverless - PostgreSQL deadlocks using SQLSTATE `40P01` are now reported as `P2034` transaction write conflicts. [#​29717](prisma/orm#29717) - PostgreSQL `RESTRICT` violations using SQLSTATE `23001` are now reported as `P2003`, preserving an available field or constraint name. [#​29554](prisma/orm#29554) - `@prisma/adapter-pg` now preserves database constraint names when reporting unique constraint violations through `P2002`. [#​29587](prisma/orm#29587) - Prisma Postgres Serverless now prefers the named constraint for `P2002`, falling back to parsed field names when no constraint name is available. [#​29801](prisma/orm#29801) - Fixed Neon HTTP adapter serialization for typed parameters such as `Bytes` and `DateTime`. [#​29747](prisma/orm#29747) ##### SQLite - `@prisma/adapter-better-sqlite3` now converts previously unhandled SQLite result codes into typed database errors instead of exposing raw driver errors. - The complete `SQLITE_BUSY` family is now mapped to socket timeout errors, with numeric extended result codes preserved where available. [#​29794](prisma/orm#29794) ##### CLI and Migrate - `prisma generate` can now offer to install Prisma's agent skills. The opt-in prompt: - Is shown at most once per machine. - Is skipped in CI, containers, Git hooks, npm lifecycle scripts, and watch mode. - Is skipped when `--no-hints` is used or Prisma skills are already installed. - Times out after 30 seconds. - Never causes generation to fail if installation is unsuccessful. [#​29690](prisma/orm#29690) - A globally installed CLI now warns during `prisma generate` when its version differs from the project's local `prisma` or `@prisma/client`, and recommends running the local CLI. The check is best-effort and does not fail generation. [#​29593](prisma/orm#29593) - `prisma version` and `prisma version --json` now include the resolved Prisma CLI package path, making global-versus-local installation issues easier to diagnose. [#​29573](prisma/orm#29573) - Empty or generator-only schema files now report `Schema must contain a datasource block` from `db pull`, `db push`, and `migrate dev`, rather than reaching the schema engine and potentially producing inconsistent errors. [#​29657](prisma/orm#29657) - CLI commands now tolerate corrupt, unreadable, or unwritable command-state files. Invalid state is reinitialized, writes are atomic, and persistence failures fall back to in-memory state. [#​29609](prisma/orm#29609) - Studio now recognizes semicolon-delimited `sqlserver://` connection strings before reporting the existing explicit message that SQL Server is not supported by Studio. [#​29623](prisma/orm#29623) - The AI-agent safety checkpoint now also covers interactive `prisma db push` confirmations involving data-loss warnings, rather than only invocations using `--accept-data-loss`. [#​29793](prisma/orm#29793) ##### Performance and reliability - Optimized query-plan execution by eagerly evaluating plans with one unconditional database operation and synchronously interpreting the remaining pure plan. Cached plans remain immutable. [#​29004](prisma/orm#29004) - Prevented call-stack overflows when rendering very large parameter lists or combining chunked results containing hundreds of thousands of rows. [#​29751](prisma/orm#29751) - Reduced ordinary query setup overhead by constructing fluent-relation field maps lazily and in linear time. Non-fluent queries no longer build this map. [#​29752](prisma/orm#29752) ##### Dependencies - Updated the transitive `fast-uri` dependency to a patched release addressing production audit advisories affecting versions through `3.1.3`. [#​29758](prisma/orm#29758) </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate CLI](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0NC4zMC4zIiwidXBkYXRlZEluVmVyIjoiNDQuMzAuMyIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==--> Reviewed-on: https://git.oirnoir.dev/OIRNOIR/YouTube-Helper-Server/pulls/41
6 participants
Fixes #29620
I traced the Prisma Studio URL validation issue on the current
mainbranch:In
packages/cli/src/Studio.ts(Line 281),URL.canParse(connectionString)rejects MSSQL URLs because they use semicolons (;) as parameter delimiters instead of the standard URL query parameters.The fix extracts the protocol before performing the URL check and skips validation for
sqlserver://. This ensures that users see the correct "not supported" message instead of a misleading "not valid" error.