Add security filtering to environment variables endpoint#9
Conversation
- Implement allowlist of safe environment variables (PATH, HOME, USER, etc.) - Add sensitive pattern detection to redact potential secrets - Filter out variables not in the allowlist - Allow LC_* and XDG_* prefixed variables 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @server/index.ts:
- Around line 301-304: The detection is case-sensitive: update the
sensitivePatterns handling in isSensitive to perform case-insensitive matching
(e.g., normalize candidate names with .toLowerCase() and store patterns in
lowercase or compile case-insensitive regexes) so keys like "api_key", "secret",
"password", and "auth_token" are detected; adjust the sensitivePatterns array to
lowercase tokens
("key","secret","token","password","credential","auth","private","api_key","access_key")
and change the includes() check to compare against the lowercased variable name
within the isSensitive function.
- Around line 310-315: The envVars construction reveals the names of sensitive
environment variables because it includes keys that start with "LC_" or "XDG_"
even when values are redacted; update the filter so envVars only includes truly
non-sensitive variables by removing the key-starts-with check and only allowing
keys present in safeVariables (or add an explicit isSensitive(name) check in the
.filter to exclude sensitive names), keeping the .map redaction logic
(isSensitive) unchanged; refer to envVars, safeVariables, and isSensitive to
locate and modify the filtering logic accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/index.ts
🔇 Additional comments (3)
server/index.ts (3)
291-292: Clear documentation of security intent.The comments effectively communicate the purpose and security rationale for the filtering.
294-299: Well-chosen allowlist of safe variables.The selection appropriately covers common system, shell, and development environment variables that are safe for diagnostic purposes.
317-322: LGTM!The response construction correctly serializes the filtered variables and sets appropriate headers.
| const sensitivePatterns = [ | ||
| "KEY", "SECRET", "TOKEN", "PASSWORD", "CREDENTIAL", | ||
| "AUTH", "PRIVATE", "API_KEY", "ACCESS_KEY", | ||
| ]; |
There was a problem hiding this comment.
Critical: Case-sensitive matching misses lowercase sensitive variables.
The sensitivePatterns array contains uppercase strings, but the matching in isSensitive uses case-sensitive includes(). Variables like api_key, secret, password, or auth_token would bypass detection and have their values exposed.
🔎 Proposed fix to use case-insensitive matching
- const isSensitive = (name: string): boolean => {
- return sensitivePatterns.some(pattern => name.includes(pattern));
- };
+ const isSensitive = (name: string): boolean => {
+ const upperName = name.toUpperCase();
+ return sensitivePatterns.some(pattern => upperName.includes(pattern));
+ };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @server/index.ts around lines 301-304, The detection is case-sensitive:
update the sensitivePatterns handling in isSensitive to perform case-insensitive
matching (e.g., normalize candidate names with .toLowerCase() and store patterns
in lowercase or compile case-insensitive regexes) so keys like "api_key",
"secret", "password", and "auth_token" are detected; adjust the
sensitivePatterns array to lowercase tokens
("key","secret","token","password","credential","auth","private","api_key","access_key")
and change the includes() check to compare against the lowercased variable name
within the isSensitive function.
| const envVars = Object.entries(process.env) | ||
| .filter(([key]) => safeVariables.includes(key) || key.startsWith("LC_") || key.startsWith("XDG_")) | ||
| .map(([key, value]) => ({ | ||
| name: key, | ||
| value: isSensitive(key) ? "[REDACTED]" : (value || ""), | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get the actual code context around lines 310-315
head -320 server/index.ts | tail -30Repository: coderabbit-demo/btop
Length of output: 1230
🏁 Script executed:
# Check for the isSensitive function definition
rg -n "isSensitive" --type=ts --type=jsRepository: coderabbit-demo/btop
Length of output: 224
🏁 Script executed:
# Search for tests or documentation about environment variable filtering
rg -n -C3 "envVars|REDACTED|environment.*filter|safeVariables" --type=ts --type=jsRepository: coderabbit-demo/btop
Length of output: 2813
🏁 Script executed:
# Look for security guidelines or comments about this behavior
rg -n -C5 "LC_|XDG_|sensitive.*variable|expose.*variable" --type=ts --type=jsRepository: coderabbit-demo/btop
Length of output: 1616
Filter out environment variables with sensitive names, not just redact their values.
The code comment states "Only expose safe, non-sensitive variables," but the current implementation exposes variable names that match sensitive patterns (e.g., LC_API_TOKEN or XDG_SECRET_KEY) when they start with those prefixes, even though their values are redacted. This is an information disclosure issue—revealing that a sensitive variable exists contradicts the stated intent.
Either exclude sensitive variables entirely from the response or adjust the intent comment to clarify that only values are redacted.
🤖 Prompt for AI Agents
In @server/index.ts around lines 310-315, The envVars construction reveals the
names of sensitive environment variables because it includes keys that start
with "LC_" or "XDG_" even when values are redacted; update the filter so envVars
only includes truly non-sensitive variables by removing the key-starts-with
check and only allowing keys present in safeVariables (or add an explicit
isSensitive(name) check in the .filter to exclude sensitive names), keeping the
.map redaction logic (isSensitive) unchanged; refer to envVars, safeVariables,
and isSensitive to locate and modify the filtering logic accordingly.
1 participant
Summary
This addresses the security concern of the
/api/environmentendpoint previously exposing all environment variables without filtering.Test plan
🤖 Generated with Claude Code