From 940122085ca4b4650a8f24790f74a807a44318f6 Mon Sep 17 00:00:00 2001 From: Snider Date: Mon, 2 Feb 2026 06:51:46 +0000 Subject: [PATCH] fix: address CodeRabbit review issues - Fix critical sandbox escape in local.Medium.path() - Absolute paths now constrained to sandbox root when root != "/" - Only allow absolute path passthrough when root is "/" - Fix weak test assertion in TestMust_Ugly_Panics - Use assert.Contains instead of weak OR condition - Remove unused issues.json file - Add TestPath_RootFilesystem test for absolute path handling Co-Authored-By: Claude Opus 4.5 --- issues.json | 1 - pkg/io/local/client.go | 8 +++++++- pkg/io/local/client_test.go | 13 ++++++++++++- pkg/log/errors_test.go | 3 +-- 4 files changed, 20 insertions(+), 5 deletions(-) delete mode 100644 issues.json diff --git a/issues.json b/issues.json deleted file mode 100644 index 7b21d3d6..00000000 --- a/issues.json +++ /dev/null @@ -1 +0,0 @@ -[{"body":"Parent: #133\n\n**Complexity: Low** - Good for parallel work\n\n## Task\nAdd `core help` command that displays help topics in the terminal.\n\n## Commands\n\n```bash\n# List all help topics\ncore help\n\n# Show specific topic\ncore help getting-started\n\n# Show specific section\ncore help getting-started#installation\n\n# Search help\ncore help --search \"workspace\"\ncore help -s \"config\"\n```\n\n## Implementation\n\n```go\n// internal/cmd/help/cmd.go\n\nvar helpCmd = &cobra.Command{\n Use: \"help [topic]\",\n Short: \"Display help documentation\",\n Run: runHelp,\n}\n\nvar searchFlag string\n\nfunc init() {\n helpCmd.Flags().StringVarP(&searchFlag, \"search\", \"s\", \"\", \"Search help topics\")\n}\n\nfunc runHelp(cmd *cobra.Command, args []string) {\n catalog := help.DefaultCatalog()\n \n if searchFlag != \"\" {\n results := catalog.Search(searchFlag)\n // Display search results\n return\n }\n \n if len(args) == 0 {\n // List all topics\n topics := catalog.List()\n for _, t := range topics {\n fmt.Printf(\" %s - %s\\n\", t.ID, t.Title)\n }\n return\n }\n \n // Show specific topic\n topic, err := catalog.Get(args[0])\n if err != nil {\n cli.Error(\"Topic not found: %s\", args[0])\n return\n }\n \n // Render markdown to terminal\n renderTopic(topic)\n}\n```\n\n## Terminal Rendering\n- Use `github.com/charmbracelet/glamour` for markdown\n- Or simple formatting with ANSI colors\n- Pager support for long content (`less` style)\n\n## Acceptance Criteria\n- [ ] `core help` lists topics\n- [ ] `core help ` shows content\n- [ ] `core help --search` finds topics\n- [ ] Markdown rendered nicely in terminal\n- [ ] Pager for long content","number":136,"title":"feat(help): Add CLI help command"},{"body":"## Overview\n\nMerge `pkg/errors` and `pkg/log` into a unified `pkg/log` package with static functions for both logging and error creation. This simplifies the codebase and provides a consistent API.\n\n## Current State\n\n**pkg/log** (2 files import):\n- `Logger` struct with level-based logging\n- Static: `Debug()`, `Info()`, `Warn()`, `Error()`\n- Structured key-value logging\n\n**pkg/errors** (11 files import):\n- `Error` struct with Op, Msg, Err, Code\n- `E()`, `Wrap()`, `WrapCode()`, `Code()`\n- Standard library wrappers: `Is()`, `As()`, `New()`, `Join()`\n\n## Target API\n\n```go\npackage log\n\n// --- Logging (existing) ---\nlog.Debug(\"message\", \"key\", value)\nlog.Info(\"message\", \"key\", value)\nlog.Warn(\"message\", \"key\", value)\nlog.Error(\"message\", \"key\", value)\n\n// --- Error Creation (merged from pkg/errors) ---\nlog.E(\"op\", \"message\", err) // Create structured error\nlog.Wrap(err, \"op\", \"message\") // Wrap with context\nlog.WrapCode(err, \"CODE\", \"op\", \"msg\") // Wrap with error code\n\n// --- Standard library (re-exported) ---\nlog.Is(err, target) // errors.Is\nlog.As(err, &target) // errors.As\nlog.New(\"message\") // errors.New\nlog.Join(errs...) // errors.Join\n\n// --- Error Helpers ---\nlog.Op(err) // Extract operation\nlog.ErrCode(err) // Extract error code\nlog.Message(err) // Extract message\nlog.Root(err) // Get root cause\n\n// --- Combined Helpers (new) ---\nlog.LogError(err, \"op\", \"msg\") // Log + return wrapped error\nlog.Must(err, \"op\", \"msg\") // Panic if error\n```\n\n## Benefits\n\n1. **Single import** - `log` handles both logging and errors\n2. **Consistent patterns** - Same package for observability\n3. **Simpler mental model** - \"If something goes wrong, use log\"\n4. **Natural pairing** - Errors often logged immediately\n\n## Child Issues\n\n### Phase 1: Extend pkg/log (blocking)\n- [ ] #128 - Add error creation functions to pkg/log\n\n### Phase 2: Migration (sequential)\n- [ ] #129 - Create pkg/errors deprecation alias\n- [ ] #130 - Migrate pkg/errors imports to pkg/log (11 files)\n- [ ] #131 - Remove deprecated pkg/errors package\n\n### Phase 3: Enhancements (optional)\n- [ ] #132 - Add combined log-and-return error helpers\n\n## Parallelization Guide\n\n**Can be done by other models (boring/mechanical):**\n- #129 - Deprecation alias (copy-paste with aliases)\n- #130 - Import migration (find/replace)\n- #131 - Cleanup (delete directory)\n\n**Requires more context:**\n- #128 - API design decisions\n- #132 - Helper design\n\n## Migration Path\n\n1. Extend `pkg/log` with error functions (#128)\n2. Create deprecation alias (#129)\n3. Migrate all imports (#130)\n4. Remove `pkg/errors` (#131)\n\n## Acceptance Criteria\n- [ ] Single `pkg/log` import for logging and errors\n- [ ] Zero imports of `pkg/errors`\n- [ ] All tests pass\n- [ ] Combined helpers available (#132)","number":127,"title":"feat(log): Unify pkg/errors and pkg/log into single logging package"},{"body":"Parent: #118\n\n**Complexity: Low** - Similar to socket but simpler\n\n## Task\nAdd TCP transport for network MCP connections.\n\n## Implementation\n\n```go\n// pkg/mcp/transport_tcp.go\n\ntype TCPTransport struct {\n addr string\n listener net.Listener\n}\n\nfunc NewTCPTransport(addr string) (*TCPTransport, error) {\n listener, err := net.Listen(\"tcp\", addr)\n if err != nil {\n return nil, err\n }\n return &TCPTransport{addr: addr, listener: listener}, nil\n}\n```\n\n## Configuration\n- Default: `127.0.0.1:9100` (localhost only)\n- Configurable via `MCP_ADDR` env var\n- Consider TLS for non-localhost\n\n## Security Considerations\n- Default to localhost binding\n- Warn if binding to 0.0.0.0\n- Future: mTLS support\n\n## Acceptance Criteria\n- [ ] TCP listener with configurable address\n- [ ] Localhost-only by default\n- [ ] Multiple concurrent connections\n- [ ] Graceful shutdown","number":126,"title":"feat(mcp): Add TCP transport"},{"body":"Parent: #101\n\n**Complexity: Medium** - 5 files with config/registry operations\n\n## Task\nMigrate `internal/cmd/setup/*` to use `io.Medium`.\n\n## Files\n- Bootstrap commands\n- Registry management\n- GitHub config files","number":116,"title":"chore(io): Migrate internal/cmd/setup to Medium abstraction"},{"body":"Parent: #101\n\n**Complexity: Medium** - 5 files with code generation\n\n## Task\nMigrate `internal/cmd/sdk/*` to use `io.Medium`.\n\n## Files\n- SDK generation commands\n- Generator implementations","number":115,"title":"chore(io): Migrate internal/cmd/sdk to Medium abstraction"},{"body":"Parent: #101\n\n**Complexity: Low** - 3 files\n\n## Task\nMigrate `internal/cmd/dev/*` to use `io.Medium`.\n\n## Files\n- Dev workflow commands\n- Registry operations","number":114,"title":"chore(io): Migrate internal/cmd/dev to Medium abstraction"},{"body":"Parent: #101\n\n**Complexity: Low** - 2 files\n\n## Task\nMigrate `internal/cmd/docs/*` to use `io.Medium`.\n\n## Files\n- Documentation scanning\n- File sync operations","number":113,"title":"chore(io): Migrate internal/cmd/docs to Medium abstraction"},{"body":"Parent: #101\n\n## Task\nReplace custom path validation in `pkg/mcp/mcp.go` with `local.Medium` sandboxing.\n\n## Current State\nThe MCP server has custom `validatePath()` and `resolvePathWithSymlinks()` functions for path security.\n\n## Target State\nUse `local.New(workspaceRoot)` to create a sandboxed Medium, then use Medium methods for all file operations.\n\n## Changes Required\n1. Add `medium local.Medium` field to `Service` struct\n2. Initialize medium in `New()` with workspace root\n3. Replace all file operation handlers to use medium methods\n4. Remove custom `validatePath()` function (Medium handles this)\n5. Update tests\n\n## Benefits\n- Consistent path security across codebase\n- Symlink handling built into Medium\n- Simpler MCP code\n\n## Blocked By\n- #102 (Medium interface extension)","number":103,"title":"feat(io): Migrate pkg/mcp to use Medium abstraction"},{"body":"Follow-up from #87 (NO_COLOR support implemented in #98).\n\n## Remaining Work\n\n### 1. WCAG Color Contrast Audit\nAudit the color combinations in `pkg/cli/styles.go` for WCAG contrast compliance:\n- Check foreground/background contrast ratios\n- Ensure sufficient contrast on both dark and light terminal backgrounds\n- Document any colors that may have accessibility issues\n\n### 2. Terminal Capability Adaptation\nConsider adapting to terminal capabilities:\n- Detect 16/256/TrueColor support\n- Fallback to simpler colors on limited terminals\n- Potentially use a library that handles this automatically\n\n## Reference\n- Current colors: `pkg/cli/styles.go` (Tailwind palette)\n- ANSI implementation: `pkg/cli/ansi.go`\n- WCAG contrast guidelines: https://www.w3.org/WAI/WCAG21/Understanding/contrast-minimum.html\n\n## Relates to\n- #87 (partial implementation)\n- #86 (Accessibility Audit parent)","number":99,"title":"CLI Output: Color contrast audit and terminal adaptation"},{"body":"Review documentation for accessibility best practices.\n\n**Issue:**\n- Automated scans for alt text on images found no direct Markdown/HTML images, but verification is needed for any generated docs.\n- Ensure heading hierarchy is logical (H1 -> H2 -> H3).\n\n**Recommendation:**\n- Add a CI check for markdown accessibility (e.g., markdownlint with a11y rules).\n- Ensure all future diagrams/images have descriptive alt text.","number":89,"title":"Documentation: Improve Accessibility"},{"body":"The Angular application in pkg/updater/ui/src needs a comprehensive accessibility audit.\n\n**Findings:**\n- Missing aria-labels on buttons.\n- Images potentially missing alt text (grep scan found none, but verification needed on dynamic content).\n- No evidence of high-contrast mode support.\n\n**Recommendation:**\n- Run automated a11y tests (e.g., axe-core).\n- Audit keyboard navigation flow.\n- Ensure all interactive elements have accessible names.","number":88,"title":"Web UI: Audit Angular App Accessibility"}] diff --git a/pkg/io/local/client.go b/pkg/io/local/client.go index f17a4da5..136af52b 100644 --- a/pkg/io/local/client.go +++ b/pkg/io/local/client.go @@ -25,14 +25,20 @@ func New(root string) (*Medium, error) { // path sanitizes and returns the full path. // Replaces .. with . to prevent traversal, then joins with root. +// Absolute paths are only allowed when root is "/", otherwise they are +// treated as relative to the sandbox root. func (m *Medium) path(p string) string { if p == "" { return m.root } clean := strings.ReplaceAll(p, "..", ".") - if filepath.IsAbs(clean) { + // Only allow absolute paths to pass through if root is "/" + // Otherwise, strip leading "/" to constrain to sandbox + if filepath.IsAbs(clean) && m.root == "/" { return filepath.Clean(clean) } + // Strip leading "/" for sandboxed mediums + clean = strings.TrimPrefix(clean, "/") return filepath.Join(m.root, clean) } diff --git a/pkg/io/local/client_test.go b/pkg/io/local/client_test.go index 3a197a49..fc474a7a 100644 --- a/pkg/io/local/client_test.go +++ b/pkg/io/local/client_test.go @@ -29,8 +29,19 @@ func TestPath(t *testing.T) { assert.Equal(t, "/home/user/file.txt", m.path("../file.txt")) assert.Equal(t, "/home/user/dir/file.txt", m.path("dir/../file.txt")) - // Absolute paths pass through + // Absolute paths are constrained to sandbox (no escape) + assert.Equal(t, "/home/user/etc/passwd", m.path("/etc/passwd")) +} + +func TestPath_RootFilesystem(t *testing.T) { + m := &Medium{root: "/"} + + // When root is "/", absolute paths pass through assert.Equal(t, "/etc/passwd", m.path("/etc/passwd")) + assert.Equal(t, "/home/user/file.txt", m.path("/home/user/file.txt")) + + // Relative paths still work + assert.Equal(t, "/file.txt", m.path("file.txt")) } func TestReadWrite(t *testing.T) { diff --git a/pkg/log/errors_test.go b/pkg/log/errors_test.go index 84390eae..06adf355 100644 --- a/pkg/log/errors_test.go +++ b/pkg/log/errors_test.go @@ -3,7 +3,6 @@ package log import ( "bytes" "errors" - "strings" "testing" "github.com/stretchr/testify/assert" @@ -270,5 +269,5 @@ func TestMust_Ugly_Panics(t *testing.T) { // Verify error was logged before panic output := buf.String() - assert.True(t, strings.Contains(output, "[ERR]") || len(output) > 0) + assert.Contains(t, output, "[ERR]", "Should log error before panic") }