feat(ext/node): support path-scoped FFI for SQLite extension loading

[charles-dyfis-net]

Previously, using allowExtension: true or calling loadExtension() required unrestricted --allow-ffi permission. This made it impossible to sandbox code that needs to load only specific, pre-approved SQLite extensions.

This change allows scoped FFI permissions:

• allowExtension: true now requires partial FFI permission (any scope)
• loadExtension(path) requires FFI permission covering that specific path

Example: --allow-ffi=/path/to/extension.so now permits loading only that extension, rather than granting unrestricted FFI access.

Fixes: #31426

[CLAassistant]

All committers have signed the CLA.

[coderabbitai]

Walkthrough

open_db signature now accepts a location: &str; it always calls set_db_config to enable or disable LOAD_EXTENSION based on allow_extension and removes global FFI checks at open time. load_extension performs deferred, path-scoped partial FFI permission checks via check_ffi_partial_with_path. Permission container borrowing was adjusted for these checks. Tests were expanded (including subprocess tests) to validate scoped FFI behavior and the test runner adds --allow-run.

Sequence Diagram

sequenceDiagram
    participant TestRunner as Test Runner
    participant Subprocess as Deno Subprocess
    participant SQLiteOps as SQLite Ops
    participant Perms as PermissionsContainer

    rect rgb(200,240,220)
    Note over TestRunner,Subprocess: Allowed extension path flow
    TestRunner->>Subprocess: spawn with --allow-read --allow-ffi=/allowed/ext.so --allow-run
    Subprocess->>SQLiteOps: open_db(...) -> set_db_config(ENABLE_LOAD_EXTENSION = allow_extension)
    SQLiteOps->>Perms: borrow_mut -> check_ffi_partial_with_path(/allowed/ext.so)
    Perms-->>SQLiteOps: allowed
    SQLiteOps-->>Subprocess: load extension -> run query -> return OK
    end

    rect rgb(240,200,200)
    Note over TestRunner,Subprocess: Denied extension path flow
    TestRunner->>Subprocess: spawn with --allow-ffi=/other/path --allow-run
    Subprocess->>SQLiteOps: open_db(...) -> set_db_config(ENABLE_LOAD_EXTENSION = allow_extension)
    Subprocess->>SQLiteOps: attempt load_extension(/denied/ext.so)
    SQLiteOps->>Perms: borrow_mut -> check_ffi_partial_with_path(/denied/ext.so)
    Perms-->>SQLiteOps: denied (NotCapable)
    SQLiteOps-->>Subprocess: propagate permission error -> stderr contains NotCapable
    end

Loading

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'support path-scoped FFI for SQLite extension loading' directly describes the main change: implementing path-based FFI permission checks instead of global FFI checks for SQLite extensions.
Description check	✅ Passed	The description clearly explains the problem (allowExtension requiring unrestricted --allow-ffi) and the solution (scoped FFI permissions), directly relating to the code changes in database.rs and tests.
Linked Issues check	✅ Passed	The PR implements all requirements from #31426: removes check_ffi_all() at database construction, adds check_ffi_partial_with_path() in load_extension, and includes tests validating scoped FFI permission enforcement for both success and denial paths.
Out of Scope Changes check	✅ Passed	All changes are in-scope: database.rs refactors FFI checks per #31426, test files add scoped permission validation tests, and integration_tests.rs adds --allow-run for test subprocess execution.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5f37c31 and 6a96f7f.

📒 Files selected for processing (3)

• ext/node/ops/sqlite/database.rs (5 hunks)
• tests/sqlite_extension_test/sqlite_extension_test.ts (3 hunks)
• tests/sqlite_extension_test/tests/integration_tests.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/sqlite_extension_test/tests/integration_tests.rs

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with --inspect-brk flag and connect Chrome DevTools to chrome://inspect
Use console.log() for debug prints in JavaScript runtime code

Files:

• tests/sqlite_extension_test/sqlite_extension_test.ts

**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or use lldb directly
Use eprintln!() or dbg!() macros for debug prints in Rust code

Files:

• ext/node/ops/sqlite/database.rs

---

⚙️ CodeRabbit configuration file

Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.

Files:

• ext/node/ops/sqlite/database.rs

🧬 Code graph analysis (2)

