fix(database-client): treat unreadable env vars as unset under Deno without --allow-env - #155
Conversation
- remove the truncated sentence at the end of the security section - fold the intro's dangling dependency claim into the sentence - explain the unsafe-integer rejection in active voice - collapse a stacked comment in env.ts to one line - describe the API surface in AGENTS.md without the D1 comparison
…ithout --allow-env Deno 2's node-compat process.env throws NotCapable on read just like Deno.env.get, but only the latter was guarded, so readEnv crashed instead of falling through to connect()'s clearer missing-URL error. One module-scope sniff type and one try now cover both globals, and env.test.ts locks in the degrade-to-unset behavior with throwing stubs.
|
@codex review |
🦋 Changeset detectedLatest commit: 15f25c0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Greptile SummaryThe PR makes database environment-variable lookup degrade to an unset value when Deno denies environment access, rather than leaking a permission exception.
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or non-blocking defects identified. The guarded lookup preserves documented Deno-first precedence and empty-value normalization while ensuring permission failures resolve to the caller’s existing missing-configuration behavior, and the added tests cover both environment access paths.
|
| Filename | Overview |
|---|---|
| packages/database-client/src/env.ts | Consolidates cross-runtime environment lookup and converts denied reads into an unset result without exposing a concrete regression. |
| packages/database-client/src/env.test.ts | Adds focused tests for process lookup, Deno precedence, empty-value fallthrough, and throwing environment APIs. |
| packages/database-client/README.md | Contains minor wording refinements with no behavioral or public-contract regression. |
| AGENTS.md | Corrects the database-client API description from D1-shaped to prepared-statement based. |
| .changeset/database-env-read.md | Correctly records the environment-read fix as a patch release. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[readEnv name] --> B{Deno value is readable and non-empty?}
B -->|Yes| C[Return Deno value]
B -->|No| D{process.env value is readable and non-empty?}
D -->|Yes| E[Return process value]
D -->|No| F[Return undefined]
B -->|Read throws| F
D -->|Read throws| F
Reviews (1): Last reviewed commit: "fix(database-client): treat unreadable e..." | Re-trigger Greptile
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Every runtime this client targets exposes process.env, Deno included, so the Deno.env.get branch and the globalThis runtime sniff were carrying no weight. readEnv() now reads process.env directly, and the examples use process.env instead of importing readEnv. The permission tolerance from #155 stays: reading can throw rather than return undefined when Deno runs without --allow-env, so readEnv still catches and reports the variable as unset. Verified that connect() with no arguments under `deno run --allow-net` still raises URL_MISSING with its usual message rather than a NotCapable crash. --allow-env is still required for Deno to read process.env, so the example's invocation line is unchanged. Behaviour is unchanged for consumers, so the existing changeset still covers it. Live smoke passes on Bun and Deno.
2 participants
Stacked on #154.
Problem
readEnvguardsDeno.env.getwith a try/catch so a missing--allow-envdegrades gracefully, then falls back toprocess.env. But Deno 2 ships a node-compatprocessglobal whoseenvreads throw the sameNotCapableerror, and that access was outside the guard. Running without env permission (e.g.deno run --no-prompt, or a granular--allow-env=OTHER_VAR) crashed with an uncaughtNotCapableinstead of reachingconnect()'s clearer "no database URL: pass { url } or set BUNNY_DATABASE_URL" error.Change
env.test.tscovers the process path, empty-string normalization, Deno-first precedence, empty-Deno fallthrough, and both throwing-global cases (throwingDeno.env.getstub, throwingprocess.envproxy).Verification
deno run --no-prompton a script callingreadEnvcrashed before and now printsundefinedwith exit 0;--allow-envstill returns the value.