fix(config): close the review findings on configuration source ownership
Two had real security consequences: The bootstrap rejection ran on npm dotenv's parser while process.loadEnvFile applied the file with Node's own. Two independently maintained dialects meant the check and the thing it guards could disagree: a name Node accepts but the checker misses would reach process.env unchecked, and BASH_ENV there runs a file of the project's choosing on every `bash -c` the bash tool issues. Parse once with node:util's parseEnv — the same engine loadEnvFile uses — and assign the entries already checked, which also drops the dotenv dependency. llm-pi-ai still returned a literal profile.apiKey ahead of everything, and it registers a settings namespace, so the defect removed from llm-deepseek survived intact in its design twin. The field is gone from the profile schema, the resolution path, and the tests. The rest are consistency and documentation defects the review named: - verify-config-source-ownership did not scan the Python runtime's bundled cordis.yml, which still inlined apiKey and baseURL. Both are covered now, and the line-anchored INLINE_DENY documents that it is a tripwire, not a parser. - The deny list missed NODE_TLS_REJECT_UNAUTHORIZED, the askpass hooks, the GIT_CONFIG_* redirections, and PYTHONHOME — all implied by its own stated rule about what a variable does. - Snapshot lookups folded case on Windows, where environment names are case-insensitive and an exact-match Map could miss a higher-ranked layer. - The credentials note claimed a read-time permission check was "not taken" while this PR implemented it; the credentials-local README still described two layers, live process.env reads, dotenv-era limitations, and a renamed anchor; the llm-deepseek README still advertised the removed literal apiKey; and web.ts and base.cordis.yml kept personal-overlay wording. - The ownership note's literal-apiKey claim now names its scope: the web-search providers keep a literal field but register no settings namespace, so nothing can shadow a stored credential through them.
This commit is contained in:
@@ -6,10 +6,10 @@
|
||||
* @module @deepseek-ai/dsh-app-boot
|
||||
*/
|
||||
|
||||
import { parseEnv } from 'node:util'
|
||||
import { pathToFileURL } from 'node:url'
|
||||
import { readFileSync } from 'node:fs'
|
||||
import { basename, dirname, resolve } from 'node:path'
|
||||
import { parse as parseDotenv } from 'dotenv'
|
||||
import * as yaml from 'js-yaml'
|
||||
import { Context, type FiberState } from 'cordis'
|
||||
import Loader, { type Entry, type EntryOptions } from '@cordisjs/plugin-loader'
|
||||
@@ -94,7 +94,13 @@ function readEnvLayer(
|
||||
// ENOENT (no .env) is fine — rely on the ambient environment.
|
||||
return undefined
|
||||
}
|
||||
const values = parseDotenv(content)
|
||||
// `node:util`'s parseEnv is the same parser `--env-file` and
|
||||
// `process.loadEnvFile` use. Checking with a second dialect (npm dotenv)
|
||||
// would leave the rejection rule and the thing it guards on independently
|
||||
// maintained parsers: a name Node accepts but the checker does not would
|
||||
// reach `process.env` unchecked, and `BASH_ENV` there runs a file of the
|
||||
// project's choosing on every `bash -c` the bash tool issues.
|
||||
const values = parseEnv(content) as Record<string, string>
|
||||
for (const name of Object.keys(values)) {
|
||||
if (!isBootstrapOnly(name)) continue
|
||||
throw new Error(
|
||||
@@ -112,9 +118,11 @@ function readEnvLayer(
|
||||
* over the Harness home's `.env`, both under the inherited process
|
||||
* environment.
|
||||
*
|
||||
* Each layer is parsed and checked before anything is applied, then applied in
|
||||
* the order that makes the layering `user < project < inherited` —
|
||||
* `process.loadEnvFile` never replaces a name already set. Values do reach
|
||||
* Each layer is parsed once, checked, and only then applied — never replacing
|
||||
* a name already set, which is what makes the layering `user < project <
|
||||
* inherited`. The single parse is deliberate: the rejection rule and the
|
||||
* values that reach `process.env` must come from the same parser, or a name
|
||||
* one dialect accepts and the other misses would slip past the check. Values do reach
|
||||
* `process.env`, because a user's own `--config` tree and third-party
|
||||
* libraries read it; the returned snapshot is the authority for everything the
|
||||
* harness itself resolves, since `process.env` alone cannot say whether a
|
||||
@@ -144,8 +152,18 @@ export function loadLayeredEnv(
|
||||
// Parse both layers first: a rejection must not leave one file applied.
|
||||
const project = readEnvLayer(binName, cwd, warn)
|
||||
const user = home === resolve(cwd) ? undefined : readEnvLayer(binName, home, warn)
|
||||
if (project !== undefined) process.loadEnvFile(project.path)
|
||||
if (user !== undefined) process.loadEnvFile(user.path)
|
||||
// Assign the entries this function already parsed and checked, rather than
|
||||
// re-reading each file through `process.loadEnvFile`. One parse means the
|
||||
// snapshot, the rejection rule, and `process.env` can never disagree about
|
||||
// what a file contains. Skipping names already set reproduces the
|
||||
// never-replace behavior that makes the layering `user < project <
|
||||
// inherited`.
|
||||
for (const layer of [project, user]) {
|
||||
if (layer === undefined) continue
|
||||
for (const [name, value] of Object.entries(layer.values)) {
|
||||
if (process.env[name] === undefined) process.env[name] = value
|
||||
}
|
||||
}
|
||||
return createEnvironmentSnapshot([
|
||||
{ source: 'process', values: inherited },
|
||||
...project === undefined ? [] : [{ source: 'project-env' as const, path: project.path, values: project.values }],
|
||||
|
||||
Reference in New Issue
Block a user