feat: allow piping multiline secrets

[sandydoo]

Part of #32.

This is more of an escape hatch. I haven't figured out a good way of getting the interactive case to work with multi-line secrets.

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	secretspec	aa5c502	Commit Preview URL Branch Preview URL	Aug 25 2025, 03:50 PM

[claude]

Claude finished @sandydoo's task —— View job

---

PR Review: Allow Piping Multiline Secrets

Todo List:

• Read and analyze changed file (secretspec/src/secrets.rs)
• Review code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Evaluate security concerns
• Review test coverage implications
• Provide comprehensive feedback

Changes Overview

This PR contains 2 commits that enhance stdin handling for setting secrets:

1. 158d714: Replace read_line() with read_to_string() to support multiline secrets via stdin
2. 1fee00f: Switch from manual password prompt to rpassword::prompt_password() for better UX

The core change is in secretspec/src/secrets.rs:372 where io::stdin().read_line(&mut buffer)?; becomes io::stdin().read_to_string(&mut buffer)?;

Code Review

✅ Code Quality & Best Practices

• Good: Clean, minimal change that addresses the specific issue
• Good: Proper use of rpassword crate for secure password input
• Good: Clear comments explaining when each input method is used
• Good: Consistent error handling patterns maintained

✅ Bug Fixes & Correctness

• Fixed: read_line() only reads single lines, while read_to_string() correctly handles multiline input
• Improved: rpassword::prompt_password() provides better UX than manual prompting
• Good: buffer.trim() appropriately removes trailing whitespace/newlines

✅ Performance Considerations