tests/sqlite_extension_test/sqlite_extension_test.ts (5)

cli/bench/sqlite.js (1)

• stmt (29-29)

ext/node/polyfills/sqlite.ts (1)

• DatabaseSync (129-129)

tests/unit/test_util.ts (1)

• assertThrows (19-19)

ext/node/ops/sqlite/mod.rs (1)

• code (174-190)

cli/js/40_test.js (1)

• exitCode (98-98)

ext/node/ops/sqlite/database.rs (1)

ext/kv/sqlite.rs (1)

• open (62-209)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)

• GitHub Check: test debug linux-x86_64
• GitHub Check: test release linux-aarch64
• GitHub Check: test debug linux-aarch64
• GitHub Check: bench release linux-x86_64
• GitHub Check: test release linux-x86_64
• GitHub Check: test release windows-x86_64
• GitHub Check: test debug windows-x86_64
• GitHub Check: test release macos-aarch64
• GitHub Check: test debug macos-aarch64
• GitHub Check: test release macos-x86_64
• GitHub Check: build libs
• GitHub Check: lint debug linux-x86_64
• GitHub Check: lint debug windows-x86_64

🔇 Additional comments (10)

tests/sqlite_extension_test/sqlite_extension_test.ts (5)

60-60: LGTM on non-null assertion.

The assertion is safe here since a SELECT statement always returns at least one row, and any extension loading failures would throw earlier.

---

67-85: Good test coverage for deferred permission checks.

This test correctly validates that creating a database with allowExtension: true succeeds without FFI permissions, and that the permission check is deferred to loadExtension(). This aligns perfectly with the PR objectives.

---

108-151: Excellent subprocess test for scoped FFI success case.

The test correctly validates that scoped FFI permissions (--allow-ffi=${extensionPath}) allow loading the specific extension. The subprocess isolation ensures proper permission boundary testing.

---

153-207: Good test for scoped FFI permission boundary.

This test properly validates that when FFI permission is granted for a different path, loading the extension fails with NotCapable. The error handling logic correctly distinguishes between expected permission errors and unexpected errors.

---

209-242: Well-implemented test for SQL vs C API extension loading.

The test correctly validates the security boundary: SQL load_extension() is disabled even with allowExtension: true, while the C API loadExtension() remains functional. Good use of parameterized statement addressing the previous review feedback.

ext/node/ops/sqlite/database.rs (5)

443-457: Clear documentation for deferred permission checks.

The docstring correctly explains that FFI permission checks are deferred to load_extension, and that only the C API for extension loading is enabled (SQL function remains disabled). This aligns with the PR objectives.

---

464-484: Correct implementation for in-memory database extension loading.

The code properly enables/disables the C API extension loading based on allow_extension without performing an upfront FFI permission check. The permission check is correctly deferred to load_extension().

---

497-520: Consistent extension loading setup for readonly databases.

The readonly path correctly follows the same pattern: configure extension loading based on allow_extension and defer the FFI permission check to load_extension().

---

522-537: Consistent implementation for read-write database path.

The read-write path maintains the same pattern: enable/disable extension loading via configuration and defer FFI permission checks. The logic is consistent across all database opening paths.

---

1091-1116: Path-scoped FFI check correctly implemented.

The updated load_extension now performs path-scoped FFI validation via check_ffi_partial_with_path, which is the core improvement enabling scoped FFI permissions (--allow-ffi=/path/to/extension.so) instead of requiring global FFI access. The use of borrow_mut() is correct since check_ffi_partial_with_path requires &mut self.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/sqlite_extension_test/sqlite_extension_test.ts (1)

97-102: Consider consolidating the extension existence check.

Both new tests duplicate the same skip logic. A helper function or beforeAll-style check could reduce duplication, but this is minor for test code.

// Optional: Extract to a helper at the top of the file
function skipIfExtensionNotFound(): boolean {
  try {
    Deno.statSync(extensionPath);
    return false;
  } catch {
    return true;
  }
}

Also applies to: 149-154

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d704ca2 and cb9591e.

📒 Files selected for processing (3)

