t4939: Add env-var-not-argument pattern to secret handling rules#4941
t4939: Add env-var-not-argument pattern to secret handling rules#4941marcusquinn merged 1 commit intomainfrom
Conversation
When a subprocess needs a secret value, pass it as an environment variable, never as a command argument. Command arguments appear in error messages, ps output, and logs -- even when the command's intent is safe. This was learned from an incident where wp db query echoed a secret in its error output. Adds section 8.2 (prevention) and 8.3 (post-execution detection) to prompts/build.txt. Updates opsec.md, gopass.md, and cloudron.md with cross-references to the new pattern. Closes #4939
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the system's secret handling policies by introducing explicit guidelines to prevent credential exposure. It addresses a critical vulnerability where secrets passed as command arguments can leak into logs, Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedPull request was closed or merged during review WalkthroughSecurity guidelines enhanced across documentation to formalize secret handling in subprocesses: secrets must pass via environment variables, not command-line arguments. Safe/unsafe patterns documented with concrete examples. Post-execution secret detection added to flag contaminated output when credentials passed as arguments fail. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Sun Mar 15 17:42:58 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![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 important security rule about not passing secrets as command-line arguments, instead favoring environment variables. The changes are well-motivated and clearly explained across several documentation files. The new rules in prompts/build.txt are comprehensive, with good examples. I've added one suggestion to make the fallback pattern for secret handling (using temporary files) even more robust by mentioning specific tools like mktemp and trap.
| - SAFE: `aidevops secret NAME -- cmd` — injects as env var with automatic output redaction | ||
| - SAFE: `SSH_AUTH_SOCK=... ssh ...` — env-based auth, no secret in argv | ||
| - The subprocess must read the value from its environment (`getenv()` in C/PHP, `process.env` in Node, `os.environ` in Python, `ENV[]` in Ruby), not from `$1`/`argv`. | ||
| - When the target program only accepts secrets as arguments (no env var support), write the secret to a temporary file (mode 0600), pass the file path as the argument, and delete the file immediately after. This is a last resort — prefer programs that support env var or stdin input. |
There was a problem hiding this comment.
This is a good fallback pattern for programs that don't support environment variables. To make the guidance more robust and secure, you could explicitly mention using mktemp for creating the temporary file and trap for ensuring cleanup. This provides more specific instructions for implementing this pattern safely.
- When the target program only accepts secrets as arguments (no env var support), write the secret to a temporary file (e.g., using `mktemp`, with mode 0600), pass the file path as the argument, and ensure the file is deleted immediately after (e.g., using a `trap` command). This is a last resort — prefer programs that support env var or stdin input.
marcusquinn
added
the
review-feedback-scanned
2 participants
Summary
prompts/build.txtsecret handling — when a subprocess needs a secret, pass it as an environment variable, never as a command argumentopsec.md,gopass.md, andcloudron.mdwith cross-references to the new patternWhy
Command arguments appear in error messages,
psoutput, and logs. An incident during a migration session showed that even "safe" commands (DB inserts) can echo secrets back when they fail. The existing rule ("don't expose secrets") was too abstract — agents assessed commands as safe based on intent, not on what error paths could reveal. This makes the safe pattern explicit with concrete SAFE/UNSAFE examples.Changes
.agents/prompts/build.txt.agents/tools/security/opsec.md.agents/tools/credentials/gopass.md.agents/services/hosting/cloudron.md-p$(cat ...)security noteCloses #4939
Summary by CodeRabbit