# Security Audit -- discordo-plus

Audited: 2026-03-20 | Auditor: Claude (automated)

---

## CRITICAL

_No critical vulnerabilities were found._

---

## HIGH

### 1. ✅ FIXED — Path Traversal via Malicious Attachment Filenames
`internal/ui/chat/messages_list.go:1202` -- The `downloadToCache` function joins a Discord-supplied `attachment.Filename` directly into a file path using `filepath.Join(dir, attachment.Filename)`. Discord attachment filenames are server-controlled metadata; a malicious or compromised Discord server, webhook, or bot could supply a filename containing path traversal sequences (e.g., `../../../.bashrc` or absolute paths on Windows). While `filepath.Join` on Go will clean `..` sequences on most platforms, it does NOT sanitize on all edge cases (e.g., a filename that is simply `..` followed by valid components, or Windows-specific `\` separators). The same vulnerable pattern appears at line 1346 in `saveAttachmentImage` where `attachment.Filename` is joined with the user-configured `image_save_dir`. On that path, an attacker-controlled filename could overwrite arbitrary files in the save directory tree.

**Fix:** Sanitize the filename before joining. Extract only the base component and strip any path separators:
```go
safeName := filepath.Base(attachment.Filename)
if safeName == "." || safeName == ".." || safeName == string(filepath.Separator) {
    safeName = "attachment"
}
path := filepath.Join(dir, safeName)
```
Apply this to both `downloadToCache` (line 1202) and `saveAttachmentImage` (line 1346).

**Implementation Risk:** Low. `filepath.Base` is the standard Go idiom for this. The only edge case is empty filenames (which `Base` returns as `.`), handled by the fallback. Existing cached files with clean filenames will continue to work.

---

### 2. ✅ FIXED — Editor Config Value Enables Command Injection via TOML
`internal/config/editor.go` (previously `editor_unix.go`) -- The `CreateEditorCommand` function on Unix previously used `sh -c` shell invocation, enabling command injection via TOML config or `$EDITOR`. Fixed by replacing `sh -c` with `strings.Fields()` + direct `exec.Command`. The two identical build-tagged files (`editor_unix.go`, `editor_default.go`) were merged into a single `editor.go` with no build tags. A slice aliasing bug in `append(parts[1:], path)` was also fixed by using explicit `make`+`copy`+`append`.

**Implementation Risk:** Low. Users relying on shell features in editor strings (pipes, env expansion) will need to adjust. The editor field now uses direct argument splitting, not shell evaluation.

---

### 3. ✅ FIXED — Overly Permissive File Permissions (os.ModePerm / 0777)
`internal/logger/logger.go:20,24`, `internal/consts/consts.go:25`, `internal/ui/chat/messages_list.go:1198,1341`, `internal/notifications/notifications.go:78` -- All `os.MkdirAll` and `os.OpenFile` calls use `os.ModePerm` (0777), which creates directories and files that are world-readable and world-writable. The log file at `~/.cache/discordo/logs.txt` is created with 0777 permissions. The log file records HTTP request URLs (which include the Discord API base but could contain query parameters in debug mode), error messages, and operational state. The attachment cache directory and saved images are also created with world-accessible permissions.

On a multi-user Linux system, any local user can read the log file, cached attachments, saved images, and the guild state file. While the Discord token is not directly logged, `ws.EnableRawEvents = true` in debug mode could cause raw gateway events (which transit the token-authenticated WebSocket) to be processed, and error messages may leak partial request/response data.

**Fix:** Use restrictive permissions:
```go
// Directories: 0700 (owner only)
os.MkdirAll(dir, 0700)

// Regular files: 0600 (owner only)
os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600)
```
Apply consistently across all six call sites.

**Implementation Risk:** Low. No other users or processes should need access to these files. Existing files will retain their current permissions until recreated.

---

## MEDIUM

### 4. ✅ FIXED — Unbounded HTTP Response Body in Attachment Downloads
`internal/ui/chat/messages_list.go:1191-1211` -- The `downloadToCache` function fetches an attachment via `http.Get(attachment.URL)` and copies the entire response body to disk with `io.Copy(file, resp.Body)`. There is no size limit. A malicious Discord message with an attachment URL pointing to an extremely large file (or a server that streams infinite data) could fill the disk. The same issue exists in `getCachedProfileImage` in `internal/notifications/notifications.go:93-99`.

**Fix:** Use `io.LimitReader` to cap the download size:
```go
maxSize := int64(100 * 1024 * 1024) // 100 MB
if _, err := io.Copy(file, io.LimitReader(resp.Body, maxSize)); err != nil {
```
Also check `resp.ContentLength` before starting the download when available.

**Implementation Risk:** Low. Discord has its own attachment size limits (typically 25-100 MB depending on tier), so a 100 MB cap is generous. The only behavioral change is rejecting pathologically large responses.

### 5. ✅ FIXED — Attachment Downloads Do Not Validate TLS or Content-Type
`internal/ui/chat/messages_list.go:1191` and `internal/notifications/notifications.go:93` -- Both attachment and avatar downloads use `net/http.Get()` (the stdlib default client) rather than the project's custom `http.NewTransport()`. This means these HTTP calls bypass the project's transport configuration. While Go's default HTTP client does validate TLS certificates, the attachment URL is entirely server-controlled -- a malicious embed or attachment URL could point to `http://` (non-TLS) endpoints, local network addresses (SSRF to `http://169.254.169.254` for cloud metadata, `http://localhost:*` for local services), or file:// URIs.

Additionally, `downloadToCache` does not validate the HTTP response status code -- a 404 or 500 response body would be silently written to disk and then opened in the image viewer.

**Fix:** (a) Validate that the URL scheme is `https://` before fetching. (b) Check `resp.StatusCode` is 200 before writing. (c) Consider using the project's custom transport for consistency.
```go
parsed, err := url.Parse(attachment.URL)
if err != nil || parsed.Scheme != "https" {
    return "", fmt.Errorf("invalid attachment URL scheme: %s", attachment.URL)
}
// ... after http.Get:
if resp.StatusCode != http.StatusOK {
    return "", fmt.Errorf("unexpected status: %d", resp.StatusCode)
}
```

**Implementation Risk:** Low. Discord CDN URLs are always HTTPS. This only blocks non-HTTPS URLs, which are either malicious or broken.

### 6. ✅ FIXED — Image Viewer Config Allows Arbitrary Command Execution
`internal/ui/chat/messages_list.go:1247-1257,1269` -- The `viewerArgs` function takes the `image_viewer` config value and uses it as the first argument to `exec.Command`. On the non-Unix code path (and even on Unix when not using `sh -c`), the viewer string is used directly as a binary name. If a user sets `image_viewer` to a value containing spaces (e.g., `image_viewer = "my viewer"`), only the first argument would be the binary name, but the heuristic at line 1248 (`strings.Contains(viewer, "mpv ")`) suggests the field is expected to be a simple binary name. The viewer value goes directly to `exec.Command(args[0], args[1:]...)` where `args[0]` is the unsanitized viewer string. Combined with a user-writable config file and the fact that the path argument comes from a downloaded (attacker-named) file, this chain could be exploited if the cached attachment path is crafted.

However, since `exec.Command` does NOT invoke a shell and the path is a separate argument, the practical injection risk is limited to the config value itself (which the user controls).

**Fix:** Validate that `image_viewer` resolves to an executable via `exec.LookPath` at config load time. Log a warning if it does not resolve:
```go
if cfg.ImageViewer != "" {
    if _, err := exec.LookPath(cfg.ImageViewer); err != nil {
        slog.Warn("image_viewer not found in PATH", "viewer", cfg.ImageViewer, "err", err)
    }
}
```

**Implementation Risk:** Low. This is a validation-only change. Users with valid viewer paths are unaffected.

### 7. PREV-FIXED -- Debug Mode Enables Raw WebSocket Event Logging
`cmd/root.go:31` -- When `--log-level=debug` is passed, `ws.EnableRawEvents = true` was set globally on the arikawa WebSocket library. This caused raw gateway event data to be processed and potentially logged. Raw gateway events may contain user tokens in initial payloads, private message content, and other sensitive data.

**Fix:** Removed `ws.EnableRawEvents = true` entirely. The raw event handler was already commented out, so this had no functional impact.

**Implementation Risk:** None.

---

## LOW

### 8. ✅ FIXED — DISCORDO_TOKEN Environment Variable Visible to All Local Processes
`internal/ui/root/model.go:88` -- The `DISCORDO_TOKEN` environment variable is checked before the keyring at startup. On Linux, environment variables of any process are readable by the same UID via `/proc/PID/environ`. Users who set this in `.bashrc` or `.zshrc` expose their Discord token to all processes running as the same user. This is documented in TECHFILE.md (item 8) but has no in-app warning.

**Fix:** Log a warning at startup when `DISCORDO_TOKEN` is used:
```go
if token := os.Getenv(tokenEnvVarKey); token != "" {
    slog.Warn("Using DISCORDO_TOKEN from environment. Environment variables are visible to all processes running as the same user. Consider using the keyring instead.")
    cmd = tokenCommand(token)
}
```

**Implementation Risk:** None. This is a logging-only change.

### 9. ✅ FIXED — Guild State File Written Without Atomic Replacement
`internal/ui/chat/guildstate.go:42-48` -- The `save()` method writes the guild state to `state.json` using `os.WriteFile`, which truncates and writes in place. If the application crashes or is killed during the write, the file will be partially written or empty, causing data loss on next load. While guild state is non-sensitive (only guild IDs and expand/collapse booleans), the non-atomic write could also cause a brief window where the file is empty and readable by other processes.

**Fix:** Write to a temporary file in the same directory and rename atomically:
```go
tmp := stateFilePath + ".tmp"
if err := os.WriteFile(tmp, data, 0644); err != nil { ... }
if err := os.Rename(tmp, stateFilePath); err != nil { ... }
```

**Implementation Risk:** Negligible. `Rename` is atomic on POSIX systems. On Windows, it may require the target to not exist; use `os.Rename` which handles this on Go 1.26+.

### 10. ✅ FIXED — Avatar Cache Path Uses Discord-Supplied Hash Without Sanitization
`internal/notifications/notifications.go:82` -- The avatar image cache path is constructed as `filepath.Join(path, avatarHash+".png")` where `avatarHash` is a `discord.Hash` (string type) received from the Discord API. While Discord avatar hashes are typically hex strings, the type is an alias for `string` and could theoretically contain path separators if the API response is manipulated. The same `filepath.Base` sanitization from finding #1 should be applied.

**Fix:** `avatarHash = filepath.Base(string(avatarHash))` before path construction.

**Implementation Risk:** Negligible.

### 11. PREV-FIXED -- Cached Files Created with os.Create (Default 0666 Permissions)
`internal/ui/chat/attachment_handler.go`, `internal/ui/chat/guildstate.go` -- Several file creation paths used `os.Create` which applies the default umask-modified 0666 permissions, potentially leaving cached attachments and state files world-readable. Related to finding #3 but covering additional call sites added after the initial fix.

**Fix:** Replaced all `os.Create` calls with `os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600)` for consistent restrictive permissions across all file creation paths.

**Implementation Risk:** None.

---

### 12. PREV-FIXED -- Avatar Download Missing HTTPS Validation
`internal/notifications/notifications.go` -- The `getCachedProfileImage` function downloaded avatar images without validating the URL scheme, allowing potential HTTP (non-TLS) downloads of user avatars. Related to finding #5 but covering the avatar-specific code path.

**Fix:** Added scheme validation to require HTTPS before downloading avatar images, consistent with the attachment download fix in finding #5.

**Implementation Risk:** None. Discord CDN always uses HTTPS.

---

### 13. PREV-FIXED -- Emoji Favorites File Not Bounded
`internal/ui/chat/emoji_picker.go` -- The emoji favorites file (`~/.cache/discordo/emoji_favorites.json`) accepted arbitrary JSON with no size limit or entry count validation. A corrupted or maliciously crafted favorites file could contain an unbounded number of entries or excessively long strings.

**Fix:** Added a maximum of 10 favorites with string length validation. The `loadFavorites` function now validates entries before accepting them.

**Implementation Risk:** None.

---

### 14. PREV-FIXED -- Emoji Favorites Mutex Lock/Unlock Ordering
`internal/ui/chat/emoji_picker.go` -- The `emojiFavorites.save()` method acquired its own lock internally, but callers also held the lock, creating potential for deadlock or inconsistent state if the lock type changed. The RLock/Lock asymmetry between read and write paths was fragile.

**Fix:** Refactored so `save()` requires the caller to hold the lock (caller-must-hold-lock pattern). All call sites were updated to hold the write lock through the entire read-modify-save cycle.

**Implementation Risk:** None.

---

### 15. PREV-FIXED -- Compiled Binary Included in Repository
The compiled `discordo-plus` binary was previously present in the working directory. Binary artifacts should not be tracked in version control as they inflate repository size and may contain stale builds.

**Fix:** The binary is not git-tracked. `.gitignore` excludes the binary name. This was already resolved before the audit.

**Implementation Risk:** None.

---

## INFORMATIONAL

### 16. Commented-Out Local Replace Directives in go.mod
`go.mod:5-7` -- The file contains commented-out `replace` directives pointing to local sibling directories (`../tview`, `../arikawa`). These are development conveniences but could confuse contributors or be accidentally uncommented, causing builds to use unvetted local code.

**Fix:** Document these in CLAUDE.md (already done) and consider removing them from the committed go.mod, using `go.work` instead for local development.

**Implementation Risk:** None.

### 17. No Certificate Pinning for Discord API Connections
The project relies on Go's default TLS verification for all Discord API and CDN connections. This is standard and acceptable for a desktop application, but provides no protection against a compromised CA issuing a fraudulent certificate for `discord.com`. Certificate pinning would add defense-in-depth but is not standard practice for TUI applications.

**Fix:** No action needed. This is informational only.

**Implementation Risk:** N/A.

### 18. QR Login WebSocket Uses Default Dialer Without Custom TLS Config
`internal/ui/login/qr/events.go:40` -- The QR login flow connects to `wss://remote-auth-gateway.discord.gg` using `websocket.DefaultDialer`, which uses Go's default TLS settings. This is functionally correct but does not benefit from the project's custom HTTP transport (gzip/brotli decompression).

**Fix:** No action needed for security. The default TLS configuration in Go is secure.

**Implementation Risk:** N/A.

---

## Summary

| Severity | Total | Fixed |
|----------|-------|-------|
| Critical | 0     | —     |
| High     | 3     | 3     |
| Medium   | 4 + 3 new | 7 (all) |
| Low      | 3 + 2 new | 5 (all) |
| Info     | 3     | 0     |

## Top 3 Priorities
1. ✅ **Path traversal via attachment filenames** (#1) -- Fixed with `filepath.Base` sanitization.
2. ✅ **File permission hardening** (#3) -- Fixed with 0700/0600 permissions.
3. ✅ **Editor command injection** (#2) -- Fixed by replacing `sh -c` with direct `exec.Command` + `strings.Fields`.

All actionable findings are now resolved. Only informational items (#16-#18) remain, which require no action.

## Positive Observations
- Token storage via OS keyring (zalando/go-keyring) is the correct default approach, avoiding plaintext token storage
- QR login uses proper RSA-2048 key generation with `crypto/rand`, SHA-256 OAEP decryption, and ephemeral key pairs
- WebSocket connection to Discord remote auth gateway uses WSS (TLS)
- Guild state persistence uses mutex correctly around all read/write operations
- The `xdotool` geometry detection degrades gracefully when the tool is absent
- Token is never logged -- `slog` calls related to token operations only log error messages, not the token value
- Build uses `-trimpath` to strip local filesystem paths from the binary
- The `sendMessageData` reference/mention handling correctly sets `AllowedMentions` to prevent unintended pings
- TOML config parsing uses the well-maintained BurntSushi/toml library with no known vulnerabilities
- Dependencies are overwhelmingly current (see TECHFILE.md) with no known CVEs in the active dependency set

---

## Files Reviewed

_Every file reviewed, listed for coverage verification._

| # | File | Type |
|---|---|---|
| 1 | `main.go` | Entry point |
| 2 | `cmd/root.go` | CLI initialization |
| 3 | `go.mod` | Module manifest |
| 4 | `.gitignore` | Git config |
| 5 | `.github/workflows/ci.yml` | CI pipeline |
| 6 | `internal/config/config.go` | Config struct + loader |
| 7 | `internal/config/config.toml` | Embedded default config |
| 8 | `internal/config/config_test.go` | Config tests (grep) |
| 9 | `internal/config/keybinds.go` | Keybind definitions |
| 10 | `internal/config/theme.go` | Theme TOML types |
| 11 | `internal/config/editor.go` | Editor command (merged from editor_unix.go + editor_default.go) |
| 13 | `internal/consts/consts.go` | Constants + cache dir |
| 14 | `internal/keyring/keyring.go` | Token keyring access |
| 15 | `internal/keyring/keyring_test.go` | Keyring tests (presence verified) |
| 16 | `internal/logger/logger.go` | Log file setup |
| 17 | `internal/http/transport.go` | Custom HTTP transport |
| 18 | `internal/http/client.go` | API client factory |
| 19 | `internal/http/headers.go` | HTTP header construction |
| 20 | `internal/http/props.go` | Identify properties + super props |
| 21 | `internal/clipboard/clipboard.go` | Clipboard types |
| 22 | `internal/clipboard/clipboard_default.go` | Non-Linux clipboard |
| 23 | `internal/clipboard/clipboard_wayland.go` | Wayland clipboard (wl-copy) |
| 24 | `internal/cache/cache.go` | Completion cache |
| 25 | `internal/markdown/renderer.go` | Markdown AST renderer |
| 26 | `internal/notifications/notifications.go` | Notification dispatch |
| 27 | `internal/notifications/desktop_toast.go` | Linux/Windows toast |
| 28 | `internal/notifications/desktop_toast_darwin.go` | macOS notification |
| 29 | `internal/ui/util.go` | UI utilities |
| 30 | `internal/ui/root/model.go` | Root model + edit config |
| 31 | `internal/ui/root/events.go` | Root events (token, keyring) |
| 32 | `internal/ui/root/keybinds.go` | Root keybinds |
| 33 | `internal/ui/root/suspend_unix.go` | Unix SIGTSTP suspend |
| 34 | `internal/ui/root/suspend_default.go` | Non-Unix suspend no-op |
| 35 | `internal/ui/chat/model.go` | Chat model + state init |
| 36 | `internal/ui/chat/messages_list.go` | Message rendering + keybinds |
| 36a | `internal/ui/chat/attachment_handler.go` | Attachment download/view/save |
| 36b | `internal/ui/chat/embed_renderer.go` | Embed types + line wrapping |
| 36c | `internal/ui/chat/url_extractor.go` | URL extraction from messages |
| 37 | `internal/ui/chat/message_input.go` | Message composition |
| 38 | `internal/ui/chat/guilds_tree.go` | Guild/channel tree |
| 39 | `internal/ui/chat/guildstate.go` | Guild state persistence |
| 40 | `internal/ui/chat/state.go` | Gateway state events |
| 41 | `internal/ui/chat/events.go` | Chat event types |
| 42 | `internal/ui/chat/keybinds.go` | Chat keybinds |
| 43 | `internal/ui/chat/util.go` | Chat utilities |
| 44 | `internal/ui/chat/attachments_picker.go` | Attachment picker |
| 45 | `internal/ui/chat/channels_picker.go` | Channel picker |
| 46 | `internal/ui/chat/mentions_list.go` | Mentions list |
| 47 | `internal/ui/login/model.go` | Login model |
| 48 | `internal/ui/login/events.go` | Login events |
| 49 | `internal/ui/login/keybinds.go` | Login keybinds |
| 50 | `internal/ui/login/token/model.go` | Token login form |
| 51 | `internal/ui/login/token/events.go` | Token events |
| 52 | `internal/ui/login/qr/model.go` | QR login model |
| 53 | `internal/ui/login/qr/events.go` | QR login events + crypto |