• ext/node/ops/sqlite/database.rs (6 hunks)
• tests/sqlite_extension_test/sqlite_extension_test.ts (1 hunks)
• tests/sqlite_extension_test/tests/integration_tests.rs (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or use lldb directly
Use eprintln!() or dbg!() macros for debug prints in Rust code

Files:

• ext/node/ops/sqlite/database.rs
• tests/sqlite_extension_test/tests/integration_tests.rs

**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with --inspect-brk flag and connect Chrome DevTools to chrome://inspect
Use console.log() for debug prints in JavaScript runtime code

Files:

• tests/sqlite_extension_test/sqlite_extension_test.ts

🧠 Learnings (2)

📓 Common learnings

Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/permissions.rs : Permission system is implemented in `runtime/permissions.rs`

📚 Learning: 2025-11-24T16:19:37.808Z

Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/permissions.rs : Permission system is implemented in `runtime/permissions.rs`

Applied to files:

• ext/node/ops/sqlite/database.rs
• tests/sqlite_extension_test/tests/integration_tests.rs

🧬 Code graph analysis (1)

tests/sqlite_extension_test/sqlite_extension_test.ts (2)

ext/node/ops/sqlite/mod.rs (1)

• code (174-189)

cli/js/40_test.js (1)

• exitCode (98-98)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

• GitHub Check: lint debug windows-x86_64
• GitHub Check: lint debug macos-x86_64
• GitHub Check: lint debug linux-x86_64
• GitHub Check: test debug linux-x86_64
• GitHub Check: test debug linux-aarch64
• GitHub Check: test debug windows-x86_64
• GitHub Check: test release linux-x86_64
• GitHub Check: test release macos-aarch64
• GitHub Check: test debug macos-x86_64
• GitHub Check: test debug macos-aarch64
• GitHub Check: build libs

🔇 Additional comments (6)

ext/node/ops/sqlite/database.rs (3)

294-294: Permission scoping implemented correctly.

The switch from check_ffi_all() to check_ffi_partial_no_path() at database open time aligns with the PR objective: requiring partial FFI permission (any scope) rather than unrestricted FFI when allowExtension: true. This defers the path-specific check to the actual loadExtension() call.

Also applies to: 332-332, 347-347

---

719-742: Path-scoped FFI check in load_extension looks correct.

The updated doc comment accurately describes the two-step requirement. The check_ffi_partial_with_path(Cow::Borrowed(Path::new(path))) call enforces that FFI permission must cover the specific extension path being loaded, matching the --allow-ffi=/path/to/extension.so behavior described in PR objectives.

---

277-277: I'll help you verify whether borrow_mut() is required at line 277 in the SQLite database code. Let me search for the relevant permission check method signatures and how they're used.
<function_calls>

find ext/node/ops/sqlite -name "*.rs" | head -20

</function_calls>

Let me search for the permission check methods to verify their signatures:
<function_calls>

rg -n "fn check_ffi_partial" --type rust

</function_calls>

Let me also look at the specific file and context around line 277:
<function_calls>

sed -n '270,290p' ext/node/ops/sqlite/database.rs

</function_calls>

<function_calls>

wc -l ext/node/ops/sqlite/database.rs

</function_calls>

<function_calls>

rg -n "check_ffi_partial_no_path|check_ffi_partial_with_path" ext/node/ops/sqlite/database.rs

</function_calls>

<function_calls>

rg -n "check_ffi" --type rust -A 3

</function_calls>

tests/sqlite_extension_test/tests/integration_tests.rs (1)

53-53: Necessary addition for subprocess-based tests.

The --allow-run flag is required to enable the new scoped FFI permission tests in sqlite_extension_test.ts that use Deno.Command to spawn subprocesses with specific permission flags.

tests/sqlite_extension_test/sqlite_extension_test.ts (2)

93-142: Well-structured test for scoped FFI success case.

The test correctly validates that --allow-ffi=${extensionPath} permits loading only that specific extension. The subprocess pattern is appropriate since Deno.test permissions don't support scoped FFI.

---

144-203: Good negative test for path restriction enforcement.

This test properly validates that FFI permission for a different path (/some/other/path) does not grant access to load the actual extension. The explicit check for Deno.errors.NotCapable confirms the expected permission error.

One minor consideration: the subprocess doesn't check exit code, only stdout. Since the test catches the error and continues to db.close(), exit code 0 is expected. This is fine.

[charles-dyfis-net]

@coderabbitai generate docstrings

[coderabbitai]

✅ Actions performed

Initiated docstring generation; will generate only if new commits exist.

[coderabbitai]

Note

Docstrings generation - SUCCESS
Generated docstrings for this pull request at #31429

[charles-dyfis-net]

I'm unclear on the docstring coverage limit being flagged -- is this perhaps applying to tests? (Do folks really want docstrings for tests?)

[charles-dyfis-net]

In attempting to address automated review feedback about docstrings, I appear to have broken the build -- it looks like #[op2] does not support /// doc comments inside the impl block. Reverted those additions, while keeping the doc comment on open_db.

[littledivy]

AFAIU this does not prevent loading extension from SQL like so:

 SELECT load_extension("a.dll");

and that was the reason sqlite required unscoped ffi perms.

[charles-dyfis-net]

AFAIU this does not prevent loading extension from SQL like so:

 SELECT load_extension("a.dll");

and that was the reason sqlite required unscoped ffi perms.

Ah! Thank you for that clarification.

As I read https://sqlite.org/c3ref/c_dbconfig_defensive.html#sqlitedbconfigenableloadextension, it should be possible to enable only the C call and not the SQL -- I'll go back and extend the tests to look for that specific state, and then see about determining how to get into it.

[charles-dyfis-net]

On updating the tests to distinguish between a normal pass and a test that was short-circuited due to the extension not being compiled, it looks like the extension tests were always being skipped because they were attempting to load the extension from the wrong location, triggering the early return path. When fixing that bug, at least on Darwin, I get a segfault akin to the following (and the exact same thing loading the extension from a separate sqlite3 executable):

* thread #11, name = 'tokio-runtime-worker', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000000000000
    frame #1: 0x000000013797ee9c libtest_sqlite_extension.dylib`mallocWithAlarm(n=40, pp=0x000000017095cff0) at sqlite3.c:30985:11
    frame #2: 0x0000000137978ee0 libtest_sqlite_extension.dylib`sqlite3Malloc(n=40) at sqlite3.c:31051:5
    frame #3: 0x0000000137982fa8 libtest_sqlite_extension.dylib`sqlite3HashInsert(pH=0x000000014b905a68, pKey="test_func", data=0x0000000140027f30) at sqlite3.c:37637:25
    frame #4: 0x000000013797ec8c libtest_sqlite_extension.dylib`sqlite3FindFunction(db=0x000000014b905800, zName="test_func", nArg=1, enc='\x01', createFlag='\x01') at sqlite3.c:129243:24
    frame #5: 0x000000013797e900 libtest_sqlite_extension.dylib`sqlite3CreateFunc(db=0x000000014b905800, zFunctionName="test_func", nArg=1, enc=1, pUserData=0x0000000000000000, xSFunc=(libtest_sqlite_extension.dylib`test_sqlite_extension::test_func::h6cec236b81aa5076 at lib.rs:12), xStep=0x0000000000000000, xFinal=0x0000000000000000, xValue=0x0000000000000000, xInverse=0x0000000000000000, pDestructor=0x0000000000000000) at sqlite3.c:184621:7
    frame #6: 0x000000013797e488 libtest_sqlite_extension.dylib`createFunctionApi(db=0x000000014b905800, zFunc="test_func", nArg=1, enc=2049, p=0x0000000000000000, xSFunc=(libtest_sqlite_extension.dylib`test_sqlite_extension::test_func::h6cec236b81aa5076 at lib.rs:12), xStep=0x0000000000000000, xFinal=0x0000000000000000, xValue=0x0000000000000000, xInverse=0x0000000000000000, xDestroy=0x0000000000000000) at sqlite3.c:184687:8
    frame #7: 0x000000013797e594 libtest_sqlite_extension.dylib`sqlite3_create_function_v2(db=0x000000014b905800, zFunc="test_func", nArg=1, enc=2049, p=0x0000000000000000, xSFunc=(libtest_sqlite_extension.dylib`test_sqlite_extension::test_func::h6cec236b81aa5076 at lib.rs:12), xStep=0x0000000000000000, xFinal=0x0000000000000000, xDestroy=0x0000000000000000) at sqlite3.c:184729:10

I suspect we'll want a separate PR to fix the extension used for test coverage before this one can be considered -- I'll get a separate ticket and PR together.

[charles-dyfis-net]

Filed ticket #31454 for the issues preventing this from being correctly tested.

[charles-dyfis-net]

About to push an updated version of this PR stacked on top of #31455. The expectation is that it will remain in draft until the latter (or something equivalent) is merged.

[charles-dyfis-net]

...with #31455 merged, rebasing onto main and removing from draft

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3e9464f and e758bf3.

📒 Files selected for processing (3)

• ext/node/ops/sqlite/database.rs (5 hunks)
• tests/sqlite_extension_test/sqlite_extension_test.ts (3 hunks)
• tests/sqlite_extension_test/tests/integration_tests.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• ext/node/ops/sqlite/database.rs

🧰 Additional context used

📓 Path-based instructions (2)

**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or use lldb directly
Use eprintln!() or dbg!() macros for debug prints in Rust code

Files:

• tests/sqlite_extension_test/tests/integration_tests.rs

---

⚙️ CodeRabbit configuration file

Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.

Files:

• tests/sqlite_extension_test/tests/integration_tests.rs

**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with --inspect-brk flag and connect Chrome DevTools to chrome://inspect
Use console.log() for debug prints in JavaScript runtime code

Files:

• tests/sqlite_extension_test/sqlite_extension_test.ts

🧠 Learnings (1)

📚 Learning: 2025-11-24T16:19:37.808Z

Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/permissions.rs : Permission system is implemented in `runtime/permissions.rs`

Applied to files:

• tests/sqlite_extension_test/tests/integration_tests.rs

🧬 Code graph analysis (1)

tests/sqlite_extension_test/sqlite_extension_test.ts (2)

tests/unit/test_util.ts (1)

• assertThrows (19-19)

ext/node/ops/sqlite/mod.rs (1)

• code (174-190)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)

