Conversation
PRD: docs/design/yahoo-mail-prd.md detailed task breakdown: docs/design/yahoo-mail-tasks.md
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - **Functions:** | ||
| - `SelectFolder(ctx context.Context, folder string) (*FolderStatus, error)` — SELECT folder, return message count + UIDVALIDITY | ||
| - `SearchUIDs(ctx context.Context, criteria *SearchCriteria) ([]uint32, error)` — SEARCH command, returns UIDs | ||
| - `FetchMessages(ctx context.Context, opts FetchOptions) ([]*RawMessage, error)` — FETCH BODY.PEEK[] + INTERNALDATE + FLAGS + RFC822.SIZE for UIDs in batches |
There was a problem hiding this comment.
🚨 App password stored in plaintext in config file (high severity)
The PRD specifies storing Yahoo App Passwords in plaintext in ~/.msgvault/config.toml. This creates a second location (beyond tokens/) where credentials exist unencrypted. If the config file has incorrect permissions (not 0600), this exposes full IMAP access. The implementation should either encrypt app passwords at rest or exclusively store them in the tokens/ directory with enforced 0600 permissions, never in the config file.
Automated security review by Claude 4.5 Sonnet - Human review still required
docs/design/yahoo-mail-tasks.md
Outdated
| - **File:** `cmd/msgvault/cmd/addaccount.go` (modify) | ||
| - **Changes:** | ||
| - Add `--provider` flag (string, default `"gmail"`, choices: `"gmail"`, `"yahoo"`) | ||
| - Add `--app-password` flag (string, hidden from help output for security) |
There was a problem hiding this comment.
The --app-password CLI flag allows passing credentials on the command line, which exposes them in shell history and process listings visible to other users. The implementation should print a security warning when this flag is used and recommend using environment variables or the config file instead. Consider marking the flag as deprecated in favor of more secure methods.
Automated security review by Claude 4.5 Sonnet - Human review still required
| { | ||
| "folders": { | ||
| "INBOX": {"uidvalidity": 12345, "last_uid": 67890}, | ||
| "Sent": {"uidvalidity": 12346, "last_uid": 45678} |
There was a problem hiding this comment.
MSGVAULT_YAHOO_APP_PASSWORD as an environment variable can leak credentials if the process environment is logged during error handling or debugging. The design should explicitly document that this variable must never be logged and implement environment variable sanitization in all logging code paths to strip credentials before output.
Automated security review by Claude 4.5 Sonnet - Human review still required
Security Review: 3 High/Medium Issues FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
Remove plaintext password from config file, remove --app-password CLI flag, use interactive terminal prompt with echo suppression instead. Add security callout for credential-at-rest storage as open issue. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| - **Encrypted token file** — encrypt the token file with a user-supplied passphrase or a machine-derived key. Adds UX friction (passphrase prompt on every sync) unless a key agent is used. | ||
| - **Status quo** (0600 file permissions) — acceptable for single-user machines, insufficient for shared systems. | ||
|
|
||
| This is a pre-existing concern that also applies to Gmail OAuth refresh tokens, but adding a second credential type makes it more pressing to resolve. |
There was a problem hiding this comment.
🚨 App Password stored as plaintext in token file (high severity)
The design stores Yahoo App Passwords as plaintext JSON in ~/.msgvault/tokens/ with only 0600 file permissions. While the PRD acknowledges this as an open issue, the current design proceeds with plaintext storage. This exposes credentials to any process running as the same user, malware with user-level access, or backup systems. Consider implementing OS keyring integration (macOS Keychain, GNOME Keyring, Windows Credential Manager) before merging, or at minimum encrypt token files with a key derived from user credentials.
Automated security review by Claude 4.5 Sonnet - Human review still required
| - **Changes:** | ||
| - Add `--provider` flag (string, default `"gmail"`, choices: `"gmail"`, `"yahoo"`) | ||
| - **No `--app-password` CLI flag** — CLI flags are visible in shell history and process listings, exposing credentials to other users. This is a deliberate omission. | ||
| - When `provider == "yahoo"`: |
There was a problem hiding this comment.
MSGVAULT_YAHOO_APP_PASSWORD environment variable can leak credentials through /proc filesystem, Docker logs, CI logs, or shell command history when exported. While the design prints a warning, environment variables are inherently insecure for secrets. Consider requiring the interactive prompt for all non-automation use cases and documenting secure alternatives like reading from stdin for scripts (e.g., echo "$PASSWORD" | msgvault add-account --provider yahoo --password-stdin).
Automated security review by Claude 4.5 Sonnet - Human review still required
|
|
||
| ## Stage 5: Testing and Polish | ||
|
|
||
| **Deliverable:** Production-ready feature with integration tests, documentation, and error handling hardening. |
There was a problem hiding this comment.
Task 5.3 requires manual log sanitization to prevent App Password leakage, but relies on developer discipline rather than structural protection. The imap.Config struct should use a custom type (e.g., SecretString) that implements String() as "[REDACTED]" to prevent accidental logging via fmt.Printf, error wrapping, or panic traces. Without this, any future developer adding logging statements could inadvertently expose credentials.
Automated security review by Claude 4.5 Sonnet - Human review still required
Security Review: 3 High/Medium Issues FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Additionally:
Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
|
Feel free to not take the security review very seriously, I set this up to protect against malicious code |
|
@wesm heh. I am using it as motivation to make changes in roborev:
See a coming PR. |
roborev: Combined ReviewSummary: Security-relevant design gaps remain; address storage-at-rest and env var guidance before proceeding. Critical
High
Medium
Low
Synthesized from 4 reviews (agents: codex, gemini | types: security, review) |
2 participants
PRD: docs/design/yahoo-mail-prd.md
detailed task breakdown: docs/design/yahoo-mail-tasks.md