• Efficient: read_to_string() reads entire stdin in one operation vs multiple read_line() calls
• Memory: Reasonable approach for secret input (secrets shouldn't be extremely large)
• I/O: Minimal performance impact, appropriate for CLI tool

✅ Security Assessment

• Good: Continues using SecretString to protect sensitive data in memory
• Good: rpassword provides secure terminal input (no echo)
• Good: Stdin reading is appropriate for piped secrets
• Note: Multiline secrets in process args/environment are still properly handled

⚠️ Test Coverage Concerns

• Missing: No tests specifically for multiline stdin input
• Existing: Tests cover set() method but only with string values, not stdin scenarios
• Gap: The stdin vs terminal detection logic (io::stdin().is_terminal()) is not tested
• Suggestion: Consider adding integration tests for echo "multiline\nsecret" | secretspec set KEY

📋 Minor Suggestions

1. Documentation: The PR description mentions this is an "escape hatch" - consider adding this context to code comments
2. Consistency: The ensure_secrets() method still uses the old manual prompting approach (lines 481-494) - might be worth updating for consistency
3. Error Messages: Could provide clearer guidance when stdin is not a terminal in interactive mode

Summary

This is a well-implemented solution that correctly addresses the multiline secret input requirement. The changes are minimal, focused, and maintain existing security practices. The switch to read_to_string() is the correct approach for handling multiline input.

Recommendation: ✅ Approve

The missing test coverage is the main concern, but this doesn't block the functionality. Consider adding integration tests in a follow-up to ensure the stdin piping behavior works correctly across different platforms.

[claude]

Claude finished @sandydoo's task —— View job

---

PR Review: Allow Piping Multiline Secrets

Todo List:

• Read and analyze changed file (secretspec/src/secrets.rs)
• Review code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Evaluate security concerns
• Review test coverage implications
• Provide comprehensive feedback

Changes Overview

This PR contains 2 commits that enhance stdin handling for setting secrets:

1. 52200c9: Replace read_line() with read_to_string() to support multiline secrets via stdin
2. d94eba5: Switch from manual password prompt to rpassword::prompt_password() for better UX

The core change is in secretspec/src/secrets.rs:372 where io::stdin().read_line(&mut buffer)?; becomes io::stdin().read_to_string(&mut buffer)?;

Code Review

✅ Code Quality & Best Practices

• Good: Clean, minimal change that addresses the specific issue
• Good: Proper use of rpassword crate for secure password input
• Good: Clear comments explaining when each input method is used
• Good: Consistent error handling patterns maintained

✅ Bug Fixes & Correctness

• Fixed: read_line() only reads single lines, while read_to_string() correctly handles multiline input
• Improved: rpassword::prompt_password() provides better UX than manual prompting
• Good: buffer.trim() appropriately removes trailing whitespace/newlines

✅ Performance Considerations

• Efficient: read_to_string() reads entire stdin in one operation vs multiple read_line() calls
• Memory: Reasonable approach for secret input (secrets shouldn't be extremely large)
• I/O: Minimal performance impact, appropriate for CLI tool

✅ Security Assessment

• Good: Continues using SecretString to protect sensitive data in memory
• Good: rpassword provides secure terminal input (no echo)
• Good: Stdin reading is appropriate for piped secrets
• Note: Multiline secrets in process args/environment are still properly handled

⚠️ Test Coverage Concerns

• Missing: No tests specifically for multiline stdin input
• Existing: Tests cover set() method but only with string values, not stdin scenarios
• Gap: The stdin vs terminal detection logic (io::stdin().is_terminal()) is not tested
• Suggestion: Consider adding integration tests for echo "multiline\nsecret" | secretspec set KEY

📋 Minor Suggestions

1. Documentation: The PR description mentions this is an "escape hatch" - consider adding this context to code comments
2. Consistency: The ensure_secrets() method still uses the old manual prompting approach (lines 481-494) - might be worth updating for consistency
3. Error Messages: Could provide clearer guidance when stdin is not a terminal in interactive mode

Summary

This is a well-implemented solution that correctly addresses the multiline secret input requirement. The changes are minimal, focused, and maintain existing security practices. The switch to read_to_string() is the correct approach for handling multiline input.

Recommendation: ✅ Approve

The missing test coverage is the main concern, but this doesn't block the functionality. Consider adding integration tests in a follow-up to ensure the stdin piping behavior works correctly across different platforms.

---

[claude]

Claude finished @sandydoo's task —— View job

---

PR Review: Allow Piping Multiline Secrets

Todo List:

• Read and analyze changed file (secretspec/src/secrets.rs)
• Review code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Evaluate security concerns
• Review test coverage implications
• Provide comprehensive feedback

Changes Overview

This PR contains 2 commits that enhance stdin handling for setting secrets:

1. 52200c9: Replace read_line() with read_to_string() to support multiline secrets via stdin
2. aa5c502: Switch from manual password prompt to rpassword::prompt_password() for better UX

The core change is in secretspec/src/secrets.rs:372-375 where:

// Old approach (single line only)
io::stdin().read_line(&mut buffer)?;

// New approach (supports multiline)
io::stdin().read_to_string(&mut buffer)?;

Code Review

✅ Code Quality & Best Practices

• Good: Clean, minimal change that addresses the specific issue
• Good: Proper use of rpassword crate for secure password input
• Good: Clear comments explaining when each input method is used
• Good: Consistent error handling patterns maintained

✅ Bug Fixes & Correctness

• Fixed: read_line() only reads single lines, while read_to_string() correctly handles multiline input
• Improved: rpassword::prompt_password() provides better UX than manual prompting
• Good: buffer.trim() appropriately removes trailing whitespace/newlines

✅ Performance Considerations

• Efficient: read_to_string() reads entire stdin in one operation vs multiple read_line() calls
• Memory: Reasonable approach for secret input (secrets shouldn't be extremely large)
• I/O: Minimal performance impact, appropriate for CLI tool

✅ Security Assessment

• Good: Continues using SecretString to protect sensitive data in memory
• Good: rpassword provides secure terminal input (no echo)
• Good: Stdin reading is appropriate for piped secrets
• Note: Multiline secrets in process args/environment are still properly handled

⚠️ Test Coverage Concerns

• Missing: No tests specifically for multiline stdin input
• Existing: Tests cover set() method but only with string values, not stdin scenarios
• Gap: The stdin vs terminal detection logic (io::stdin().is_terminal()) is not tested
• Suggestion: Consider adding integration tests for echo "multiline\nsecret" | secretspec set KEY

📋 Minor Suggestions

1. Documentation: The PR description mentions this is an "escape hatch" - consider adding this context to code comments
2. Consistency: The ensure_secrets() method still uses the old manual prompting approach (lines 481-494) - might be worth updating for consistency
3. Error Messages: Could provide clearer guidance when stdin is not a terminal in interactive mode

Summary

This is a well-implemented solution that correctly addresses the multiline secret input requirement. The changes are minimal, focused, and maintain existing security practices. The switch to read_to_string() is the correct approach for handling multiline input.

Recommendation: ✅ Approve

The missing test coverage is the main concern, but this doesn't block the functionality. Consider adding integration tests in a follow-up to ensure the stdin piping behavior works correctly across different platforms.

---