• GitHub Check: test debug linux-aarch64
• GitHub Check: test debug macos-aarch64
• GitHub Check: test release linux-aarch64
• GitHub Check: test debug linux-x86_64
• GitHub Check: bench release linux-x86_64
• GitHub Check: test release linux-x86_64
• GitHub Check: test debug windows-x86_64
• GitHub Check: test debug macos-x86_64
• GitHub Check: lint debug windows-x86_64
• GitHub Check: lint debug macos-x86_64
• GitHub Check: lint debug linux-x86_64
• GitHub Check: build libs

🔇 Additional comments (5)

tests/sqlite_extension_test/tests/integration_tests.rs (1)

66-76: Adding --allow-run is appropriate for subprocess-based tests

This aligns the CLI invocation with the new tests that use Deno.Command and require run permission; scoped to tests, so security impact is acceptable.

tests/sqlite_extension_test/sqlite_extension_test.ts (4)

60-60: Non-null assertion on stmt.get() is reasonable here

Given the query always returns a row when the extension works, asserting non-null via stmt.get()! is appropriate for the test.

---

71-84: Deferring the NotCapable error to loadExtension() matches the new permission model

Creating DatabaseSync with allowExtension: true under ffi: false and expecting Deno.errors.NotCapable only when loadExtension() is called accurately encodes the intended deferred FFI check.

