Skip to content

fix(security): keep Lakebase PAT off the command line + unify shell escaping#167

Open
antonyprasad-db wants to merge 1 commit into
databricks-solutions:mainfrom
antonyprasad-db:fix/harden-shell-out-seams
Open

fix(security): keep Lakebase PAT off the command line + unify shell escaping#167
antonyprasad-db wants to merge 1 commit into
databricks-solutions:mainfrom
antonyprasad-db:fix/harden-shell-out-seams

Conversation

@antonyprasad-db

Copy link
Copy Markdown

What

Two hardening fixes to the deploy / UC / secret shell-out seams.

1. 🔴 Lakebase PAT no longer passed on the command line

ensureLakebaseSecretAuth stored the minted PAT with:

databricks secrets put-secret <scope> <key> --string-value "<pat>" …

--string-value puts the secret in the process table (ps auxww) and any shell trace for the call's lifetime — visible to any other process/user on the host. This is the one file that mints and stores a long-lived token, so it's the highest-value seam to fix.

The Databricks CLI documents stdin as the alternative to --string-value ("Pass the secret via standard input"). The PAT is now fed on stdin, so it never appears as a process argument. exec() gains an input option that writes to the child's stdin and closes it.

Note: get-connection.ts already uses execFileSync (no shell) specifically to keep short-lived credentials out of ps — this brings the PAT path up to the same bar.

2. 🟠 Unified shell escaping on the canonical shq()

Six modules — secret-auth, deploy-credentials, deploy-rollback, uc-resources, databricks-host, deploy-app-endpoint — each defined a local escapeShellArg that only escaped ". Inside the double-quoted interpolations they were used in, that left $, backticks, and other metacharacters shell-active — a correctness bug (and injection surface) for any scope / key / catalog / comment value containing them.

All call sites now use the canonical POSIX single-quote escaper shq() from util/exec.ts (already used throughout scripts/git/* and covered by git-utils.test.ts). The six duplicated local escapers are deleted.

Why

Both are latent security/correctness gaps on the credential + deploy paths — exactly the surfaces that run against customer/partner workspaces. Behavior is unchanged for well-formed inputs; the fixes close the ps exposure and the metacharacter gap.

Testing

  • Tier 1 (hermetic, npm test): ✅ 2410 passed, 0 failed. Added exec() stdin coverage (tests/bdd/exec.test.ts); shq correctness is already covered in git-utils.test.ts.
  • npm run typecheck: ✅ clean.
  • Tier 3 (live): ⚠️ not run — no billable Lakebase workspace wired for the integration suite in this environment. The changed CLI invocations are argument-quoting/stdin only (no endpoint/flag semantics changed); the put-secret stdin path is the CLI's documented equivalent of --string-value. Flagging per CONTRIBUTING so a reviewer with workspace access can green Tier 3.

Notes

…scaping

Two hardening fixes to the deploy/UC/secret shell-out seams.

1. secret-auth.ts stored the minted PAT with
   `databricks secrets put-secret … --string-value "<pat>"`, putting the
   secret in the process table (ps) and any shell trace for the call's
   lifetime. Feed the PAT on stdin instead (the CLI's documented
   alternative to --string-value); it no longer appears as a process arg.
   exec() gains an `input` option that writes to the child's stdin and
   closes it.

2. Six modules (secret-auth, deploy-credentials, deploy-rollback,
   uc-resources, databricks-host, deploy-app-endpoint) each defined a
   local escapeShellArg that only escaped double quotes, leaving $,
   backticks, and other metacharacters shell-active inside the
   double-quoted interpolations - a correctness bug and injection surface
   for any scope/key/catalog/comment value containing them. Replace every
   call site with the canonical POSIX single-quote escaper shq() from
   util/exec.ts and delete the duplicated local escapers.

Behavior is unchanged for well-formed inputs; the fixes close the ps
exposure and the metacharacter gap. Full hermetic suite green; added
exec() stdin coverage.

Co-authored-by: Isaac
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant