security: harden credential handling, pipeline JS execution, and the HTTP/stream layer#5
Open
lhfer wants to merge 5 commits into
Open
security: harden credential handling, pipeline JS execution, and the HTTP/stream layer#5lhfer wants to merge 5 commits into
lhfer wants to merge 5 commits into
Conversation
- config set: mask api_key/access_token/access_key_id/access_key_secret in the confirmation echo. It previously printed the stored secret verbatim to stdout (CI logs, pipes, screen shares), unlike `config show` / `auth status` which already maskToken(). - http / knowledge retrieve: use maskToken() in --verbose request logs instead of printing the first 8 chars of the bearer token / AccessKey id. - telemetry: write telemetry.jsonl with mode 0600 (was created world-readable by default), matching the other credential-area writers. - ensureConfigDir: chmod 0700 after mkdir, so a pre-existing ~/.bailian created by an older build/another tool (where mkdir's mode is ignored) holding cleartext credentials gets locked down too. Best-effort; never fatal. https://claude.ai/code/session_017ZGQCjwNQF5Pz96gLUnnG1
- endpoints: encodeURIComponent the id segments (task_id, app_id, node_id, schema_id) interpolated into request URLs. task_id in particular comes from the server's async-submit response and is fetched back with the bearer token attached, so an unencoded value could steer the authenticated follow-up request to a different path on the host. - stream (SSE parser): cap the in-memory buffer (16 MiB). A stream that never emits a newline, or that builds one enormous event from many data: lines, could otherwise grow the buffer without bound and exhaust process memory. https://claude.ai/code/session_017ZGQCjwNQF5Pz96gLUnnG1
…rrency - expressions: never execute $js during planning/dry-run. `pipeline run --dry-run` is the command a cautious user runs to preview an unfamiliar pipeline; it must not run embedded JavaScript. Planning now returns the expression placeholder instead of calling new Function. - schema (getByJsonPointer): block __proto__/constructor/prototype and require own properties, so a crafted $from/$input path cannot pull object internals (e.g. constructor) out of step output and feed them downstream. - scheduler: clamp --concurrency to a maximum (64) to bound fan-out so a single run cannot launch an unbounded number of concurrent API calls / downloads. Note: the runtime new Function sinks in script/js and $js (arbitrary host code execution) are intentionally left unchanged here — remediating them is a design decision (sandbox vs. literal-only code) for the maintainers; see PR notes. https://claude.ai/code/session_017ZGQCjwNQF5Pz96gLUnnG1
…ted-code RCE)
script/js executes its `code` as host JavaScript (via new Function), and a step's
`code` is a *resolved* input — so it could be written as `{ $from: <chat-step> }`,
turning model/API output into the body of the executed function (untrusted data
-> arbitrary host code execution). Pipeline validation now requires script/js
`code` to be a literal string: any $from/expression-sourced code is rejected.
Authoring a literal script/js step remains supported (the pipeline file is the
trust boundary, like a shell/npm script). Combined with "dry-run never executes
$js", this closes the path where untrusted text reaches the JS sink.
Adds regression tests: $from-sourced code rejected, literal code accepted,
dry-run does not execute $js, getByJsonPointer blocks prototype/inherited keys,
and concurrency clamps to the maximum.
https://claude.ai/code/session_017ZGQCjwNQF5Pz96gLUnnG1
…) URLs The config file accepted any value that merely starts with "http" (so even "httpfoo://evil" passed) for base_url and console_gateway_url — origins the client sends the Bearer token to. Validate them with `new URL()` and an http:/https: protocol check instead, rejecting malformed values. Valid http(s) URLs (including custom proxies and local http) are unaffected. https://claude.ai/code/session_017ZGQCjwNQF5Pz96gLUnnG1
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
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.
Summary
A security review surfaced several issues across credential handling, the pipeline
engine, and the HTTP/stream layer. This PR fixes the cleanly-fixable ones with
regression tests; a few items that need a product/contract decision are listed at
the end rather than changed unilaterally.
Source-only (no dependency changes).
vp checkis green; unit suites pass(
core9,cli81; e2e skipped without an API key).Fixes
Credential handling
config setno longer echoes secrets in cleartext.bl config set --key api_key …(and
access_token/access_key_id/access_key_secret) printed the stored valueverbatim to stdout — unlike
config show/auth status, which mask. The confirmationecho now masks secret-valued keys.
--verboserequest logs usemaskToken()instead of printing the first 8 chars ofthe bearer token / AccessKey id (
http.ts,knowledge retrieve).telemetry.jsonlis written0600(was default/world-readable).ensureConfigDirenforces0700via an explicitchmod(mkdir's mode is ignored fora pre-existing
~/.bailianthat may already hold cleartext credentials).Pipeline engine
script/jscodemust be a literal string. A step'scodeis a resolved input, soit could be written as
{ $from: <chat-step> }— turning model/API output into the bodyof
new Function(untrusted data → arbitrary host code execution). Validation now rejects$from/expression-sourcedcode; authoring a literalscript/jsstep is unchanged.--dry-runnever executes$js. Planning previously ran no-arg$jsvianew Function;a preview of an untrusted pipeline must not run code. Planning now returns the placeholder.
getByJsonPointerblocks__proto__/constructor/prototypeand only follows ownproperties.
--concurrencyis clamped to 64 to bound fan-out.HTTP / stream layer
encodeURIComponent-d:task_id(from the server's async-submitresponse, fetched back with the bearer token attached) plus
app_id/node_id/schema_id.base_url/console_gateway_urlare validated as real http(s) URLs vianew URL,replacing a
startsWith("http")check that also accepted e.g.httpfoo://….Not changed here (maintainer decision)
script/js/$jsruntime still usesnew Function(full host authority). With theliteral-
coderule, untrusted data can no longer become the code body; remaining exposureis authored code (the file is the trust boundary). Stronger isolation is a product call.
base_urlhost allow-listing — would break proxy/self-host users.--consoleloopback callback (wildcard CORS, token via GET query, no Host check,stateembedded in the
noticeparam) — hardening depends on the console server contract.config.jsonkey silently overridesDASHSCOPE_API_KEY.device_idfrom the MAC address and forwards raw errormessages to
gm.mmstat.com(default-on;DO_NOT_TRACK=1opts out).Testing
vp check: pass ·core9 tests pass ·cli81 tests pass (48 e2e skipped)