Skip to content

Fix remaining security findings from red team review#7

Merged
shiftcontrol-dan merged 1 commit intomainfrom
final-security-fixes
Mar 15, 2026
Merged

Fix remaining security findings from red team review#7
shiftcontrol-dan merged 1 commit intomainfrom
final-security-fixes

Conversation

@shiftcontrol-dan
Copy link
Contributor

Summary

Second red team review found 4 actionable issues:

# Finding Severity Fix
1 SET/GET case mismatch — SET preserved case, GET uppercased lookup HIGH Uppercase name in handle_set
2 SIGHUP drops secrets on partial failure — transient 1P outage wipes cache HIGH Merge successes, keep stale values for failures
3 File watcher no size limit — read_to_string with no cap LOW Reject files >1MB
4 Stale "Claude Code" doc comment LOW Updated to generic

Deferred to v0.2 (require protocol redesign):

  • Newline in secret values truncates GET response (needs base64 wire encoding)
  • SET value visible in /proc/PID/cmdline (needs stdin piping to op CLI)

Test plan

  • cargo test passes (6 tests)
  • SET myToken op://... value then GET MYTOKEN succeeds (case match)
  • SIGHUP with partial 1P failure keeps stale secrets for failed refs

🤖 Generated with Claude Code

@amazon-inspector-singapore
Copy link

⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done

@amazon-inspector-singapore
Copy link

✅ I finished the code review, and didn't find any security or code quality issues.

- Fix SET/GET case mismatch: SET now uppercases name before storing,
  matching GET's uppercase lookup. Previously SET preserved case,
  making SET-inserted secrets unretrievable via GET.

- Fix SIGHUP partial failure: if some secrets fail to re-resolve
  (e.g., 1Password rate limit), merge successes into existing store
  instead of replacing entirely. Stale values kept for failures,
  preventing a transient outage from wiping working secrets.

- Add file watcher size limit: reject files >1MB before reading,
  preventing OOM from unexpectedly large files.

- Fix watcher doc comment: remove "Claude Code" reference.

Known issues deferred to v0.2 (require protocol change):
- Newline in secret values truncates GET response (needs base64 framing)
- SET value visible in /proc/PID/cmdline (needs stdin piping to op CLI)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Dan Gericke <dan@shiftcontrol.io>
@shiftcontrol-dan shiftcontrol-dan merged commit 5278d23 into main Mar 15, 2026
4 checks passed
@shiftcontrol-dan shiftcontrol-dan deleted the final-security-fixes branch March 15, 2026 11:45
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