Repository navigation
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #62957 +/- ##
==========================================
- Coverage 92.93% 90.59% -2.35%
==========================================
Files 428 799 +371
Lines 195236 278863 +83627
Branches 30176 53645 +23469
==========================================
+ Hits 181442 252622 +71180
- Misses 13467 16547 +3080
- Partials 327 9694 +9367
🚀 New features to boost your workflow:
|
8ffad93 to
877530d
Compare
RafaelGSS
left a comment
RafaelGSS
left a comment
There was a problem hiding this comment.
As long as it doesn't open precedence for reports targeting SQLite on permission model, I'm +1.
There was a problem hiding this comment.
Hmm, I think this needs to take into account https://sqlite.org/uri.html
- A database can be opened read-only with
mode=roorimmutable=1. - Another way to create an in-memory database is with
mode=memory.
Doing more string processing on the database path sounds a bit hacky though.
Edit: looks like we have not enabled SQLITE_USE_URI=1, but it would be good to leave a comment here that this needs to be updated when we do at the very least.
As long as it doesn't open precedence for reports targeting SQLite on permission model, I'm +1.
Passing a custom VFS to SQLite that can really prevent reads/writes might be a more comprehensive solution worth considering...
https://sqlite.org/vfs.html sounds really cool! I will take a look. |
2bef788 to
b1cfecf
Compare
|
@mcollina This needs a rebase, and the first commit needs to be signed. |
b1cfecf to
191231b
Compare
araujogui
changed the title
e630e6b to
24a8650
Compare
|
@nodejs/sqlite I rebased branch and fixed some edge cases, could you review? |
|
@araujogui Can you rebase to edit the first commit message and body, as that will land on review. |
Database opened database files without consulting the permission model, so code running with --permission could read and write any database regardless of --allow-fs-read and --allow-fs-write. Check read access, and write access unless the database is read-only, before opening a file-backed database. In-memory databases are exempt. SQLite also opens journal, WAL and temporary files, and ATTACH and backup() open further databases, so route every connection through a VFS shim that checks each path SQLite opens, deletes or probes. Reject URI filenames that select a VFS, since that would bypass the shim, and deny ATTACH when SQLite does not pass the filename to the authorizer. Refs: https://hackerone.com/reports/3686625 Co-authored-by: Guilherme Araújo <[email protected]> Assisted-by: Claude Code Signed-off-by: Guilherme Araújo <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
Deny ATTACH under the permission model when SQLite does not pass the filename to the authorizer. SQLite only does so for string literals, so a bound or computed filename could select a URI VFS and bypass the permission VFS. Resolve VFS paths lexically, as the permission model does, instead of following symbolic links, and match SQLite's exact super-journal name format. Share one permission VFS per Environment, release it when open() fails, skip the authorizer guards when no JavaScript callback is set, check each path once, and reuse FSPermission::is_granted() for side-effect-free checks. Assisted-by: Claude Code Signed-off-by: Guilherme Araújo <[email protected]>
Assisted-by: Claude Code Signed-off-by: Guilherme Araújo <[email protected]>
24a8650 to
a9c53ba
Compare
Enforce the permission model for
node:sqlitefile access.open()checks--allow-fs-readfor read-only databases, and read plus write for read-write ones. In-memory databases are exempt.ATTACHtargets, backup destinations, and journal, WAL and SHM files. A database grant also covers its own sidecar files.vfs=parameters are rejected, so a connection cannot switch to an unchecked VFS.