Pre-monetization security hardening and quality fixes#57
Conversation
Implements configurable failure mode for RecourseOS evaluations: - 'closed': Block action when evidence unavailable (safest) - 'review': Escalate to human review (default for OSS) - 'open': Allow despite missing evidence (dangerous) Changes: - Add FailureMode type and utilities in src/core/failure-mode.ts - Add failureMode option to LocalPolicy - Apply failure mode logic in terraform evaluator - Add retry with exponential backoff to AWS client (3 retries, jitter) - Add comprehensive tests for failure mode behavior Pro/Enterprise deployments should use 'closed' mode by default. OSS/self-hosted uses 'review' mode to escalate to human. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Changed useClassifier default from false to true. Unknown resource types now automatically run through the decision tree classifier instead of just using the weak default handler. This provides better safety coverage for resources without explicit handlers. The classifier can be explicitly disabled via options.classifier = false if needed. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
## Security Fixes (Critical) - Fix URL command injection in http/server.ts (exec → execFile) - Add MAX_BODY_SIZE limits to HTTP endpoints (DoS prevention) - Add MAX_FRAME_SIZE validation to MCP server (DoS prevention) ## Security Fixes (High) - Add RECOURSE_INSTANCE_URL env var with production warning for localhost - Log attestation persistence failures instead of silent swallowing ## Code Quality Fixes (Medium) - Add parseRiskLevels() validator to replace unsafe 'as any' casts - Add input validation to shell command execution - Add error logging for file read failures - Document shell:true usage with security rationale ## Features - Add SLA_TARGETS and EvaluationTimer for latency benchmarking - Add timing metrics to all evaluator outputs - Add test:coverage script with 50% threshold gates - Add coverage step to CI workflow ## Cleanup - Convert incomplete TODOs to documented future enhancements - Remove references to unimplemented PagerDuty/Opsgenie integrations - Update notification docs to reflect actual supported channels Fixes: #52, #53, #54, #55, #56 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![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: ec49ba6b8f
ℹ️ 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".
| console.warn(`[WARN] Invalid risk levels ignored: ${invalid.join(', ')}. Valid: ${VALID_RISK_LEVELS.join(', ')}`); | ||
| } | ||
|
|
||
| return valid.length > 0 ? valid : ['allow', 'warn']; // Default if all invalid |
There was a problem hiding this comment.
parseRiskLevels() currently returns ['allow', 'warn'] when all provided values are invalid (including an empty RECOURSE_ALLOWED_LEVELS), which silently broadens policy instead of rejecting misconfiguration; in both gateway/session-broker paths this can permit operations or credential issuance that operators expected to block. This should fail closed (or throw) when no valid levels are parsed.
Useful? React with 👍 / 👎.
| if (contentLength > MAX_FRAME_SIZE) { | ||
| throw new Error(`MCP frame size ${contentLength} exceeds maximum allowed size of ${MAX_FRAME_SIZE} bytes`); | ||
| } |
There was a problem hiding this comment.
The new max-frame guard throws from readFrame() on large Content-Length, but runMcpServer calls readFrame() inside the input.on('data') loop without a surrounding try/catch, so a single oversized frame can terminate the process via uncaught exception (remote DoS). This path should reject/close the connection gracefully instead of throwing out of the stream handler.
Useful? React with 👍 / 👎.
1 participant
Summary
This PR addresses critical security vulnerabilities and code quality issues identified during a pre-monetization audit. All changes are backward-compatible and include comprehensive tests.
Security Fixes (Critical)
exec()toexecFile()in http/server.ts to prevent shell injection via malicious URLsMAX_BODY_SIZE(10MB) limits to all HTTP endpointsMAX_FRAME_SIZE(10MB) validation to MCP server frame parsingSecurity Fixes (High)
RECOURSE_INSTANCE_URLenv var support with warnings when using localhost in productionCode Quality Fixes (Medium)
parseRiskLevels()validator to replace unsafeas anycasts (5 locations)shell:trueusage with security rationaleFeatures
SLA_TARGETSandEvaluationTimerfor tracking evaluation performancetest:coveragescript with 50% threshold gatesCleanup
Test plan
recourse serveto verify URL opening still worksRelated Issues
Fixes #52, #53, #55, #56
🤖 Generated with Claude Code