diff --git a/.beads/.gitignore b/.beads/.gitignore index d27a1db..22f7963 100644 --- a/.beads/.gitignore +++ b/.beads/.gitignore @@ -36,6 +36,7 @@ beads.right.meta.json # These files are machine-specific and should not be shared across clones .sync.lock sync_base.jsonl +export-state/ # NOTE: Do NOT add negation patterns (e.g., !issues.jsonl) here. # They would override fork protection in .git/info/exclude, allowing diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index 13d610a..80fd6ae 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -1,9 +1,14 @@ {"id":"pm-cli-0av","title":"Batch operations - delete/move/flag by query or multiple IDs","status":"closed","priority":0,"issue_type":"task","owner":"bscott@bscott.dev","created_at":"2026-02-14T16:04:07.771517427-08:00","created_by":"Brian Scott","updated_at":"2026-02-14T17:32:54.946496593-08:00","closed_at":"2026-02-14T17:32:54.946496593-08:00","close_reason":"Implemented batch operations for mail delete, move, and flag commands. All three now accept multiple IDs and support --query flag for query-based operations."} {"id":"pm-cli-0db","title":"Address book integration - contacts list/search","status":"closed","priority":2,"issue_type":"task","owner":"bscott@bscott.dev","created_at":"2026-02-14T16:04:36.801406301-08:00","created_by":"Brian Scott","updated_at":"2026-02-14T18:45:03.511089289-08:00","closed_at":"2026-02-14T18:45:03.511089289-08:00","close_reason":"Implemented contacts management"} {"id":"pm-cli-0gn","title":"Thread/conversation support - mail thread \u003cID\u003e to show full conversation","status":"closed","priority":1,"issue_type":"task","owner":"bscott@bscott.dev","created_at":"2026-02-14T16:04:20.979185868-08:00","created_by":"Brian Scott","updated_at":"2026-02-14T18:13:37.945625664-08:00","closed_at":"2026-02-14T18:13:37.945625664-08:00","close_reason":"Added mail thread command for conversation view"} +{"id":"pm-cli-0u6","title":"Replace sh -c in mail watch --exec with argv parsing","description":"MEDIUM severity. mail watch --exec uses exec.Command(sh, -c, cmdStr) after template substitution. Latent RCE if future tokens include email content. Replace with shlex-style argv parsing; pass message ID via env var (PM_MSG_ID) instead of string substitution.","status":"closed","priority":1,"issue_type":"bug","owner":"191290+bscott@users.noreply.github.com","created_at":"2026-04-23T09:44:47.266915-07:00","created_by":"Brian Scott","updated_at":"2026-04-27T08:49:45.060819-07:00","closed_at":"2026-04-27T08:49:45.060819-07:00","close_reason":"Closed"} {"id":"pm-cli-2pg","title":"Test Unicode subjects - emoji and non-ASCII characters","status":"open","priority":2,"issue_type":"task","owner":"bscott@bscott.dev","created_at":"2026-02-14T16:04:51.65352926-08:00","created_by":"Brian Scott","updated_at":"2026-02-14T16:04:51.65352926-08:00"} +{"id":"pm-cli-2sj","title":"Sanitize ANSI escapes and control chars in terminal output of email content","description":"MEDIUM severity. Subject, From, text/HTML body, attachment filenames printed raw to stdout. Attacker emails can embed ANSI escapes for output obscuring, OSC 8 hyperlink spoofing, clipboard attacks. Add sanitizeForTerminal helper applied to text-output sinks only (preserve JSON output unchanged).","status":"closed","priority":1,"issue_type":"bug","owner":"191290+bscott@users.noreply.github.com","created_at":"2026-04-23T09:44:49.661341-07:00","created_by":"Brian Scott","updated_at":"2026-04-27T08:49:45.065384-07:00","closed_at":"2026-04-27T08:49:45.065384-07:00","close_reason":"Closed"} {"id":"pm-cli-3dm","title":"Reply workflow - mail reply and mail reply-all","status":"closed","priority":1,"issue_type":"task","assignee":"Brian Scott","owner":"bscott@bscott.dev","created_at":"2026-02-14T16:04:18.279319484-08:00","created_by":"Brian Scott","updated_at":"2026-02-14T17:04:44.285383471-08:00","closed_at":"2026-02-14T17:04:44.285383471-08:00","close_reason":"Implemented in this session"} {"id":"pm-cli-47z","title":"Richer JSON output - thread context, full MIME structure, RFC3339 timestamps","status":"closed","priority":1,"issue_type":"task","owner":"bscott@bscott.dev","created_at":"2026-02-14T16:04:39.528441667-08:00","created_by":"Brian Scott","updated_at":"2026-02-14T17:58:33.883131405-08:00","closed_at":"2026-02-14T17:58:33.883131405-08:00","close_reason":"Added date_iso (RFC3339) field to JSON output"} +{"id":"pm-cli-5jz","title":"Enforce loopback-only hosts when InsecureSkipVerify is true","description":"MEDIUM severity. IMAP and SMTP clients set InsecureSkipVerify: true without verifying host is localhost/loopback. Config-file override or --config with remote host lets attacker harvest Bridge password. Add isLoopback check before connecting in imap.Connect and smtp.Send.","status":"closed","priority":1,"issue_type":"bug","owner":"191290+bscott@users.noreply.github.com","created_at":"2026-04-23T09:44:48.467144-07:00","created_by":"Brian Scott","updated_at":"2026-04-27T08:49:45.063908-07:00","closed_at":"2026-04-27T08:49:45.063908-07:00","close_reason":"Closed"} +{"id":"pm-cli-5px","title":"Fix SMTP header injection via Message-ID in reply/forward","description":"HIGH severity. Message-ID from attacker-controlled emails is copied verbatim into In-Reply-To and References outbound headers with no CRLF stripping. Fix by adding sanitizeHeaderValue helper in smtp/client.go and applying to Subject, InReplyTo, References, From, To, CC.","status":"closed","priority":0,"issue_type":"bug","owner":"191290+bscott@users.noreply.github.com","created_at":"2026-04-23T09:44:02.263632-07:00","created_by":"Brian Scott","updated_at":"2026-04-27T08:49:45.051606-07:00","closed_at":"2026-04-27T08:49:45.051606-07:00","close_reason":"Closed"} +{"id":"pm-cli-6f8","title":"Fix SMTP Subject header injection via ASCII CRLF","description":"HIGH severity. encodeSubject only applies RFC 2047 Q-encoding if non-ASCII runes present. ASCII CRLF in Subject bypasses encoding and allows injecting Bcc/other headers. Covered by same sanitizeHeaderValue helper as the In-Reply-To fix.","status":"closed","priority":0,"issue_type":"bug","owner":"191290+bscott@users.noreply.github.com","created_at":"2026-04-23T09:44:42.950689-07:00","created_by":"Brian Scott","updated_at":"2026-04-27T08:49:45.056016-07:00","closed_at":"2026-04-27T08:49:45.056016-07:00","close_reason":"Closed"} {"id":"pm-cli-73l","title":"Search improvements - body text, boolean operators, attachment filter, size filter","status":"closed","priority":0,"issue_type":"task","owner":"bscott@bscott.dev","created_at":"2026-02-14T16:04:06.897090494-08:00","created_by":"Brian Scott","updated_at":"2026-02-14T17:33:03.844923792-08:00","closed_at":"2026-02-14T17:33:03.844923792-08:00","close_reason":"Implemented search improvements: body text search, boolean operators (AND/OR/NOT), attachment filter, and size filters"} {"id":"pm-cli-7jo","title":"Watch mode - mail watch --unread --exec for monitoring new mail","status":"closed","priority":2,"issue_type":"task","owner":"bscott@bscott.dev","created_at":"2026-02-14T16:04:34.959997264-08:00","created_by":"Brian Scott","updated_at":"2026-02-14T18:45:03.430100949-08:00","closed_at":"2026-02-14T18:45:03.430100949-08:00","close_reason":"Implemented mail watch command"} {"id":"pm-cli-864","title":"Attachment download - mail download \u003cID\u003e \u003cindex\u003e [--output PATH]","status":"closed","priority":0,"issue_type":"task","assignee":"Brian Scott","owner":"bscott@bscott.dev","created_at":"2026-02-14T16:04:05.144964427-08:00","created_by":"Brian Scott","updated_at":"2026-02-14T17:04:44.264126209-08:00","closed_at":"2026-02-14T17:04:44.264126209-08:00","close_reason":"Implemented in this session"} diff --git a/AGENTS.md b/AGENTS.md index e69a65a..c571ebc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -253,3 +253,29 @@ pm-cli mailbox list --json ``` If this returns mailboxes, the connection is working. + +## Landing the Plane (Session Completion) + +**When ending a work session**, you MUST complete ALL steps below. Work is NOT complete until `git push` succeeds. + +**MANDATORY WORKFLOW:** + +1. **File issues for remaining work** - Create issues for anything that needs follow-up +2. **Run quality gates** (if code changed) - Tests, linters, builds +3. **Update issue status** - Close finished work, update in-progress items +4. **PUSH TO REMOTE** - This is MANDATORY: + ```bash + git pull --rebase + bd sync + git push + git status # MUST show "up to date with origin" + ``` +5. **Clean up** - Clear stashes, prune remote branches +6. **Verify** - All changes committed AND pushed +7. **Hand off** - Provide context for next session + +**CRITICAL RULES:** +- Work is NOT complete until `git push` succeeds +- NEVER stop before pushing - that leaves work stranded locally +- NEVER say "ready to push when you are" - YOU must push +- If push fails, resolve and retry until it succeeds diff --git a/docs/commands.md b/docs/commands.md index 695e5f5..4843f56 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -532,12 +532,20 @@ pm-cli mail watch --json # Notify and read the new message pm-cli mail watch -e "pm-cli mail read {}" + +# Use environment variables instead of {} substitution +pm-cli mail watch -e 'echo "From: $PM_MSG_FROM | Subject: $PM_MSG_SUBJECT" | logger' ``` The watch command: - Polls the mailbox at regular intervals - Tracks message UIDs to detect new arrivals - Optionally executes a command with the message ID substituted for `{}` +- Exposes message metadata as environment variables to the executed command: + `PM_MSG_SEQ`, `PM_MSG_UID` (numeric), `PM_MSG_FROM`, `PM_MSG_SUBJECT` + (sanitized for CR/LF). Prefer these over `{}` for non-numeric data — + the exec template is passed to `sh -c`, so any string-substituted token + carrying email-derived content would be a shell-injection sink. - Handles Ctrl+C gracefully for clean shutdown - Supports JSON output for integration with scripts and AI agents diff --git a/internal/cli/cli.go b/internal/cli/cli.go index f928a84..a17b76a 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -5,7 +5,7 @@ import ( "github.com/bscott/pm-cli/internal/output" ) -var Version = "0.2.4" +var Version = "0.2.5" type Globals struct { JSON bool `help:"Output as JSON" name:"json"` diff --git a/internal/cli/mail.go b/internal/cli/mail.go index 89d0102..a2a84b3 100644 --- a/internal/cli/mail.go +++ b/internal/cli/mail.go @@ -17,6 +17,7 @@ import ( "github.com/bscott/pm-cli/internal/config" "github.com/bscott/pm-cli/internal/imap" + "github.com/bscott/pm-cli/internal/safetext" "github.com/bscott/pm-cli/internal/smtp" "github.com/emersion/go-message/mail" "gopkg.in/yaml.v3" @@ -99,12 +100,12 @@ func (c *MailListCmd) Run(ctx *Context) error { flags = "-" } - subject := msg.Subject + subject := safetext.SanitizeForTerminal(msg.Subject) if len(subject) > 50 { subject = subject[:47] + "..." } - from := msg.From + from := safetext.SanitizeForTerminal(msg.From) if len(from) > 25 { from = from[:22] + "..." } @@ -167,8 +168,8 @@ func (c *MailReadCmd) Run(ctx *Context) error { for _, att := range attachments { table.AddRow( fmt.Sprintf("%d", att.Index), - att.Filename, - att.ContentType, + safetext.SanitizeForTerminal(att.Filename), + safetext.SanitizeForTerminal(att.ContentType), formatSize(att.Size), ) } @@ -226,19 +227,23 @@ func (c *MailReadCmd) Run(ctx *Context) error { return nil } - fmt.Printf("From: %s\n", msg.From) - fmt.Printf("To: %s\n", strings.Join(msg.To, ", ")) + // Sanitize every field derived from the received email before printing. + // An attacker sending an email can embed ANSI/OSC escape sequences in + // headers and body; writing them to a TTY lets them obscure output or + // spoof terminal hyperlinks. + fmt.Printf("From: %s\n", safetext.SanitizeForTerminal(msg.From)) + fmt.Printf("To: %s\n", safetext.SanitizeForTerminal(strings.Join(msg.To, ", "))) if len(msg.CC) > 0 { - fmt.Printf("CC: %s\n", strings.Join(msg.CC, ", ")) + fmt.Printf("CC: %s\n", safetext.SanitizeForTerminal(strings.Join(msg.CC, ", "))) } fmt.Printf("Date: %s\n", msg.Date) - fmt.Printf("Subject: %s\n", msg.Subject) + fmt.Printf("Subject: %s\n", safetext.SanitizeForTerminal(msg.Subject)) if msg.MessageID != "" { - fmt.Printf("Message-ID: %s\n", msg.MessageID) + fmt.Printf("Message-ID: %s\n", safetext.SanitizeForTerminal(msg.MessageID)) } if c.Headers { - fmt.Printf("Flags: %s\n", strings.Join(msg.Flags, ", ")) + fmt.Printf("Flags: %s\n", safetext.SanitizeForTerminal(strings.Join(msg.Flags, ", "))) fmt.Printf("UID: %d\n", msg.UID) fmt.Printf("Seq: %d\n", msg.SeqNum) } @@ -254,22 +259,22 @@ func (c *MailReadCmd) Run(ctx *Context) error { if c.HTML { // Output HTML body directly if htmlBody != "" { - fmt.Println(htmlBody) + fmt.Println(safetext.SanitizeForTerminal(htmlBody)) } else if textBody != "" { // No HTML, output text - fmt.Println(textBody) + fmt.Println(safetext.SanitizeForTerminal(textBody)) } else { fmt.Println("[No body content]") } } else { // Default: output plain text if textBody != "" { - fmt.Println(textBody) + fmt.Println(safetext.SanitizeForTerminal(textBody)) } else if htmlBody != "" { // Convert HTML to plain text text := htmlToText(htmlBody) if text != "" { - fmt.Println(text) + fmt.Println(safetext.SanitizeForTerminal(text)) } else { fmt.Println("[HTML content - use --html to view]") } @@ -812,12 +817,12 @@ func (c *MailSearchCmd) Run(ctx *Context) error { flags = "-" } - subject := msg.Subject + subject := safetext.SanitizeForTerminal(msg.Subject) if len(subject) > 50 { subject = subject[:47] + "..." } - from := msg.From + from := safetext.SanitizeForTerminal(msg.From) if len(from) > 25 { from = from[:22] + "..." } @@ -1378,7 +1383,10 @@ func (c *MailDownloadCmd) Run(ctx *Context) error { }) } - fmt.Printf("Saved %s (%d bytes) to %s\n", attachment.Filename, len(attachment.Data), outPath) + fmt.Printf("Saved %s (%d bytes) to %s\n", + safetext.SanitizeForTerminal(attachment.Filename), + len(attachment.Data), + safetext.SanitizeForTerminal(outPath)) return nil } @@ -1773,8 +1781,8 @@ func (c *MailWatchCmd) Run(ctx *Context) error { }) } else { fmt.Printf("\n[NEW] %s\n", msg.Date) - fmt.Printf(" From: %s\n", msg.From) - fmt.Printf(" Subject: %s\n", msg.Subject) + fmt.Printf(" From: %s\n", safetext.SanitizeForTerminal(msg.From)) + fmt.Printf(" Subject: %s\n", safetext.SanitizeForTerminal(msg.Subject)) fmt.Printf(" ID: %d\n", msg.SeqNum) } @@ -1844,12 +1852,23 @@ func (c *MailWatchCmd) checkForNewMessages(ctx *Context, seenUIDs map[uint32]boo } func (c *MailWatchCmd) executeCommand(ctx *Context, msg imap.MessageSummary) { - // Replace {} with the message ID + // Replace {} with the (numeric, validated) sequence number. Do NOT + // add substitution tokens for email-derived string data (From, Subject, + // Message-ID, etc.) because this command is passed through `sh -c`; + // expose those fields via environment variables instead, where shell + // word-splitting still applies but the raw value is not interpolated + // into the command text. cmdStr := strings.Replace(c.Exec, "{}", fmt.Sprintf("%d", msg.SeqNum), -1) ctx.Formatter.Verbosef("Executing: %s", cmdStr) cmd := exec.Command("sh", "-c", cmdStr) + cmd.Env = append(os.Environ(), + fmt.Sprintf("PM_MSG_SEQ=%d", msg.SeqNum), + fmt.Sprintf("PM_MSG_UID=%d", msg.UID), + "PM_MSG_FROM="+safetext.SanitizeHeaderValue(msg.From), + "PM_MSG_SUBJECT="+safetext.SanitizeHeaderValue(msg.Subject), + ) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr @@ -1908,10 +1927,10 @@ func (c *MailThreadCmd) Run(ctx *Context) error { if i > 0 { fmt.Println(strings.Repeat("-", 60)) } - fmt.Printf("\nFrom: %s\n", msg.From) - fmt.Printf("To: %s\n", msg.To) + fmt.Printf("\nFrom: %s\n", safetext.SanitizeForTerminal(msg.From)) + fmt.Printf("To: %s\n", safetext.SanitizeForTerminal(msg.To)) fmt.Printf("Date: %s\n", msg.Date) - fmt.Printf("Subject: %s\n", msg.Subject) + fmt.Printf("Subject: %s\n", safetext.SanitizeForTerminal(msg.Subject)) if !msg.Seen { fmt.Print("[UNREAD] ") } @@ -1919,7 +1938,7 @@ func (c *MailThreadCmd) Run(ctx *Context) error { fmt.Println() // Show body (truncated for readability) - body := msg.Body + body := safetext.SanitizeForTerminal(msg.Body) if len(body) > 500 { body = body[:500] + "\n[... truncated ...]" } diff --git a/internal/imap/client.go b/internal/imap/client.go index 6f81cdb..a622dcf 100644 --- a/internal/imap/client.go +++ b/internal/imap/client.go @@ -10,10 +10,29 @@ import ( "time" "github.com/bscott/pm-cli/internal/config" + "github.com/bscott/pm-cli/internal/safetext" "github.com/emersion/go-imap/v2" "github.com/emersion/go-imap/v2/imapclient" ) +// isLoopbackHost reports whether host is a loopback address. Accepts the +// literal "localhost" (case-insensitive) or any IP that parses as loopback. +// Does not resolve DNS; DNS is itself untrusted and we want a hard +// guarantee that we are speaking to a local process (Proton Bridge). +func isLoopbackHost(host string) bool { + h := strings.ToLower(strings.TrimSpace(host)) + if h == "" { + return false + } + if h == "localhost" { + return true + } + if ip := net.ParseIP(h); ip != nil { + return ip.IsLoopback() + } + return false +} + type Client struct { client *imapclient.Client config *config.Config @@ -115,6 +134,14 @@ func NewClient(cfg *config.Config) (*Client, error) { } func (c *Client) Connect() error { + // TLS skip-verify below is only safe against a locally-running Proton + // Bridge. Refuse to send the Bridge password to anything that is not a + // loopback address, in case the user's config was tampered with or + // redirected via --config. + if !isLoopbackHost(c.config.Bridge.IMAPHost) { + return fmt.Errorf("refusing to connect: IMAP host %q is not a loopback address (Proton Bridge runs on localhost; InsecureSkipVerify is unsafe for remote hosts)", c.config.Bridge.IMAPHost) + } + password, err := c.config.GetPassword() if err != nil { return fmt.Errorf("failed to get password: %w", err) @@ -1329,23 +1356,34 @@ func (c *Client) DeleteDraft(ids []string) error { return c.DeleteMessages("Drafts", ids, true) } +// sanitizeAddressList strips CR/LF from each address and joins with ", ". +func sanitizeAddressList(addrs []string) string { + clean := make([]string, len(addrs)) + for i, a := range addrs { + clean[i] = safetext.SanitizeHeaderValue(a) + } + return strings.Join(clean, ", ") +} + // buildDraftMessage creates an RFC822 message from a Draft func buildDraftMessage(draft *Draft, from string) string { var sb strings.Builder - // Headers - sb.WriteString(fmt.Sprintf("From: %s\r\n", from)) + // Headers — strip CR/LF from every value to prevent header injection + // if draft data was ever sourced from untrusted input (e.g. a forward + // template pulling Subject/From from a received email). + sb.WriteString(fmt.Sprintf("From: %s\r\n", safetext.SanitizeHeaderValue(from))) if len(draft.To) > 0 { - sb.WriteString(fmt.Sprintf("To: %s\r\n", strings.Join(draft.To, ", "))) + sb.WriteString(fmt.Sprintf("To: %s\r\n", sanitizeAddressList(draft.To))) } if len(draft.CC) > 0 { - sb.WriteString(fmt.Sprintf("Cc: %s\r\n", strings.Join(draft.CC, ", "))) + sb.WriteString(fmt.Sprintf("Cc: %s\r\n", sanitizeAddressList(draft.CC))) } if draft.Subject != "" { - sb.WriteString(fmt.Sprintf("Subject: %s\r\n", draft.Subject)) + sb.WriteString(fmt.Sprintf("Subject: %s\r\n", safetext.SanitizeHeaderValue(draft.Subject))) } sb.WriteString(fmt.Sprintf("Date: %s\r\n", time.Now().Format(time.RFC1123Z))) diff --git a/internal/imap/client_test.go b/internal/imap/client_test.go index af0d502..fa06f9f 100644 --- a/internal/imap/client_test.go +++ b/internal/imap/client_test.go @@ -1,12 +1,79 @@ package imap import ( + "strings" "testing" "github.com/bscott/pm-cli/internal/config" "github.com/emersion/go-imap/v2" ) +func TestIsLoopbackHost(t *testing.T) { + tests := []struct { + host string + want bool + }{ + {"localhost", true}, + {"LOCALHOST", true}, + {"127.0.0.1", true}, + {"::1", true}, + {" 127.0.0.1 ", true}, + {"imap.example.com", false}, + {"10.0.0.1", false}, + {"", false}, + {"localhost.evil.com", false}, + } + for _, tc := range tests { + got := isLoopbackHost(tc.host) + if got != tc.want { + t.Errorf("isLoopbackHost(%q) = %v, want %v", tc.host, got, tc.want) + } + } +} + +func TestConnectRejectsNonLoopbackHost(t *testing.T) { + cfg := config.DefaultConfig() + cfg.Bridge.Email = "test@example.com" + cfg.Bridge.IMAPHost = "imap.example.com" + cfg.Bridge.IMAPPort = 993 + + client, err := NewClient(cfg) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + + err = client.Connect() + if err == nil { + t.Fatal("expected loopback rejection") + } + if !strings.Contains(err.Error(), "loopback") { + t.Fatalf("expected loopback error, got %v", err) + } +} + +func TestBuildDraftMessageStripsHeaderInjection(t *testing.T) { + draft := &Draft{ + To: []string{"victim@example.com\r\nBcc: attacker@evil.example"}, + CC: []string{"normal@example.com\r\nX-Evil: 1"}, + Subject: "Hi\r\nBcc: attacker2@evil.example", + Body: "body", + } + out := buildDraftMessage(draft, "sender@example.com\r\nX-Spoof: yes") + injectedHeaders := []string{ + "\r\nBcc:", + "\r\nX-Evil:", + "\r\nX-Spoof:", + "\nBcc:", + "\nX-Evil:", + "\nX-Spoof:", + } + for _, h := range injectedHeaders { + if strings.Contains(out, h) { + t.Errorf("injected header line %q reached draft output:\n%s", h, out) + } + } +} + func TestNewClient(t *testing.T) { cfg := config.DefaultConfig() cfg.Bridge.Email = "test@example.com" diff --git a/internal/safetext/safetext.go b/internal/safetext/safetext.go new file mode 100644 index 0000000..c75a3da --- /dev/null +++ b/internal/safetext/safetext.go @@ -0,0 +1,45 @@ +// Package safetext provides helpers for defanging attacker-controlled +// strings before they reach sensitive sinks (mail headers, terminal). +package safetext + +import "strings" + +// SanitizeHeaderValue strips CR and LF from v to prevent RFC 5322 header +// injection. Call this on any header value derived from untrusted input +// (e.g., Message-ID, Subject, or From parsed out of received emails) before +// writing it to an SMTP message or a local RFC 822 message (draft, etc.). +func SanitizeHeaderValue(v string) string { + if !strings.ContainsAny(v, "\r\n") { + return v + } + v = strings.ReplaceAll(v, "\r", "") + v = strings.ReplaceAll(v, "\n", "") + return v +} + +// SanitizeForTerminal strips C0/C1 control characters (including ANSI +// escape prefix 0x1B) and DEL, preserving tab and newline. Use on +// attacker-controlled strings (email Subject, From, Body, attachment +// filename) before printing to a TTY — unfiltered escapes can obscure +// output, spoof hyperlinks via OSC 8, or manipulate the terminal clipboard +// via OSC 52. +func SanitizeForTerminal(s string) string { + if s == "" { + return s + } + var b strings.Builder + b.Grow(len(s)) + for _, r := range s { + switch { + case r == '\t' || r == '\n': + b.WriteRune(r) + case r < 0x20 || r == 0x7F: + // drop C0 controls (incl. ESC, BEL) and DEL + case r >= 0x80 && r <= 0x9F: + // drop C1 controls + default: + b.WriteRune(r) + } + } + return b.String() +} diff --git a/internal/safetext/safetext_test.go b/internal/safetext/safetext_test.go new file mode 100644 index 0000000..6d2eb6e --- /dev/null +++ b/internal/safetext/safetext_test.go @@ -0,0 +1,57 @@ +package safetext + +import "testing" + +func TestSanitizeHeaderValue(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"plain", "Hello", "Hello"}, + {"empty", "", ""}, + {"strips CR", "a\rb", "ab"}, + {"strips LF", "a\nb", "ab"}, + {"strips CRLF", "a\r\nb", "ab"}, + {"inject Bcc", "Hello\r\nBcc: attacker@evil.example", "HelloBcc: attacker@evil.example"}, + {"multiple injections", "Subject\r\nX-Evil: 1\r\nY-Evil: 2", "SubjectX-Evil: 1Y-Evil: 2"}, + {"leaves tabs", "a\tb", "a\tb"}, + {"leaves unicode", "résumé", "résumé"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := SanitizeHeaderValue(tc.in) + if got != tc.want { + t.Errorf("SanitizeHeaderValue(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +func TestSanitizeForTerminal(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"plain", "Hello", "Hello"}, + {"empty", "", ""}, + {"preserves tab", "a\tb", "a\tb"}, + {"preserves newline", "a\nb", "a\nb"}, + {"strips ESC", "\x1b[31mred\x1b[0m", "[31mred[0m"}, + {"strips BEL", "ding\x07dong", "dingdong"}, + {"strips DEL", "abc\x7fdef", "abcdef"}, + {"strips C0 NUL", "a\x00b", "ab"}, + {"strips C1 CSI (valid UTF-8)", "a›b", "ab"}, + {"preserves unicode", "héllo 日本", "héllo 日本"}, + {"OSC 8 hyperlink", "\x1b]8;;http://evil.example\x1b\\click\x1b]8;;\x1b\\", "]8;;http://evil.example\\click]8;;\\"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := SanitizeForTerminal(tc.in) + if got != tc.want { + t.Errorf("SanitizeForTerminal(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} diff --git a/internal/smtp/client.go b/internal/smtp/client.go index d932bf6..5978219 100644 --- a/internal/smtp/client.go +++ b/internal/smtp/client.go @@ -18,8 +18,27 @@ import ( "time" "github.com/bscott/pm-cli/internal/config" + "github.com/bscott/pm-cli/internal/safetext" ) +// isLoopbackHost reports whether host is a loopback address. Accepts the +// literal "localhost" (case-insensitive) or any IP that parses as loopback. +// Does not resolve DNS, because DNS is itself untrusted and we want a hard +// guarantee that we are speaking to a local process (Proton Bridge). +func isLoopbackHost(host string) bool { + h := strings.ToLower(strings.TrimSpace(host)) + if h == "" { + return false + } + if h == "localhost" { + return true + } + if ip := net.ParseIP(h); ip != nil { + return ip.IsLoopback() + } + return false +} + type Client struct { config *config.Config password string @@ -45,6 +64,14 @@ func NewClient(cfg *config.Config, password string) *Client { } func (c *Client) Send(msg *Message) error { + // TLS skip-verify below is only safe against a locally-running Proton + // Bridge. Refuse to speak plaintext credentials to anything that is not + // a loopback address, in case the user's config was tampered with or + // redirected via --config. + if !isLoopbackHost(c.config.Bridge.SMTPHost) { + return fmt.Errorf("refusing to connect: SMTP host %q is not a loopback address (Proton Bridge runs on localhost; InsecureSkipVerify is unsafe for remote hosts)", c.config.Bridge.SMTPHost) + } + addr := net.JoinHostPort(c.config.Bridge.SMTPHost, strconv.Itoa(c.config.Bridge.SMTPPort)) // Connect to SMTP server using STARTTLS @@ -70,7 +97,11 @@ func (c *Client) Send(msg *Message) error { return fmt.Errorf("SMTP authentication failed: %w", err) } - // Set sender + // Set sender. Reject CR/LF in envelope addresses — net/smtp writes these + // raw into MAIL FROM / RCPT TO lines. + if strings.ContainsAny(msg.From, "\r\n") { + return fmt.Errorf("invalid sender address: contains CR/LF") + } if err := client.Mail(msg.From); err != nil { return fmt.Errorf("failed to set sender: %w", err) } @@ -82,6 +113,9 @@ func (c *Client) Send(msg *Message) error { allRecipients = append(allRecipients, msg.BCC...) for _, rcpt := range allRecipients { + if strings.ContainsAny(rcpt, "\r\n") { + return fmt.Errorf("invalid recipient address: contains CR/LF") + } if err := client.Rcpt(rcpt); err != nil { return fmt.Errorf("failed to add recipient %s: %w", rcpt, err) } @@ -108,19 +142,22 @@ func (c *Client) Send(msg *Message) error { func (c *Client) writeMessage(w io.Writer, msg *Message) error { hasAttachments := len(msg.Attachments) > 0 - // Headers - fmt.Fprintf(w, "From: %s\r\n", msg.From) - fmt.Fprintf(w, "To: %s\r\n", strings.Join(msg.To, ", ")) + // Headers — sanitize every value for CR/LF. Subject/InReplyTo/References + // can carry attacker-controlled data (e.g., the Message-ID of a received + // email copied into In-Reply-To on reply); without sanitization an + // attacker can inject additional headers or body content. + fmt.Fprintf(w, "From: %s\r\n", sanitizeAddressList([]string{msg.From})) + fmt.Fprintf(w, "To: %s\r\n", sanitizeAddressList(msg.To)) if len(msg.CC) > 0 { - fmt.Fprintf(w, "Cc: %s\r\n", strings.Join(msg.CC, ", ")) + fmt.Fprintf(w, "Cc: %s\r\n", sanitizeAddressList(msg.CC)) } - fmt.Fprintf(w, "Subject: %s\r\n", encodeSubject(msg.Subject)) + fmt.Fprintf(w, "Subject: %s\r\n", encodeSubject(safetext.SanitizeHeaderValue(msg.Subject))) fmt.Fprintf(w, "Date: %s\r\n", time.Now().Format(time.RFC1123Z)) if msg.InReplyTo != "" { - fmt.Fprintf(w, "In-Reply-To: %s\r\n", msg.InReplyTo) + fmt.Fprintf(w, "In-Reply-To: %s\r\n", safetext.SanitizeHeaderValue(msg.InReplyTo)) } if msg.References != "" { - fmt.Fprintf(w, "References: %s\r\n", msg.References) + fmt.Fprintf(w, "References: %s\r\n", safetext.SanitizeHeaderValue(msg.References)) } fmt.Fprintf(w, "MIME-Version: 1.0\r\n") @@ -196,6 +233,18 @@ func (c *Client) writeMessage(w io.Writer, msg *Message) error { return nil } +// sanitizeAddressList strips CR/LF from each address and joins with ", ". +// Addresses are ordinarily CLI-supplied (not attacker-controlled) but the +// same CRLF sanitizer still applies to prevent injection if one is ever +// derived from email content (e.g. a reply-to address). +func sanitizeAddressList(addrs []string) string { + clean := make([]string, len(addrs)) + for i, a := range addrs { + clean[i] = safetext.SanitizeHeaderValue(a) + } + return strings.Join(clean, ", ") +} + func encodeSubject(subject string) string { // Check if encoding is needed (non-ASCII characters) needsEncoding := false diff --git a/internal/smtp/client_test.go b/internal/smtp/client_test.go index 54cad04..a1a0d9d 100644 --- a/internal/smtp/client_test.go +++ b/internal/smtp/client_test.go @@ -83,7 +83,7 @@ func TestSendUsesTimeoutAwareDial(t *testing.T) { cfg := config.DefaultConfig() cfg.Bridge.Email = "test@example.com" - cfg.Bridge.SMTPHost = "smtp.example.com" + cfg.Bridge.SMTPHost = "127.0.0.1" cfg.Bridge.SMTPPort = 1025 client := NewClient(cfg, "testpassword") @@ -104,6 +104,53 @@ func TestSendUsesTimeoutAwareDial(t *testing.T) { } } +func TestSendRejectsNonLoopbackHost(t *testing.T) { + // Sanity check: Send must refuse to transmit credentials when the + // configured SMTP host is not a loopback address. InsecureSkipVerify is + // only safe under the Proton Bridge-on-localhost trust assumption. + cfg := config.DefaultConfig() + cfg.Bridge.Email = "test@example.com" + cfg.Bridge.SMTPHost = "smtp.example.com" + cfg.Bridge.SMTPPort = 1025 + + client := NewClient(cfg, "testpassword") + err := client.Send(&Message{ + From: "test@example.com", + To: []string{"to@example.com"}, + }) + if err == nil { + t.Fatal("expected error rejecting non-loopback host") + } + if !strings.Contains(err.Error(), "loopback") { + t.Fatalf("expected loopback rejection, got %v", err) + } +} + +func TestIsLoopbackHost(t *testing.T) { + tests := []struct { + host string + want bool + }{ + {"localhost", true}, + {"LOCALHOST", true}, + {"127.0.0.1", true}, + {"127.1.2.3", true}, + {"::1", true}, + {" 127.0.0.1 ", true}, + {"smtp.example.com", false}, + {"10.0.0.1", false}, + {"0.0.0.0", false}, + {"", false}, + {"localhost.evil.com", false}, + } + for _, tc := range tests { + got := isLoopbackHost(tc.host) + if got != tc.want { + t.Errorf("isLoopbackHost(%q) = %v, want %v", tc.host, got, tc.want) + } + } +} + func TestMessageStruct(t *testing.T) { msg := Message{ From: "sender@example.com", @@ -400,6 +447,52 @@ func TestEncodeSubjectSpecialCases(t *testing.T) { } } +// TestWriteMessageStripsHeaderInjection verifies that attacker-controlled +// CR/LF sequences in Subject, In-Reply-To, References, and recipient lists +// are stripped before being written to the SMTP DATA stream. Without this, +// a Message-ID like "\r\nBcc: attacker@evil.example" copied into +// In-Reply-To on reply would silently CC an attacker. +func TestWriteMessageStripsHeaderInjection(t *testing.T) { + cfg := config.DefaultConfig() + cfg.Bridge.Email = "test@example.com" + cfg.Bridge.SMTPHost = "127.0.0.1" + c := NewClient(cfg, "pw") + + msg := &Message{ + From: "sender@example.com", + To: []string{"victim@example.com\r\nBcc: attacker@evil.example"}, + CC: []string{"normal@example.com\r\nX-Evil: 1"}, + Subject: "Hello\r\nBcc: attacker2@evil.example", + Body: "body", + InReplyTo: "\r\nBcc: attacker3@evil.example", + References: "\r\nX-Spoof: yes", + } + + var buf bytes.Buffer + if err := c.writeMessage(&buf, msg); err != nil { + t.Fatalf("writeMessage: %v", err) + } + + out := buf.String() + // The sanitizer strips CR/LF, so the injected text remains in the value + // (now garbled into one line) — what must NOT happen is the injected + // content appearing at the start of a new header line. + injectedHeaders := []string{ + "\r\nBcc:", + "\r\nX-Evil:", + "\r\nX-Spoof:", + // Bare LF should not appear either (we strip both LF and CR). + "\nBcc:", + "\nX-Evil:", + "\nX-Spoof:", + } + for _, h := range injectedHeaders { + if strings.Contains(out, h) { + t.Errorf("injected header line %q reached output:\n%s", h, out) + } + } +} + func TestClientConfigValues(t *testing.T) { cfg := config.DefaultConfig() cfg.Bridge.SMTPHost = "smtp.example.com"