Harden QLC CLI and smoke wiring#6
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an optional Bouncy Castle perimeter signature layer to workflow metadata via a local bcctl provider, adding new CLI commands (bc-status, bc-sign, bc-verify), integration into the workflow generation, documentation, and unit tests. The review feedback highlights several robust improvements for the new bc_perimeter.py module, including generalizing the path redaction regex to support Unix absolute paths, explicitly specifying utf-8 encoding in subprocess.run to prevent platform-specific decoding errors, falling back to shutil.which for command-name configurations, and adding type checks to validation functions to avoid potential AttributeError exceptions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| _SECRETISH_RE = re.compile( | ||
| r"(?i)(api[_-]?key|authorization|bearer|credential|password|passphrase|private[_-]?key|secret|token)" | ||
| ) | ||
| _WINDOWS_PATH_RE = re.compile(r"[A-Za-z]:/[^\s\"']+") |
There was a problem hiding this comment.
The current _WINDOWS_PATH_RE regex only matches Windows-style absolute paths (e.g., C:/Users/...). Absolute Unix paths (e.g., /home/user/... or /var/log/...) are not matched or redacted, which could leak local filesystem paths in public status outputs. Let's generalize this regex to match both Windows and Unix absolute paths.
| _WINDOWS_PATH_RE = re.compile(r"[A-Za-z]:/[^\s\"']+") | |
| _PATH_RE = re.compile(r"(?:[A-Za-z]:)?/(?:[^/\s\"\']+/)*[^/\s\"\']+") |
| completed = subprocess.run( | ||
| command, | ||
| shell=False, | ||
| check=False, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=self.timeout_seconds, | ||
| ) |
There was a problem hiding this comment.
Calling subprocess.run with text=True but without specifying an explicit encoding will use the system's default locale encoding (e.g., cp1252 on Windows). If the external bcctl tool outputs UTF-8 encoded JSON, this can lead to a UnicodeDecodeError on non-UTF-8 platforms. Specifying encoding="utf-8" prevents this issue.
| completed = subprocess.run( | |
| command, | |
| shell=False, | |
| check=False, | |
| capture_output=True, | |
| text=True, | |
| timeout=self.timeout_seconds, | |
| ) | |
| completed = subprocess.run( | |
| command, | |
| shell=False, | |
| check=False, | |
| capture_output=True, | |
| encoding="utf-8", | |
| timeout=self.timeout_seconds, | |
| ) |
| def _resolve_bcctl_executable() -> str | None: | ||
| configured = os.environ.get("FFED_BCCTL_PATH") | ||
| if configured: | ||
| path = Path(configured) | ||
| return str(path) if path.is_file() else None | ||
| return shutil.which("bcctl") |
There was a problem hiding this comment.
If FFED_BCCTL_PATH is configured as a simple command name (e.g., "bcctl-custom") rather than an absolute or relative path to a file, Path(configured).is_file() will return False (unless it happens to exist in the current working directory). This prevents the CLI from resolving custom executable names that are available on the system PATH. Let's fall back to shutil.which for the configured name if it is not a direct file path.
| def _resolve_bcctl_executable() -> str | None: | |
| configured = os.environ.get("FFED_BCCTL_PATH") | |
| if configured: | |
| path = Path(configured) | |
| return str(path) if path.is_file() else None | |
| return shutil.which("bcctl") | |
| def _resolve_bcctl_executable() -> str | None: | |
| configured = os.environ.get("FFED_BCCTL_PATH") | |
| if configured: | |
| path = Path(configured) | |
| if path.is_file(): | |
| return str(path) | |
| resolved = shutil.which(configured) | |
| if resolved: | |
| return resolved | |
| return None | |
| return shutil.which("bcctl") |
| def _validate_key_id(value: str) -> None: | ||
| if not _SAFE_TOKEN_RE.fullmatch(value) or _SECRETISH_RE.search(value): | ||
| raise ValueError("key_id must be a short public-safe token") |
There was a problem hiding this comment.
If value is not a string (e.g., None or an integer), calling .fullmatch() or .search() on it will raise an AttributeError. Adding an explicit isinstance(value, str) check ensures safe handling of invalid inputs.
| def _validate_key_id(value: str) -> None: | |
| if not _SAFE_TOKEN_RE.fullmatch(value) or _SECRETISH_RE.search(value): | |
| raise ValueError("key_id must be a short public-safe token") | |
| def _validate_key_id(value: str) -> None: | |
| if not isinstance(value, str) or not _SAFE_TOKEN_RE.fullmatch(value) or _SECRETISH_RE.search(value): | |
| raise ValueError("key_id must be a short public-safe token") |
| def _validate_hex_digest(name: str, value: str) -> None: | ||
| if not _HEX_DIGEST_RE.fullmatch(value): | ||
| raise ValueError(f"{name} must be a public hex digest") |
There was a problem hiding this comment.
If value is not a string, calling .fullmatch() on it will raise an AttributeError. Adding an explicit isinstance(value, str) check ensures safe handling of invalid inputs.
| def _validate_hex_digest(name: str, value: str) -> None: | |
| if not _HEX_DIGEST_RE.fullmatch(value): | |
| raise ValueError(f"{name} must be a public hex digest") | |
| def _validate_hex_digest(name: str, value: str) -> None: | |
| if not isinstance(value, str) or not _HEX_DIGEST_RE.fullmatch(value): | |
| raise ValueError(f"{name} must be a public hex digest") |
|
|
||
| def _sanitize_output_text(value: str) -> str: | ||
| compact = " ".join(value.replace("\\", "/").split()) | ||
| compact = _WINDOWS_PATH_RE.sub("[path]", compact) |
There was a problem hiding this comment.
Update the path replacement to use the generalized _PATH_RE regex to ensure absolute Unix paths are also redacted.
| compact = _WINDOWS_PATH_RE.sub("[path]", compact) | |
| compact = _PATH_RE.sub("[path]", compact) |
SeCuReDmE-main-dev
deleted the
new/security-remediation-refactor-20260625
branch
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91717e52dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _sanitize_output_text(value: str) -> str: | ||
| compact = " ".join(value.replace("\\", "/").split()) | ||
| compact = _WINDOWS_PATH_RE.sub("[path]", compact) | ||
| compact = _SECRETISH_RE.sub("[redacted]", compact) |
There was a problem hiding this comment.
When bcctl returns a JSON error detail such as API_KEY=sk_live_... or Bearer abc..., this substitution only replaces the secret-ish label and leaves the actual value in the BcctlError/CLI traceback, so bc-sign or protect-workflow --bcctl-sign can expose credentials despite the public-safe boundary. Redact the whole key/value or bearer-token span, not just the keyword.
Useful? React with 👍 / 👎.
| signature_b64 = str(payload.get("signature_b64") or "") | ||
| signature_digest = hashlib.sha256(signature_b64.encode("utf-8")).hexdigest() |
There was a problem hiding this comment.
When bcctl returns status: ok but omits or nulls signature_b64, this coerces it to an empty string and publishes the SHA-256 of that empty value as a successful signature. In that provider-output scenario, protect-workflow --bcctl-sign emits a perimeter receipt with no usable signature instead of failing closed, so later verification cannot validate the receipt.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Public-safe boundary