security: add structured security audit module with --json and --fix#48
security: add structured security audit module with --json and --fix#48benvinegar wants to merge 2 commits into
Conversation
- Create lib/security-audit.mjs with typed findings (checkId, severity, title, detail, remediation, fixable) - Checks: file permissions, symlink detection, deploy manifest integrity, secret exposure, stale .env detection, configuration validation - --json flag for CI consumption, --fix for auto-remediation - Human-readable terminal output via formatReport() - Add 31 tests covering all check functions - Complements bin/security-audit.sh (which handles root-level system checks)
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Greptile SummaryAdds a new Node.js security audit module (
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI["CLI Entry Point<br/>node lib/security-audit.mjs"]
CLI -->|parse args| FLAGS{"--json / --fix / homeDir"}
FLAGS --> RUN["runAudit({ homeDir, fix })"]
RUN --> PERMS["Permission Checks<br/>.env, .ssh, .pi, settings.json"]
RUN --> SYMLINK["Symlink Checks<br/>extensions/, skills/"]
RUN --> INTEGRITY["Manifest Integrity<br/>SHA256 hash comparison"]
RUN --> SECRETS["Secret Exposure<br/>.env perms + stale copies"]
RUN --> CONFIG["Configuration<br/>SLACK_ALLOWED_USERS"]
PERMS -->|fix=true| CHMOD["fs.chmod auto-fix"]
SECRETS -->|fix=true| CHMOD2["fs.chmod auto-fix"]
PERMS --> REPORT["Build AuditReport<br/>{ timestamp, summary, findings }"]
SYMLINK --> REPORT
INTEGRITY --> REPORT
SECRETS --> REPORT
CONFIG --> REPORT
REPORT -->|--json| JSON["JSON stdout"]
REPORT -->|default| HUMAN["formatReport() → terminal"]
REPORT --> EXIT{"Exit code<br/>0=pass, 1=warn, 2=critical"}
Last reviewed commit: 70e8f9d |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const summary = { | ||
| critical: findings.filter((f) => f.severity === "critical").length, | ||
| warn: findings.filter((f) => f.severity === "warn").length, | ||
| info: findings.filter((f) => f.severity === "info").length, | ||
| pass: permChecks.length + symlinkChecks.length - findings.length + fixedCount, | ||
| fixed: fixedCount, | ||
| }; |
There was a problem hiding this comment.
pass count can go negative
The pass formula subtracts findings.length (all findings from every check category) but only adds permChecks.length + symlinkChecks.length as the base. Findings from checkManifestIntegrity, checkSecretExposure, and checkConfiguration are included in findings.length but their check counts aren't part of the denominator. For example, if .config/.env is missing, checkConfiguration adds 2 findings (missing env + no allowed users) and checkManifestIntegrity adds 1 (manifest missing) — that's 3 findings from checks not represented in the base count, making pass potentially negative.
Consider tracking pass/fail per check category, or counting findings only from perm + symlink checks in this formula:
| const summary = { | |
| critical: findings.filter((f) => f.severity === "critical").length, | |
| warn: findings.filter((f) => f.severity === "warn").length, | |
| info: findings.filter((f) => f.severity === "info").length, | |
| pass: permChecks.length + symlinkChecks.length - findings.length + fixedCount, | |
| fixed: fixedCount, | |
| }; | |
| const summary = { | |
| critical: findings.filter((f) => f.severity === "critical").length, | |
| warn: findings.filter((f) => f.severity === "warn").length, | |
| info: findings.filter((f) => f.severity === "info").length, | |
| pass: Math.max(0, permChecks.length + symlinkChecks.length - findings.filter((f) => f.checkId.startsWith("perms-") || f.checkId.startsWith("symlink-")).length + fixedCount), | |
| fixed: fixedCount, | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/security-audit.mjs
Line: 395-401
Comment:
**`pass` count can go negative**
The `pass` formula subtracts `findings.length` (all findings from every check category) but only adds `permChecks.length + symlinkChecks.length` as the base. Findings from `checkManifestIntegrity`, `checkSecretExposure`, and `checkConfiguration` are included in `findings.length` but their check counts aren't part of the denominator. For example, if `.config/.env` is missing, `checkConfiguration` adds 2 findings (missing env + no allowed users) and `checkManifestIntegrity` adds 1 (manifest missing) — that's 3 findings from checks not represented in the base count, making `pass` potentially negative.
Consider tracking pass/fail per check category, or counting findings only from perm + symlink checks in this formula:
```suggestion
const summary = {
critical: findings.filter((f) => f.severity === "critical").length,
warn: findings.filter((f) => f.severity === "warn").length,
info: findings.filter((f) => f.severity === "info").length,
pass: Math.max(0, permChecks.length + symlinkChecks.length - findings.filter((f) => f.checkId.startsWith("perms-") || f.checkId.startsWith("symlink-")).length + fixedCount),
fixed: fixedCount,
};
```
How can I resolve this? If you propose a fix, please make it concise.| const permChecks = [ | ||
| [path.join(homeDir, ".config/.env"), "600", "Secrets file"], | ||
| [path.join(homeDir, ".ssh"), "700", "SSH directory"], | ||
| [path.join(homeDir, ".pi"), "700", "Pi state directory"], | ||
| [path.join(homeDir, ".pi/agent"), "700", "Pi agent directory"], | ||
| [path.join(homeDir, ".pi/session-control"), "700", "Pi session-control directory"], | ||
| [path.join(homeDir, ".pi/agent/settings.json"), "600", "Pi settings"], | ||
| ]; |
There was a problem hiding this comment.
Duplicate .env permission check
.config/.env permissions are checked here in permChecks (line 337) and again independently inside checkSecretExposure() (line 228-238). When both detect a mismatch, two separate findings are added to the report (perms-.env and secret-env-perms) for the same underlying issue. This inflates finding counts and confuses the summary.
Either remove the .config/.env entry from permChecks (since checkSecretExposure already covers it with the same severity logic), or skip the .env perm check inside checkSecretExposure when called from runAudit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/security-audit.mjs
Line: 336-343
Comment:
**Duplicate `.env` permission check**
`.config/.env` permissions are checked here in `permChecks` (line 337) and again independently inside `checkSecretExposure()` (line 228-238). When both detect a mismatch, two separate findings are added to the report (`perms-.env` and `secret-env-perms`) for the same underlying issue. This inflates finding counts and confuses the summary.
Either remove the `.config/.env` entry from `permChecks` (since `checkSecretExposure` already covers it with the same severity logic), or skip the `.env` perm check inside `checkSecretExposure` when called from `runAudit`.
How can I resolve this? If you propose a fix, please make it concise.| const isMain = process.argv[1] && ( | ||
| process.argv[1].endsWith("/security-audit.mjs") || | ||
| process.argv[1].endsWith("\\security-audit.mjs") | ||
| ); |
There was a problem hiding this comment.
isMain check matches unrelated filenames
endsWith("security-audit.mjs") will match any file whose name ends with those characters, e.g. my-security-audit.mjs or run-security-audit.mjs. If someone creates a wrapper script with a similar name and imports this module, the CLI block would execute unexpectedly.
A more precise approach:
| const isMain = process.argv[1] && ( | |
| process.argv[1].endsWith("/security-audit.mjs") || | |
| process.argv[1].endsWith("\\security-audit.mjs") | |
| ); | |
| const isMain = process.argv[1] && | |
| path.basename(process.argv[1]) === "security-audit.mjs"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/security-audit.mjs
Line: 453-456
Comment:
**`isMain` check matches unrelated filenames**
`endsWith("security-audit.mjs")` will match any file whose name ends with those characters, e.g. `my-security-audit.mjs` or `run-security-audit.mjs`. If someone creates a wrapper script with a similar name and imports this module, the CLI block would execute unexpectedly.
A more precise approach:
```suggestion
const isMain = process.argv[1] &&
path.basename(process.argv[1]) === "security-audit.mjs";
```
How can I resolve this? If you propose a fix, please make it concise.- Fix pass count going negative by only counting perm/symlink findings against the perm/symlink base total - Remove duplicate .env permission check (already covered by checkSecretExposure) - Use path.basename() for isMain check to avoid matching wrapper scripts
|
Closing — the bash audit ( |
| critical: findings.filter((f) => f.severity === "critical").length, | ||
| warn: findings.filter((f) => f.severity === "warn").length, | ||
| info: findings.filter((f) => f.severity === "info").length, | ||
| pass: Math.max(0, permChecks.length + symlinkChecks.length - permSymlinkFindings + fixedCount), |
There was a problem hiding this comment.
Bug: The security audit's pass count is incorrectly inflated when using the --fix flag because fixedCount is added to the total after fixed items are already excluded from failures.
Severity: MEDIUM
Suggested Fix
Remove the + fixedCount term from the pass count calculation in lib/security-audit.mjs. The correct formula should be Math.max(0, permChecks.length + symlinkChecks.length - permSymlinkFindings) to accurately reflect the number of checks that passed.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: lib/security-audit.mjs#L401
Potential issue: The security audit's summary calculation for passed checks is incorrect
when using the `--fix` flag. When a fixable issue is found, `fixedCount` is incremented,
but the issue is not added to the `findings` array. The final `pass` count is calculated
as `permChecks.length + symlinkChecks.length - permSymlinkFindings + fixedCount`. This
incorrectly adds the `fixedCount` back, leading to a pass count greater than the total
number of checks. For example, with 7 total checks and 2 auto-fixed issues, the report
would incorrectly show 9 passed checks.
| fixable: false, | ||
| }); | ||
| } else { | ||
| const users = allowedMatch[1].split(",").filter(Boolean); |
There was a problem hiding this comment.
Bug: The security audit validation for SLACK_ALLOWED_USERS is missing a .trim() call, causing it to incorrectly accept whitespace-only user IDs that the bridge itself would reject.
Severity: MEDIUM
Suggested Fix
In lib/security-audit.mjs, add a .map(s => s.trim()) call after split(',') and before filter(Boolean) when parsing the SLACK_ALLOWED_USERS variable. This will align the audit's validation logic with the bridge's implementation.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: lib/security-audit.mjs#L306
Potential issue: The validation for `SLACK_ALLOWED_USERS` in the security audit is
inconsistent with the validation used by the Slack bridge itself. The audit code at
`lib/security-audit.mjs:306` splits the user list by commas but fails to trim whitespace
from each entry before validation. This causes whitespace-only entries (e.g., `" "`) to
be considered valid by the audit. However, the bridge's parsing function correctly uses
`.trim()`, and would reject this configuration, preventing startup. This discrepancy can
give a false positive, where the audit passes a configuration that is actually invalid.
1 participant
Adds a Node.js security audit module that provides structured, typed findings — complementing the existing bash audit for system-level checks.
What changed
lib/security-audit.mjs— structured audit module with:--jsonoutput for CI consumption--fixmode for auto-remediation (chmod fixes)formatReport()for human-readable terminal outputlib/security-audit.test.mjs— 31 testsbin/test.sh— includes new test fileDesign
Each check function is independently testable and returns typed findings:
The bash audit (
bin/security-audit.sh) remains for checks requiring root access (firewall, /proc hidepid, systemd services, docker group). This module handles everything else in a portable, testable way.Usage