---

105-151: Positive-path scoped FFI subprocess test is well-scoped

The subprocess setup correctly:

• Grants --allow-read/--allow-ffi only for extensionPath.
• Uses run permission solely to start deno via Deno.Command.
• Asserts both exit code and "OK" output, which tightly verifies the success path.

---

153-207: Negative scoped FFI test correctly distinguishes permission failure

Granting FFI for a different path and asserting that the child prints "EXPECTED_PERMISSION_ERROR" when NotCapable is thrown cleanly validates that path-scoped FFI is enforced, not just “any FFI”.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/sqlite_extension_test/sqlite_extension_test.ts (1)

153-206: Good negative test for path-scoped FFI boundaries.

Verifies that --allow-ffi=/some/other/path doesn't grant permission to load an extension from a different location. The subprocess error handling properly distinguishes NotCapable from other error types.

Minor suggestion: Consider also asserting exitCode === 0 since the subprocess catches the error and exits cleanly.

-    const { stdout } = await child.output();
+    const { code: exitCode, stdout } = await child.output();
     const stdoutText = new TextDecoder().decode(stdout);
 
+    assertEquals(exitCode, 0, "Subprocess should exit cleanly after catching error");
     assertEquals(
       stdoutText.trim(),
       "EXPECTED_PERMISSION_ERROR",
       `Expected NotCapable error but got: ${stdoutText}`,
     );

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e758bf3 and af5a4c9.

