Fix Picoclaw runtime security contracts#16
Conversation
|
Warning Review limit reached
More reviews will be available in 45 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens the Picoclaw service’s runtime install/start/stop pathways and session handling to better enforce security contracts (bounded downloads, safer archive extraction, redacted command output, and stricter session ID validation). It also introduces a dedicated security subpackage for reusable/testable security helpers.
Changes:
- Add a
service/picoclaw/securityhelper package (size-bounded streaming copy, safe tar entry filtering, session ID validation, fixed argv builder, and output redaction) plus unit tests. - Replace shell-form runtime script execution (
sh -c) with fixed-argv execution helpers and redact runtime output / sensitive install details. - Validate Picoclaw session IDs before using them for locks/session lookups/filesystem access, and avoid releasing sessions solely based on a header value.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/service/picoclaw/session_manager.go | Reject invalid session IDs at registration time. |
| server/service/picoclaw/session_history.go | Validate session IDs for session read/delete endpoints; route validation through new security helper. |
| server/service/picoclaw/security/security.go | New security utilities (bounded copy, safe tar entry checks, session ID validation, fixed argv, output redaction). |
| server/service/picoclaw/security/security_test.go | Unit tests covering new security helpers. |
| server/service/picoclaw/runtime_start_stop.go | Replace sh -c with argv-based execution and redact command output. |
| server/service/picoclaw/runtime_session.go | Validate session IDs and avoid releasing unknown sessions from header alone. |
| server/service/picoclaw/runtime_install.go | Harden runtime install cache perms, archive download perms, enforce max download size, safe archive extraction, and redact checksum mismatch details. |
| server/service/picoclaw/runtime_handlers.go | Redact runtime install response fields that may expose local paths/URLs. |
| server/service/picoclaw/runtime_constants.go | Add explicit maximum archive size constant. |
| server/service/picoclaw/gateway_proxy.go | Validate gateway session_id query parameter before lock/session usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| output, execErr := picoclawScriptCommand(ctx, scriptPath, "onboard").CombinedOutput() | ||
| trimmedOutput := picoclawSecurity.SanitizeRuntimeOutput(output) | ||
| if execErr != nil { | ||
| if trimmedOutput == "" { | ||
| trimmedOutput = execErr.Error() | ||
| trimmedOutput = "picoclaw command failed" | ||
| } |
Summary
Tests