Skip to content

fix(variable): mask values by default (PLA-2368) - #274

Open
canyugs wants to merge 2 commits into
mainfrom
can/pla-2368-mask-variable-values
Open

fix(variable): mask values by default (PLA-2368)#274
canyugs wants to merge 2 commits into
mainfrom
can/pla-2368-mask-variable-values

Conversation

@canyugs

@canyugs canyugs commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • stop variable create, delete, and env mutation results from echoing full variable maps; report only affected keys
  • mask service and readonly values in variable list table and JSON output by default
  • add explicit --show-values opt-in for callers that need plaintext list values
  • mask values in the interactive delete selector
  • share the existing variable mask policy with variable update and make it Unicode-safe

Verified scope

The original create / list report is reproducible against dc6168a (v0.21.0, current npm latest) in both table and JSON paths. A full variable-command audit also found the same plaintext-output class in delete and env, so this PR closes those paths too. Full values remain available internally for API mutation payloads and, for list, only through explicit --show-values.

Verification

  • command-level red/green regression tests for create, delete, env, and list table/JSON output
  • interactive delete selector regression test
  • go test ./...
  • go test -race ./internal/cmd/variable/... -count=1
  • golangci-lint run --new-from-rev=origin/main ./...
  • go run ./cmd/main.go variable list --help

Full-repo lint still reports two pre-existing prealloc findings in internal/cmd/domain/create/create.go and pkg/selector/selector.go; the changed scope and new-from-main lint are clean.

Fixes PLA-2368.

The create and list commands passed complete variable values directly to table and JSON printers. Keep full values only in the API update payload, show created keys without values, and require --show-values to reveal list output.
@linear-code

linear-code Bot commented Aug 25, 2026

Copy link
Copy Markdown

PLA-2368

@opencodezebra

opencodezebra Bot commented Aug 25, 2026

Copy link
Copy Markdown

Review Council started (round 1).

Baseline:

  • Scope: 7 files changed (+295/−17)
  • CI/checks: pending (0 contexts) at 3548eec

The council is reviewing this pull request; the verdict will follow as a separate comment when the round closes.

@opencodezebra

opencodezebra Bot commented Aug 25, 2026

Copy link
Copy Markdown

CHANGES REQUESTED ⚠️variable delete retains the same secret-echo leak class this PR fixes in create/list/update.
Reviewed at 3548eec (round 1)

What This PR Does

Prevents the Zeabur CLI from printing plaintext variable values by default. variable create now reports only the keys it created (no merged map), variable list masks values behind a --show-values opt-in flag, and variable update's interactive selector shares the new variablevalue.Mask helper — which is Unicode-safe (rune-based). Tests cover regression for all three commands.

How It Works

  • create/create.go — replaces the full varMap table with a key-only slice of the just-created keys.
  • list/list.go — adds maskVariables() that deep-copies each variable and rewrites .Value via the shared Mask() before rendering; gated behind !opts.showValues.
  • update/update.go — removes the local maskValue helper and calls the shared variablevalue.Mask.
  • value/value.go — canonical Mask(raw string) string using []rune for safe Unicode prefix extraction.

Findings

ID Severity Finding Location
F1 🟡 variable delete echoes all remaining variable values unmasked in table output and interactive selector — same leak class fixed elsewhere (raised by: rev-claude) internal/cmd/variable/delete/delete.go:150
F2 🟢 variable env echoes parsed .env values in output — inconsistent with mask-by-default policy, though values are local (raised by: rev-claude) internal/cmd/variable/env/env.go:118
F3 🟢 Both service and readonly variable lists are copied and masked before rendering — no mutation of API-returned data (raised by: rev-codex) internal/cmd/variable/list/list.go:97
F4 🟢 Rune-based prefix in Mask() avoids malformed UTF-8 or byte-slicing leakage (raised by: rev-codex) internal/cmd/variable/value/value.go:7
Finding Details

🟡 F1: variable delete echoes remaining variable values unmasked

After deletion succeeds, runDeleteVariableNonInteractive (line 150) iterates opts.keys (all remaining variables fetched via ListVariables) and prints a {"Key","Value"} table with raw values. The interactive selector (line 86) also displays fmt.Sprintf("%s = %s", k, v) without masking — unlike update.go's selector which now uses variablevalue.Mask.

Fix direction: Apply the same key-only table pattern used in the fixed create.go (report only deleted keys) for non-interactive output, and call variablevalue.Mask(v) in the interactive selector to match update.go.

