implement timing-based paste detection for multi-line input #6
Conversation
- Add timing-based detection to distinguish manual Enter from pasted newlines - Manual Enter (≥15ms gap) terminates input, pasted Enter (<15ms) preserved - Update PromptUser and PromptSecret to use new paste detection - Maintain character masking for secrets with asterisks - Support backspace, Ctrl+C, and fallback for terminal issues - Preserve backward compatibility for single-line input - Add documentation and test guidance Co-Authored-By: sketch <[email protected]> Change-ID: sc882e6b2736d3af2k
|
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
test_paste_detection.sh (1)
1-25: Add basic safety flags and TTY hint for manual run.This script is intended for humans, but a couple of guardrails make it nicer and more predictable when devs run it from different environments (shells, CI, IDE terminals).
Apply this diff:
#!/bin/bash + +set -euo pipefail + # Test script to verify paste detection functionality @@ -echo "Manual test required - this functionality needs interactive testing." +echo "Manual test required — this functionality needs interactive testing." +echo "(Tip: ensure you're in a real TTY. If running inside a non-interactive env, try: script -q /dev/null ./secret_share)"tui/interface.go (5)
97-109: Backspace handling copies the entire buffer; consider a pop-capable buffer.strings.Builder has no Truncate, so you re-create it per backspace. For long inputs, this is O(n) per backspace. Since we only accept ASCII/tab here, a byte slice is simpler and efficient.
Sketch (not full diff): keep a []byte acc, and on backspace do acc = acc[:len(acc)-1]. On return, convert with string(acc).
Also, if you later accept UTF-8, operate on runes and delete whole runes, not bytes.
109-113: Restrict to ASCII or handle UTF-8 properly.The condition ch >= 32 || ch == '\t' allows bytes ≥128, which may be partial UTF-8 sequences. Backspace removes single bytes, potentially corrupting multi-byte characters.
Either:
- enforce ASCII: (ch >= 32 && ch <= 126) || ch == '\t', or
- decode runes with utf8.Decoder and handle per-rune edits.
Given the doc states “ASCII 32+ and tab,” enforcing ASCII is consistent.
- } else if ch >= 32 || ch == '\t' { // Printable characters and tab + } else if (ch >= 32 && ch <= 126) || ch == '\t' { // Printable ASCII and tab
119-123: Extract the 15ms magic number into a constant and consider configurability.Hard-coded thresholds make testing and tuning harder.
Add near the top of this file:
const pasteThreshold = 15 * time.MillisecondThen:
- if gap >= 15*time.Millisecond { + if gap >= pasteThreshold {Optionally read override from an env var (e.g., SECRET_SHARE_PASTE_THRESHOLD_MS) for advanced users and test harnesses.
178-193: Backspace and ASCII constraints — same considerations as non-secret path.
- Avoid O(n) backspace by keeping a []byte or []rune buffer.
- Enforce ASCII or handle UTF-8 decoding; masking makes rune-accurate deletion important if you later accept Unicode.
134-205: Reduce duplication between input and secret-input readers.The two functions are nearly identical except for echo behavior. Consider a single helper with parameters for masking and newline echoing to reduce maintenance burden and bug surface.
Sketch:
func readWithPasteDetection(mask bool) string { // shared core, with: // - echoChar: if mask -> print("*"), else print actual // - echoNewline: fmt.Println() in both cases is fine }If you’d like, I can prepare a concrete refactor diff.
PASTE_DETECTION.md (2)
7-11: Minor wording/consistency nits.
- Hyphenate “Base64-encoded.”
- Use a consistent “multi-line” spelling across the doc.
Apply this diff:
-- Base64 encoded content with line breaks +- Base64-encoded content with line breaks -- Multi-line certificates +- Multi-line certificates -- Secrets that naturally contain newlines +- Secrets that naturally contain newlines
21-25: Make the threshold explicit and configurable (doc-level).Document the constant and consider noting an override for advanced users/testing.
Proposed doc tweak:
- 15 ms is the default threshold (constant pasteThreshold).
- Can be overridden via SECRET_SHARE_PASTE_THRESHOLD_MS (if you choose to add this).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
PASTE_DETECTION.md(1 hunks)test_paste_detection.sh(1 hunks)tui/interface.go(2 hunks)
🧰 Additional context used
🪛 LanguageTool
PASTE_DETECTION.md
[grammar] ~7-~7: There might be a mistake here.
Context: ...g the content. This was problematic for: - Multi-line certificates - Base64 encoded...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...oblematic for: - Multi-line certificates - Base64 encoded content with line breaks ...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ... Base64 encoded content with line breaks - Secrets that naturally contain newlines ...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...approach is used to distinguish between: 1. Manual Enter: User manually presses En...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...m previous character) → Terminates input 2. Pasted Enter: Newline character in pas...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...ementation Details ### Timing Threshold - 15 milliseconds is used as the thresho...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...ons Modified - PromptUser(): Now uses readInputWithPasteDetection() - PromptSecret(): Now uses `readSecretWithPasteDetection...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...ecretWithPasteDetection()` ### Features - Character echoing for regular input (mas...
(QB_NEW_EN)
[grammar] ~31-~31: There might be a mistake here.
Context: ...ular input (masked with * for secrets) - Backspace support - Ctrl+C handling (tre...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...ith * for secrets) - Backspace support - Ctrl+C handling (treated as quit) - Fall...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...port - Ctrl+C handling (treated as quit) - Fallback to original behavior if termina...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...l raw mode fails ### Character Handling - Printable characters (ASCII 32+) and tab...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...ed based on timing - Backspace (127, 8) removes last character - Ctrl+C (3) terminates ...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ... last character - Ctrl+C (3) terminates with quit signal ## Testing To test the fu...
(QB_NEW_EN)
[grammar] ~46-~46: There might be a mistake here.
Context: ...nctionality: 1. Manual typing test: - Type text character by character - Pr...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ...ould terminate input 2. Paste test: - Copy multi-line text to clipboard - P...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ...tation maintains backward compatibility: - Single-line input continues to work as b...
(QB_NEW_EN)
🔇 Additional comments (7)
tui/interface.go (5)
53-54: Nice upgrade: PromptUser now uses timing-based paste detection.The switch to readInputWithPasteDetection() keeps the public API stable and meaningfully improves UX for multi-line pastes. Looks good.
86-94: Empty input requires two Enters — confirm this is intentional.When the first key pressed is Enter (firstChar==true), the newline is always treated as content, not termination. Users must press Enter again (with ≥15ms gap) to end input, which diverges from the fallback path (Scanner) that returns an empty string on a single Enter.
If you want Enter-on-empty to terminate (closer to current single-line behavior), consider:
- if ch == '\n' || ch == '\r' { - if !firstChar { + if ch == '\n' || ch == '\r' { + if firstChar { + fmt.Println() + break + } else { gap := currentTime.Sub(lastCharTime) - if gap >= 15*time.Millisecond { + if gap >= pasteThreshold { fmt.Println() break } - } + } // Otherwise, treat as part of pasted content ...If the current behavior is intentional, please document the difference vs. fallback and the rationale in PASTE_DETECTION.md.
114-117: Ctrl+C handling differs from PromptUserSingleChar. Pick one strategy.Here you “return 'q'” while PromptUserSingleChar calls os.Exit(0). Divergent behavior can surprise callers.
Recommend standardizing on returning "q" and letting higher layers handle quit (safer for cleanup and tests). If you agree, consider changing PromptUserSingleChar to match (separate PR ok).
131-132: PromptSecret now uses paste-detecting input — good consistency.Parity with PromptUser for multi-line secrets is great, and masking is preserved.
195-198: Align Ctrl+C behavior across inputs.Return "q" consistently (as here), or exit consistently, but avoid mixing.
PASTE_DETECTION.md (2)
36-41: Call out CRLF handling to avoid surprises on Windows terminals.State that CRLF sequences are treated as a single newline to prevent double line breaks in pasted content.
Add a short bullet: “On platforms emitting CRLF, CR+LF are coalesced into a single newline.”
58-62: Clarify empty-input behavior vs fallback.In raw mode, the first Enter on empty input is currently treated as content (requires a second Enter to terminate), while the fallback line-reader returns an empty string on a single Enter. This mismatch can confuse users.
Either:
- align behaviors (recommended), or
- explicitly document the difference and why it exists.
I can update the code or the doc per your preference.
| oldState, err := term.MakeRaw(int(syscall.Stdin)) | ||
| if err != nil { | ||
| // Fallback to line-based input | ||
| scanner := bufio.NewScanner(os.Stdin) | ||
| scanner.Scan() | ||
| return scanner.Text() | ||
| } | ||
| defer term.Restore(int(syscall.Stdin), oldState) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer os.Stdin.Fd() and strengthen the line-based fallback.
- Portability: term.MakeRaw/Restore are typically called with int(os.Stdin.Fd()) for cross-platform correctness. Using int(syscall.Stdin) works in many cases but is less idiomatic and can be brittle across OSes.
- Fallback: bufio.Scanner has a 64KB token limit. Very long single-line secrets would truncate silently. Also, return value should be gated on Scan() and Err().
Apply this diff:
- oldState, err := term.MakeRaw(int(syscall.Stdin))
+ oldState, err := term.MakeRaw(int(os.Stdin.Fd()))
if err != nil {
// Fallback to line-based input
- scanner := bufio.NewScanner(os.Stdin)
- scanner.Scan()
- return scanner.Text()
+ scanner := bufio.NewScanner(os.Stdin)
+ // Allow up to 1MiB lines in fallback mode
+ buf := make([]byte, 0, 1024*1024)
+ scanner.Buffer(buf, 1024*1024)
+ if scanner.Scan() {
+ return scanner.Text()
+ }
+ // Ignore EOF vs error differences here; return empty on failure.
+ return ""
}
- defer term.Restore(int(syscall.Stdin), oldState)
+ defer term.Restore(int(os.Stdin.Fd()), oldState)Optionally also gate raw-mode attempt with term.IsTerminal(int(os.Stdin.Fd())) to avoid surprising errors when stdin is not a TTY.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oldState, err := term.MakeRaw(int(syscall.Stdin)) | |
| if err != nil { | |
| // Fallback to line-based input | |
| scanner := bufio.NewScanner(os.Stdin) | |
| scanner.Scan() | |
| return scanner.Text() | |
| } | |
| defer term.Restore(int(syscall.Stdin), oldState) | |
| oldState, err := term.MakeRaw(int(os.Stdin.Fd())) | |
| if err != nil { | |
| // Fallback to line-based input | |
| scanner := bufio.NewScanner(os.Stdin) | |
| // Allow up to 1 MiB lines in fallback mode | |
| buf := make([]byte, 0, 1024*1024) | |
| scanner.Buffer(buf, 1024*1024) | |
| if scanner.Scan() { | |
| return scanner.Text() | |
| } | |
| // Ignore EOF vs error differences here; return empty on failure. | |
| return "" | |
| } | |
| defer term.Restore(int(os.Stdin.Fd()), oldState) |
🤖 Prompt for AI Agents
In tui/interface.go around lines 61 to 69, replace the int(syscall.Stdin) usage
and weak fallback: use int(os.Stdin.Fd()) for term.MakeRaw and term.Restore
(optionally guard the raw-mode attempt with
term.IsTerminal(int(os.Stdin.Fd()))), and strengthen the line-based fallback by
reading from stdin without bufio.Scanner's 64KB token limit (e.g., use a
bufio.Reader to read until newline or EOF), check and return errors from the
read (ensure you only return the line when the read succeeded), and
propagate/handle read errors instead of silently returning truncated or empty
input.
| // Handle newline/carriage return | ||
| if ch == '\n' || ch == '\r' { | ||
| if !firstChar { | ||
| // Check timing: if >=15ms since last char, treat as manual Enter | ||
| gap := currentTime.Sub(lastCharTime) | ||
| if gap >= 15*time.Millisecond { | ||
| fmt.Println() // Echo the newline | ||
| break // End of input | ||
| } | ||
| } | ||
| // Otherwise, treat as part of pasted content | ||
| result.WriteByte('\n') | ||
| fmt.Println() // Echo the newline | ||
| } else if ch == 127 || ch == 8 { // Backspace or DEL |
There was a problem hiding this comment.
CRLF handling bug: pasted/typed CRLF will produce double newlines.
The code treats both '\r' and '\n' as independent newlines. On Windows or some terminals that emit CRLF, this will append two newlines (and echo two), altering content.
Apply this diff to coalesce CRLF into a single newline:
- // Handle newline/carriage return
- if ch == '\n' || ch == '\r' {
+ // Handle newline/carriage return; coalesce CRLF to a single newline
+ if ch == '\r' {
+ // Treat CR as newline; if the next byte is LF, skip it on the next loop
+ result.WriteByte('\n')
+ fmt.Println()
+ // Set a small read deadline-like delay is not available; instead, remember CR and skip LF next
+ lastCharTime = currentTime
+ firstChar = false
+ // Mark a sentinel by using a zero gap window to treat an immediate LF as part of the same newline
+ // (implemented below by skipping LF when the last char was CR)
+ continue
+ }
+ if ch == '\n' {
+ // If the previous char was CR, skip this LF to avoid double newline
+ // We detect that by checking the immediate timing: if the last was just set by CR at the same timestamp.
+ // Note: This relies on the 'continue' above; otherwise, keep existing behavior.
+ }
+ // Handle newline
+ if ch == '\n' {
if !firstChar {
// Check timing: if >=15ms since last char, treat as manual Enter
gap := currentTime.Sub(lastCharTime)
- if gap >= 15*time.Millisecond {
+ if gap >= pasteThreshold {
fmt.Println() // Echo the newline
break // End of input
}
}
// Otherwise, treat as part of pasted content
result.WriteByte('\n')
fmt.Println() // Echo the newline
}Note: The above shows intent; if you prefer a clearer approach, track a boolean lastWasCR and skip a subsequent '\n' when true.
Additionally, extract the 15ms literal into a constant (see separate comment).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tui/interface.go around lines 84 to 97, the input loop treats '\r' and '\n'
independently causing pasted CRLF to produce double newlines and double echoes;
update the logic to coalesce CRLF by tracking if the last read char was '\r'
(e.g., a boolean lastWasCR) and when a '\n' immediately follows a '\r' skip
appending/echoing the second newline, while preserving the existing timing-based
Enter detection; also replace the 15*time.Millisecond literal with a named
constant (e.g., pasteGapThreshold) and use it in the timing comparison.
| oldState, err := term.MakeRaw(int(syscall.Stdin)) | ||
| if err != nil { | ||
| return "" | ||
| // Fallback to standard masked input | ||
| bytes, err := term.ReadPassword(int(syscall.Stdin)) | ||
| fmt.Println() | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| return string(bytes) | ||
| } | ||
| defer term.Restore(int(syscall.Stdin), oldState) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use os.Stdin.Fd() and keep the fallback robust.
Same notes as the non-secret path: prefer os.Stdin.Fd() for MakeRaw/Restore and guard the ReadPassword fallback.
Apply this diff:
- oldState, err := term.MakeRaw(int(syscall.Stdin))
+ oldState, err := term.MakeRaw(int(os.Stdin.Fd()))
if err != nil {
// Fallback to standard masked input
- bytes, err := term.ReadPassword(int(syscall.Stdin))
+ bytes, err := term.ReadPassword(int(os.Stdin.Fd()))
fmt.Println()
if err != nil {
return ""
}
return string(bytes)
}
- defer term.Restore(int(syscall.Stdin), oldState)
+ defer term.Restore(int(os.Stdin.Fd()), oldState)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oldState, err := term.MakeRaw(int(syscall.Stdin)) | |
| if err != nil { | |
| return "" | |
| // Fallback to standard masked input | |
| bytes, err := term.ReadPassword(int(syscall.Stdin)) | |
| fmt.Println() | |
| if err != nil { | |
| return "" | |
| } | |
| return string(bytes) | |
| } | |
| defer term.Restore(int(syscall.Stdin), oldState) | |
| oldState, err := term.MakeRaw(int(os.Stdin.Fd())) | |
| if err != nil { | |
| // Fallback to standard masked input | |
| bytes, err := term.ReadPassword(int(os.Stdin.Fd())) | |
| fmt.Println() | |
| if err != nil { | |
| return "" | |
| } | |
| return string(bytes) | |
| } | |
| defer term.Restore(int(os.Stdin.Fd()), oldState) |
🤖 Prompt for AI Agents
In tui/interface.go around lines 139 to 149, the code calls term.MakeRaw and
term.Restore using int(syscall.Stdin) and the ReadPassword fallback is
unguarded; change both MakeRaw and Restore to use os.Stdin.Fd() (cast to int
when the term functions require it) and update the ReadPassword fallback to use
term.ReadPassword(int(os.Stdin.Fd())) while checking and returning on any error
(and still printing a newline after ReadPassword). Ensure the defer calls
term.Restore with the same fd variable so Restore always runs with the correct
file descriptor.
| // Handle newline/carriage return | ||
| if ch == '\n' || ch == '\r' { | ||
| if !firstChar { | ||
| // Check timing: if >=15ms since last char, treat as manual Enter | ||
| gap := currentTime.Sub(lastCharTime) | ||
| if gap >= 15*time.Millisecond { | ||
| fmt.Println() // Echo the newline | ||
| break // End of input | ||
| } | ||
| } | ||
| // Otherwise, treat as part of pasted content | ||
| result.WriteByte('\n') | ||
| fmt.Println() // Echo the newline (but don't show the actual character) | ||
| } else if ch == 127 || ch == 8 { // Backspace or DEL |
There was a problem hiding this comment.
CRLF handling and first-Enter semantics mirror the non-secret path — apply the same fixes.
For secrets, CRLF currently yields double newlines, and the first Enter does not terminate input. Apply the same CRLF coalescing and (if desired) empty-input termination adjustments as suggested for readInputWithPasteDetection().
I can provide a consolidated patch for both functions if you prefer.
🤖 Prompt for AI Agents
In tui/interface.go around lines 165-178, the secret-input branch is currently
producing double newlines on CRLF and not treating a first quick Enter as
termination; update this branch to mirror the non-secret
readInputWithPasteDetection behavior by: detect CRLF pairs and coalesce them
into a single newline (do not write two newlines), measure gap since
lastCharTime and if >= 15ms treat that Enter as a manual termination (echo a
single newline and break), for quick/pasted newlines append a single '\n' to
result and echo once, and ensure backspace/DEL handling remains unchanged.
Change-ID: sc882e6b2736d3af2k
Summary by CodeRabbit
New Features
Documentation
Tests