[codex] Harden release secret hygiene scans#8730
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![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: e95a0f29e3
ℹ️ 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".
| or path.is_relative_to(ROOT / "plugins" / "apps-js") | ||
| path.is_relative_to(ROOT / "web") | ||
| or path.is_relative_to(ROOT / "plugins") | ||
| or path.is_relative_to(ROOT / "mcp") |
There was a problem hiding this comment.
This extends the scanner to mcp Dockerfiles, but the change detectors that decide whether to run it were not updated: .github/workflows/lint.yml:43 and scripts/pre-commit:25 still match only plugins/apps-js, web/frontend, web/personas-open-source, and desktop/macos (pre-push likewise omits mcp). A PR that only changes mcp/Dockerfile or mcp/release.sh can therefore introduce a denied build arg/log leak and skip this check entirely in PR CI/hooks, so the new coverage is not enforced for those paths.
Useful? React with 👍 / 👎.
| errors.append( | ||
| f"{rel}:{lineno}: public env file write references server-only {', '.join(sorted(denied_refs))}" | ||
| ) | ||
| writes_to_stdout = ">" not in line and "|" not in line |
There was a problem hiding this comment.
Because writes_to_stdout is false for every line containing |, the new log-hygiene check silently allows release commands such as echo "$OPENAI_API_KEY" | cat or printf ... | tee /dev/stderr, even though those still print the secret into CI logs. Only safe sinks like docker login --password-stdin need this exception; otherwise piped echo/printf lines with denied refs should still be reported.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 5 files
Confidence score: 2/5
- In
scripts/check-public-client-secrets.py, the trace detection (SHELL_TRACE_RE) misses common forms likeset -xe/set -euxo pipefail, so traced secrets can slip past the scanner and reach logs or artifacts; that creates a real secret-exposure risk if merged as-is — broaden the trace regex to cover combined/ordered shell flags before merging. - In
scripts/check-public-client-secrets.py(writes_to_stdout), treating any command with|as non-emitting skips cases likeecho "$SECRET" | catandprintf ... | tee /dev/stderr, which can still print secrets and evade checks; merging now leaves a concrete blind spot in leak detection — tighten the stdout heuristic so piped echo/printf patterns are still flagged. - The scanner’s scope expansion to
mcp/**isn’t matched by CI/pre-push path filters, somcp-only changes may bypassscripts/check-public-client-secrets.pyentirely and ship unscanned — update workflow and hook path filters to includemcp/before merge to keep enforcement consistent.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/check-public-client-secrets.py">
<violation number="1" location="scripts/check-public-client-secrets.py:451">
P2: The scanner now covers `mcp/**` Dockerfiles, but the CI workflow and pre-push hook path filters that gate when this scanner runs were not updated to include `mcp/` paths. A PR that only touches `mcp/Dockerfile` or `mcp/release.sh` could merge without triggering this check, leaving the new coverage unenforced.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| files.append(path) | ||
| elif path.name == "release.sh" and ( | ||
| path.is_relative_to(ROOT / "app") | ||
| or path.is_relative_to(ROOT / "mcp") |
There was a problem hiding this comment.
P2: The scanner now covers mcp/** Dockerfiles, but the CI workflow and pre-push hook path filters that gate when this scanner runs were not updated to include mcp/ paths. A PR that only touches mcp/Dockerfile or mcp/release.sh could merge without triggering this check, leaving the new coverage unenforced.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/check-public-client-secrets.py, line 451:
<comment>The scanner now covers `mcp/**` Dockerfiles, but the CI workflow and pre-push hook path filters that gate when this scanner runs were not updated to include `mcp/` paths. A PR that only touches `mcp/Dockerfile` or `mcp/release.sh` could merge without triggering this check, leaving the new coverage unenforced.</comment>
<file context>
@@ -385,9 +405,110 @@ def check_codemagic(policy: dict) -> list[str]:
+ files.append(path)
+ elif path.name == "release.sh" and (
+ path.is_relative_to(ROOT / "app")
+ or path.is_relative_to(ROOT / "mcp")
+ or path.is_relative_to(ROOT / "plugins")
+ or path.is_relative_to(ROOT / "sdks")
</file context>
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/check-public-client-secrets.py">
<violation number="1" location="scripts/check-public-client-secrets.py:451">
P2: The scanner now covers `mcp/**` Dockerfiles, but the CI workflow and pre-push hook path filters that gate when this scanner runs were not updated to include `mcp/` paths. A PR that only touches `mcp/Dockerfile` or `mcp/release.sh` could merge without triggering this check, leaving the new coverage unenforced.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Git-on-my-level
force-pushed
the
codex/8726-release-secret-hygiene
branch
from
69c19c2 to
8625493
Compare
Git-on-my-level
force-pushed
the
codex/8726-release-secret-hygiene
branch
from
8625493 to
bbc8ec1
Compare
1 participant
Summary
Closes #8726.
This audits the post-#8717 public release secret boundary and fills the remaining public-safe scanner gaps:
set -xblocks that touch server-only env refsweb/**,plugins/**,mcp/**, anddesktop/**Secret-safety note
No production credentials were read, printed, stored, or committed. Tests use fake sentinel values only. No release, deploy, provider, or artifact publication actions were run.
Validation
python3 scripts/test-public-secret-scanners.py-> OK, 4 testspython3 scripts/check-public-client-secrets.py-> passedgit diff --checkpython3 -m py_compile scripts/check-public-client-secrets.py scripts/scan-public-artifact-secrets.py scripts/test-public-secret-scanners.pyblack --check --line-length 120 --skip-string-normalization scripts/check-public-client-secrets.py scripts/test-public-secret-scanners.pyorigin/codex/8726-release-secret-hygiene-> passed, including OpenAPI up-to-date check