What's Good (🟢)
  • Masking covers both service and readonly variable collections before any rendering path executes — no mutation of API data.
  • Rune-based Mask() handles multi-byte/Unicode values safely without byte-slice corruption.
  • --show-values is a clean explicit opt-in; cannot be triggered implicitly.
  • Regression tests assert absence of secret substrings (not just presence of mask pattern), providing strong leak detection.
  • create output now reports only the created keys, eliminating the previous full-map echo entirely.
Baseline Check
  • Main already has: maskValue in update.go (byte-based, local only).
  • Net-new value: shared Unicode-safe Mask(), mask-by-default in list, key-only create output, --show-values flag, and regression tests.
  • CI: lint, build-test, Analyze (go) in progress (no failures); [code]smith skipped.
Review Metadata
  • Reviewers: rev-codex (approve), rev-claude (request changes)
  • Consensus: request changes
  • Absent reviewers: none

🔴×0 🟡×1 🟢×3 · 💬 Comment @opencodezebra <question> for a follow-up · 🔁 Push new commits or comment @opencodezebra review <fix notes> to re-run the council · ✋ Disagree with a finding? @opencodezebra dismiss F1 <why it is not a defect>

@opencodezebra opencodezebra Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Council request_changes — 🔴0 🟡1 🟢3. Reviewed at 3548eec. Full report: #274 (comment)

The same audit found delete exposing full values in its selector and success table, while env echoed imported secrets in table and JSON output. Mask delete selections and report only affected keys after mutations.
@opencodezebra

opencodezebra Bot commented Aug 25, 2026

Copy link
Copy Markdown

Review Council started (round 2).

Baseline:

  • Scope: 11 files changed (+567/−29)
  • CI/checks: pending (0 contexts) at 94a2f66

The council is reviewing this pull request; the verdict will follow as a separate comment when the round closes.

@opencodezebra

opencodezebra Bot commented Aug 25, 2026

Copy link
Copy Markdown

LGTM ✅ — F1 resolved; all variable commands now mask values by default with consistent key-only mutation output.
Reviewed at 94a2f66 (round 2)

Delta since 3548eec

  • delete.go — interactive selector now calls variablevalue.Mask(v); non-interactive table/JSON output reports only opts.deleteKeys (key-only column), not the remaining variable map.
  • env.go — mutation success output mirrors the create key-only pattern for both table and JSON paths.
  • delete_test.go — new regression tests asserting absence of secret substrings in both non-interactive and interactive (selector) paths.
  • env_test.go — new regression test confirming imported values are not echoed.
  • list_test.go, value_test.go — additional coverage for mask/show-values behavior (carried from round 1).

Findings

Resolved Severity Finding Fixed in
F1 🟡 variable delete echoed remaining variable values unmasked in table output and interactive selector 94a2f66
New Severity Finding Location
No new findings
Finding Details

F1 (Resolved): variable delete unmasked values

Round 1 identified that delete.go printed all remaining variable values in its success table and showed plaintext in the interactive selector. Verified at current head: line 84 now uses variablevalue.Mask(v) in the selector, and lines 163–175 iterate only opts.deleteKeys to produce a key-only []string{key} table. JSON output emits deletedKeys array (keys only). The leak class is closed.

What's Good (🟢)
  • Masking consistently reuses the shared variablevalue.Mask helper across all four commands (create/list/delete/update) — no per-command reimplementation.
  • Rune-based Mask() safely handles multi-byte/Unicode values without byte-slice corruption.
  • --show-values is an explicit opt-in; cannot be triggered implicitly or by default.
  • Regression tests assert the absence of secret substrings rather than just presence of mask patterns — stronger leak detection.
  • API payloads retain full unmasked values; masking is display-only with no data-loss risk.
Baseline Check
  • Main already has: byte-based maskValue local to update.go.
  • Net-new value: shared Unicode-safe Mask(), mask-by-default in list, key-only output for create/delete/env, --show-values flag, and 6 new regression test functions.
  • CI: build-test ✅, lint ✅, Analyze (go) ✅ (completed during review), [code]smith skipped.
Review Metadata
  • Reviewers: rev-codex (approve), rev-claude (approve)
  • Consensus: approve
  • Absent reviewers: none

🔴×0 🟡×0 🟢×0 · 💬 Comment @opencodezebra <question> for a follow-up · 🔁 Push new commits or comment @opencodezebra review <fix notes> to re-run the council · ✋ Disagree with a finding? @opencodezebra dismiss F1 <why it is not a defect>

@opencodezebra opencodezebra Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Council approve — 🔴0 🟡0 🟢0. Reviewed at 94a2f66. Full report: #274 (comment)

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