SQLiteKit: embedder-bound hardened policy for untrusted SQL#1
SQLiteKit: embedder-bound hardened policy for untrusted SQL#1ronaldmannak wants to merge 37 commits into
Conversation
Add an opt-in, trusted-layer security policy to the sqlite3 shell port so it can run untrusted (LLM-generated) SQL safely under SwiftBash, without breaking sqlite3 parity or the sqlite-vec/FTS5 feature surface. Control surface: the policy is bound by the embedder (Sqlite3Builtin(policy:)) or auto-derived from an active sandbox; argv/dot-commands can only tighten it, never relax it. This is the key property — opt-in flags would be no boundary when the agent writes the command line. Shell-layer controls (all no-ops unless hardened): - result output byte cap with a truncation notice - runtime limits (ATTACHED=0, LENGTH, SQL_LENGTH) + .limit raise-blocking - PRAGMA temp_store=MEMORY - canonicalize-before-authorize on the db open, ATTACH, and file dot-commands - policy re-application on .open (not just safe mode) - attempted-tier audit (JSON Lines, O_NOFOLLOW + preflight) outside the DB - script-budget timeout between statements Hardened mode deliberately does not touch the authorizer/PRAGMA/vtables, so vec0 + FTS5 keep working. Engine-level follow-ups (intra-statement interrupt, committed-row audit, NOFOLLOW, defensive config, compile flags) require the SQLiteKit SDK handle and are specced in Docs/PORTING-FROM-SWIFTSQLITE.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8VSWQJZ3ZxVcG6kwiwygJ
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a robust hardening policy (SQLitePolicy) for the sqlite3 shell port to safely execute untrusted, LLM-generated SQL under a sandbox. Key additions include resource limits, result output byte capping, script timeouts, symlink canonicalization before authorization, and an attempted-audit trail logged to an external file. The feedback highlights critical performance optimizations: avoiding redundant directory creation checks on every audit log write, and replacing an inefficient string-splitting truncation method with a direct UTF-8 prefix slice. Additionally, a minor cleanup is suggested to use a constant instead of a mutable variable in the audit sink.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| private func openLog() -> Int32 { | ||
| try? FileManager.default.createDirectory( | ||
| at: url.deletingLastPathComponent(), withIntermediateDirectories: true) | ||
| return url.path.withCString { | ||
| open($0, O_WRONLY | O_CREAT | O_APPEND | O_NOFOLLOW, 0o600) | ||
| } | ||
| } |
There was a problem hiding this comment.
Performance Bottleneck
FileManager.default.createDirectory is called on every single openLog() invocation, which happens on every record(_:) call (before every SQL statement execution). This introduces a massive performance bottleneck due to redundant file system checks.
Since preflight() already ensures the directory exists, we can optimize openLog() to try opening the file first. If it fails with ENOENT (directory doesn't exist), only then should we create the directory and retry. This completely eliminates the overhead of createDirectory in the common case.
private func openLog() -> Int32 {
let path = url.path
var fd = path.withCString {
open($0, O_WRONLY | O_CREAT | O_APPEND | O_NOFOLLOW, mode_t(0o600))
}
if fd < 0 && errno == ENOENT {
try? FileManager.default.createDirectory(
at: url.deletingLastPathComponent(), withIntermediateDirectories: true)
fd = path.withCString {
open($0, O_WRONLY | O_CREAT | O_APPEND | O_NOFOLLOW, mode_t(0o600))
}
}
return fd
}There was a problem hiding this comment.
Fixed in c7ee27f — openLog() now tries the open first and only calls createDirectory on ENOENT, so the common (directory-exists) path is a single syscall. Thanks.
Generated by Claude Code
| private static func truncateToUTF8Bytes(_ s: String, limit: Int) -> String { | ||
| guard limit > 0, s.utf8.count > limit else { return s.utf8.count <= limit ? s : "" } | ||
| var result = "" | ||
| var count = 0 | ||
| for line in s.split(separator: "\n", omittingEmptySubsequences: false) { | ||
| let lineBytes = line.utf8.count + 1 // include the newline | ||
| if count + lineBytes > limit { break } | ||
| result += String(line) + "\n" | ||
| count += lineBytes | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Performance Bottleneck
Using s.split(separator: "\n", omittingEmptySubsequences: false) splits the entire string s into an array of substrings. If s is very large (e.g., megabytes of query results), this will allocate a huge number of substrings and consume significant memory and CPU, even though we only need to truncate it to a small limit of bytes.
We can optimize this by working directly on the UTF-8 prefix of length limit, finding the last newline character (0x0a), and slicing the string up to that index. This avoids splitting the entire string and is extremely efficient.
private static func truncateToUTF8Bytes(_ s: String, limit: Int) -> String {
guard limit > 0, s.utf8.count > limit else { return s.utf8.count <= limit ? s : "" }
let utf8 = s.utf8
let prefix = utf8.prefix(limit)
if let lastNewlineIndex = prefix.lastIndex(of: 0x0a) {
return String(s[...lastNewlineIndex])
}
return ""
}There was a problem hiding this comment.
Fixed in c7ee27f — now takes the first limit bytes and cuts at the last newline in them, instead of splitting the whole string. (Worth noting this path only runs once per run, when the cap is first exceeded, but the cleaner version is still a clear improvement.)
Generated by Claude Code
| } | ||
|
|
||
| public func record(_ event: AuditEvent) async { | ||
| var blob = Data((event.jsonLine + "\n").utf8) |
There was a problem hiding this comment.
The blob variable is never mutated after initialization. Declaring it as a let constant instead of var is cleaner and prevents Swift compiler warnings.
| var blob = Data((event.jsonLine + "\n").utf8) | |
| let blob = Data((event.jsonLine + "\n").utf8) |
There was a problem hiding this comment.
- openLog: try the open first, create the parent dir only on ENOENT, so the common path is one syscall instead of a createDirectory on every record. - truncateToUTF8Bytes: cut on the last newline within the first `limit` bytes instead of splitting the whole (possibly large) string. - audit record: `blob` is never mutated → `let`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8VSWQJZ3ZxVcG6kwiwygJ
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0c6bc3d53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -782,7 +933,11 @@ final class Session { | |||
| let replacement = try SQLiteDatabase(location) | |||
There was a problem hiding this comment.
When an embedder binds SQLitePolicy(forceReadOnly: true, ...), the initial database open respects it, but this .open path creates the replacement connection without readonly: policy.forceReadOnly. In a sandboxed/untrusted script, .open writable.db then relaxes the trusted read-only policy in-band and subsequent SQL can write to that database; reopen with the same readonly condition used for the initial handle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2135ae9 — .open now creates the replacement handle with readonly: policy.forceReadOnly, so a forced read-only policy survives an in-band .open writable.db. Good catch; this was the same .open control-surface gap I'd only partly closed (limits/temp). 👍
Generated by Claude Code
| db.limit(SQLitePolicy.limitAttached, newValue: 0) | ||
| db.limit(SQLitePolicy.limitLength, newValue: SQLitePolicy.lengthCeiling) | ||
| db.limit(SQLitePolicy.limitSQLLength, newValue: SQLitePolicy.sqlLengthCeiling) | ||
| _ = try? db.evaluate("PRAGMA temp_store=MEMORY;") |
There was a problem hiding this comment.
With hardened mode enabled this sets temp_store only once, while the SQL surface still permits PRAGMA statements; an untrusted script can immediately run PRAGMA temp_store=FILE; and SQLite changes the setting back, so later sorts/temp tables may spill to disk despite the hardened policy's temp confinement. If this is part of the boundary, special-case this PRAGMA (or otherwise reapply/deny changes) instead of relying on the startup assignment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2135ae9 — hardened mode now re-pins PRAGMA temp_store=MEMORY before each statement, so an in-band PRAGMA temp_store=FILE; can't make later sorts/temp tables spill to disk. This is best-effort at the shell layer; the definitive fix is the compile flag SQLITE_TEMP_STORE=3 in the engine SDK, which I've added to Docs/PORTING-FROM-SWIFTSQLITE.md. 👍
Generated by Claude Code
| for set in try database.evaluate(sql) { | ||
| out(formatter.render(set)) | ||
| if !emitResult(formatter.render(set)) { break } |
There was a problem hiding this comment.
The hardened byte cap is only enforced when SQL result sets go through emitResult, but dot-commands such as .dump, .schema, .fullschema, and .print still write directly via out(...). In a sandboxed builtin, an untrusted script can therefore dump a large database and send unbounded output back to stdout or $(...) despite maxResultBytes; route all shell output that can be data-sized through the same cap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2135ae9 — moved the byte cap into the central out() path, so all data output is bounded: .dump/.schema/.fullschema/.print and .output/.once redirects now count against maxResultBytes too, not just SQL result sets. Added a test (hardenedCapsDotCommandOutput). 👍
Generated by Claude Code
| var written = 0 | ||
| while written < raw.count { | ||
| let n = write(fd, base + written, raw.count - written) | ||
| if n <= 0 { Self.reportFailure(url: url); return } |
There was a problem hiding this comment.
If an embedder configures auditURL and the log becomes unwritable after preflight (for example, disk full, permissions changed, or the leaf is swapped so O_NOFOLLOW rejects it), this path only reports the failure and returns from record; runStatement then continues and executes the SQL without a corresponding audit entry. Because audit is a trusted policy control recorded before execution, write failures need to propagate back and stop the statement instead of allowing unaudited actions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2135ae9 — AuditSink.record now returns a success flag, and the attempted-tier record (recorded before execution, in both runStatement and handleDot) fails closed: if the write fails, the statement/command is refused and the run stops rather than executing unaudited. 👍
Generated by Claude Code
All four are channels an untrusted script could use to sidestep the bound policy; each is now subordinated to it: - .open: open the replacement handle with readonly: policy.forceReadOnly, so `.open writable.db` can't relax a forced read-only policy (P1). - temp_store: re-pin PRAGMA temp_store=MEMORY before each hardened statement, so an in-band `PRAGMA temp_store=FILE;` can't make later sorts spill to disk. Definitive fix (SDK SQLITE_TEMP_STORE=3) noted in the porting doc (P2). - output cap: move the byte cap into the central out() path so data-bearing dot-commands (.dump/.schema/.print) and .output redirects are bounded too, not just SQL result sets (P2). - audit: AuditSink.record now returns success; an attempted-tier write failure fails closed (refuse the statement/command) instead of running unaudited (P2). Adds hardened tests for the dot-command cap, temp_store re-pin, and fail-closed audit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8VSWQJZ3ZxVcG6kwiwygJ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2135ae98b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // would otherwise undo the startup setting and let later sorts spill to | ||
| // disk. (The definitive fix is the SDK compile flag SQLITE_TEMP_STORE=3 | ||
| // — see Docs/PORTING-FROM-SWIFTSQLITE.md.) | ||
| if policy.hardened { _ = try? database.evaluate("PRAGMA temp_store=MEMORY;") } |
There was a problem hiding this comment.
Fresh evidence: the new re-pin still happens only once per runStatement, but this shell already accepts multiple semicolon-separated statements in a single line or SQL argument and then passes the whole chunk to database.evaluate(sql). In hardened mode, an input such as PRAGMA temp_store=FILE; CREATE TEMP TABLE ...; in the same chunk changes the setting after this single re-pin and lets the later statement use file-backed temp storage, so the in-band PRAGMA can still relax the intended temp confinement; split/evaluate one statement at a time or deny this PRAGMA before executing the chunk.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in be3cdba — right, the per-statement re-pin doesn't help a PRAGMA temp_store=FILE; in the same chunk handed to evaluate. Hardened mode now denies a statement that sets temp_store (regex on the chunk: pragma\s+temp_store\s*[=(]), so the same-chunk case is blocked; reads are still allowed and the re-pin stays as belt-and-suspenders. This is best-effort (comment-interleaving could evade the regex); the evasion-proof fix is the SDK compile flag SQLITE_TEMP_STORE=3, which is specced in Docs/PORTING-FROM-SWIFTSQLITE.md. 👍
Generated by Claude Code
| if await audit.record(.attempted( | ||
| kind: "sql", text: sql.trimmingCharacters(in: .whitespacesAndNewlines))) == false { |
There was a problem hiding this comment.
When .eqp on is active and the configured audit log becomes unwritable after preflight, renderQueryPlan(sql) has already prepared/evaluated EXPLAIN QUERY PLAN and emitted schema-derived output before this fail-closed audit check runs. In audited hardened sessions that means an unaudited input can still cause database work and output before being refused; move the attempted audit ahead of echo/eqp rendering so audit failure blocks all effects of the statement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in be3cdba — moved the attempted-audit record (and its fail-closed check) to the very top of runStatement, before echo and renderQueryPlan, so a failed audit blocks all effects of the statement including the EXPLAIN QUERY PLAN work/output. 👍
Generated by Claude Code
| do { | ||
| for set in try database.evaluate(sql) { | ||
| out(formatter.render(set)) | ||
| if outputCapped { break } // stop rendering further sets once capped |
There was a problem hiding this comment.
This early exit only stops rendering additional SQL result sets; data-bearing dot commands such as .dump and .fullschema also call out, but their loops never check outputCapped, so after a hardened cap is reached they keep querying and walking every table/row while silently dropping output. Against a large database, .dump can still burn unbounded time/memory after the cap notice; have the dot-command loops stop when the cap is hit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in be3cdba — .dump and .fullschema now check outputCapped in their table/row/object loops and stop querying once the cap is hit, so they no longer walk the whole database while dropping output. 👍
Generated by Claude Code
| // The continuation prompt shows while a statement is still open. | ||
| out(buffer.isEmpty ? "sqlite> " : " ...> ") | ||
| guard let line = await stdin.readLine() else { out("\n"); break } | ||
| if budgetExceeded() { break } |
There was a problem hiding this comment.
In hardened interactive mode, this timeout check is skipped when readLine() returns EOF, and the code then falls through to the leftover block that runs any buffered unterminated SQL unconditionally. If a caller feeds -interactive input such as SELECT ... without a semicolon and EOF arrives after the deadline, the final statement still executes despite statementTimeout; guard the EOF leftover execution with shouldQuit/budgetExceeded() as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in be3cdba — the EOF-leftover execution in both runInteractive and process is now guarded with !shouldQuit, !budgetExceeded(), so a buffered unterminated statement won't run once the time budget is spent. 👍
Generated by Claude Code
…imeout, eqp audit order) - temp_store: deny a statement that *sets* temp_store under a hardened policy (regex on the chunk), so a same-chunk `PRAGMA temp_store=FILE; CREATE TEMP …` can't escape the per-statement re-pin. Reads still allowed; the re-pin stays as belt-and-suspenders. Definitive fix remains SDK SQLITE_TEMP_STORE=3. - .dump/.fullschema: check outputCapped in their table/row/object loops so they stop querying after the output cap, instead of walking the whole DB silently. - interactive/process EOF leftover: guard the unterminated-statement execution with shouldQuit/budgetExceeded so a past-deadline final statement won't run. - audit ordering: record the attempted event before echo / EXPLAIN QUERY PLAN, so a fail-closed audit blocks all effects (incl. eqp DB work), not just evaluate. Updates the temp_store test to the deny-on-set behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8VSWQJZ3ZxVcG6kwiwygJ
|
@codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be3cdba047
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private func openLog() -> Int32 { | ||
| let path = url.path | ||
| var fd = path.withCString { | ||
| open($0, O_WRONLY | O_CREAT | O_APPEND | O_NOFOLLOW, mode_t(0o600)) |
There was a problem hiding this comment.
In the required Windows build, Sqlite3Shell is still compiled but canImport(Darwin)/canImport(Glibc) are false, so this new POSIX-only open(... O_NOFOLLOW ..., mode_t(...)) path has no imported definitions for open, O_*, or mode_t. Any Windows CI build of the package will fail before tests run unless this is guarded or implemented with WinSDK/Foundation equivalents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 715ad03 — important catch (confirmed there's a build-windows job that builds and tests). Audit.swift's O_NOFOLLOW append and Sqlite3Executable's realpath canonicalization are now guarded with #if canImport(Darwin) || canImport(Glibc) and have a Foundation fallback under #else for Windows. The Windows fallback loses the leaf-symlink (O_NOFOLLOW) guarantee — noted as a limitation — but the package builds and the audit trail works; POSIX behavior is unchanged. 👍
Generated by Claude Code
| /// used to refuse temp-store changes under a hardened policy; the | ||
| /// evasion-proof fix is the SDK compile flag SQLITE_TEMP_STORE=3. | ||
| private static func setsTempStore(_ sql: String) -> Bool { | ||
| sql.range(of: #"(?i)pragma\s+temp_store\s*[=(]"#, options: .regularExpression) != nil |
There was a problem hiding this comment.
When hardened mode is enabled, SQLite still accepts schema-qualified syntax such as PRAGMA main.temp_store=FILE;, but this matcher only catches PRAGMA temp_store.... In a same-line multi-statement chunk like PRAGMA main.temp_store=FILE; CREATE TEMP TABLE ...;, the statement reaches database.evaluate after the pre-statement re-pin and later statements in that same chunk can run with file-backed temp storage, bypassing the new hardened temp-store pinning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 715ad03 — the matcher now allows an optional schema qualifier: pragma\s+(?:\w+\s*\.\s*)?temp_store\s*[=(], so PRAGMA main.temp_store=FILE; is caught too. Added a test. 👍
Generated by Claude Code
|
|
||
| /// Force read-only opens regardless of argv. Off by default so hardened | ||
| /// mode does not block legitimate writes (incl. vec0/FTS5 index builds). | ||
| public var forceReadOnly: Bool |
There was a problem hiding this comment.
When an embedder sets forceReadOnly (with or without hardened mode), the flag is only applied to the main connection and .open; the .backup handler still creates its destination with SQLiteDatabase(.file(url.path)). In that context an untrusted script can run .backup allowed.db to create/write a database file despite the trusted read-only policy, so the policy needs to block .backup or make that auxiliary open honor forceReadOnly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 715ad03 — under forceReadOnly, .backup and .restore are now refused (a backup destination can't be opened read-only and still receive the copy, so blocking is the right call). 👍
Generated by Claude Code
| let url = Shell.resolve(path) | ||
| // Canonicalize before authorizing so the authorized path is the same | ||
| // symlink-resolved string the file op then uses (see canonicalize()). | ||
| let url = Sqlite3Executable.canonicalize(Shell.resolve(path)) |
There was a problem hiding this comment.
If an embedder places policy.auditURL under a path the sandbox authorizes, this shared resolver lets in-band dot-commands target the audit file itself; for example .output <auditURL> buffers output and later overwrites the JSONL trail after the attempted .output/.print records have been appended. Since audit is now a fail-closed trusted control, file-touching dot-commands should reject the resolved audit URL (or otherwise reserve it) instead of authorizing it like ordinary user output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 715ad03 — the Session now canonicalizes policy.auditURL once and resolveAuthorized refuses any file-touching dot-command (.output/.once/.backup/.restore/.import/.read/.open) that resolves to it, so an in-band .output <auditURL> can't overwrite the trail. Added a test. 👍
Generated by Claude Code
- P1 Windows: Audit.swift's O_NOFOLLOW append and Sqlite3Executable's realpath canonicalization were POSIX-only, breaking the build-windows job. Both now have a Foundation fallback under #else (no leaf-symlink guard on Windows, but it builds and the trail works); the POSIX path is unchanged on Darwin/Glibc. - temp_store deny now also matches the schema-qualified form (PRAGMA main.temp_store=…). - forceReadOnly now blocks .backup/.restore (which create/write a DB file in band despite the read-only policy). - The audit-log path is reserved: file-touching dot-commands (.output/.backup/…) are refused if they target the audit file, so the trail can't be overwritten. Adds tests for the three policy edge cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8VSWQJZ3ZxVcG6kwiwygJ
|
@codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 715ad03d0f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !safeMode, sql.range(of: "attach", options: .caseInsensitive) != nil { | ||
| for target in database.attachTargets(in: sql) | ||
| where !target.isEmpty && target != ":memory:" { | ||
| do { | ||
| try await Shell.authorize(Shell.resolve(target)) | ||
| try await Shell.authorize(Self.canonicalize(Shell.resolve(target))) |
There was a problem hiding this comment.
When the sandbox/force-read-only policy is active, this gate only pre-authorizes ATTACH targets before handing the whole SQL string to SQLite. VACUUM INTO '/tmp/copy.db' is also SQL-level file output and SQLite allows it even from a read-only source database, so an untrusted script can still create or overwrite a database file without going through Shell.authorize or the .backup read-only block. Please reject or authorize VACUUM INTO before database.evaluate(sql) under these policies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fc4cd51 — strong catch. VACUUM … INTO is now refused under a hardened/read-only policy (it writes a DB file directly, bypassing Shell.authorize and the .backup block, and works from a read-only source — same class as ATTACH). Detection strips comments first so VACUUM/**/INTO is caught too. Added a test. 👍
Generated by Claude Code
| sql.range(of: #"(?i)pragma\s+(?:\w+\s*\.\s*)?temp_store\s*[=(]"#, | ||
| options: .regularExpression) != nil |
There was a problem hiding this comment.
This regex only allows whitespace between PRAGMA and temp_store, but SQLite treats comments as token separators. In hardened mode, a single chunk such as PRAGMA/**/temp_store=FILE; CREATE TEMP TABLE ...; misses this check; the re-pin runs before the chunk, then SQLite changes temp_store and executes the following statements with file-backed temp storage. Strip/tokenize comments or enforce this in the engine before relying on the shell guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fc4cd51 — the temp_store guard (and the new VACUUM INTO guard) now strip /* … */ and -- … comments before matching, so PRAGMA/**/temp_store=FILE is caught. Over-stripping a comment marker inside a string literal can only cause a false positive (a refusal), which is acceptable under the strict hardened policy. The fully evasion-proof fix remains SQLITE_TEMP_STORE=3 in the SDK. Added a test. 👍
Generated by Claude Code
| // authorize is byte-identical to the one SQLite opens. This reduces | ||
| // — but does not eliminate — the symlink-swap window: fully closing | ||
| // it needs SQLITE_OPEN_NOFOLLOW, which lives in the SDK open. | ||
| let url = canonicalize(Shell.resolve(path)) |
There was a problem hiding this comment.
When an audit destination is configured, resolveAuthorized reserves that path for dot-commands, but the initial database filename is canonicalized and authorized without the same check. If untrusted argv names the audit log as the database, the run uses one file for both SQLite storage and the trusted JSONL trail, so the first audited statement can corrupt the database/log or otherwise defeat the assumption that audit is outside the DB. Compare the canonical database URL against policy.auditURL before opening it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fc4cd51 — the initial database open now rejects a path that canonicalizes to policy.auditURL (before authorize/open), matching the dot-command reservation, so the DB and the trail can never be the same file. Added a test. 👍
Generated by Claude Code
| func stopsOnError() -> Bool { context == .inline || bail } | ||
| for line in lines { | ||
| if shouldQuit { return true } | ||
| if budgetExceeded() { return false } |
There was a problem hiding this comment.
This budget check runs once per input line/chunk, but a single completed buffer can contain many semicolon-separated statements that are then handed to one database.evaluate call. If hardened SQL submits SELECT ...; SELECT ...; on one line, all of those statements run without another deadline check, so statementTimeout is not actually enforced between statements as documented. Split complete statements or move the deadline check into the per-statement evaluation path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged — this is the same SDK-boundary limitation as the intra-query interrupt, not separately fixable at the shell layer. The shell hands a whole ;-separated chunk to one evaluate, and without a SQL tokenizer it can't reliably split statements to insert deadline checks between them; even if it could, it still couldn't interrupt a single slow query. I deliberately didn't add a between-sets check that only gates output (execution already happened), to avoid implying enforcement that isn't real. I've updated the budgetExceeded doc to state the per-chunk / within-chunk limitation explicitly, and real per-statement + mid-query enforcement is specced as the sqlite3_progress_handler + sqlite3_interrupt SDK change in Docs/PORTING-FROM-SWIFTSQLITE.md. Leaving the shell budget as the coarse guard it is.
Generated by Claude Code
- P1 VACUUM INTO: refuse `VACUUM … INTO` under a hardened/read-only policy — it writes a database file directly, bypassing Shell.authorize and the .backup block (and works from a read-only source), same class as ATTACH. - temp_store guard now strips SQL comments first, so `PRAGMA/**/temp_store=FILE` is caught; the VACUUM INTO matcher uses the same stripping. - The initial database path is now rejected if it resolves to the audit log, matching the dot-command reservation (no shared file for DB + trail). - Documented that the script time budget is per-chunk and can't preempt between statements within one evaluate chunk — real enforcement needs the SDK progress handler (already specced), so not faked at the shell layer. Adds tests for VACUUM INTO, commented temp_store, and DB-path-is-audit-log. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8VSWQJZ3ZxVcG6kwiwygJ
|
@codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc4cd51bed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| strippedOfComments(sql).range( | ||
| of: #"(?i)pragma\s+(?:\w+\s*\.\s*)?temp_store\s*[=(]"#, | ||
| options: .regularExpression) != nil |
There was a problem hiding this comment.
In hardened mode this regex only recognizes a bare temp_store name, but SQLite accepts quoted pragma identifiers such as PRAGMA "temp_store"=FILE and PRAGMA main."temp_store"=FILE. An untrusted script can put one of those forms in the same statement chunk before temp-table or sort work, so the guard misses it and the pre-statement PRAGMA temp_store=MEMORY is immediately undone, allowing temp files despite the hardened policy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8a425a1 — the matcher now handles quoted identifiers ("temp_store", [temp_store], `temp_store`), an optional quoted schema qualifier, and no space before a quote (PRAGMA"temp_store"). Candidly, this is the 3rd lexical-evasion round on the same guard (comments, schema-qualifier, now quotes), which is exactly why I keep flagging it as defense-in-depth, not a boundary — the airtight fix is SQLITE_TEMP_STORE=3 at compile time in the SDK (specced in Docs/PORTING-FROM-SWIFTSQLITE.md). The realistic impact of a miss is temp files spilling within the already-sandbox-authorized workspace, not an escape. Added a quoted-form test. 👍
Generated by Claude Code
| if let auditURL = policy.auditURL, canonicalize(auditURL).path == url.path { | ||
| stderr.write("sqlite3: Error: the database path may not be the audit log\n") |
There was a problem hiding this comment.
When the audit log does not exist yet, a database path that is a symlink to that future log path is not resolved by canonicalize because realpath fails on the dangling leaf, so this comparison sees the symlink path rather than the audit path. preflight() then creates the log and SQLite opens the symlink as the database, putting the trusted audit trail and DB in the same file; creating/preflighting the log before this check or rechecking afterward would close that bypass.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8a425a1 — the audit log is now preflighted (created) before the database-path check, so canonicalize can resolve a database path that's a symlink to the (previously dangling) log and the DB==audit-log comparison catches it. 👍
Generated by Claude Code
| /// run, before execution — not which rows actually committed. A committed / | ||
| /// per-row tier needs SQLite's commit/update/rollback hooks, which live in the | ||
| /// SDK (out of this package) and are tracked as a follow-up. | ||
| public enum AuditEvent: Sendable { |
There was a problem hiding this comment.
This new file defines AuditEvent, AuditSink, NoOpAuditSink, FileAuditSink, and AuditWriteError together, but AGENTS.md for /workspace/SwiftPorts requires "One declaration per file." Please split these declarations into separate files so the new shell audit code follows the repo's stated SwiftPM organization convention.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8a425a1 — Audit.swift is split into one declaration per file (AuditEvent.swift, AuditSink.swift, NoOpAuditSink.swift, FileAuditSink.swift), per the AGENTS.md convention. The AuditWriteError type was dropped entirely (preflight throws NSError now), so there's no fifth file. 👍
Generated by Claude Code
| if await audit.record(.attempted( | ||
| kind: "sql", text: sql.trimmingCharacters(in: .whitespacesAndNewlines))) == false { |
There was a problem hiding this comment.
When an auditURL is configured, this records the full statement text before SQLite enforces the hardened SQLITE_LIMIT_SQL_LENGTH ceiling. A single oversized untrusted statement can therefore be serialized into the out-of-DB JSONL audit log first, growing it far beyond the configured SQL limit even if SQLite later rejects the statement; enforce the policy length before audit.record or truncate the audited text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8a425a1 — each audited record's text is now capped at 64 KiB (with a …[truncated] marker) before record, so an oversized untrusted statement can't bloat the out-of-DB JSONL trail ahead of the prepare-time SQLITE_LIMIT_SQL_LENGTH. 👍
Generated by Claude Code
…link
- Split Audit.swift into one-declaration-per-file (AGENTS.md convention):
AuditEvent.swift, AuditSink.swift, NoOpAuditSink.swift, FileAuditSink.swift.
Dropped the custom AuditWriteError type (preflight throws NSError instead).
- temp_store guard now also matches quoted identifiers ("temp_store",
[temp_store], `temp_store`), optional quoted schema qualifier, and no space
before a quote. Still defense-in-depth; SQLITE_TEMP_STORE=3 (SDK) is the
boundary.
- Cap each audited record's text (64 KiB) so an oversized untrusted statement
can't bloat the out-of-DB JSONL trail before SQLITE_LIMIT_SQL_LENGTH applies.
- Build/preflight the audit log before the DB-path check so canonicalize can
resolve a database path that is a symlink to the (previously not-yet-created)
log, closing that DB==audit-log bypass.
Adds a quoted-temp_store test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E8VSWQJZ3ZxVcG6kwiwygJ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a425a1633
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard policy.hardened else { return } | ||
| db.limit(SQLitePolicy.limitAttached, newValue: 0) |
There was a problem hiding this comment.
When an embedder sets forceReadOnly without also enabling hardened, this guard skips the only place that disables ATTACH, so SQL can still open a writable auxiliary database. A script can run ATTACH 'out.db' AS aux; CREATE TABLE aux.t(x); and create/write a new DB even though .backup and VACUUM INTO are blocked for the same read-only policy; disable ATTACH whenever forceReadOnly is true as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 11995c4 — applyHardening now sets SQLITE_LIMIT_ATTACHED=0 whenever the policy is hardened or forceReadOnly, so a read-only-but-not-hardened policy also blocks ATTACH (which would open a writable auxiliary DB). Added a test. 👍
Generated by Claude Code
| // `temp_store`; and no space before a quote (`PRAGMA"temp_store"`). | ||
| // Quote chars: " [ ] ` (backtick = \x60). | ||
| strippedOfComments(sql).range( | ||
| of: #"(?i)\bpragma\b\s*(?:[\"\[\x60]?\w+[\"\]\x60]?\s*\.\s*)?[\"\[\x60]?temp_store[\"\]\x60]?\s*[=(]"#, |
There was a problem hiding this comment.
Fresh evidence: the quoted-identifier fix handles double quotes/brackets/backticks, but SQLite also accepts single-quoted pragma names such as PRAGMA 'temp_store'=FILE and PRAGMA main.'temp_store'=FILE. In hardened mode, a same-chunk input like PRAGMA 'temp_store'=FILE; CREATE TEMP TABLE ...; misses this regex, runs after the one pre-evaluate re-pin, and lets later statements use file-backed temp storage; include single quotes here or tokenize the PRAGMA name instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partially addressed, with a deliberate trade-off in 11995c4 — and I want to be transparent rather than keep playing whack-a-mole on this guard (this is the 4th temp_store lexical variant: comments → schema → double-quote/bracket/backtick → now single-quote).
The single-quoted form PRAGMA 'temp_store' directly conflicts with the sibling finding (#3439521916) that ordinary SQL like SELECT 'vacuum into' must not be refused. Both hinge on single-quoted strings, and at the lexical level you cannot simultaneously (a) treat 'temp_store' right after PRAGMA as a name to block and (b) treat '…' as inert text everywhere else — only a real SQL tokenizer distinguishes them. I chose to prioritize zero false positives on legitimate SQL (strip single-quoted literals before matching), which means the single-quoted pragma-name form is an accepted, documented gap (see the setsTempStore comment). The double-quote / […] / backtick identifier forms are still caught.
Why this is acceptable: the impact of a missed temp_store change is temp files spilling within the already-sandbox-authorized workspace, not a sandbox escape; and the airtight fix is the compile-time SQLITE_TEMP_STORE=3 in the SDK, specced in Docs/PORTING-FROM-SWIFTSQLITE.md. So I'm treating further single-/lexical-variant temp_store reports as already-addressed-best-effort unless they show an actual escape.
Generated by Claude Code
| private static func reportFailure(url: URL, reason: String) { | ||
| FileHandle.standardError.write( | ||
| Data("sqlite3: AUDIT WRITE FAILED for \(url.path): \(reason)\n".utf8)) |
There was a problem hiding this comment.
If the audit log becomes unwritable after preflight, this diagnostic bypasses the stderr sink passed to Sqlite3Executable and writes the raw audit URL to the process-global stderr. In embedded ShellKit runs that means the failure is not captured by the caller's virtual stderr/pipeline and can leak a host path that the rest of the shell code normally folds with Shell.displayPath; report this through the session's stderr path or omit the raw path here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 11995c4 — FileAuditSink no longer writes to the process-global stderr (that bypassed the shell's stderr sink and leaked the host path). record just returns false; the caller already fails closed and reports through the session's stderr (err(...)) without the path. 👍
Generated by Claude Code
| /// are stripped first so `VACUUM/**/INTO` is caught. | ||
| private static func writesViaVacuumInto(_ sql: String) -> Bool { | ||
| strippedOfComments(sql).range( | ||
| of: #"(?i)\bvacuum\b[^;]*\binto\b"#, options: .regularExpression) != nil |
There was a problem hiding this comment.
In hardened or forced-read-only runs, this regex scans raw SQL after only comment stripping, so ordinary statements such as SELECT 'vacuum into'; or INSERT INTO t VALUES('vacuum into') are refused even though they do not write a file. Since the policy docs say hardened mode preserves the SQL feature surface and only blocks the escape boundary, the guard should ignore string literals (or tokenize) before matching VACUUM ... INTO.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 11995c4 — the guard matchers now strip single-quoted string literals (in addition to comments) before matching, so SELECT 'vacuum into' / INSERT … VALUES('vacuum into') are no longer refused, and the VACUUM matcher is tightened to VACUUM [schema] INTO. Added a test asserting the string-literal case runs normally. 👍
Generated by Claude Code
…positives - forceReadOnly now also disables ATTACH (LIMIT_ATTACHED=0), even without hardened mode — ATTACH opens a writable auxiliary DB, an in-band file write the read-only policy must block (like .backup / VACUUM INTO). - FileAuditSink no longer writes failures to process-global stderr (it bypassed the shell's stderr sink and leaked the host path). The caller already fails closed and reports through the session stderr; record() just returns false. - Guard matchers now strip single-quoted string literals (as well as comments) before matching, so legitimate SQL like `SELECT 'vacuum into'` is no longer refused, and tightened the VACUUM matcher to `VACUUM [schema] INTO`. Consequence/accepted gap: a single-quoted PRAGMA *name* (`PRAGMA 'temp_store'`) is stripped as a literal and not caught — distinguishing it from a value literal needs a real tokenizer (the SDK SQLITE_TEMP_STORE=3 boundary). The double-quote/bracket/backtick identifier forms are still caught. Adds tests: VACUUM-INTO-in-a-string-literal allowed; read-only blocks ATTACH. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8VSWQJZ3ZxVcG6kwiwygJ
|
@codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11995c4e1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // hardened policy it may show or *lower* a limit, but a raise above | ||
| // the current value is refused — otherwise `.limit attached 10` | ||
| // would undo applyHardening()'s SQLITE_LIMIT_ATTACHED=0. | ||
| if policy.hardened, newValue > database.limit(entry.code) { |
There was a problem hiding this comment.
When an embedder sets SQLitePolicy(hardened: false, forceReadOnly: true), applyHardening initially sets SQLITE_LIMIT_ATTACHED to 0, but this guard only rejects .limit raises when policy.hardened is true. An untrusted script can therefore run .limit attached 1 before ATTACH 'out.db' AS aux; and regain a writable auxiliary database despite the read-only policy, bypassing the boundary this change explicitly tries to enforce for forceReadOnly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 74338b8 — the .limit raise guard now fires under forceReadOnly as well as hardened, so .limit attached 1 can't undo the read-only ATTACH lockout. Good catch (consequence of the read-only-ATTACH change last round). Added a test. 👍
Generated by Claude Code
| /// This bounds *output* (what flows back to the caller), not the engine's | ||
| /// per-statement materialization — a true row cap needs the SDK stepping API. | ||
| private func out(_ s: String) { | ||
| guard policy.hardened, let cap = policy.maxResultBytes else { rawOut(s); return } |
There was a problem hiding this comment.
With a hardened maxResultBytes policy, only writes routed through out are capped; stderr still goes straight through err. A malicious script can therefore emit arbitrarily large output via errors, for example a huge unknown dot-command or invalid SQL line that report echoes with a caret, so the caller/LLM still receives unbounded bytes despite the hardened output cap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 74338b8 — out and err now share one capped emit path, so the hardened maxResultBytes budget covers stderr (errors, caret blocks) as well as stdout. A script can no longer exfiltrate unbounded bytes via the error stream. Added a test. 👍
Generated by Claude Code
| do { | ||
| try await sink.preflight() | ||
| } catch { | ||
| stderr.write("sqlite3: Error: cannot open audit log \"\(auditURL.path)\": \(error)\n") |
There was a problem hiding this comment.
If an embedder supplies an audit URL outside the sandbox and preflight fails, this error reports auditURL.path verbatim to the untrusted command's stderr. In a sandboxed SwiftBash context that leaks the trusted host filesystem location of the audit trail even though the destination is intentionally embedder-controlled and not part of the in-band path surface.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 74338b8 — the audit-log preflight error no longer echoes the raw host path; it prints a generic "cannot open the configured audit log" plus the errno message (which carries no path). 👍
Generated by Claude Code
| var s = sql.replacingOccurrences( | ||
| of: #"'(?:[^']|'')*'"#, with: " ", options: .regularExpression) |
There was a problem hiding this comment.
SQLite accepts single-quoted pragma names, so a hardened script can use PRAGMA 'temp_store'=FILE; and this stripper removes the name before setsTempStore checks it. When that pragma is placed in the same input chunk as later temp-heavy SQL, the pre-execution re-pin happens before the pragma and the in-band statement can still switch temp storage back to files despite the hardened temp confinement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the same single-quoted PRAGMA 'temp_store' case from the previous round (#discussion_r3439521910), which I addressed there with a deliberate, documented trade-off rather than a fix — and I'm holding that position.
Recap of why it's not chased: catching PRAGMA 'temp_store' (single-quoted name) and not refusing ordinary SQL like SELECT 'vacuum into' (single-quoted value) are mutually exclusive at the lexical level — only a real tokenizer tells a string-as-name from a string-as-value. I chose zero false positives on legitimate SQL, so the single-quoted pragma-name form is an accepted gap, marked in the setsTempStore source comment. Impact is bounded (temp files within the already-authorized workspace, not an escape), and the airtight fix is SQLITE_TEMP_STORE=3 in the SDK (Docs/PORTING-FROM-SWIFTSQLITE.md). Marking this resolved-by-design.
Generated by Claude Code
- .limit raise guard now also fires under forceReadOnly (not just hardened), so `.limit attached 1` can't undo the read-only ATTACH lockout. - The hardened output cap now covers stderr as well as stdout (one shared budget via a unified emit path), so a script can't exfiltrate unbounded bytes through error/caret output. - The audit-log preflight error no longer echoes the raw host path of the embedder-set log to the untrusted command's stderr (the errno message has no path). The single-quoted PRAGMA 'temp_store' report is the same accepted, documented lexical-guard gap from the prior round (see setsTempStore comment); not chased further — the boundary is SQLITE_TEMP_STORE=3 in the SDK. Adds tests: read-only blocks .limit raise; hardened cap covers stderr. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8VSWQJZ3ZxVcG6kwiwygJ
|
@codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74338b8920
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| strippedForGuards(sql).range( | ||
| of: #"(?i)\bpragma\b\s*(?:[\"\[\x60]?\w+[\"\]\x60]?\s*\.\s*)?[\"\[\x60]?temp_store[\"\]\x60]?\s*[=(]"#, | ||
| options: .regularExpression) != nil |
There was a problem hiding this comment.
Under a hardened policy, SQLite still accepts PRAGMA 'temp_store'=FILE; and changes temp storage to disk, but this check calls strippedForGuards before matching, which removes the single-quoted pragma name and lets the statement execute. That gives an untrusted script a direct bypass of the temp-store pinning this policy is trying to enforce; handle the single-quoted pragma-name form before stripping literals or tokenize PRAGMA statements.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same single-quoted PRAGMA 'temp_store' case as rounds 6–7 — still resolved-by-design (documented accepted gap in the setsTempStore comment). The statement anchoring added in 1beafb0 tightens the guard further (no value-position false positives) but, as explained before, the single-quoted name form can't be caught lexically without also breaking ordinary '…' value literals — that needs the engine authorizer / SQLITE_TEMP_STORE=3 (SDK), and its impact is bounded to temp-file spill within the already-authorized workspace. Not chasing this lexical variant further.
Generated by Claude Code
| if outputCapped { break } // stop querying once the cap is hit | ||
| guard case .text(let name) = t[0], case .text(let createSQL) = t[1] else { continue } | ||
| out(createSQL + ";\n") |
There was a problem hiding this comment.
When a hardened .dump hits the output cap while writing a table's CREATE statement, outputCapped becomes true after this out(...), but the code still continues to call nonGeneratedColumns and database.evaluate("SELECT ... FROM ...") for that table. For a table with a long schema and many rows, an untrusted script can therefore force the shell to materialize the whole table even though no row output will be emitted; add a cap check immediately after emitting the schema line before querying rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1beafb0 — .dump now re-checks outputCapped immediately after emitting a table's CREATE line, before calling nonGeneratedColumns / SELECT … FROM. So once the cap is hit, a capped dump won't keep introspecting and materializing later tables' rows. 👍
Generated by Claude Code
| /// policy guards match only real keywords/identifiers — never text inside a | ||
| /// literal or comment (which would wrongly refuse statements like | ||
| /// `SELECT 'vacuum into'`). Detection only; the original SQL executes. | ||
| /// Strings are stripped first so a comment marker inside a string (`'-- x'`) | ||
| /// isn't mistaken for a comment. |
There was a problem hiding this comment.
SQLite's default double-quoted-string behavior means statements such as SELECT "vacuum into"; or SELECT "pragma temp_store=("; are valid ordinary queries, but strippedForGuards only removes single-quoted literals, leaving those double-quoted values for the hardened lexical guards to misclassify as policy violations. In hardened/read-only mode this breaks normal queries solely because a string value contains guarded keywords; use a tokenizer or otherwise distinguish double-quoted literals from identifiers before scanning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1beafb0 — both guards are now anchored to a statement boundary (^ or after ;), so a keyword inside a value (SELECT "vacuum into", SELECT "pragma temp_store=(") is never matched (VACUUM/PRAGMA only act as keywords at statement start, not mid-expression). This removes the double-quoted-string false positives without needing to distinguish DQS identifiers from strings. Added a regression test. 👍
Generated by Claude Code
| // Reserve the audit log: a file-touching dot-command must not target it, | ||
| // or an in-band `.output <auditURL>` could overwrite the trusted trail. | ||
| if let auditCanonicalPath, url.path == auditCanonicalPath { |
There was a problem hiding this comment.
If the configured audit log already exists and the script names a hard link to it, this path-string check does not fire because the canonical paths differ even though both names reference the same inode. A command such as .backup <hardlink> can then open and overwrite the trusted JSONL trail that the policy is trying to reserve; for existing paths, compare file identity (device/inode) in addition to canonical path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1beafb0 — the audit-log reservation now also compares by file identity (fileResourceIdentifier, i.e. inode) in addition to canonical path, so a hard link / alternate name to the existing trail is caught — by both the initial DB-path check and the file-touching dot-commands (.backup/.output/…). 👍
Generated by Claude Code
- Anchor the temp_store and VACUUM-INTO lexical guards to a statement boundary
(^ or after ;), so a guarded keyword inside a *value* — `SELECT 'vacuum into'`,
`SELECT "pragma temp_store=("` — is never misclassified as a policy violation
(fixes the double-quoted-string false positives). Real catches (leading
PRAGMA/VACUUM, incl. quoted-identifier and schema-qualified) still fire.
The single-quoted PRAGMA-name form remains the documented accepted gap.
- .dump: re-check outputCapped right after emitting a table's CREATE line, so a
capped dump doesn't still introspect columns / materialize that table's rows.
- Audit-log reservation now also compares by file identity (inode via
fileResourceIdentifier), so a hard link / alternate name to the trail is
caught by both the DB-path check and the file-touching dot-commands.
Adds a test that guarded keywords inside a string value are allowed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E8VSWQJZ3ZxVcG6kwiwygJ
|
@codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1beafb0c0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let auditCanonicalPath, | ||
| url.path == auditCanonicalPath || Sqlite3Executable.sameFile(url.path, auditCanonicalPath) { | ||
| err("Error: cannot direct a command at the audit log\n") | ||
| return nil |
There was a problem hiding this comment.
When a file-touching dot-command is refused because it targets the audit log, this branch only prints an error and returns nil. Callers such as .output, .once, .backup, .restore, and .open just return on nil, so under an audit policy a script containing only .output <auditURL> reports success (exit 0) even though the trusted policy blocked the command. Set exitCode = 1 before returning nil so these policy denials are observable like the other hardened/read-only refusals.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6b1f846. resolveAuthorized now sets exitCode = 1 on the audit-log-target denial, so refusing a dot-command that points at the audit log is observable to the caller like every other policy refusal (and .bail semantics apply).
Generated by Claude Code
| var s = sql.replacingOccurrences( | ||
| of: #"'(?:[^']|'')*'"#, with: " ", options: .regularExpression) |
There was a problem hiding this comment.
strippedForGuards removes only single-quoted literals before the temp-store and VACUUM guards search for statement-boundary keywords. SQLite's default double-quoted-string behavior accepts inputs like SELECT "x; PRAGMA temp_store=FILE";, but the guard sees the semicolon inside the string and rejects it under -hardened even though no PRAGMA executes. Strip double-quoted tokens as well, or use a tokenizer, so hardened mode preserves ordinary text values containing SQL snippets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6b1f846. strippedForGuards now strips double-quoted strings ("…" with "" escaping) in addition to single-quoted ones and comments, before the VACUUM-INTO guard runs. This is safe here because the only remaining guard looks for the VACUUM/INTO keywords, which are never identifiers — so removing double-quoted tokens can't hide a real statement keyword, it only prevents false-positives on SELECT "x; vacuum into y".
Note this also closes the related temp_store-guard reports (single/double/bracket-quoted, schema-qualified, commented forms): rather than keep extending an unsound lexical matcher, I removed the PRAGMA temp_store guard entirely — see the reply on that thread. The double-quote stripping now only serves the VACUUM-INTO guard.
Generated by Claude Code
| var fd = path.withCString { | ||
| open($0, O_WRONLY | O_CREAT | O_APPEND | O_NOFOLLOW, mode_t(0o600)) | ||
| } |
There was a problem hiding this comment.
When the configured audit path already exists as a FIFO, character device, or other special file, this open accepts it because O_NOFOLLOW only rejects symlinks. A FIFO can block preflight() before any SQL runs, while a sink such as /dev/null makes record return success without leaving a JSONL trail, so an explicit audit policy no longer fails closed. After opening, fstat the fd and require a regular file before treating audit as enabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6b1f846. After the O_NOFOLLOW open, openLogPOSIX now fstats the fd and requires a regular file (st_mode & S_IFMT == S_IFREG); a FIFO, device, or socket at the leaf closes the fd and returns -1 with EINVAL, so preflight() throws and the run fails closed instead of writing the trail into a non-file. (Windows/non-POSIX still uses the Foundation fallback, which only creates a regular file anyway.)
Generated by Claude Code
| let keys: Set<URLResourceKey> = [.fileResourceIdentifierKey] | ||
| guard | ||
| let idA = try? URL(fileURLWithPath: a).resourceValues(forKeys: keys).fileResourceIdentifier, | ||
| let idB = try? URL(fileURLWithPath: b).resourceValues(forKeys: keys).fileResourceIdentifier | ||
| else { return false } |
There was a problem hiding this comment.
This hard-link guard depends on Foundation's fileResourceIdentifierKey, which is not reliably populated on Linux (it returns nil in this environment), so sameFile falls back to false for two paths that are actually the same inode. In Linux sandboxed runs, an existing hard link to the audit log can therefore be used as the database or a dot-command target without tripping the reservation check, putting the audit trail and SQLite storage on the same file. Use stat/GetFileInformationByHandle-style device+inode identity for this policy check instead of this optional resource key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6b1f846. sameFile now compares POSIX stat st_dev + st_ino directly instead of Foundation's fileResourceIdentifierKey (which returns nil on Linux, making the hard-link check a no-op there). On non-POSIX platforms it returns false and callers fall back to the exact canonical-path comparison.
Generated by Claude Code
| if await audit.record(.attempted(kind: "dot", text: Self.cappedAuditText(line))) == false { | ||
| err("Error: audit write failed; refusing unaudited command\n") |
There was a problem hiding this comment.
For dot-commands, .echo on writes the command line in process before control reaches this audit check, unlike SQL statements where audit happens before echoing. If the audit sink starts failing after preflight (for example disk full or a revoked log path), a script can still emit unaudited dot-command text before the fail-closed refusal. Move the echo to after a successful dot-command audit so audit failure prevents all externally visible effects of that command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6b1f846. The -echo for dot-commands moved out of both dispatch sites (process and runInteractive) into handleDot, where it now runs after the attempted-audit record check. So if the audit log can't be written, the command is refused and not echoed — no echo without a durable trail, matching the SQL-statement path.
Generated by Claude Code
…cho-after-audit
- Remove the unsound lexical PRAGMA temp_store guard entirely. Across rounds
it could not distinguish the pragma name from a value across quote styles
('…' "…" […] `…`) and comments. Temp confinement now rests on the
per-statement temp_store=MEMORY re-pin (cross-statement) and the SDK
SQLITE_TEMP_STORE=3 compile flag (within-chunk). VACUUM INTO — a real file
write with no identifier/value ambiguity — keeps its anchored guard, now
stripping both quote styles. (#390)
- FileAuditSink: fstat the opened fd and require a regular file, so a FIFO,
device, or socket at the leaf fails the open and audit fails closed. (#99)
- Echo dot-commands inside handleDot after the attempted-audit check, so a
fail-closed audit suppresses the echo too. (#720)
- sameFile: POSIX stat dev+inode instead of Foundation's
fileResourceIdentifierKey (nil on Linux). (#195)
- resolveAuthorized: set exitCode=1 on the audit-log-target denial. (#1174)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E8VSWQJZ3ZxVcG6kwiwygJ
|
@codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b1f84696a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| strippedForGuards(sql).range( | ||
| of: #"(?i)(?:^|;)\s*vacuum\b\s+(?:\w+\s+)?into\b"#, | ||
| options: .regularExpression) != nil |
There was a problem hiding this comment.
In hardened/read-only mode this guard is the only thing preventing VACUUM INTO from writing an arbitrary database file, but the optional schema match only accepts an unquoted \w+. SQLite also accepts quoted schema identifiers such as VACUUM [main] INTO 'out.db'; and VACUUM `main` INTO 'out.db';, so those forms skip writesViaVacuumInto and then run through database.evaluate, bypassing both Shell.authorize and the hardened/read-only write block.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a59f0f5. writesViaVacuumInto's optional schema group now matches a bracketed ([main]) or back-quoted (`main`) identifier as well as a bare \w+, so VACUUM [main] INTO 'out.db' and the backtick form are now refused under the hardened/read-only policy. (A double-quoted schema "main" was already neutralized — strippedForGuards collapses it to whitespace that the (?:…)? skips.) Added hardenedBlocksQuotedSchemaVacuumInto covering both forms.
Generated by Claude Code
| if !leftover.isEmpty, !shouldQuit, !budgetExceeded() { | ||
| if !(await runStatement(leftover, startLine: statementStart, context: context)) && stopsOnError() { |
There was a problem hiding this comment.
When the only or last input line is already a complete statement and the script has no trailing newline, the budget is checked before runStatement and then this leftover-only end check is skipped, so a statement that runs past statementTimeout can finish and still return exit 0. Please check the budget again after each completed statement or unconditionally at the end so hardened callers can detect an exceeded budget even for the final statement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a59f0f5. process now re-checks the budget after the statement loop (if !shouldQuit { _ = budgetExceeded() }). The final completed statement runs inside the loop, where the budget is only checked before the next line, and the leftover branch is skipped when there's no trailing fragment — so a last statement that overran statementTimeout previously still returned 0. budgetExceeded() sets exitCode = 1; the shouldQuit guard avoids re-reporting a budget already reported above or a .quit.
Generated by Claude Code
| #else | ||
| return false // non-POSIX: rely on the canonical-path comparison |
There was a problem hiding this comment.
On Windows or any other non-Darwin/Glibc build this always returns false, but resolveAuthorized relies on it to reject alternate names for the configured audit log. If an allowed directory contains a hard link to that log, file-touching dot commands can pass the path comparison because the spellings differ, letting an untrusted script target the audit trail despite the reservation that works on POSIX; use a platform file-id comparison or fail closed here instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a59f0f5. The non-POSIX (#else) branch of sameFile now falls back to Foundation's fileResourceIdentifierKey comparison instead of returning false, so on Windows a hard link / alternate name to the configured audit log is still caught by resolveAuthorized's reservation. Darwin/Glibc keep the POSIX stat dev+inode path (Foundation's id is nil on Linux, which was the original #195 bug). This is best-effort on Windows, consistent with FileAuditSink's documented Windows leaf-guard limitation.
Generated by Claude Code
…t, Windows audit identity - writesViaVacuumInto: the optional schema name now also matches bracketed ([main]) and back-quoted (`main`) identifiers, not just bare \w+. SQLite accepts those, so VACUUM [main] INTO 'out.db' previously skipped the guard and wrote a file directly, bypassing Shell.authorize and the read-only block. (double-quoted schema was already handled by strippedForGuards.) (P1) - process(): re-check the time budget after the statement loop. The final completed statement runs inside the loop, where the budget is only checked *before* the next line; with no trailing leftover that check was skipped, so a last statement overrunning statementTimeout still exited 0. (P2) - sameFile: restore a Foundation file-id comparison on non-POSIX (Windows) builds instead of returning false, so a hard link / alternate name to the audit log is still caught there (best-effort, matching the Windows leaf-guard caveat); Darwin/Glibc keep the POSIX stat dev+inode path. (P2) - Test: hardenedBlocksQuotedSchemaVacuumInto covers the bracket/backtick forms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8VSWQJZ3ZxVcG6kwiwygJ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24223e86e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| where !target.isEmpty && target != ":memory:" { | ||
| do { | ||
| try await Shell.authorize(Shell.resolve(target)) | ||
| try await Shell.authorize(Sqlite3Executable.canonicalize(Shell.resolve(target))) |
There was a problem hiding this comment.
When an audit sink is configured without hardened/read-only mode, ATTACH targets only go through sandbox authorization here. Unlike the initial database and .open/.backup paths, this never calls the audit-log collision check, so a policy with auditURL set to /tmp/a.db-journal can run ATTACH '/tmp/a.db' AS aux; CREATE TABLE aux.t(x); and let SQLite create/delete the journal sidecar over the JSONL trail after the attempt record was written. Apply the same audit sidecar reservation to ATTACH targets before evaluating the statement.
Useful? React with 👍 / 👎.
| // `.backup` creates/writes a database file — a write the trusted | ||
| // read-only policy must not allow in-band (the destination can't be | ||
| // opened read-only and still receive the backup). | ||
| if policy.forceReadOnly { |
There was a problem hiding this comment.
This adds explicit forceReadOnly refusals for .backup/.restore, but .import above is also an in-band database write and still proceeds to read the whole file before calling importDelimited. On a forced read-only connection the SQLite write error is caught inside introspect without setting exitCode, so a script containing only .import can report success after the policy prevented the write; reject it before resolving/reading the import file.
Useful? React with 👍 / 👎.
| if policy.hardened || policy.forceReadOnly, Self.writesViaVacuumInto(sql) { | ||
| err("Error: VACUUM INTO is not permitted under the hardened/read-only policy\n") |
There was a problem hiding this comment.
When an embedder configures only auditURL (without hardened/read-only), this guard does not run and VACUUM INTO also bypasses the path reservation used for database opens. With auditURL set to /tmp/out.db-journal, a script can run VACUUM INTO '/tmp/out.db'; and let SQLite remove or rewrite that journal sidecar, erasing the JSONL trail even though the action was audited; either reserve the derived sidecars for VACUUM targets or block this form whenever an audit sink is active.
Useful? React with 👍 / 👎.
Three Codex P2s from the 24223e8 review, completing audit-collision / read-only coverage across the remaining SQLite-write entry points: - VACUUM INTO: the guard was gated on hardened/read-only, so with only auditURL set, `VACUUM INTO '/tmp/out.db'` could rewrite /tmp/out.db-journal (= the audit log). Also refuse VACUUM INTO whenever an audit sink is active — it writes a DB file directly that can't be reserved against the trail or audited. - ATTACH targets: went through sandbox authorize but skipped the audit-collision check, so ATTACH of a base whose -wal/-journal sidecar is the audit log could clobber the trail. Apply auditCollidesWithDatabase() to ATTACH targets too. - .import: an in-band DB write not refused under forceReadOnly like .backup/.restore; worse, the read-only INSERT error was swallowed inside introspect() without setting exitCode, so an .import-only script reported success. Refuse it up front under read-only. Adds three regression tests. CI was fully green on 24223e8 (all 5 platforms). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a617cccad3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let auditCanonicalPath, | ||
| Sqlite3Executable.auditCollidesWithDatabase(auditPath: auditCanonicalPath, dbPath: url.path) { | ||
| err("Error: cannot direct a command at the audit log (or its -wal/-shm/-journal sidecar)\n") | ||
| exitCode = 1 // make the policy denial observable, like other refusals |
There was a problem hiding this comment.
When .bail on is active, this policy denial sets exitCode but does not set shouldQuit, and process continues unconditionally after handleDot returns. For example, under an audit policy, .bail on followed by .output <auditURL> and then SELECT 1; still executes the SELECT after the audit-log refusal, even though sqlite dot-command errors should stop the script when bail is enabled; set shouldQuit for these denied dot-command paths when bail is true or have handleDot report failure to the caller.
Useful? React with 👍 / 👎.
Codex P2 (a617ccc review): under `.bail on` (or the inline command-line context), a dot-command policy denial — resolveAuthorized's audit-log refusal, the read-only .backup/.restore/.import refusals, etc. — set exitCode but not shouldQuit, and `process` kept going. So `.bail on` followed by a denied `.output <auditURL>` and then `SELECT 1;` still ran the SELECT. Mirror the statement path (`... && stopsOnError() { return false }`): a dot-command that changes exitCode now halts the script when bail/inline requires stop-on-error. The interactive REPL is unchanged — it intentionally never halts on per-statement errors. Adds a regression test. CI was fully green on a617ccc (all 5 platforms). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13e5b7627b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // when bail/inline says stop-on-error — matching the statement | ||
| // path below — rather than letting later statements run. handleDot | ||
| // reports failure by setting exitCode. (Codex review P2, PR #1.) | ||
| if exitCode != exitCodeBefore, stopsOnError() { return false } |
There was a problem hiding this comment.
Fresh evidence: the new stop-on-error check infers dot-command failure only from an exit-code change. If an earlier statement already set exitCode to 1, then .bail on followed by a denied dot-command such as .output <auditURL> leaves exitCode unchanged, this condition is false, and the script continues to execute subsequent SQL despite bail being enabled; have handleDot return a per-command success/failure or track a separate error flag instead of comparing the accumulated process exit code.
Useful? React with 👍 / 👎.
| // holds across statement boundaries even if an earlier statement set | ||
| // FILE; fail closed if it can't be applied (see helper). (The within-chunk | ||
| // case is covered by the SDK compile flag SQLITE_TEMP_STORE=3.) | ||
| if !repinTempStoreOrFailClosed() { return false } |
There was a problem hiding this comment.
When .eqp on is active under a hardened policy, renderQueryPlan(sql) runs a database.evaluate("EXPLAIN QUERY PLAN ...") before this re-pin executes. A script can first run PRAGMA temp_store=FILE;, enable .eqp, and then the next statement's query-plan evaluation runs with FILE temp storage even though the main statement is re-pinned afterward; move the fail-closed re-pin ahead of any DB-backed prework such as EQP rendering.
Useful? React with 👍 / 👎.
| let replacement = try SQLiteDatabase(location) | ||
| // Honor the trusted read-only policy on the new handle too — | ||
| // otherwise `.open writable.db` would relax forceReadOnly in-band. | ||
| let replacement = try SQLiteDatabase(location, readonly: policy.forceReadOnly) |
There was a problem hiding this comment.
With forceReadOnly set, .open now opens the replacement handle read-only, so opening a missing or otherwise non-readable target throws here; the catch below only prints the SQLite error and leaves exitCode unchanged. A policy-bound script containing just .open new.db can therefore report success even though the trusted read-only policy prevented the requested database switch, unlike the other read-only dot-command denials that set a nonzero exit.
Useful? React with 👍 / 👎.
| do { | ||
| database = try SQLiteDatabase(location, readonly: options.readonly) | ||
| database = try SQLiteDatabase(location, | ||
| readonly: options.readonly || policy.forceReadOnly) |
There was a problem hiding this comment.
When the trusted policy forces this handle read-only, an untrusted argv can still select -interactive; SQLite write failures then go through the .interactive branch of report, which deliberately does not set exitCode. A piped interactive run such as CREATE TABLE t(x); therefore returns 0 even though the read-only policy blocked the write, so policy-driven SQLite errors need to set a nonzero exit even in interactive mode.
Useful? React with 👍 / 👎.
| if let auditCanonicalPath, | ||
| Sqlite3Executable.auditCollidesWithDatabase(auditPath: auditCanonicalPath, dbPath: url.path) { |
There was a problem hiding this comment.
Under forceReadOnly, this shared path check reserves the audit log but still lets write-capable dot-commands target the currently open database file. A script can open /tmp/db read-only, run .output /tmp/db (or .once /tmp/db) and .print ..., and finishOutput will overwrite the database file outside SQLite despite the trusted read-only policy; write redirects should reject the current database path and its sidecars as well.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| } | ||
| return failure |
There was a problem hiding this comment.
The audit contract says record only succeeds once the event is durably recorded, but this POSIX path returns success after write(2) without an fsync/fdatasync. If the process or host crashes after the subsequent SQL commits, the JSONL attempt can be lost even though the audit sink returned true and allowed the action to run; sync the fd before returning success, or weaken the durability guarantee.
Useful? React with 👍 / 👎.
From Codex's 13e5b76 review — completing policy-denial exit-code coverage, read-only enforcement, and audit durability: - bail (:577): track each dot-command's failure in isolation — zero exitCode across the handleDot call, then restore the prior code — so a denial halts under bail even when an earlier (non-bail) error already set exitCode. Replaces the exitCode-delta heuristic that masked the failure in that case. - interactive (:122): a read-only policy's errors now set a nonzero exit even under -interactive, so an untrusted argv can't dodge enforcement via the REPL. - .open (:1267): a denied read-only .open now reports failure, not exit 0. - output redirect (:1322): under read-only, .output/.once may no longer target the open database file or its -wal/-shm/-journal sidecars — finishOutput would otherwise overwrite the database outside SQLite. - EQP (:708): re-pin temp_store=MEMORY before EXPLAIN QUERY PLAN rendering so temp confinement covers the EQP evaluate(), not just the main statement. - audit durability (FileAuditSink): fsync (POSIX) / synchronize (Foundation) before record() returns success, honoring the durably-recorded contract. Adds 4 regression tests. CI was fully green on a617ccc / 13e5b76. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b32fa5b261
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // event is on disk, so flush before returning success — a bare write(2) | ||
| // leaves the JSONL in the page cache, which a crash after the | ||
| // subsequent SQL commit would lose. (Codex review P2, PR #1.) | ||
| if failure == nil, fsync(fd) != 0 { |
There was a problem hiding this comment.
When this run creates a new audit log, fsync(fd) flushes the file contents/inode but does not make the parent directory entry durable on POSIX filesystems. A crash after the first successful record and subsequent SQL commit can therefore lose the just-created JSONL file even though record returned true under the durable-audit contract; track whether O_CREAT created the file and fsync the parent directory before allowing execution.
Useful? React with 👍 / 👎.
| if let auditCanonicalPath, | ||
| Sqlite3Executable.auditCollidesWithDatabase(auditPath: auditCanonicalPath, dbPath: resolved.path) { | ||
| err("Error: cannot ATTACH a database whose path (or -wal/-shm/-journal sidecar) is the audit log\n") |
There was a problem hiding this comment.
This reservation only runs for concrete targets returned from attachTargets(in:), but SQLite accepts the ATTACH filename as an expression such as ATTACH '/tmp/' || 'a.db' AS aux. Any expression form not returned here proceeds to database.evaluate without the new audit-sidecar collision check, so under an audit-only policy with auditURL set to /tmp/a.db-journal, SQLite can still create/delete that journal sidecar after the attempted record is written. Reject non-literal ATTACH targets when audit is active or capture the evaluated filename via SQLite itself.
Useful? React with 👍 / 👎.
| // error under that policy (e.g. a blocked write), even though the plain | ||
| // REPL leaves exit unset. (Codex review P2, PR #1.) | ||
| header = (error.phase == .prepare ? "Parse error: " : "Runtime error: ") | ||
| if policy.forceReadOnly { exitCode = 1 } |
There was a problem hiding this comment.
Fresh evidence: this interactive error branch only sets a nonzero exit for forceReadOnly, but hardened mode also enforces policy through SQLite limits such as SQLITE_LIMIT_ATTACHED=0. In a sandboxed builtin, an untrusted command can choose -interactive; then ATTACH ':memory:' AS aux; is refused by the hardened limit but reports through this branch and exits 0, making a policy-denied run look successful to the caller. Treat hardened-limit errors in interactive mode as nonzero as well, or classify those policy errors explicitly.
Useful? React with 👍 / 👎.
…ed REPL exit From Codex's b32fa5b review: - ATTACH expression targets (Sqlite3Executable :739): attachTargets only captures literal filenames, so `ATTACH '/d/' || 'x.db' AS a` (or a function/parameter/ number) slipped past the audit-sidecar reservation and could let SQLite touch the audit log's -journal/-wal sidecar. New lexical guard attachHasNonLiteralTarget (built on the existing strippedForGuards, like the VACUUM-INTO guard) refuses non-literal targets when an audit trail is active. hardened/forceReadOnly already block all ATTACH via SQLITE_LIMIT_ATTACHED=0, so this is the audit-only gap. - Audit log directory durability (FileAuditSink :67): fsync(fd) persists the inode but not the new directory entry on POSIX, so a crash could lose a just-created log even after record() returned true. openLogPOSIX now reports whether the log was newly created (empty at open) and the writer fsyncs the parent directory in that case (best-effort: EINVAL/ENOTSUP tolerated). - Hardened REPL exit (Sqlite3Executable :822): the interactive error branch only set a nonzero exit under forceReadOnly; a hardened-limit denial (e.g. ATTACH refused by SQLITE_LIMIT_ATTACHED=0) under -interactive exited 0, making a policy-denied run look successful. Now hardened OR read-only surfaces nonzero. Adds 5 regression tests (incl. a direct unit test of the ATTACH lexical guard across literal/expression/comment/mixed forms). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
…sitive CI on cae3f7a failed one test (attachNonLiteralTargetDetection, all platforms): the lexical guard wrongly flagged the literal `ATTACH DATABASE 'a.db' AS x`. The optional group `(?:database\b\s*)?` backtracks — it gives DATABASE back when the following `(?!as\b)` lookahead fails, then `\S` matches the `D` of DATABASE, yielding a false positive (and refusing a legitimate literal ATTACH under audit). Replace the optional group with an alternation that can't backtrack into matching DATABASE as the target token: either DATABASE is present and followed by a non-AS token, or the first token isn't DATABASE/AS. Validated end-to-end (strippedForGuards + pattern) against 16 cases with a portable regex engine; uses only constructs already in the original pattern (lookahead/alternation/\b/\s/\S), so behavior is identical across ICU. Adds 4 more unit cases (schema named "database", leading comment, DATABASE+param, numeric expr). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56fa79fa54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // within-chunk case is covered by the SDK compile flag SQLITE_TEMP_STORE=3. | ||
| // (Re-pin precedes EQP per Codex review P2, PR #1.) | ||
| if !repinTempStoreOrFailClosed() { return false } | ||
| if eqp { renderQueryPlan(sql) } |
There was a problem hiding this comment.
When .eqp on is enabled and one input chunk contains multiple statements, this renderQueryPlan(sql) call evaluates EXPLAIN QUERY PLAN ... before the ATTACH authorization/sidecar checks below. Because evaluate is used for semicolon-separated chunks, a chunk such as SELECT 1; ATTACH '/tmp/a.db' AS aux; CREATE TABLE aux.t(x); can execute the tail statements during EQP rendering; under an audit-only policy with auditURL at /tmp/a.db-journal, that can clobber the audit sidecar before the collision check runs. Move the ATTACH gate ahead of EQP rendering or render only the first statement.
Useful? React with 👍 / 👎.
Codex P2 on 56fa79f (Sqlite3Executable.swift:737): with `.eqp on`, renderQueryPlan evaluate()s `EXPLAIN QUERY PLAN <chunk>`. The EXPLAIN prefix binds only to the FIRST statement, so in a multi-statement chunk like `SELECT 1; ATTACH '/tmp/a.db' AS aux; CREATE TABLE aux.t(x);` the tail statements EXECUTE during EQP rendering — before the ATTACH authorization/sidecar-collision gate runs (clobbering an audit sidecar under an audit-only policy), and a second time in the real run. Two fixes: - renderQueryPlan now skips multi-statement chunks (new hasTrailingStatement: a top-level `;`, after masking strings/comments, followed by more non-whitespace). This is the root cause — it also fixes the general double-execution of any side-effecting tail (INSERT/CREATE/ATTACH) under `.eqp on`. - The ATTACH gate now runs *before* EQP rendering (belt-and-suspenders, per Codex's primary suggestion) so a colliding/non-literal ATTACH is rejected before any EXPLAIN evaluate, regardless of EXPLAIN's execution semantics. Both detectors validated end-to-end against a portable regex engine (11 cases each). Adds a hasTrailingStatement unit test and an integration test proving a two-INSERT chunk yields 2 rows (not 3) under `.eqp on`. Existing EQP tests use single statements, so they still render. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67f5dcdc74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let replacement = try SQLiteDatabase(location) | ||
| // Honor the trusted read-only policy on the new handle too — | ||
| // otherwise `.open writable.db` would relax forceReadOnly in-band. | ||
| let replacement = try SQLiteDatabase(location, readonly: policy.forceReadOnly) |
There was a problem hiding this comment.
When forceReadOnly is active, an existing .output/.once redirect can be aimed at a file before it becomes the open database, then .open switches to that same path without rechecking the active redirect. For example, starting from :memory:, .output /tmp/ro.db; .open /tmp/ro.db; .print pwned leaves finishOutput() to overwrite /tmp/ro.db outside SQLite even though it is now the read-only database; redirectHitsOpenDatabase only runs when the redirect is created, so .open needs to reject or close redirects that now collide with the replacement database.
Useful? React with 👍 / 👎.
| return false | ||
| } | ||
| let openDB = Sqlite3Executable.canonicalize(Shell.resolve(filename)).path | ||
| guard Sqlite3Executable.auditCollidesWithDatabase(auditPath: url.path, dbPath: openDB) else { |
There was a problem hiding this comment.
On case-insensitive filesystems such as the default macOS/Windows volumes, a read-only run can still redirect to a not-yet-existing sidecar using different casing, e.g. open /tmp/data.db read-only and then .output /tmp/DATA.DB-wal. Because the string comparison is case-sensitive and sameFile cannot help before that sidecar exists, the check allows the redirect and finishOutput() creates/writes SQLite’s sidecar path outside the read-only policy; normalize or use a platform-aware path comparison for absent sidecars as well.
Useful? React with 👍 / 👎.
| if let auditCanonicalPath, | ||
| Sqlite3Executable.auditCollidesWithDatabase(auditPath: auditCanonicalPath, dbPath: resolved.path) { |
There was a problem hiding this comment.
This audit-sidecar reservation only runs for filenames returned by the prepare-only attachTargets scan. If an earlier statement in the same chunk creates schema needed by an intervening statement, the scan can stop before a later literal ATTACH, while the real database.evaluate(sql) then executes the whole chunk; under an audit-only policy, CREATE TABLE t(x); INSERT INTO t VALUES(1); ATTACH '/tmp/a.db' AS aux; can therefore bypass this collision check and let /tmp/a.db-journal overwrite the audit log. Refuse multi-statement chunks whose attach scan is incomplete, or split/execute with the ATTACH gate after each statement.
Useful? React with 👍 / 👎.
…multi-stmt ATTACH Three Codex P2s on 67f5dcd, all gaps in the audit/read-only path reservations: - :1432 case-insensitive sidecars — auditCollidesWithDatabase compared paths with `==`; on macOS/Windows (case-insensitive volumes) a not-yet-existing sidecar in different casing (e.g. `.output /tmp/DATA.DB-wal` against an open `/tmp/data.db`) slipped past, since sameFile can't compare an absent file. New pathsEqual folds case on Darwin/Windows, stays exact on Linux; auditCollidesWithDatabase uses it, so every caller (initial open, .open/.backup/.restore, .output/.once redirect) is covered. - :1339 .open redirect recheck — redirectHitsOpenDatabase only ran at redirect creation, so `.output /tmp/ro.db; .open /tmp/ro.db; .print` left finishOutput to overwrite the now-read-only DB. .open now re-checks any active redirect against the new database and drops a colliding one. - :777 multi-statement ATTACH under audit — attachTargets scans by preparing each statement, so a prepare failure mid-chunk (a later statement needing schema an earlier one creates) halts the scan before a trailing ATTACH, whose target then skipped the sidecar reservation yet still ran. With an audit trail active, refuse a multi-statement chunk containing ATTACH (new hasAttachStatement detector). Detectors validated end-to-end against a portable regex engine. Adds unit tests for pathsEqual/auditCollidesWithDatabase case handling and hasAttachStatement, plus integration tests for the .open redirect drop and the multi-statement ATTACH refusal. Existing single-statement ATTACH and hardened-ATTACH tests are unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4de07c924a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var s = sql.replacingOccurrences( | ||
| of: #"'(?:[^']|'')*'"#, with: " ", options: .regularExpression) |
There was a problem hiding this comment.
When a real SQL comment contains a quote before a guarded statement, this string-literal replacement runs before comment removal and consumes from the comment quote through the later filename quote; for example, under a hardened/read-only/audit policy /* ' */ VACUUM INTO 'out.db'; leaves no VACUUM INTO in strippedForGuards, so the guard is skipped and SQLite creates the output file without the intended authorization/policy block. Use a tokenizer/state-machine ordering that recognizes comments before treating quotes inside them as string delimiters.
Useful? React with 👍 / 👎.
| /// re-pin. (Codex review P1, PR #1.) The within-chunk case still needs SDK | ||
| /// SQLITE_TEMP_STORE=3. | ||
| private func repinTempStoreOrFailClosed() -> Bool { | ||
| guard policy.hardened else { return true } |
There was a problem hiding this comment.
When an embedder sets SQLitePolicy(hardened: false, forceReadOnly: true), this guard skips the temp-store re-pin entirely, so a script can run PRAGMA temp_store=FILE; and then create/write TEMP tables using SQLite's file-backed temp database even though the same read-only policy now blocks writable auxiliary databases such as ATTACH and .backup. Include forceReadOnly in the temp-store pin/re-pin path, or the read-only policy still permits this in-band database write channel when the engine is not compiled with SQLITE_TEMP_STORE=3.
Useful? React with 👍 / 👎.
| } | ||
| for target in database.attachTargets(in: sql) | ||
| where !target.isEmpty && target != ":memory:" { | ||
| let resolved = Sqlite3Executable.canonicalize(Shell.resolve(target)) |
There was a problem hiding this comment.
When an audit sink is active without hardened/read-only mode, a literal SQLite URI filename such as ATTACH 'file:/tmp/a.db?mode=rwc' AS aux is resolved and checked here as the sandbox string file:/tmp/a.db?mode=rwc, but SQLite opens /tmp/a.db. If the audit log is /tmp/a.db-journal, the sidecar collision check therefore misses and WAL/journal creation can overwrite the JSONL trail; reject URI filenames or canonicalize them the same way SQLite will before authorizing/reserving.
Useful? React with 👍 / 👎.
| /// `lstat` (not `stat`) so a symlink swapped onto the canonical leaf after | ||
| /// authorization is rejected rather than followed. (Codex review P2, PR #1.) | ||
| static func isRegularFileOrAbsent(_ path: String) -> Bool { | ||
| #if canImport(Darwin) || canImport(Glibc) |
There was a problem hiding this comment.
On Android this target is still built and tested, but libc symbols are exposed through the Android module rather than Glibc, so this condition falls into the non-POSIX fallback that only rejects directories. A hardened/read-only invocation can still point the initial database or .open/.read target at a FIFO/device on Android and block before the timeout/session protections run; include canImport(Android) and use the same lstat regular-file check there.
Useful? React with 👍 / 👎.
…ibc, URI ATTACH Codex review of 4de07c9 — one P1 and three P2s. P1 (:535) strippedForGuards comment/string ordering — the guard masker ran independent regex passes (strings, then comments), so a quote inside a comment opened a phantom string that swallowed a following keyword: `/* ' */ VACUUM INTO 'out.db';` left no VACUUM INTO to match, skipping the policy block (and likewise hid ATTACH from its detectors). Replaced with a single left-to-right scan that recognizes whichever delimiter opens first, so an in-comment quote is part of the comment and an in-string comment marker is part of the string. This underpins every lexical guard (VACUUM INTO + all ATTACH detectors). P2 (:1494) temp-store under read-only — the temp_store=MEMORY pin (initial and per-statement re-pin) was hardened-only, so SQLitePolicy(forceReadOnly:true) without hardened let a script PRAGMA temp_store=FILE and write file-backed TEMP tables — an in-band write the read-only policy must block (like ATTACH/.backup). Both the applyHardening pin and repinTempStoreOrFailClosed now include forceReadOnly. (Belt-and-suspenders to SQLITE_TEMP_STORE=3.) P2 (:282) Android libc — isRegularFileOrAbsent/sameFile and FileAuditSink gated the POSIX path on canImport(Darwin)||canImport(Glibc); on Android libc is the `Android` module, so those fell to the non-POSIX fallback (no FIFO/device reject, no inode identity, no O_NOFOLLOW leaf guard). Added canImport(Android). No effect on macOS/Linux/Windows/iOS (their branches are unchanged). P2 (:806) URI ATTACH under audit — a literal SQLite URI target (`ATTACH 'file:/tmp/a.db?mode=rwc'`) is opened by SQLite as its path component, not the string reserved here, so the sidecar collision check missed it. Refuse file: URI targets while an audit trail is active. State-machine masker and detectors validated end-to-end against a portable regex engine (comment-injection + full regression set). Adds unit + integration tests for the comment-injection guards and the URI refusal. The temp-store invariant is masked by SQLITE_TEMP_STORE=3 on this build, and the Android path is exercised by the Android CI build; both fixes are otherwise straightforward. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
…tion fallback
Round 25's Android change broke the Android build: the Bionic libc isn't importable
as a POSIX module from Swift — `errno` is a function-like macro ("structure not
supported"), and stat/lstat/S_IFMT/S_IFREG aren't in scope via `import Android`.
So the `canImport(Android)` POSIX branches I added to isRegularFileOrAbsent,
sameFile, and FileAuditSink could not compile.
Revert those POSIX-on-Android branches (back to the Foundation fallback Android was
already using and green with). Instead address the original finding portably:
strengthen isRegularFileOrAbsent's non-POSIX fallback to allow only an absent path
or a *regular* file via FileManager.attributesOfItem — rejecting directories,
sockets, devices, and FIFOs (typeUnknown) — so a FIFO/device target still can't
block the open before the session timeout on Android/Windows. This uses only
Foundation (no libc), matching the attributesOfItem(.type) pattern already used by
FdKit/ZipKit/TarKit which build on Android.
Net change vs. before round 25: only isRegularFileOrAbsent's fallback is improved;
sameFile and FileAuditSink are unchanged. The macOS/Linux POSIX path is untouched.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea5ea84fea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !FileManager.default.fileExists(atPath: url.path) { | ||
| FileManager.default.createFile(atPath: url.path, contents: nil) | ||
| } | ||
| let handle = try FileHandle(forWritingTo: url) |
There was a problem hiding this comment.
On Android this Foundation fallback is selected because the POSIX branch only checks Darwin/Glibc; FileHandle(forWritingTo:) can still open a pre-existing FIFO audit path before any nonblocking or regular-file validation, so an audit URL in a user-writable directory can hang preflight() instead of failing closed. Include Android in the raw open(... O_NONBLOCK ...) path, or add an equivalent pre-open special-file check before this call.
Useful? React with 👍 / 👎.
| guard let line = await stdin.readLine() else { out("\n"); break } | ||
| if budgetExceeded() { break } |
There was a problem hiding this comment.
When a hardened run is invoked with -interactive and stdin is a live pipe/TTY that does not produce another line, this timeout check is reached only after await stdin.readLine() returns. An untrusted argv can therefore sit at the sqlite> prompt indefinitely and bypass statementTimeout; check the deadline before/around the read or reject interactive mode under hardened policies.
Useful? React with 👍 / 👎.
| guard policy.hardened, let cap = policy.maxResultBytes else { | ||
| rawWrite(s, toStderr: toStderr); return |
There was a problem hiding this comment.
If an embedder configures a cap-only or read-only-plus-cap policy such as SQLitePolicy(hardened: false, maxResultBytes: 1024), this guard bypasses the cap entirely because hardened is false, even though maxResultBytes is a public policy field and other fields like statementTimeout, forceReadOnly, and auditURL are honored independently. Apply the cap whenever maxResultBytes is set, or reject/document cap values when hardening is disabled, so callers don't get unbounded output after explicitly setting a limit.
Useful? React with 👍 / 👎.
…k on fallback Codex review of ea5ea84 — three P2s. :621 output cap outside hardened — the maxResultBytes cap (in emit and the early writeError path) was gated on policy.hardened, so SQLitePolicy(hardened:false, maxResultBytes:N) silently produced unbounded output despite the explicit limit, unlike statementTimeout/forceReadOnly/auditURL which apply independently. Apply the cap whenever maxResultBytes is set; message reworded from "hardened policy output cap" to "output cap". :748 interactive vs statementTimeout — the budget check runs only after stdin.readLine() returns, so under a timeout-bounded policy an untrusted argv could select -interactive and sit at the prompt forever, dodging the budget. A blocking read can't be bounded without async cancellation machinery, so refuse interactive mode when statementTimeout is set (the hardened preset sets one). :84 audit FIFO on the Foundation fallback — the prior round moved Android off the POSIX audit path (Bionic libc isn't Swift-importable), so it uses FileHandle- (forWritingTo:), which blocks opening a pre-existing FIFO and hangs preflight instead of failing closed. The POSIX path uses O_NONBLOCK+fstat; the fallback now does a pre-open regular-file check (Foundation, no libc) via isRegularFileOrAbsent. Updates the hardened-interactive test to assert the refusal, and adds a cap-without- hardened test. Default (.permissive) policy has neither field set, so existing permissive/interactive tests are unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 652f3a4156
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // pinning by reconnecting via `.open` (control-surface rule). | ||
| if safeMode { database.enableSafeMode() } | ||
| applyHardening(to: database) | ||
| filename = path // as-typed, matching sqlite3's `.show` |
There was a problem hiding this comment.
When forceReadOnly is active, switching away from a real database with .open :memory: makes redirectHitsOpenDatabase forget the database file that was just opened read-only, because this assignment replaces filename with :memory:. A script can therefore start on /tmp/ro.db, run .open :memory:, then .output /tmp/ro.db and .print ..., and finishOutput() will overwrite the original database outside SQLite despite the trusted read-only policy; keep previously opened read-only database paths reserved (or reject redirects to them) for the duration of the run.
Useful? React with 👍 / 👎.
Codex P2 on 652f3a4 (:1461): redirectHitsOpenDatabase only checked the *current* filename, so under forceReadOnly a script could open /tmp/ro.db, run `.open :memory:` (making filename ":memory:"), then `.output /tmp/ro.db` + `.print` — finishOutput would overwrite the file just opened read-only, because the guard had forgotten it. Track the as-typed name of every database file opened read-only this run (initial open + each `.open`) in a reserved set, and check a redirect target against the whole set (canonicalizing at check time). A file opened read-only therefore stays reserved against output redirects for the entire run, even after `.open` switches away from it. Reworded the rejection message accordingly. Adds a regression test (open ro.db → `.open :memory:` → `.output ro.db` is refused and the file is untouched); updates the existing redirect test's message assertion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6862fc7879
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // dodging the budget (the per-line check only runs *after* a line arrives). | ||
| // Refuse interactive mode when a timeout is set (the hardened preset sets | ||
| // one) rather than run unbounded. (Codex review P2, PR #1.) | ||
| if policy.statementTimeout != nil { |
There was a problem hiding this comment.
This refusal only covers explicit -interactive, but the default no-SQL path in run still does await stdin.readAllString() before process can check the new deadline. In a sandboxed/hardened builtin invocation such as sqlite3 with no SQL argument and a live stdin/pipe that never reaches EOF, the command can block indefinitely despite the 30s statementTimeout; apply the same timeout/refusal logic to non-interactive stdin reads or race the read against the deadline.
Useful? React with 👍 / 👎.
| if policy.hardened || policy.forceReadOnly, | ||
| !Sqlite3Executable.isRegularFileOrAbsent(url.path) { |
There was a problem hiding this comment.
When an embedder sets SQLitePolicy(hardened: false, statementTimeout: ...), this guard is skipped even though the timeout is otherwise honored independently. A script can then run a file dot-command such as .read or .import against an authorized FIFO with no writer and block inside String(contentsOf:), where the statement budget cannot run or interrupt it; include timeout-bound policies in this regular-file preflight as well.
Useful? React with 👍 / 👎.
| let dbPath = Sqlite3Executable.canonicalize(Shell.resolve(name)).path | ||
| return Sqlite3Executable.auditCollidesWithDatabase(auditPath: url.path, dbPath: dbPath) |
There was a problem hiding this comment.
When a read-only database is opened through a symlink in a writable directory, this check re-resolves the saved name later instead of comparing against the path that was authorized/opened. If that symlink is retargeted after the open, a subsequent .output to the original database target no longer collides with the reservation and finishOutput() can overwrite the database outside SQLite; reserve the canonical opened path when .open/startup succeeds rather than re-canonicalizing the user spelling here.
Useful? React with 👍 / 👎.
…s at open time Codex review of 6862fc7 — addressing two of three P2s (the third deferred, below). :1545/:108 special-file preflight under timeout-only policies — the FIFO/device rejection on the initial database and on .read/.import targets was gated on hardened||forceReadOnly, so SQLitePolicy(hardened:false, statementTimeout:…) skipped it. A .read of a writerless FIFO then blocks in String(contentsOf:) where the budget can't interrupt. Both guards now also fire when statementTimeout is set. :1566 reserve canonical read-only DB paths at open time — round 28 stored the as-typed name and re-canonicalized at check time, so a symlink retargeted after the open would move the reservation off the real database. Now the canonical path is captured when the open succeeds (initial open in run, and each .open via the resolved location) and compared directly, so a later symlink swap can't unprotect the file. Tests: timeout-only policy rejects a non-regular (.read of a directory) target via the preflight message; a symlinked read-only DB is reserved by its canonical path; a timeout policy still processes finite stdin. Deferred (:755, bound batch stdin reads under timeout): bounding stdin.readAllString needs to race it in a child task, which requires ShellKit's InputSource to be Sendable (addTask capture) and cancellation-aware. Neither is visible from this repo, and a wrong guess is an uncatchable compile/runtime risk, so it's split out rather than gambled in this commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
Per maintainer decision, the remaining Codex P2 (bound batch stdin reads under a statementTimeout, :755) is left as a documented limitation rather than implemented: bounding stdin.readAllString needs to race it in a child task, which requires ShellKit's InputSource to be Sendable and cancellation-aware — neither verifiable from this package, and a wrong guess is an uncatchable compile/runtime risk. Add an in-code note at the batch-read site and a caveat in PORTING-FROM-SWIFTSQLITE. The narrow edge (hardened/timeout `sqlite3`, no SQL arg, a never-EOF stdin pipe) and the parallel interactive-refusal are both called out. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
There was a problem hiding this comment.
💡 Codex Review
Stop after failed -cmd policy checks
When a -cmd chunk hits a policy denial, Session.process(..., context: .inline) returns false but usually does not set shouldQuit; this loop discards that return value and proceeds to the trailing SQL anyway. For example, with a hardened/audit policy, -cmd "VACUUM INTO 'x';" :memory: "SELECT 1;" refuses the unsafe VACUUM but still executes the later statement, so inline policy/setup failures are not fail-closed. Break here the same way the trailing SQL loop does when process returns false.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if policy.hardened || policy.forceReadOnly || policy.statementTimeout != nil, | ||
| !Sqlite3Executable.isRegularFileOrAbsent(url.path) { |
There was a problem hiding this comment.
This policy guard only rejects non-regular targets, but regular files still pass through to .read/-init and .import, which synchronously load the entire file with String(contentsOf:) before statementTimeout, SQLITE_LIMIT_SQL_LENGTH, or the output cap can run. Under a hardened or timeout-bound sandbox, an untrusted script can point at an authorized multi-GB regular file and tie up memory/CPU despite the new hardening; check file size or stream/cap the read before accepting the target.
Useful? React with 👍 / 👎.
| if policy.forceReadOnly { | ||
| err("Error: .backup is not permitted under the read-only policy\n") | ||
| exitCode = 1 | ||
| return | ||
| } |
There was a problem hiding this comment.
This guard only handles forceReadOnly, so the default hardened policy (statementTimeout set, forceReadOnly false) still reaches the synchronous database.backup(to:) call below with no deadline checks until it returns. In a sandboxed run against a large authorized database, .backup can therefore keep copying and writing well past the intended script budget; reject or perform incremental backup/restore under timeout-bound policies so these dot-commands cannot bypass the timeout.
Useful? React with 👍 / 👎.
…file reads (Codex P2) Three P2 findings from the Codex review of b68033b, all in the sqlite3 shell port's hardening layer. F1 (-cmd loop, :170): a denied/failed `-cmd` returns false under `.inline`, but the loop discarded that and ran the trailing SQL anyway — so a policy denial (e.g. `-cmd "VACUUM INTO 'x';"`) wasn't fail-closed. Under any active policy (or -bail), arm a full-run shutdown on inline failure so the trailing SQL argument AND the stdin loop are skipped, not just the rest of the -cmd list. The permissive policy keeps stock sqlite3's continue-on-error (only -bail makes it fatal there). F2 (.backup/.restore, :1360): the guard only handled forceReadOnly, so the default hardened policy (statementTimeout set, forceReadOnly false) reached the synchronous `database.backup(to:)`, which can't honor the between-statements budget and would copy a large DB past the deadline. Refuse both under a statementTimeout (mirrors the interactive-mode refusal); a deadline-checked incremental copy needs the SDK's step-wise backup API (added as an SDK follow-up). F3 (file reads, :1565): the guard only rejected non-regular targets, but a regular file still passed to `.read`/`-init`/`.import`, which slurp it whole via String(contentsOf:) before statementTimeout / SQLITE_LIMIT_SQL_LENGTH / the output cap can apply. Add a configurable `maxInputBytes` (64 MiB under the hardened preset, parallels maxResultBytes) and refuse an oversized regular file up front under a bounding policy. Adds tests for each fix plus the permissive/-bail fidelity cases, and documents the new controls and the step-wise-backup follow-up in PORTING-FROM-SWIFTSQLITE. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
…ce method `tightenedToHardened()` is an instance method, so the unqualified static member reference failed to compile on all platforms: SQLitePolicy.swift:91: static member 'defaultInputByteCeiling' cannot be used on instance of type 'SQLitePolicy' Qualify it with `Self.` (the sibling `hardened()` is a static func, so its reference already resolved). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a156a2fa3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for command in options.commands where !session.shouldQuit { | ||
| _ = await session.process(command, context: .inline) | ||
| if await session.process(command, context: .inline) == false, failClosedOnInlineError { | ||
| session.requestStop() |
There was a problem hiding this comment.
Under an active policy, this fail-closed handling only starts with the -cmd loop, after -init has already been processed by runScript in .script context. A denied init statement such as an init file containing VACUUM INTO 'x'; under .hardened() sets exitCode but does not set shouldQuit, so the trailing SQL argument or stdin still runs even though setup was policy-denied; mirror the same stop logic after runScript before continuing.
Useful? React with 👍 / 👎.
Follow-up to the inline fail-closed fix: `-init FILE` is processed by runScript *before* the -cmd loop, in `.script` context where `process` doesn't return false on a non-bail denial (and runScript discards its return anyway). So a denied init script (e.g. an init file with `VACUUM INTO 'x';` under .hardened()) set exitCode but not shouldQuit, and the trailing SQL / stdin still ran as if setup succeeded. Hoist the fail-closed gate above the -init handling and, after runScript, arm the run-stop when the gate is active and the init script left a nonzero exitCode (a policy denial, SQL error, or unreadable/denied init path). exitCode is 0 until that point, so a nonzero value is unambiguously from the init script. The permissive policy still keeps stock sqlite3's continue-on-error (unless -bail). Renamed the gate failClosedOnInlineError -> failClosedOnSetupError since it now covers both -init and -cmd. Adds tests for the denied-init and permissive cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce58c79e9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard text.utf8.count > maxAuditTextBytes else { return text } | ||
| return String(decoding: text.utf8.prefix(maxAuditTextBytes), as: UTF8.self) + "…[truncated]" |
There was a problem hiding this comment.
When an audit sink is active, this truncates the recorded SQL/dot-command but runStatement still executes the full text. For example, an audit-only policy, or the hardened default with a 1 MB SQL limit, can run a statement whose first 64 KiB are padding/comment text and whose destructive tail is omitted from the JSONL record, so the trusted attempted-audit trail no longer captures the action that was allowed to execute. Fail closed on oversized audit text or include an integrity-preserving full record/digest rather than silently shortening it.
Useful? React with 👍 / 👎.
| @@ -675,6 +1400,26 @@ final class Session { | |||
| importDelimited(text, into: args[1]) | |||
There was a problem hiding this comment.
When statementTimeout is set (including the hardened preset), .import still reads an under-cap CSV and then calls importDelimited, which performs the whole per-row INSERT loop inside one dot-command without any deadline checks. A large authorized CSV below maxInputBytes can therefore keep the sandboxed run busy well past the timeout, unlike .backup/.restore which this patch now refuses under timeout-bound policies; either reject .import in that context or check the deadline during the import loop.
Useful? React with 👍 / 👎.
| for t in tables { | ||
| if outputCapped { break } // stop querying once the cap is hit | ||
| guard case .text(let name) = t[0], case .text(let createSQL) = t[1] else { continue } | ||
| out(createSQL + ";\n") |
There was a problem hiding this comment.
Under a timeout-bound policy, .dump runs entirely inside one dot-command and this loop only checks the output cap; budgetExceeded() is not reached again until after handleDot returns. A large authorized database (especially with a timeout-only policy, or a raised/absent output cap) can therefore spend well past statementTimeout materializing and emitting rows, so either refuse .dump under timeout policies or check the deadline inside these dump loops.
Useful? React with 👍 / 👎.
…eout (Codex P2) Three P2 findings from the Codex review of ce58c79, all about actions that escape a bound because they run inside a single long dot-command / statement. Audit integrity (:601): the audit record is byte-capped (maxAuditTextBytes, 64 KiB) but runStatement/handleDot executed the FULL text, so a statement padded past 64 KiB could run a destructive tail that never appears in the trusted trail. Fail closed: under a real audit sink, refuse text too large to record whole (new tooLargeToAudit, checked before recording on both the SQL and dot paths) rather than execute it with a truncated entry. No-op without an audit sink. .import under timeout (:1400): importDelimited ran the whole per-row INSERT loop inside one dot-command with no deadline check, so a large authorized CSV (under maxInputBytes) could run past statementTimeout. Check budgetExceeded() per row — a complete fix since each INSERT is its own evaluate. .dump under timeout (:1241): the dump loops only checked the output cap, so a large DB could materialize/emit past statementTimeout. Check budgetExceeded() between tables and rows. A single huge table's SELECT * still materializes in one evaluate — the same SDK-stepping limit the output cap already has (follow-up #2). budgetExceeded() is a no-op (nil deadline) without a timeout, so permissive/ no-timeout import and dump are unchanged. Adds tests for the audit refusal, the audit happy path, and import/dump still working under a timeout policy; documents the audit fail-closed and the import/dump deadline checks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrXFwb2Es4srboCFtyL993
2 participants
What
Adds an opt-in, embedder-bound security policy (
SQLitePolicy) to thesqlite3shell port so it can run untrusted, LLM-generated SQL safely under SwiftBash — without breaking sqlite3 parity or thesqlite-vec/ FTS5 feature surface.This is the SwiftPorts half of a comparison with PicoMLX/SwiftSQLite#1: SwiftSQLite is a purpose-built security-first sandboxed engine; SwiftPorts is a fidelity-first sqlite3 clone. This PR brings SwiftSQLite's escape-boundary and DoS hardening over, while leaving the SQL feature surface (vtables, functions, PRAGMA) untouched.
The core idea: policy is not argv
When the command is a SwiftBash builtin, the LLM writes the command line, so any hardening expressed as an opt-in flag is no boundary — the agent just omits or inflates it. So the policy is bound by the trusted embedder (
Sqlite3Builtin(policy:)) or auto-derived fromShell.current.sandbox, and no in-band channel — argv or dot-command — may relax it:-hardenedcan only turn hardening on (no flag turns a bound policy off).limitmay lower/show a limit but not raise it (else.limit attached NundoesATTACHED=0).openre-applies the full policy to the new handle (previously it re-armed only safe mode)Shell-layer controls (all no-ops unless hardened)
$(…)SQLITE_LIMIT_ATTACHED=0,…_LENGTH,…_SQL_LENGTHPRAGMA temp_store=MEMORY(temp confinement)FileAuditSink, JSON Lines,O_NOFOLLOW+ preflight) written outside the DB; destination is embedder-set, never an argv pathFeature preservation (deliberate)
Hardened mode does not port SwiftSQLite's blanket default-deny authorizer (which would reject
CREATE VIRTUAL TABLEand break vec0/FTS5). It touches only limits / temp / caps / audit / canonicalization, so vector and full-text workloads keep working. There's a test asserting hardened output equals permissive for ordinary queries, and one asserting DDL/DML/PRAGMA still work.Honest scope
Several deeper guarantees need the raw
sqlite3*handle, which lives in the externalCocoanetics/SQLiteKitSDK (not in this package). These are specced inDocs/PORTING-FROM-SWIFTSQLITE.mdas follow-up PRs:sqlite3_progress_handler+sqlite3_interrupt) — the shell budget is a script budget, can't interrupt one long querySQLITE_OPEN_NOFOLLOW;DBCONFIG_DEFENSIVE/TRUSTED_SCHEMA=0; compile flagsNotes for review
-hardened(tighten-only). Existing default and-safebehavior are unchanged — the 60-test parity suite should stay green.🤖 Generated with Claude Code
https://claude.ai/code/session_01E8VSWQJZ3ZxVcG6kwiwygJ
Generated by Claude Code