-
Notifications
You must be signed in to change notification settings - Fork 90
docs(#6499): add credential-redaction invariant to Go contributing guide #6500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -233,6 +233,73 @@ This matches the pattern in `internal/cli/tracker_client.go` and `internal/cli/f | |
|
|
||
| When multiple code paths produce errors for the same condition across different forges or providers, ensure they mention the same remediation options. For example, if one "no token found" error suggests both the environment variable and the `--token` flag, other forge-specific token errors should do the same — so users see consistent guidance regardless of which code path triggers. | ||
|
|
||
| ## Security: credential redaction for external content | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] pattern-inconsistency The new section heading '## Security: credential redaction for external content' uses a colon-separated Category: subtitle pattern that no other ## heading in this file uses. Existing headings are plain noun phrases or gerund phrases. Suggested fix: Rephrase to match the existing noun-phrase style, e.g., '## Credential redaction for external content'. |
||
|
|
||
| Any runner feature that processes external content — validation script | ||
| output, CI logs, script stdout/stderr — for injection into LLM prompts, | ||
| logging, or file storage **must** redact credentials before that content | ||
| leaves the runner boundary. The validation loop's `redactFeedback` | ||
| function in `internal/cli/run.go` is the canonical implementation. | ||
|
|
||
| ### Invariants | ||
|
|
||
| 1. **Scan `RunnerEnv` for credential literal values.** Iterate the | ||
| runner environment map and replace every value whose key is | ||
| classified as sensitive by `sensitiveEnvKey` (explicit names like | ||
| `PUSH_TOKEN`, `GH_TOKEN`, plus suffix matches on `_TOKEN`, | ||
| `_SECRET`, `_PASSWORD`, `_KEY`, `_CREDENTIALS`) with | ||
| `[REDACTED:<key>]`. Skip values shorter than | ||
| `minRedactableSecretLen` (currently 8) — short values like `"main"` | ||
| or `"true"` cause false-positive mangling. | ||
|
|
||
| 2. **Apply `security.SecretRedactor` as a second-pass fallback.** | ||
| The `RunnerEnv` scan only catches credentials the harness declared. | ||
| A `security.NewSecretRedactor().Scan(content)` call catches | ||
| credentials with recognizable shapes (high-entropy tokens, PEM | ||
| blocks, connection strings) that never passed through the runner | ||
| environment — for example, a key baked into a test fixture or a | ||
| pre-commit hook printing its own secrets. | ||
|
|
||
| 3. **Use `truncateUTF8` when enforcing size limits on external | ||
| content.** Naive byte slicing (`s[:max]`) can split a multi-byte | ||
| UTF-8 rune, producing invalid text that breaks downstream JSON | ||
| serialization or LLM tokenization. Use `truncateUTF8(s, max)` | ||
| (defined in `internal/cli/run.go`), which backs up to the last | ||
| valid rune boundary before appending a `[truncated]` marker. | ||
|
|
||
| 4. **Write files containing potential secrets with mode `0600`.** | ||
| Feedback files, redacted logs, and any file derived from external | ||
| content must use `os.WriteFile(path, data, 0o600)` — not `0644`. | ||
| The run directory is uploaded as a CI artifact; restrictive | ||
| permissions limit exposure if the artifact is downloaded to a | ||
| shared filesystem. | ||
|
|
||
| ### Why both passes are needed | ||
|
|
||
| Neither pass alone is sufficient. Opaque tokens (e.g., a GitHub | ||
| installation token with no recognizable prefix) have no pattern for the | ||
| `SecretRedactor` to match — only the literal `RunnerEnv` scan catches | ||
| those. Conversely, credentials that never entered the runner environment | ||
| (a PEM key printed by a repo hook, a fixture secret) are invisible to | ||
| the env scan — only the pattern-based `SecretRedactor` catches those. | ||
|
|
||
| ### When this applies | ||
|
|
||
| Apply these invariants whenever external content crosses a trust | ||
| boundary in the runner: | ||
|
|
||
| - Validation script output injected into the next iteration's LLM | ||
| prompt (`feedback_mode`) | ||
| - Pre-commit or post-script output routed back to the agent | ||
| - CI log fragments stored in the run directory | ||
| - Any new feature that captures subprocess output for prompt injection, | ||
| storage, or logging | ||
|
|
||
| See also [#2107](https://github.com/fullsend-ai/fullsend/issues/2107) | ||
| (replicate existing security patterns) and | ||
| [#2872](https://github.com/fullsend-ai/fullsend/issues/2872) | ||
| (post-script security invariants) for related guidance in other layers. | ||
|
|
||
| ## Running the fullsend CLI | ||
|
|
||
| **Audience:** contributors and agents working from a **repo checkout**. Do not | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[medium] protected-path
This PR modifies AGENTS.md, which is a protected governance file. The PR links to issue #6499 which explicitly authorizes the table entry update, and the PR body explains the rationale. Human approval is always required for protected-path changes, regardless of context.