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]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

open_db and load_extension now borrow_mut the PermissionsContainer and perform granular, path-aware FFI permission checks instead of global checks. If an extension path is provided, a path-scoped partial FFI check is performed; if allow_extension is true without a path, a path-agnostic partial FFI check is done and SQLITE_DBCONFIG_ENABLE_LOAD_EXTENSION is toggled via db_config. load_extension uses per-path partial FFI validation and permits enabling the C API only for extension loading. Tests were added to validate scoped FFI via subprocesses and the test runner now includes --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(...) -> attempt load_extension(/allowed/ext.so)
    SQLiteOps->>Perms: borrow_mut -> check partial_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(...) -> attempt load_extension(/denied/ext.so)
    SQLiteOps->>Perms: borrow_mut -> check partial_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 clearly and specifically describes the main change: adding path-scoped FFI permission support for SQLite extension loading.
Description check	✅ Passed	The description is directly related to the changeset, explaining the previous limitation and how the changes enable scoped FFI permissions for SQLite extensions.
Linked Issues check	✅ Passed	The PR addresses all coding requirements from issue #31426: replaces check_ffi_all() with partial checks at database construction, adds path-scoped checks in load_extension(), and includes comprehensive tests validating the new scoped FFI behavior.
Out of Scope Changes check	✅ Passed	All changes are directly related to implementing path-scoped FFI for SQLite extensions: core FFI permission logic changes, new tests validating scoped FFI behavior, and test infrastructure updates to support the new tests.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

---

_{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.