Skip to content

sqlite: throw on invalid URL path instead of abort - #66026

Open
araujogui wants to merge 2 commits into
nodejs:mainfrom
araujogui:sqlite-invalid-url-abort
Open

sqlite: throw on invalid URL path instead of abort#66026
araujogui wants to merge 2 commits into
nodejs:mainfrom
araujogui:sqlite-invalid-url-abort

Conversation

@araujogui

@araujogui araujogui commented Sep 14, 2026

Copy link
Copy Markdown
Member

ValidateDatabasePath() routed any object with a string href into URL parsing and asserted the result with CHECK(ada::can_parse(location)). The parse outcome is user input, not an invariant, so new DatabaseSync({ href: 'not a url' }) aborted the process.

Throws ERR_INVALID_URL instead, matching node_file.cc. Reachable through both DatabaseSync() and backup(); both covered by tests.

@araujogui
sqlite: throw on invalid URL path instead of abort
46f012f 
ValidateDatabasePath() treats any object exposing a string `href` as a
URL, then asserts the parse result with CHECK(ada::can_parse(location)).
Whether that string parses depends on user input rather than on an
invariant the code guarantees, so a value such as { href: 'not a url' }
aborted the process instead of throwing.

Replace the CHECK with an ERR_INVALID_URL exception, matching how
node_file.cc reports an unparsable URL. Both DatabaseSync() and backup()
validate their path through this function, so both paths are covered.

Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Assisted-by: Claude Code
Copilot AI lite review requested due to automatic review settings September 14, 2026 22:15
@nodejs-github-bot

nodejs-github-bot commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

Copilot AI reviewed Sep 14, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Sep 14, 2026
@codecov

codecov Bot commented Sep 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.24%. Comparing base (c1e2478) to head (1b77232).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #66026      +/-   ##
==========================================
+ Coverage   90.23%   90.24%   +0.01%     
==========================================
  Files         785      789       +4     
  Lines      269670   270478     +808     
  Branches    51589    51746     +157     
==========================================
+ Hits       243325   244083     +758     
- Misses      16825    16867      +42     
- Partials     9520     9528       +8
Files with missing lines Coverage Δ
src/node_sqlite.cc 82.12% <100.00%> (+0.04%) ⬆️

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 15, 2026
aduh95
aduh95 reviewed Sep 15, 2026
View reviewed changes
Comment thread test/parallel/test-sqlite-backup.mjs Outdated
aduh95
aduh95 reviewed Sep 15, 2026
View reviewed changes
Comment thread test/parallel/test-sqlite-database-sync.js Outdated
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 15, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 15, 2026
@nodejs-github-bot

nodejs-github-bot commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/77510/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jasnell jasnell jasnell approved these changes
@lpinca lpinca lpinca approved these changes
@meixg meixg meixg approved these changes
@aduh95 aduh95 aduh95 approved these changes
@geeksilva97 geeksilva97 geeksilva97 approved these changes

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@araujogui @nodejs-github-bot @jasnell @lpinca @meixg @aduh95 @geeksilva97