diff --git a/PR87-test-checklist.md b/PR87-test-checklist.md new file mode 100644 index 0000000..aa1b231 --- /dev/null +++ b/PR87-test-checklist.md @@ -0,0 +1,50 @@ +# PR #87 — "Move to environment variable" (#83): manual test checklist + +The logic is covered by 15 unit tests in CI; what CI **can't** exercise is the GUI (no PySide6). This checklist is only the parts a human needs to click. Should take ~10 minutes. + +## Setup + +```bash +cd ~/Documents/Claude/Projects/BetterClaudeConfig/better-claude-config +git fetch origin +git checkout feat/83-move-to-env-var +git pull # ensure you're on 8fdbe90 or later +source .venv/bin/activate # or recreate: python3 -m venv .venv && source .venv/bin/activate && pip install -r requirements.txt +python bcc.py +``` + +Pick a **Claude Code** profile (e.g. `~/.claude.json`) that has, or add, a server with an env value that looks like a secret — e.g. `env: { "API_KEY": "ghp_test123" }`. (You can use a throwaway value; nothing is sent anywhere.) + +## The checklist + +### Gating — where the action appears +- [ ] Right-click the **value cell** of a secret env row (`API_KEY`) on a **Claude Code** profile → a **"Move to environment variable…"** item appears. +- [ ] Right-click a **non-secret** row (e.g. `REGION` = `us-east-1`) → the item does **not** appear. +- [ ] Right-click a row whose value is already a reference (`${API_KEY}`) → the item does **not** appear. +- [ ] Switch to a **Claude Desktop** profile (a `claude_desktop_config.json`), right-click the same kind of secret row → the item does **not** appear. (Desktop doesn't expand `${VAR}`, so offering it would break the config — this is the important gate.) + +### The dialog +- [ ] Trigger the action → dialog opens with **Variable** pre-filled from the key, sanitized to a legal shell name (e.g. `api-key` → `API_KEY`). +- [ ] Edit the variable name → the shown **shell line updates live** and matches your platform (`export VAR='…'` on macOS/Linux, `setx VAR "…"` on Windows), with the other platform shown in parentheses. +- [ ] If you type a variable name that **is already set** in your shell environment, the green "already looks set" note appears; if not, it's hidden. +- [ ] **Cancel** → nothing changes (value still the raw secret, no dirty state). + +### The conversion +- [ ] **Move && copy secret** → the cell now shows the reference `${VAR}` (visible, **not** masked to dots), and the window goes dirty (Save enabled). +- [ ] Paste from your clipboard somewhere → it's the **original secret value** (handed back before removal). +- [ ] The reference value is **not** flagged as a secret warning anymore (it's the recommended state). + +### Headers + persistence +- [ ] Repeat on a **remote server's Headers** table (e.g. an `Authorization` header) → same behavior. +- [ ] **Save**, then open the config file on disk in a text editor → the servers block holds `${VAR}`, and the **plaintext secret is gone** from the file. +- [ ] Re-open the profile in BCC → the row still shows `${VAR}` (round-trips). + +### Undo (nice-to-have) +- [ ] After a conversion, **Ctrl+Z / Cmd+Z** restores the previous value. + +## Known scope (not bugs) +- **Args rows** are out of scope for this PR — the core supports them, but the args editor is a free-text widget, so wiring that UI is a deliberate follow-up. Right-clicking args won't offer the action yet. +- The "already set" check reads **BCC's** environment, which may differ from the client's — it's advisory, worded that way. + +## If anything's off +Tell me which checkbox failed and what you saw; I'll fix on the branch and re-push. If everything passes, approve/merge #87 (or tell me to merge it). diff --git a/bcc.py b/bcc.py index dc224f7..a37eaeb 100644 --- a/bcc.py +++ b/bcc.py @@ -272,6 +272,226 @@ class _SecretMaskDelegate(QStyledItemDelegate): option.text = core.MASK +class MoveToEnvDialog(QDialog): + """Confirm moving a plaintext secret out to a ${VAR} reference (#83). + + The secret is about to leave the config file, so this dialog's whole job is + to hand it back first: it lets the user name the variable, shows the exact + shell line to set it, and (on accept) the caller copies the secret to the + clipboard. If the variable already looks set in this environment, it says so + and drops the urgency. + """ + + def __init__(self, parent, key: str, secret: str): + super().__init__(parent) + self.setWindowTitle("Move to environment variable") + self.setMinimumWidth(460) + self._secret = secret + v = QVBoxLayout(self) + v.setSpacing(10) + + intro = QLabel( + "This replaces the value in place with a ${VAR} reference. The secret " + "moves to your shell/OS environment — not this config file, and not the " + "Environment variables table below. Run the line below to set it there, " + "or the server won't authenticate." + ) + intro.setWordWrap(True) + v.addWidget(intro) + + grid = QGridLayout() + grid.setSpacing(8) + lbl = QLabel("Variable:") + lbl.setObjectName("muted") + grid.addWidget(lbl, 0, 0) + self._name_edit = QLineEdit(core.sanitize_env_var_name(key)) + self._name_edit.textChanged.connect(self._refresh) + grid.addWidget(self._name_edit, 0, 1) + v.addLayout(grid) + + self._already = QLabel("") + self._already.setWordWrap(True) + self._already.setStyleSheet(f"color: {GOOD};") + v.addWidget(self._already) + + set_lbl = QLabel("Set it with:") + set_lbl.setObjectName("muted") + v.addWidget(set_lbl) + self._cmd = QLabel("") + self._cmd.setWordWrap(True) + self._cmd.setTextInteractionFlags(Qt.TextInteractionFlag.TextSelectableByMouse) + self._cmd.setStyleSheet("font-family: monospace;") + v.addWidget(self._cmd) + + note = QLabel("The secret will be copied to your clipboard when you continue.") + note.setObjectName("muted") + note.setWordWrap(True) + v.addWidget(note) + + btns = QDialogButtonBox( + QDialogButtonBox.StandardButton.Ok | QDialogButtonBox.StandardButton.Cancel + ) + ok = btns.button(QDialogButtonBox.StandardButton.Ok) + ok.setText("Move && copy secret") + ok.setObjectName("primary") + btns.accepted.connect(self.accept) + btns.rejected.connect(self.reject) + v.addWidget(btns) + + self._refresh() + + def var_name(self) -> str: + return core.sanitize_env_var_name(self._name_edit.text()) + + def _refresh(self, *_): + name = self.var_name() + lines = core.shell_export_lines(name, self._secret) + if sys.platform == "win32": + self._cmd.setText(f"{lines['windows']}\n\n(macOS/Linux: {lines['posix']})") + else: + self._cmd.setText(f"{lines['posix']}\n\n(Windows: {lines['windows']})") + if core.is_env_var_set(name): + self._already.setText(f"{name} already looks set in this environment.") + self._already.show() + else: + self._already.hide() + + +class MoveArgToEnvDialog(QDialog): + """Confirm relocating a secret arg into the env block (#83, kept in file). + + Unlike the reference move, this keeps the value in the config -- it just + moves it out of the argument list (visible in process listings) and into + the Environment variables table, where the user can see and edit it. It + changes how the server is launched, so it says so plainly. + """ + + def __init__(self, parent, key: str, value: str): + super().__init__(parent) + self.setWindowTitle("Move into environment variables") + self.setMinimumWidth(460) + v = QVBoxLayout(self) + v.setSpacing(10) + + intro = QLabel( + "This moves the secret out of the arguments and into the Environment " + "variables table below, where you can see and edit its value. The value " + "stays in this config file." + ) + intro.setWordWrap(True) + v.addWidget(intro) + + warn = QLabel( + "⚠ This changes how the server is launched: the flag is dropped and the " + "value is set as an environment variable instead. It only works if the " + "server reads this secret from that variable." + ) + warn.setWordWrap(True) + warn.setStyleSheet(f"color: {WARN};") + v.addWidget(warn) + + grid = QGridLayout() + grid.setSpacing(8) + lbl = QLabel("Variable:") + lbl.setObjectName("muted") + grid.addWidget(lbl, 0, 0) + self._name_edit = QLineEdit(core.sanitize_env_var_name(key)) + grid.addWidget(self._name_edit, 0, 1) + v.addLayout(grid) + + btns = QDialogButtonBox( + QDialogButtonBox.StandardButton.Ok | QDialogButtonBox.StandardButton.Cancel + ) + ok = btns.button(QDialogButtonBox.StandardButton.Ok) + ok.setText("Move into env") + ok.setObjectName("primary") + btns.accepted.connect(self.accept) + btns.rejected.connect(self.reject) + v.addWidget(btns) + + def var_name(self) -> str: + return core.sanitize_env_var_name(self._name_edit.text()) + + +class ReferencedVarsDialog(QDialog): + """Show every ${VAR} the loaded server references and whether it's set (#83). + + After a secret becomes a reference, the variable lives in the user's + environment, not the config -- so this is where they confirm it exists and + get the command to set it. Read-only; BCC can't (and shouldn't) store the + value. + """ + + def __init__(self, parent, data: dict): + super().__init__(parent) + self.setWindowTitle("Referenced variables") + self.setMinimumWidth(560) + v = QVBoxLayout(self) + v.setSpacing(10) + + self._usages = core.referenced_env_vars(data) + if not self._usages: + v.addWidget(QLabel("This server references no ${VAR} variables.")) + btns = QDialogButtonBox(QDialogButtonBox.StandardButton.Close) + btns.rejected.connect(self.reject) + btns.accepted.connect(self.accept) + v.addWidget(btns) + return + + intro = QLabel( + "These references are read from your shell/OS environment when the client " + "runs. ✓ means it's set in BCC's environment (which may differ from the " + "client's) or has a default; ✗ means nothing would fill it." + ) + intro.setWordWrap(True) + v.addWidget(intro) + + self._table = QTableWidget(len(self._usages), 3) + self._table.setHorizontalHeaderLabels(["Variable", "Status", "Used in"]) + self._table.horizontalHeader().setSectionResizeMode(0, QHeaderView.ResizeMode.Stretch) + self._table.horizontalHeader().setSectionResizeMode(2, QHeaderView.ResizeMode.Stretch) + self._table.verticalHeader().setVisible(False) + self._table.setSelectionBehavior(QAbstractItemView.SelectionBehavior.SelectRows) + self._table.setEditTriggers(QAbstractItemView.EditTrigger.NoEditTriggers) + for r, u in enumerate(self._usages): + is_set = core.is_env_var_set(u.name) + status = "✓ set" if is_set else ("✓ default" if u.has_default else "✗ not set") + self._table.setItem(r, 0, QTableWidgetItem(u.name)) + self._table.setItem(r, 1, QTableWidgetItem(status)) + self._table.setItem(r, 2, QTableWidgetItem(", ".join(u.fields))) + self._table.selectionModel().selectionChanged.connect(self._refresh_cmd) + v.addWidget(self._table, 1) + + set_lbl = QLabel("Set the selected variable with:") + set_lbl.setObjectName("muted") + v.addWidget(set_lbl) + self._cmd = QLabel("") + self._cmd.setWordWrap(True) + self._cmd.setTextInteractionFlags(Qt.TextInteractionFlag.TextSelectableByMouse) + self._cmd.setStyleSheet("font-family: monospace;") + v.addWidget(self._cmd) + + btns = QDialogButtonBox(QDialogButtonBox.StandardButton.Close) + btns.rejected.connect(self.reject) + btns.accepted.connect(self.accept) + v.addWidget(btns) + + self._table.selectRow(0) + + def _refresh_cmd(self, *_): + rows = self._table.selectionModel().selectedRows() + if not rows: + self._cmd.setText("") + return + name = self._usages[rows[0].row()].name + # A placeholder value -- BCC doesn't hold the secret, this shows the shape. + lines = core.shell_export_lines(name, "") + if sys.platform == "win32": + self._cmd.setText(f"{lines['windows']}\n\n(macOS/Linux: {lines['posix']})") + else: + self._cmd.setText(f"{lines['posix']}\n\n(Windows: {lines['windows']})") + + class KeyValueTable(QWidget): def __init__(self, key_label="Key", val_label="Value", on_change=None, before_change=None): super().__init__() @@ -292,6 +512,11 @@ class KeyValueTable(QWidget): self.table.setSelectionBehavior(QAbstractItemView.SelectionBehavior.SelectRows) self.table.setMinimumHeight(90) self.table.itemChanged.connect(self._changed) + # Right-click a secret row to move it out to a ${VAR} reference (#83). + # Set by the owner (ServerEditor) so the action can gate on the client. + self.profile_provider = None + self.table.setContextMenuPolicy(Qt.ContextMenuPolicy.CustomContextMenu) + self.table.customContextMenuRequested.connect(self._context_menu) # Secret-looking values (API_KEY, TOKEN, ...) render masked by default. self._mask_delegate = _SecretMaskDelegate(self.table) self.table.setItemDelegateForColumn(1, self._mask_delegate) @@ -315,6 +540,58 @@ class KeyValueTable(QWidget): self.reveal_btn.setText("Hide secrets" if on else "Show secrets") self.table.viewport().update() + def _row_key_value(self, row: int): + key_item = self.table.item(row, 0) + val_item = self.table.item(row, 1) + key = key_item.text().strip() if key_item else "" + # The mask is display-only (a delegate); the model text is the real value. + value = val_item.text() if val_item else "" + return key, value + + def _context_menu(self, pos): + item = self.table.itemAt(pos) + if item is None: + return + row = item.row() + key, value = self._row_key_value(row) + # Only a real stored secret is worth moving; nothing to offer otherwise. + if not core.should_mask_value(key, value): + return + profile = self.profile_provider() if self.profile_provider else None + menu = QMenu(self) + act = QAction("Replace with a ${VAR} reference (out of file)…", self) + if core.can_move_value_to_env_ref(key, value, profile): + act.triggered.connect(lambda: self._move_row_to_env(row)) + else: + # Show it disabled with the reason rather than an empty menu, so the + # feature is discoverable and Claude Desktop's gating is explained. + act.setEnabled(False) + act.setText("Replace with ${VAR} reference — unavailable for Claude Desktop") + act.setToolTip( + "Claude Desktop doesn't expand ${VAR}, so a reference would reach " + "the server as literal text." + ) + menu.addAction(act) + menu.exec(self.table.viewport().mapToGlobal(pos)) + + def _move_row_to_env(self, row: int): + key, secret = self._row_key_value(row) + if not secret: + return + dlg = MoveToEnvDialog(self.window(), key, secret) + if not dlg.exec(): + return + var_name = dlg.var_name() + # Hand the secret back before it leaves the file: clipboard now holds it, + # and the dialog showed the exact shell line to set it. + QGuiApplication.clipboard().setText(secret) + if self._before_change: + self._before_change() + val_item = self.table.item(row, 1) + if val_item is not None: + # setText fires itemChanged -> _changed -> on_change (dirty + revalidate). + val_item.setText(f"${{{var_name}}}") + def _changed(self, item=None, *_): if item is not None and item.column() == 0: new_key = item.text().strip() @@ -520,6 +797,12 @@ class ServerEditor(QFrame): self.logs_btn = QPushButton("View logs") self.logs_btn.setToolTip("Open this server's MCP log in a read-only, auto-tailing viewer") self.logs_btn.clicked.connect(self._view_logs) + self.vars_btn = QPushButton("Variables…") + self.vars_btn.setToolTip( + "Show the ${VAR} references this server uses and whether each is set in " + "your environment" + ) + self.vars_btn.clicked.connect(self._show_referenced_vars) self.details_btn = QPushButton("Details ▸") self.details_btn.setCheckable(True) self.details_btn.toggled.connect(self._toggle_diag) @@ -531,6 +814,7 @@ class ServerEditor(QFrame): dep.addWidget(self.test_btn) dep.addWidget(self.spawn_btn) dep.addWidget(self.logs_btn) + dep.addWidget(self.vars_btn) dep.addWidget(self.details_btn) dep.addWidget(recheck) outer.addLayout(dep) @@ -631,6 +915,58 @@ class ServerEditor(QFrame): v.addWidget(self.headers, 1) return w + def set_profile_provider(self, provider): + """Let the env/headers tables and the args editor gate the secret-move + actions on which client the loaded profile targets (#83), and wire the + args editor's two move actions back to this editor (which owns the whole + form, since moving an arg into env touches both fields).""" + self.env.profile_provider = provider + self.headers.profile_provider = provider + self.args.profile_provider = provider + self.args.on_move_to_ref = self.move_arg_to_reference + self.args.on_move_to_env = self.move_arg_into_env + + # --- secret moves from args (#83) ------------------------------------ # + def _reload_from_data(self, new_data: dict): + """Repopulate the form from a transformed data dict and mark dirty.""" + self.load_entry(core.ServerEntry(self.current_name(), new_data, True)) + self._emit() + + def move_arg_to_reference(self, index: int): + """Args secret -> ${VAR} reference in place (secret leaves the file).""" + data = self.dump_data() + args = data.get("args") or [] + if not (0 <= index < len(args)): + return + dlg = MoveToEnvDialog( + self.window(), core.suggested_env_var_for_arg(args, index), args[index] + ) + if not dlg.exec(): + return + conv = core.move_value_to_env_ref(data, field="args", index=index, var_name=dlg.var_name()) + if conv is None: + return + QGuiApplication.clipboard().setText(conv.secret) + self._reload_from_data(conv.data) + + def move_arg_into_env(self, index: int): + """Args secret -> env block, kept in this config (visible/editable).""" + data = self.dump_data() + args = data.get("args") or [] + if not (0 <= index < len(args)): + return + default_name = core.suggested_env_var_for_arg(args, index) + dlg = MoveArgToEnvDialog(self.window(), default_name, args[index]) + if not dlg.exec(): + return + new = core.move_arg_to_env_block(data, index, var_name=dlg.var_name()) + if new is None: + return + self._reload_from_data(new) + + def _show_referenced_vars(self): + ReferencedVarsDialog(self.window(), self.dump_data()).exec() + # --- model <-> form -------------------------------------------------- # def load_entry(self, entry: core.ServerEntry | None): self._loading = True @@ -908,6 +1244,52 @@ class ArgsEdit(QPlainTextEdit): self.blockCountChanged.connect(self._update_gutter_width) self.updateRequest.connect(self._on_update_request) self._update_gutter_width() + # Wired by ServerEditor: gate on the loaded client, and the two move + # actions (which the editor performs, since moving an arg into env + # touches both the args and the env table). Indices are into the + # non-blank arg list, matching dump_data()'s args. + self.profile_provider = None + self.on_move_to_ref = None + self.on_move_to_env = None + + def contextMenuEvent(self, event): + menu = self.createStandardContextMenu() # keep cut/copy/paste + lines = self.toPlainText().splitlines() + block = self.cursorForPosition(event.pos()).blockNumber() + if 0 <= block < len(lines) and lines[block].strip(): + # This editor is one arg per line; map the clicked block to its + # index among the non-blank args the model actually sees. + cleaned = [ln for ln in lines if ln.strip() != ""] + idx = sum(1 for ln in lines[:block] if ln.strip() != "") + if idx in set(core.secret_arg_indices(cleaned)): + profile = self.profile_provider() if self.profile_provider else None + expands = profile is None or core.client_expands_env_refs(profile) + first = menu.actions()[0] if menu.actions() else None + + # Reference (secret leaves the file) -- needs an expanding client. + ref_act = QAction("Replace with a ${VAR} reference (out of file)…", self) + if expands and self.on_move_to_ref: + ref_act.triggered.connect(lambda: self.on_move_to_ref(idx)) + else: + ref_act.setEnabled(False) + ref_act.setText( + "Replace with ${VAR} reference — unavailable for Claude Desktop" + ) + ref_act.setToolTip( + "Claude Desktop doesn't expand ${VAR}, so a reference would " + "reach the server as literal text." + ) + + # Move into the env block (kept in file) -- works on any client. + env_act = QAction("Move into Environment variables (kept in this config)…", self) + if self.on_move_to_env: + env_act.triggered.connect(lambda: self.on_move_to_env(idx)) + + menu.insertAction(first, ref_act) + menu.insertAction(first, env_act) + if first is not None: + menu.insertSeparator(first) + menu.exec(event.globalPos()) def gutter_width(self) -> int: digits = max(1, len(str(self.blockCount()))) @@ -1649,6 +2031,7 @@ class MainWindow(QMainWindow): split.setHandleWidth(10) split.addWidget(self._build_left()) self.editor = ServerEditor(on_change=self._editor_changed, before_change=self._push_undo) + self.editor.set_profile_provider(lambda: self.current_profile) split.addWidget(self.editor) split.setStretchFactor(0, 3) split.setStretchFactor(1, 4) diff --git a/bcc_core.py b/bcc_core.py index d709b28..f77441c 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -1837,6 +1837,211 @@ def env_ref_warnings( ] +# --------------------------------------------------------------------------- # +# "Move to environment variable" (issue #83) +# +# Turn a plaintext secret in a config into a ${VAR} reference, so the secret +# stops living in the file. Two things make this more than a string swap: +# * the replaced value is the ONLY copy of the secret the user may have, so +# the caller must hand it back (clipboard + the exact shell line) before it +# leaves the config -- shell_export_lines builds that; +# * only clients that expand ${VAR} (Claude Code, not Desktop) should be +# offered this, or it walks the user straight into a broken config -- that +# gate is can_move_value_to_env_ref, reusing client_expands_env_refs. +# The rewrite itself is pure (data in -> data out) and lives here; the GUI does +# clipboard, the confirm dialog, and the already-set check. +# --------------------------------------------------------------------------- # +@dataclass(frozen=True) +class EnvRefConversion: + """The result of moving one secret out to a ${VAR} reference.""" + + var_name: str # the sanitised shell variable name chosen + reference: str # "${VAR_NAME}" -- what now sits in the config + secret: str # the plaintext value that was removed (hand this back!) + data: dict # a NEW server-definition dict with the value replaced + + +def sanitize_env_var_name(name: str) -> str: + """Coerce an arbitrary key into a legal, conventional shell variable name. + + POSIX names are `[A-Za-z_][A-Za-z0-9_]*`; env vars are conventionally + upper-case. Non-alphanumerics (and any non-ASCII) become `_`, a leading + digit gets an `_` prefix, and an empty/degenerate result falls back to VAR. + 'api-key' -> 'API_KEY'; '2fa' -> '_2FA'; '' -> 'VAR'. + """ + cleaned = "".join( + ch if (ch.isascii() and (ch.isalnum() or ch == "_")) else "_" for ch in (name or "") + ) + if not cleaned.strip("_"): + return "VAR" + if cleaned[0].isdigit(): + cleaned = "_" + cleaned + return cleaned.upper() + + +def _posix_single_quote(s: str) -> str: + """Wrap s in single quotes, safely, for a POSIX shell (handles embedded ').""" + return "'" + s.replace("'", "'\\''") + "'" + + +def shell_export_lines(var_name: str, secret: str) -> dict[str, str]: + """The exact command to set `var_name`=`secret` in the user's shell. + + Returned per-platform so the UI can show the one that fits (or both). This + is what makes moving the secret out safe: the user gets the setter before + the value leaves the file. + """ + return { + "posix": f"export {var_name}={_posix_single_quote(secret)}", + "windows": f'setx {var_name} "{secret}"', + } + + +def can_move_value_to_env_ref(key: str, value, profile: Profile | None = None) -> bool: + """Whether to offer "move to environment variable" for one env/header row. + + True only for a real stored secret (`should_mask_value`) that isn't already + a reference, on a client that expands references. Offering it on Claude + Desktop would produce a config that reaches the server as literal `${VAR}` + text -- the exact failure #76 exists to prevent -- so a non-expanding + profile refuses outright. + """ + if profile is not None and not client_expands_env_refs(profile): + return False + if is_env_ref(value): + return False + return should_mask_value(key, value) + + +def move_value_to_env_ref( + data: dict, *, field: str, key: str | None = None, index: int | None = None, var_name=None +) -> EnvRefConversion | None: + """Replace one secret value in `data` with a `${VAR}` reference. + + `field` is "env" or "headers" (addressed by `key`) or "args" (addressed by + `index`). `var_name` defaults to the row's key (sanitised); args have no + key, so a var name should be supplied there. Returns an EnvRefConversion + carrying a NEW data dict (the input is never mutated) and the removed + secret, or None if the target isn't found or isn't a string to move. + """ + new = copy.deepcopy(data) + + if field in ("env", "headers"): + block = new.get(field) + if not isinstance(block, dict) or key not in block: + return None + current = block[key] + if not isinstance(current, str) or is_env_ref(current): + return None + vn = sanitize_env_var_name(var_name if var_name is not None else key) + block[key] = f"${{{vn}}}" + elif field == "args": + args = new.get("args") + if not isinstance(args, list) or index is None or not (0 <= index < len(args)): + return None + current = args[index] + if not isinstance(current, str) or is_env_ref(current): + return None + vn = sanitize_env_var_name(var_name if var_name is not None else f"ARG_{index}") + args[index] = f"${{{vn}}}" + else: + return None + + return EnvRefConversion(var_name=vn, reference=f"${{{vn}}}", secret=current, data=new) + + +def is_env_var_set(var_name: str, environ: dict | None = None) -> bool: + """Whether `var_name` is already present (non-empty) in the environment. + + Lets the UI skip the copy-the-secret ceremony when the variable is already + set. Best-effort: BCC's environment may differ from the client's. + """ + env = os.environ if environ is None else environ + return bool(env.get(var_name)) + + +class EnvVarUsage(NamedTuple): + """One ${VAR} referenced by a server definition, and whether it resolves. + + `resolved` is best-effort: a variable is considered resolvable if it's set + in the checked environment OR any occurrence carries a `:-default`. Since + BCC's environment isn't necessarily the client's, this is advisory (the UI + says so). + """ + + name: str + fields: tuple[str, ...] # which config fields it appears in (sorted) + has_default: bool + resolved: bool + + +def referenced_env_vars(data: dict, environ: dict | None = None) -> list[EnvVarUsage]: + """Every distinct ${VAR} a server definition references, with its status. + + This is the "where did my secret go / is it wired up?" readout for #83: + after a value becomes `${VAR}`, the variable lives in the user's + environment, not the config, so BCC surfaces the list and whether each one + currently resolves. Sorted by name; deduped across fields. + """ + env = os.environ if environ is None else environ + by_name: dict[str, dict] = {} + for ref in server_env_refs(data): + slot = by_name.setdefault(ref.name, {"fields": set(), "has_default": False}) + slot["fields"].add(ref.field) + slot["has_default"] = slot["has_default"] or ref.has_default + out: list[EnvVarUsage] = [] + for name in sorted(by_name): + slot = by_name[name] + has_default = slot["has_default"] + out.append( + EnvVarUsage( + name=name, + fields=tuple(sorted(slot["fields"])), + has_default=has_default, + resolved=has_default or name in env, + ) + ) + return out + + +def move_arg_to_env_block(data: dict, index: int, var_name=None) -> dict | None: + """Relocate one secret arg into the `env` block, keeping the value in-file. + + The "managed in-file" alternative to a ${VAR} reference: the secret leaves + the args (where it's visible in process listings) and becomes an env entry + the user can see and edit in BCC's table. Returns a NEW data dict, or None + if the target isn't a movable string. + + When the arg follows a `--flag`, the flag is removed too, since a server + that reads the secret from an env var no longer needs the switch. This + changes how the server is launched -- the GUI warns before doing it. + """ + new = copy.deepcopy(data) + args = new.get("args") + if not isinstance(args, list) or not (0 <= index < len(args)): + return None + value = args[index] + if not isinstance(value, str) or is_env_ref(value): + return None + + vn = sanitize_env_var_name( + var_name if var_name is not None else suggested_env_var_for_arg(args, index) + ) + + # Drop the value, and the preceding flag if there is one (--api-key SECRET). + remove_from = index + if index > 0 and isinstance(args[index - 1], str) and args[index - 1].startswith("-"): + remove_from = index - 1 + del args[remove_from : index + 1] + + env = new.get("env") + if not isinstance(env, dict): + env = {} + new["env"] = env + env[vn] = value + return new + + def is_secret_key(name: str) -> bool: """Does this env-var / header / flag name look like it holds a secret?""" return bool(_SECRET_KEY_RE.search(name or "")) @@ -1928,6 +2133,54 @@ def args_secret_warning(data: dict) -> str | None: return None +def secret_arg_indices(args: list) -> list[int]: + """Indices of args that look like a raw credential. + + Same detection as `args_secret_warning`, but per-arg so the "move to + environment variable" action (#83) knows exactly which arg to offer on. + Flags a token-prefixed positional (ghp_..., sk-...), an embedded-credential + URL, or the value following a secret-named flag (`--token abc`). A `${VAR}` + reference is never flagged -- it's the fix, not the problem. + """ + out: list[int] = [] + mask_next = False + for i, a in enumerate(str(x) for x in args): + if is_env_ref(a): + mask_next = False + continue + if mask_next: + mask_next = False + if not a.startswith("-"): + out.append(i) + continue + if a.startswith("-") and "=" in a: + continue + if a.startswith("-") and is_secret_key(a): + mask_next = True + continue + if a.startswith("-"): + continue + if _is_secret_value(a) or _EMBEDDED_CRED_RE.search(a): + out.append(i) + return out + + +def suggested_env_var_for_arg(args: list, index: int) -> str: + """A default variable name for moving `args[index]` to a reference. + + Uses the preceding flag when there is one (`--api-key ` -> + API_KEY), since that names what the value is; otherwise falls back to a + generic SECRET. Always a legal shell name. + """ + if 0 < index <= len(args): + prev = str(args[index - 1]) if index - 1 < len(args) else "" + if prev.startswith("-"): + base = prev.lstrip("-").split("=", 1)[0] + if base: + return sanitize_env_var_name(base) + return "SECRET" + + def split_suspicious_args(args: list[str]) -> tuple[list[str], list[str]]: """ Detect the classic argument-entry mistake: several argv tokens typed on one diff --git a/tests/test_core.py b/tests/test_core.py index fd36e80..7c8c0ec 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -3059,3 +3059,175 @@ def test_discover_project_configs_skips_non_object_and_garbage(tmp_path): assert str(array / ".mcp.json") not in paths assert str(garbage / ".mcp.json") not in paths assert str(missing / ".mcp.json") not in paths + + +# --------------------------------------------------------------------------- # +# Move to environment variable (issue #83) +# --------------------------------------------------------------------------- # +@pytest.mark.parametrize( + "raw,expected", + [ + ("API_KEY", "API_KEY"), + ("api-key", "API_KEY"), + ("x.y z", "X_Y_Z"), + ("2fa", "_2FA"), + ("", "VAR"), + ("***", "VAR"), + ("clé", "CL_"), # non-ASCII becomes _ + ], +) +def test_sanitize_env_var_name(raw, expected): + assert c.sanitize_env_var_name(raw) == expected + + +def test_shell_export_lines_quote_safely(): + lines = c.shell_export_lines("TOKEN", "ab'cd") + assert lines["posix"] == "export TOKEN='ab'\\''cd'" + assert lines["windows"] == 'setx TOKEN "ab\'cd"' + + +def test_can_move_gate_requires_secret_and_expanding_client(): + desktop = c.Profile(label="d", path="/x/Claude/claude_desktop_config.json", config_exists=True) + code = c.Profile(label="c", path=Path.home() / ".claude.json", config_exists=True) + # real secret on an expanding client -> offer + assert c.can_move_value_to_env_ref("API_KEY", "ghp_abc", code) is True + assert c.can_move_value_to_env_ref("API_KEY", "ghp_abc", None) is True + # non-secret key -> no + assert c.can_move_value_to_env_ref("REGION", "us-east-1", code) is False + # already a reference -> no + assert c.can_move_value_to_env_ref("API_KEY", "${API_KEY}", code) is False + # non-expanding client (Claude Desktop) -> refuse even a real secret + assert c.can_move_value_to_env_ref("API_KEY", "ghp_abc", desktop) is False + + +def test_move_env_value_replaces_with_reference_and_returns_secret(): + data = {"command": "x", "env": {"API_KEY": "ghp_secret", "REGION": "us"}} + conv = c.move_value_to_env_ref(data, field="env", key="API_KEY") + assert conv is not None + assert conv.var_name == "API_KEY" + assert conv.reference == "${API_KEY}" + assert conv.secret == "ghp_secret" + assert conv.data["env"]["API_KEY"] == "${API_KEY}" + # non-secret row untouched + assert conv.data["env"]["REGION"] == "us" + # input never mutated + assert data["env"]["API_KEY"] == "ghp_secret" + + +def test_move_derives_and_sanitises_var_name_from_key(): + data = {"headers": {"x-api-key": "sekret"}} + conv = c.move_value_to_env_ref(data, field="headers", key="x-api-key") + assert conv.var_name == "X_API_KEY" + assert conv.data["headers"]["x-api-key"] == "${X_API_KEY}" + + +def test_move_honours_explicit_var_name(): + data = {"env": {"tok": "sekret"}} + conv = c.move_value_to_env_ref(data, field="env", key="tok", var_name="GITHUB_TOKEN") + assert conv.reference == "${GITHUB_TOKEN}" + assert conv.data["env"]["tok"] == "${GITHUB_TOKEN}" + + +def test_move_args_by_index(): + data = {"command": "x", "args": ["--token", "ghp_secret"]} + conv = c.move_value_to_env_ref(data, field="args", index=1, var_name="GH_TOKEN") + assert conv.secret == "ghp_secret" + assert conv.data["args"] == ["--token", "${GH_TOKEN}"] + + +def test_move_returns_none_on_missing_or_nonstring_or_already_ref(): + data = {"env": {"API_KEY": "${API_KEY}", "N": 5}} + assert c.move_value_to_env_ref(data, field="env", key="ABSENT") is None + assert c.move_value_to_env_ref(data, field="env", key="N") is None # not a string + assert c.move_value_to_env_ref(data, field="env", key="API_KEY") is None # already a ref + assert c.move_value_to_env_ref({}, field="bogus") is None + assert c.move_value_to_env_ref({"args": ["a"]}, field="args", index=9) is None + + +def test_is_env_var_set(): + assert c.is_env_var_set("FOO", {"FOO": "x"}) is True + assert c.is_env_var_set("FOO", {"FOO": ""}) is False + assert c.is_env_var_set("FOO", {}) is False + + +# --------------------------------------------------------------------------- # +# Args secret indices + suggested var name (issue #83, args surface) +# --------------------------------------------------------------------------- # +def test_secret_arg_indices_flags_token_and_flag_value(): + args = ["--port", "8080", "ghp_deadbeef", "--token", "sk-abc", "--flag=val"] + idxs = c.secret_arg_indices(args) + assert 2 in idxs # ghp_ token prefix + assert 4 in idxs # value following --token + assert 1 not in idxs # 8080 + assert 5 not in idxs # --flag=val inline pair + + +def test_secret_arg_indices_flags_embedded_url_credentials(): + args = ["postgres://user:pass@host/db"] + assert c.secret_arg_indices(args) == [0] + + +def test_secret_arg_indices_excludes_existing_references(): + args = ["--token", "${GH_TOKEN}"] + assert c.secret_arg_indices(args) == [] + + +def test_suggested_env_var_for_arg_uses_preceding_flag(): + args = ["--api-key", "sk-secret"] + assert c.suggested_env_var_for_arg(args, 1) == "API_KEY" + + +def test_suggested_env_var_for_arg_falls_back_when_no_flag(): + args = ["ghp_secret"] + assert c.suggested_env_var_for_arg(args, 0) == "SECRET" + + +# --------------------------------------------------------------------------- # +# Referenced-variables readout + args->env relocation (issue #83, "both") +# --------------------------------------------------------------------------- # +def test_referenced_env_vars_dedupes_and_reports_status(): + data = { + "command": "npx", + "args": ["--token", "${GH_TOKEN}", "${GH_TOKEN}"], + "env": {"API_KEY": "${API_KEY}", "REGION": "${REGION:-us-east-1}"}, + } + usages = c.referenced_env_vars(data, environ={"GH_TOKEN": "x"}) + by = {u.name: u for u in usages} + assert set(by) == {"GH_TOKEN", "API_KEY", "REGION"} + # GH_TOKEN appears only in args, deduped to one entry, set in env -> resolved + assert by["GH_TOKEN"].fields == ("args",) + assert by["GH_TOKEN"].resolved is True + # API_KEY not set, no default -> unresolved + assert by["API_KEY"].resolved is False + # REGION has a default -> resolved regardless of environment + assert by["REGION"].has_default is True + assert by["REGION"].resolved is True + # sorted by name + assert [u.name for u in usages] == sorted(u.name for u in usages) + + +def test_referenced_env_vars_empty_when_no_refs(): + assert c.referenced_env_vars({"command": "npx", "args": ["-y", "pkg"]}) == [] + + +def test_move_arg_to_env_block_removes_flag_and_value(): + data = {"command": "x", "args": ["--api-key", "sk-secret", "run"], "env": {"KEEP": "1"}} + out = c.move_arg_to_env_block(data, 1) + assert out["args"] == ["run"] # flag + value both gone + assert out["env"]["API_KEY"] == "sk-secret" + assert out["env"]["KEEP"] == "1" # existing env preserved + # input not mutated + assert data["args"] == ["--api-key", "sk-secret", "run"] + + +def test_move_arg_to_env_block_bare_positional_keeps_no_flag(): + data = {"command": "x", "args": ["ghp_secret", "serve"]} + out = c.move_arg_to_env_block(data, 0, var_name="GITHUB_TOKEN") + assert out["args"] == ["serve"] + assert out["env"] == {"GITHUB_TOKEN": "ghp_secret"} + + +def test_move_arg_to_env_block_none_on_bad_target(): + assert c.move_arg_to_env_block({"args": ["a"]}, 5) is None + assert c.move_arg_to_env_block({"args": ["${REF}"]}, 0) is None # already a ref + assert c.move_arg_to_env_block({}, 0) is None