📒 Files selected for processing (3)

• ext/node/ops/sqlite/database.rs (5 hunks)
• tests/sqlite_extension_test/sqlite_extension_test.ts (3 hunks)
• tests/sqlite_extension_test/tests/integration_tests.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/sqlite_extension_test/tests/integration_tests.rs

🧰 Additional context used

📓 Path-based instructions (2)

**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or use lldb directly
Use eprintln!() or dbg!() macros for debug prints in Rust code

Files:

• ext/node/ops/sqlite/database.rs

---

⚙️ CodeRabbit configuration file

Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.

Files:

• ext/node/ops/sqlite/database.rs

**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with --inspect-brk flag and connect Chrome DevTools to chrome://inspect
Use console.log() for debug prints in JavaScript runtime code

Files:

• tests/sqlite_extension_test/sqlite_extension_test.ts

🧠 Learnings (1)

📚 Learning: 2025-11-24T16:19:37.808Z

Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/permissions.rs : Permission system is implemented in `runtime/permissions.rs`

Applied to files:

• ext/node/ops/sqlite/database.rs

🧬 Code graph analysis (1)

tests/sqlite_extension_test/sqlite_extension_test.ts (3)

ext/node/polyfills/sqlite.ts (1)

• DatabaseSync (129-129)

tests/unit/test_util.ts (1)

• assertThrows (19-19)

ext/node/ops/sqlite/mod.rs (1)

• code (174-190)

🔇 Additional comments (8)

ext/node/ops/sqlite/database.rs (4)

443-452: LGTM on the documentation.

Clear explanation of the deferred permission check design and the distinction between C API and SQL function for extension loading.

---

475-481: Consistent extension configuration across all database open paths.

The SQLITE_DBCONFIG_ENABLE_LOAD_EXTENSION is now uniformly set based on allow_extension in all three branches (in-memory, read-only, and standard file). This ensures consistent behavior regardless of how the database is opened.

Also applies to: 511-517, 524-530

---

1091-1116: Path-scoped FFI permission check implementation looks correct.

The flow is sound:

1. Verify allowExtension was set at database creation
2. Check FFI permission for the specific extension path via check_ffi_partial_with_path
3. Proceed with loading

This correctly implements the deferred, path-scoped permission model described in the PR objectives.

---

459-462: Verify that mutable borrow is necessary.

The change from borrow() to borrow_mut() for PermissionsContainer is notable. If check_has_all_permissions and check_open only require shared access, this could be reverted to avoid unnecessary mutable borrows.

#!/bin/bash
# Check if PermissionsContainer methods require &mut self
ast-grep --pattern 'fn check_has_all_permissions(&mut self$$$)'
ast-grep --pattern 'fn check_open(&mut self$$$)'
rg -n "fn check_has_all_permissions|fn check_open" --type rust -A 2

tests/sqlite_extension_test/sqlite_extension_test.ts (4)

60-60: Non-null assertion is appropriate here.

The test explicitly expects stmt.get() to return a result. If it doesn't, failing fast with a clear error is the right behavior.

---

67-85: Test correctly validates deferred permission behavior.

The test now properly verifies that:

1. Creating DatabaseSync with allowExtension: true succeeds without FFI permission
2. The NotCapable error is thrown only when loadExtension() is called

Good cleanup with db.close() at the end.

---

108-151: Well-structured subprocess test for scoped FFI.

Using a subprocess is the right approach since Deno.test permissions don't support path-scoped FFI. The test properly:

• Scopes both --allow-read and --allow-ffi to the extension path
• Validates both the exit code and stdout output
• Includes helpful error message in assertion

---

209-242: Important security test confirming SQL-level load_extension remains disabled.

This validates a key security property: even with allowExtension: true, only the C API loadExtension() works—the SQL load_extension() function stays disabled. Using the parameterized statement addresses the prior review feedback about path interpolation.

[charles-dyfis-net]

