Open a database with configuration, and so read-only - #41
Merged
Merged
Conversation
duckdb_open takes no configuration, so there was no way to ask for a read-only database. A process whose only job is to read should not be able to write by accident, and asking the engine to enforce that is worth more than intending it. Options arrive as a DSN query string and go through duckdb_open_ext. The last ? separates, so a path containing one is still openable, and a path with no ? takes the original code path unchanged.
make fmt is gofumpt -extra; the file arrived unformatted by it.
The driver passes every open-time option DuckDB accepts -- 299 in 1.5.5 -- through to duckdb_open_ext, with no allowlist and no denylist. That is the right default for a library: a driver cannot know whether the caller is a trusted ETL job or a multi-tenant query endpoint, and a policy that fits one is wrong for the other. The decision belongs to the integrator, so document what the decision is about. Four concern categories, each grounded in the actual settings: - Privileged execution. An extension is native code in your process. allow_unsigned_extensions defaults to false, so reaching it is a strict gain in capability; paired with custom_extension_repository it turns "open a database" into "fetch and run code from a chosen host". - Permission escalation. access_mode, and the allowed_paths / allowed_configs exceptions that survive your own restrictions. - Filesystem and network reach, including where secrets are stored and whether they can be printed unredacted. - Resource exhaustion: memory, threads, spill files, WAL growth. Plus the debug and force options, which are reachable from a DSN like any other and exist for DuckDB's own test suite. Also records that the trust boundary is the DSN string rather than the Go map, and shows the dozen-line allowlist an integrator can apply at that boundary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014AXhuxzvPQrpS89q1cQKou
…a disclaimer
url.ParseQuery keeps every value for a repeated key, and taking the last one
silently resolved a question that has no safe answer:
?access_mode=READ_ONLY&access_mode=READ_WRITE -> read_write
DSNs get built by concatenation, so the two halves can come from different
parts of a program. Quietly honouring one of them is how a database that was
asked to be read-only becomes writable. splitDSN now returns an error, Open
propagates it, and nothing has to guess. Nobody writes a duplicate key on
purpose, so refusing costs callers nothing.
The docs now open with the disclaimer that this package is glue rather than a
secure wrapper, and that the security model belongs to the integrating
application. That framing replaces the prose arguing the same point; what
remains is reference -- what the open-time options can actually reach, grouped
as privileged execution, permission escalation, filesystem and network reach,
resource exhaustion, and durability.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014AXhuxzvPQrpS89q1cQKou
🤖 AI Review (klein) — turn 2The incremental change correctly fixes the previously reported malformed-query fallback: splitDSN now returns ParseQuery errors and Open propagates them. It also adds the close/disconnect ordering and documentation/tests for empty trailing queries. However, the newly documented doubled- turn 2 (incremental review of d23ceda) · comments: total 2, active 1 (+1 new, −1 resolved) · verdict: request_changes |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
… filename Three fixes, two from review and one from the Windows CI job. Conn.Close closed the database without disconnecting first. The C API holds the instance by reference count, so duckdb_close does not release it while a connection is alive: the file stays open. POSIX lets the next open succeed anyway, so the leak was invisible until TestReadOnlyRefusesAWrite reopened the same path in one process and Windows refused with "the process cannot access the file because it is being used by another process". Disconnect, then close, and nil both so a second Close cannot double-free. splitDSN swallowed the url.ParseQuery error and returned the whole DSN as a path, so `file.duckdb?access_mode=%ZZ` quietly created a file named after the typo instead of reporting the bad option -- the opposite of the documented rule that a bad value fails the open. It is an error now. The trailing `?` escape did not work either: `odd?name.duckdb?` returned the path with the `?` still attached, so the file odd?name.duckdb stayed unopenable and the test asserted that. The trailing `?` now means "no options", and doubling it names a file that really does end in one. TestCloseReleasesTheFile reopens a path three times, which is the shape that failed on Windows and passes everywhere without the fix on POSIX alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014AXhuxzvPQrpS89q1cQKou
Comment on lines
+50
to
+52
| values, err := url.ParseQuery(query) | ||
| if err != nil { | ||
| return "", nil, errors.Wrapf(err, "duckdb: cannot parse the options in %q", dsn) |
There was a problem hiding this comment.
[major] Handle the documented doubled-? escape before calling url.ParseQuery: when the DSN ends in ??, strip the final separator and treat the preceding ? as part of the path (return dsn[:at+1], nil settings). As written, odd?name.duckdb?? parses the query ? as an option and is rejected by DuckDB instead of opening the file whose name ends in ?.
Rationale
Verified splitDSN uses LastIndex to make the second-to-last ? the path boundary, then passes the remaining single ? to url.ParseQuery; Go's parser accepts that as a key named ?, so the resulting settings map is non-empty and NewDB sends an unknown ? setting to duckdb_open_ext. This contradicts the added test and the DSN documentation promising doubled-? escaping.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
duckdb_opentakes no configuration, so there was no way to open a database read-only through this driver. A process whose only job is to read should not be able to write by accident, and asking the engine to enforce that is worth more than intending it.Options arrive as a DSN query string and go through
duckdb_open_ext:NewDuckDBWithSettings(path, settings)is the direct form.NewDuckDB(path)is unchanged and delegates to it.DSN rules
?takes the originalduckdb_openpath unchanged, so nothing existing changes behaviour.?separates, not the first. A DuckDB path may legitimately contain one, and taking the first would make such a file unopenable with no way to say otherwise. A path whose name ends in?is written./odd?name.duckdb?.?access_mode=READ_ONLY&access_mode=READ_WRITEis refused rather than resolved.That last rule is a change from the original PR, which took the last value silently. DSNs get built by concatenation, so the two halves can come from different parts of a program, and quietly honouring one is how a database asked to be read-only becomes writable. There is no answer that is safe to guess, and nobody writes a duplicate key on purpose, so refusing costs callers nothing.
Documentation
Both entry points accept every option DuckDB accepts at open time — 299 of them in DuckDB 1.5.5. Some govern extension loading (native code in the process), filesystem and network reach, and machine resources.
The driver applies no allowlist and no denylist. A driver cannot know whether the caller is a trusted ETL job or a multi-tenant query endpoint, and a policy that fits one is wrong for the other. So the README and
docs/CONFIGURATION.mdnow lead with:docs/CONFIGURATION.mdis the reference behind that decision: the DSN parsing rules, and what the open-time options reach, grouped as privileged execution, permission escalation, filesystem and network reach, resource exhaustion, and durability.Verification
Every documented behaviour was checked against libduckdb 1.5.5 rather than assumed, including that
enable_external_accessis one-way (Cannot enable external access while database is running).TestSplitDSN— the split, including the awkward pathsTestSplitDSNRejectsARepeatedOptionandTestOpenRejectsARepeatedOption— refused, and the error reaches asql.OpencallerTestReadOnlyRefusesAWrite— writes to a database, reopens it read-only, reads successfully, and checks the write is refused, so the flag is proven to reach DuckDB rather than merely be passed to itgo build/go test ./...pass withCGO_ENABLED=0;golangci-lint runreports 0 issuesCredit
The feature and its first commit are Calvin Cheng's, from #39, and that commit keeps their authorship. #39 was closed in error — its author compared against their fork's
main, where the work had genuinely landed, rather than this repository's. Rebased here onto the correct base, without the fork-maintenance commits it carried (a module rename togithub.com/calvinchengx/go-pduckdb, a fork README banner, and the deletion of the AI review workflow), which were not intended for this repository.Closes #39.