Skip to content

Harden cache: sanitize names, atomic writes, correct paths#1

Merged
itsdevcoffee merged 1 commit intomainfrom
security/harden-cache-integrity
Dec 16, 2025
Merged

Harden cache: sanitize names, atomic writes, correct paths#1
itsdevcoffee merged 1 commit intomainfrom
security/harden-cache-integrity

Conversation

@itsdevcoffee
Copy link
Owner

Summary

Fix three critical security/reliability issues in marketplace cache handling.

Problems Fixed

1. Path Traversal (CRITICAL)

  • Risk: Malicious marketplace names could overwrite arbitrary files
  • Example: Name "../../.ssh/authorized_keys" would overwrite SSH keys
  • Fix: validateMarketplaceName() rejects path separators, .., and non-alphanumeric chars

2. Cache Corruption (HIGH)

  • Risk: Process crash mid-write leaves corrupted JSON
  • Example: User presses Shift+U, process is killed → cache unreadable
  • Fix: Atomic writes (temp file + rename), POSIX-guaranteed atomicity

3. Cross-platform Paths (MEDIUM)

  • Risk: String concatenation breaks on Windows
  • Fix: Use filepath.Join() everywhere

Changes Made

  • internal/marketplace/cache.go:

    • Add validateMarketplaceName() with 17 test cases
    • Replace os.WriteFile with atomic pattern (temp + rename)
    • Tighten permissions: dir 0700, files 0600
    • Make PlumCacheDir testable via internal variable
  • internal/marketplace/registry.go:

    • Apply same atomic write pattern
    • Fix string path concat → filepath.Join
    • Add missing "path/filepath" import
  • internal/marketplace/cache_test.go (NEW):

    • TestValidateMarketplaceName: path traversal, special chars, unicode
    • TestSaveToCache_AtomicWrite: atomic write + perms verification
    • TestCacheDirectoryPermissions: directory perms correct

Test Results

```
$ go test -v -race ./internal/marketplace/
=== RUN TestValidateMarketplaceName
--- PASS: TestValidateMarketplaceName (17 cases)
=== RUN TestSaveToCache_AtomicWrite
--- PASS: TestSaveToCache_AtomicWrite
=== RUN TestCacheDirectoryPermissions
--- PASS: TestCacheDirectoryPermissions
PASS
ok github.com/itsdevcoffee/plum/internal/marketplace 1.018s
```

How to Validate

Automated

```bash
go test -v -race ./internal/marketplace/
go build ./cmd/plum/
```

Manual (kill test)

  1. Run plum and press Shift+U to trigger cache refresh
  2. While refreshing, kill -9 $(pgrep plum) in another terminal
  3. Restart plum → cache should auto-recover (no corruption errors)
  4. Check ls -l ~/.plum/cache/marketplaces/ → files should be 0600, directory 0700

Security validation

Try to exploit path traversal (should fail gracefully):
```go
// This would fail if a malicious registry returned:
// {"marketplaces": [{"name": "../../../etc/passwd"}]}
// Now validateMarketplaceName rejects it before filesystem access
```

Checklist

  • Tests added and passing
  • Race detector clean
  • Build succeeds
  • Backward compatible (external behavior unchanged)
  • Permissions hardened (0700/0600)
  • Cross-platform paths fixed

Follow-up Work (Future PRs)

## What
Fix three critical security/reliability issues in marketplace cache:
1. Path traversal via unsanitized marketplace names
2. Cache corruption from non-atomic writes
3. Windows path handling (string concat → filepath.Join)

## Changes
- Add validateMarketplaceName() to reject path traversal attempts
- Implement atomic cache writes (temp file + rename pattern)
- Tighten permissions: cache dir 0700, cache files 0600
- Fix cross-platform path joins in registry cache
- Add comprehensive unit tests

## Why
- **Security**: Malicious registry could write to arbitrary files
- **Reliability**: Crash mid-write corrupts cache requiring manual cleanup
- **Privacy**: Cache files should be user-only readable