(The checks marked as failing -- at least, the two so far -- appear to be related to the macos-13 runner being removed in favor of macos-15, and not related to the content of the PR at hand).

[charles-dyfis-net]

Taking a closer look at CodeRabbit's review notes, only one of the two borrow -> borrow_mut changes is strictly necessary (check_ffi_partial_with_path requires &mut self, but check_has_all_permissions does not).

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/sqlite_extension_test/sqlite_extension_test.ts (1)

108-151: Consider adding null check for consistency

Line 119 in the subprocess code does const { result } = stmt.get(); without a null check. If the query returns no rows, this would throw a confusing destructuring error rather than a clear test failure.

For consistency with line 60 (which uses stmt.get()!), consider:

      const stmt = db.prepare("SELECT test_func('test') AS result");
-      const { result } = stmt.get();
+      const row = stmt.get();
+      if (!row) throw new Error("Expected a result row");
+      const { result } = row;

This is minor since a test failure would still surface the issue.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between af5a4c9 and 5f37c31.

📒 Files selected for processing (3)

• ext/node/ops/sqlite/database.rs (5 hunks)
• tests/sqlite_extension_test/sqlite_extension_test.ts (3 hunks)
• tests/sqlite_extension_test/tests/integration_tests.rs (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or use lldb directly
Use eprintln!() or dbg!() macros for debug prints in Rust code

Files:

• tests/sqlite_extension_test/tests/integration_tests.rs
• ext/node/ops/sqlite/database.rs

---

⚙️ CodeRabbit configuration file

Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.

Files:

• tests/sqlite_extension_test/tests/integration_tests.rs
• ext/node/ops/sqlite/database.rs

**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with --inspect-brk flag and connect Chrome DevTools to chrome://inspect
Use console.log() for debug prints in JavaScript runtime code

Files:

• tests/sqlite_extension_test/sqlite_extension_test.ts

🧠 Learnings (1)

📚 Learning: 2025-11-24T16:19:37.808Z

Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/permissions.rs : Permission system is implemented in `runtime/permissions.rs`

Applied to files:

• tests/sqlite_extension_test/tests/integration_tests.rs
• ext/node/ops/sqlite/database.rs

🧬 Code graph analysis (2)

tests/sqlite_extension_test/sqlite_extension_test.ts (1)

ext/node/ops/sqlite/mod.rs (1)

• code (174-190)

ext/node/ops/sqlite/database.rs (1)

ext/kv/sqlite.rs (1)

• open (62-209)

🔇 Additional comments (9)

tests/sqlite_extension_test/tests/integration_tests.rs (1)

71-71: LGTM!

The --allow-run permission is required for the new subprocess-based scoped FFI tests in the TypeScript test file.

tests/sqlite_extension_test/sqlite_extension_test.ts (4)

60-60: LGTM!

Non-null assertion is appropriate here since the test expects a valid row result from the query.

---

67-85: LGTM!

Good refactor to validate the new deferred permission model - database creation with allowExtension: true now succeeds, and the permission check happens at loadExtension() time.

---

153-207: LGTM!

Good negative test case validating that scoped FFI permission enforcement rejects paths outside the granted scope.

---

209-242: LGTM!

Important security test verifying that SQL-level load_extension() remains disabled (defensive mode) while the C API loadExtension() works. This prevents SQL injection from bypassing extension controls.

ext/node/ops/sqlite/database.rs (4)

443-452: LGTM! Clear documentation of the deferred permission model.

The docstring accurately explains that FFI permission checks are deferred to load_extension where path-scoped validation can occur.

---

475-481: LGTM!

Clean consolidation of extension loading configuration. Using set_db_config with SQLITE_DBCONFIG_ENABLE_LOAD_EXTENSION correctly enables only the C API while keeping the SQL load_extension() function disabled.

---

511-530: LGTM!

Consistent application of the extension loading configuration across read-only and read-write paths.

---

1091-1116: LGTM! Core security change implementing path-scoped FFI validation.

The change from a global FFI check to check_ffi_partial_with_path with the specific extension path correctly implements scoped permissions like --allow-ffi=/path/to/extension.so. The method exists in PermissionsContainer and returns the allowed path or a PermissionCheckError, enforcing the permission boundary as intended.