## Testing
- TestValidateMarketplaceName: 17 test cases including hostile inputs
- TestSaveToCache_AtomicWrite: Verifies atomic write + correct perms
- All tests pass with -race detector

## Validation
\`\`\`bash
go test -v -race ./internal/marketplace/
# Manual: Shift+U refresh, kill -9 mid-write, restart → no corruption
\`\`\`
@itsdevcoffee itsdevcoffee merged commit 4b12835 into main Dec 16, 2025
itsdevcoffee added a commit that referenced this pull request Dec 16, 2025
Focused follow-up pass to close remaining edge cases after PRs #1 and #2:

1. Windows-safe atomic file replacement:
   - Add atomicRename helper with remove+retry fallback
   - Fixes os.Rename failures when destination exists on Windows
   - Applied to both cache.go and registry.go

2. Tightened retry policy:
   - Only retry transient failures (5xx, 429, network/timeout errors)
   - Skip retries for permanent failures (4xx except 429)
   - Add httpStatusError type for structured error handling
   - Add isRetryableError() helper with proper error type checking

3. HTTP tests made real:
   - Convert GitHubRawBase from const to package variable
   - Remove all t.Skip() scaffolding
   - Add asserting tests for:
     * Body size limit enforcement
     * Retry behavior on transient failures
     * Non-retry behavior on permanent failures (404)
     * Invalid JSON handling
     * Timeout handling (skipped in short mode)

4. CI polish:
   - Switch from hardcoded go-version to go-version-file: go.mod
   - Improves maintainability and version consistency

All tests passing with race detector enabled.
itsdevcoffee added a commit that referenced this pull request Feb 4, 2026
Implements foundational UX improvements to address user confusion
between plugin search and marketplace browser interfaces.

Key Changes:
- Unified terminology: "filter" → "view", "sort mode" → "order"
- Made @marketplace filtering discoverable with placeholder hints
- Added context breadcrumbs when returning from marketplace browser
- Enhanced help view with context-aware sections and annotations
- Fixed critical bugs: pointer stability, bounds checking, string parsing
- Simplified code with helper functions and reduced duplication

UX Improvements:
- Tab key now shows "next view" (plugin list) vs "change order" (marketplace)
- Search placeholder: "Search plugins (or @marketplace to filter)..."
- Status bar displays active marketplace filter: "@marketplace-name (N results)"
- Help view now groups shortcuts by context (plugin actions, marketplace actions, etc.)
- Breadcrumb appears briefly when returning to plugin list: "← from Marketplace Browser"

Bug Fixes:
- Fixed stale pointer reference when selecting marketplace (copy value instead of pointer to slice)
- Added final bounds check to prevent negative scroll offset
- Replaced broken string parsing with strings.SplitN for plugin@marketplace format
- Improved viewport height bounds checking for very small terminals
- Added URL validation before exec.Command calls (GitHub prefix check)

Code Quality:
- Extracted helper functions: formatPluginCount, formatGitHubStats, openURL, openPath
- Consolidated flash message clearing with unified clearFlashAfter function
- Extracted viewport initialization into dedicated functions
- Simplified status bar rendering with focused helper functions
- All tests passing, linter clean, build successful

Related: docs/research/2026-02-04-tui-ux-improvement-options.md
Tasks completed: #1-4 (Option 1 - Conservative Polish)
itsdevcoffee added a commit that referenced this pull request Feb 4, 2026
…ers (#9)

* feat: improve TUI UX with conservative polish (Option 1)

Implements foundational UX improvements to address user confusion
between plugin search and marketplace browser interfaces.

Key Changes:
- Unified terminology: "filter" → "view", "sort mode" → "order"
- Made @marketplace filtering discoverable with placeholder hints
- Added context breadcrumbs when returning from marketplace browser
- Enhanced help view with context-aware sections and annotations
- Fixed critical bugs: pointer stability, bounds checking, string parsing
- Simplified code with helper functions and reduced duplication

UX Improvements:
- Tab key now shows "next view" (plugin list) vs "change order" (marketplace)
- Search placeholder: "Search plugins (or @marketplace to filter)..."
- Status bar displays active marketplace filter: "@marketplace-name (N results)"
- Help view now groups shortcuts by context (plugin actions, marketplace actions, etc.)
- Breadcrumb appears briefly when returning to plugin list: "← from Marketplace Browser"

Bug Fixes:
- Fixed stale pointer reference when selecting marketplace (copy value instead of pointer to slice)
- Added final bounds check to prevent negative scroll offset
- Replaced broken string parsing with strings.SplitN for plugin@marketplace format
- Improved viewport height bounds checking for very small terminals
- Added URL validation before exec.Command calls (GitHub prefix check)

Code Quality:
- Extracted helper functions: formatPluginCount, formatGitHubStats, openURL, openPath
- Consolidated flash message clearing with unified clearFlashAfter function
- Extracted viewport initialization into dedicated functions
- Simplified status bar rendering with focused helper functions
- All tests passing, linter clean, build successful

Related: docs/research/2026-02-04-tui-ux-improvement-options.md
Tasks completed: #1-4 (Option 1 - Conservative Polish)

* refactor: remove breadcrumb feature based on user feedback

The breadcrumb was just a temporary text indicator that didn't provide
real navigation value. User feedback indicated expectation of a proper
navigation history stack (back/forward), which would be a larger feature.

Keeping the 3 core improvements:
- Unified terminology (next view vs change order)
- @marketplace filtering discoverability
- Contextual help view

* feat: add intelligent marketplace autocomplete with picker UI

Implements sophisticated @marketplace filtering with autocomplete UX
based on user feedback during testing.

Key Features:
- Marketplace picker appears when typing @ (replaces "no plugins found")
- Arrow keys navigate marketplace list
- Enter selects marketplace and autocompletes @marketplace-name
- Pattern: @marketplace-name [search terms] for fuzzy search within marketplace
- Example: "@claude-code-plugins test" searches for "test" in that marketplace

UX Improvements:
- Placeholder updated: "or @marketplace-name to filter" (more specific)
- Picker shows marketplace names with plugin counts
- Helpful hint: "↑↓ to navigate • Enter to select • Keep typing to filter"
- Autocomplete deactivates when user adds search terms after marketplace name

Implementation:
- Added marketplaceAutocompleteActive, marketplaceAutocompleteList, marketplaceAutocompleteCursor to Model
- UpdateMarketplaceAutocomplete() detects @ mode and filters marketplace list
- SelectMarketplaceAutocomplete() completes the marketplace name
- Enhanced filteredSearch() to parse "@marketplace-name search" pattern
- Arrow keys and Enter intercepted when autocomplete active
- renderMarketplaceAutocomplete() displays marketplace picker

Note: Syntax coloring for @marketplace deferred (requires textinput cursor customization)

Closes user feedback: confusing "no plugins found" when typing @

* fix: lazy-load marketplace items for autocomplete

Marketplace items weren't loaded at startup, causing 'No marketplaces found'
when typing @. Now lazy-loads marketplace data on first @ keypress.

* fix: autocomplete should exit on any space, not just non-empty search

When user types '@marketplace-name ' (with trailing space), should show
all plugins from that marketplace, not re-enter autocomplete mode.

* feat: add background color to @marketplace-name for visual separation

The @marketplace-name filter now displays with:
- Burnt orange background (PlumMedium)
- White text, bold
- Padding for breathing room
- Creates clear visual distinction from search terms

Example: '@claude-code-plugins test' shows the filter with background
and regular search text without, making the active filter obvious.

* feat: make filter tab counts dynamic based on search results

Filter tab counts now update in real-time as you type or change filters.

Before: All (527) | Discover (74) | Ready (444) | Installed (9)
        (static, never changed)

After:  All (15) | Discover (3) | Ready (10) | Installed (2)
        (updates as you search for 'test')

Implementation:
- Added getDynamicFilterCounts() method
- Calculates count for each filter mode based on current query
- Runs filteredSearch() for each mode to get accurate counts
- Updates in real-time as user types

UX Benefit: Users can see at a glance how many results exist in each
filter without switching tabs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant