Code Review Guidelines

A review guide for Go + vanilla JS + Wails v2 desktop applications, organized by priority and impact. This app spawns external processes, has no database, and targets Windows.

Table of Contents

Security — CRITICAL

1. Command Injection Prevention
2. XSS Prevention in Wails
3. Path Traversal Prevention

Performance — HIGH

1. Goroutine and Channel Safety

Correctness — HIGH

1. Error Handling (Go + JS)
2. Race Conditions and Mutex Usage

Maintainability — MEDIUM

1. Naming and Code Style

Security

Command Injection Prevention

Impact: CRITICAL | Category: security | Tags: exec, process, injection, yt-dlp, ffmpeg

Never pass unsanitized user input as arguments to exec.Command or exec.CommandContext. This app spawns yt-dlp and FFmpeg processes with user-controlled parameters (URLs, file paths, extra args).

Why This Matters

Unlike web apps where SQL injection is the top risk, desktop apps that spawn processes face command injection. An attacker-crafted URL or filename can inject shell metacharacters or additional arguments.

❌ Incorrect

// Dangerous: user-controlled string interpolated into command
func runYtDlp(url string) error {
    cmd := exec.Command("sh", "-c", "yt-dlp " + url)
    return cmd.Run()
}
// Input: "https://example.com; rm -rf /"
// Executes both commands!

// Dangerous: unsanitized extra args passed directly
func buildArgs(extraArgs string) []string {
    return append(baseArgs, strings.Split(extraArgs, " ")...)
}
// Input: "--exec rm -rf /"
// yt-dlp's --exec flag runs arbitrary commands!

✅ Correct

// Safe: pass URL as a single argument, not through shell
func runYtDlp(url string) error {
    cmd := exec.Command(ytDlpPath, "--no-exec", url)
    return cmd.Run()
}

// Safe: quote-aware split and validate args
func buildArgs(extraArgs string) []string {
    args := splitQuotedArgs(extraArgs)
    // Validate no dangerous flags
    for _, arg := range args {
        if strings.HasPrefix(arg, "--exec") {
            continue // skip dangerous flags
        }
    }
    return append(baseArgs, args...)
}

Key principle: Always use exec.Command(binary, arg1, arg2) with separate arguments. Never use exec.Command("sh", "-c", concatenatedString). Go's exec passes args directly to the process without shell interpretation.

➡️ Full details: security-command-injection.md

XSS Prevention in Wails

Impact: CRITICAL | Category: security | Tags: xss, wails, innerHTML, onclick, javascript

Never insert unsanitized file paths, URLs, or user input into HTML via innerHTML or inline event handlers. This was a real past bug in this codebase.

❌ Incorrect

// Dangerous: file path in inline onclick
const html = `<button onclick="openFile('${filePath}')">Open</button>`;
container.innerHTML = html;
// filePath: "file'); alert('xss"
// Breaks out of the string and executes arbitrary JS

// Dangerous: URL injected into innerHTML
const html = `<a href="${url}">${title}</a>`;
listContainer.innerHTML += html;

✅ Correct

// Safe: create elements programmatically
const btn = document.createElement('button');
btn.textContent = 'Open';
btn.addEventListener('click', () => openFile(filePath));
container.appendChild(btn);

// Safe: if innerHTML is unavoidable, escape the values
function escapeJS(str) {
    return str.replace(/\\/g, '\\\\')
              .replace(/'/g, "\\'")
              .replace(/"/g, '\\"')
              .replace(/\n/g, '\\n');
}
const html = `<button onclick="openFile('${escapeJS(filePath)}')">Open</button>`;

This app's specific risk: File paths from yt-dlp output and download URLs are user-influenced and can contain quotes, backslashes, and special characters. Any inline onclick handler using these values must escape them properly.

➡️ Full details: security-xss-wails.md

Path Traversal Prevention

Impact: CRITICAL | Category: security | Tags: path, traversal, file, directory

Validate that file operations stay within expected directories. User-controlled paths (save folder, output template, cookies file) can escape intended boundaries.

❌ Incorrect

// Dangerous: user controls the path, no validation
func saveFile(userPath string, data []byte) error {
    return os.WriteFile(userPath, data, 0644)
}
// Input: "../../../etc/passwd" or "C:\Windows\System32\something"

✅ Correct

// Safe: resolve and validate the path is within allowed directory
func saveFile(baseDir, filename string, data []byte) error {
    absBase, _ := filepath.Abs(baseDir)
    fullPath := filepath.Join(absBase, filepath.Clean(filename))

    // Verify it's still under the base directory
    if !strings.HasPrefix(fullPath, absBase) {
        return fmt.Errorf("path escapes base directory: %s", filename)
    }
    return os.WriteFile(fullPath, data, 0644)
}

➡️ Full details: security-path-traversal.md

Performance

Goroutine and Channel Safety

Impact: HIGH | Category: performance | Tags: goroutine, channel, leak, process, context

Goroutine leaks and zombie processes are the "N+1 queries" of desktop apps. Every spawned yt-dlp or FFmpeg process must be tracked and cleanable.

❌ Incorrect

// Leak: goroutine blocks forever if channel is never read
func startDownload(url string) {
    ch := make(chan string)
    go func() {
        result := download(url)
        ch <- result // blocks forever if nobody reads ch
    }()
    // ch is never read on error paths
}

// Leak: process outlives its parent context
func convert(path string) {
    cmd := exec.Command("ffmpeg", "-i", path, "out.mp4")
    cmd.Start()
    // If function returns early, ffmpeg is orphaned
}

✅ Correct

// Safe: use context for cancellation, buffered channel
func startDownload(ctx context.Context, url string) (string, error) {
    ch := make(chan string, 1) // buffered so goroutine doesn't block
    go func() {
        ch <- download(url)
    }()

    select {
    case result := <-ch:
        return result, nil
    case <-ctx.Done():
        return "", ctx.Err()
    }
}

// Safe: track process and kill on cancel
func convert(ctx context.Context, path string) error {
    cmd := exec.CommandContext(ctx, "ffmpeg", "-i", path, "out.mp4")
    if err := cmd.Start(); err != nil {
        return err
    }
    return cmd.Wait() // CommandContext kills process when ctx cancels
}

This app's specific patterns:

• Download queue uses a channel semaphore for concurrency. New channels must be buffered to match MaxConcurrentDownloads.
• NtSuspendProcess/NtResumeProcess must always be paired. A suspended process that's never resumed is a zombie.
• FFmpeg scanner goroutines reading stdout/stderr must exit when the process exits.

➡️ Full details: performance-goroutine-safety.md

Correctness

Error Handling

Impact: HIGH | Category: correctness | Tags: errors, go, javascript, reliability

Go: Wrap errors with context, never discard them

❌ Incorrect

// Bad: error discarded silently
data, _ := os.ReadFile(path)

// Bad: error returned without context
func loadSettings() (*Settings, error) {
    data, err := os.ReadFile(settingsPath)
    if err != nil {
        return nil, err // caller has no idea what file or why
    }
}

✅ Correct

// Good: wrapped with context
func loadSettings() (*Settings, error) {
    data, err := os.ReadFile(settingsPath)
    if err != nil {
        return nil, fmt.Errorf("loading settings from %s: %w", settingsPath, err)
    }
    var s Settings
    if err := json.Unmarshal(data, &s); err != nil {
        return nil, fmt.Errorf("parsing settings JSON: %w", err)
    }
    return &s, nil
}

JavaScript: Handle async errors, don't swallow rejections

❌ Incorrect

// Bad: unhandled promise rejection
async function startDownload(url) {
    const result = await window.go.main.App.AddDownload(url);
    updateUI(result);
    // If AddDownload throws, the UI shows nothing
}

✅ Correct

// Good: catch and handle
async function startDownload(url) {
    try {
        const result = await window.go.main.App.AddDownload(url);
        updateUI(result);
    } catch (err) {
        showError(`Download failed: ${err.message}`);
    }
}

➡️ Full details: correctness-error-handling.md

Race Conditions and Mutex Usage

Impact: HIGH | Category: correctness | Tags: mutex, sync, goroutine, race, settings

This app has multiple goroutines accessing shared state: settings, download queue, history. All shared state access must use proper synchronization.

❌ Incorrect

// Race: reading settings field directly from another goroutine
func (q *Queue) getMaxConcurrent() int {
    return q.settings.General.MaxConcurrentDownloads // data race!
}

// Race: map accessed from multiple goroutines without lock
func (q *Queue) addItem(item *Item) {
    q.items[item.ID] = item // concurrent map write = panic
}

✅ Correct

// Safe: use the thread-safe accessor
func (q *Queue) getMaxConcurrent() int {
    return q.settings.GetMaxConcurrentDownloads()
}

// Safe: protect map access with mutex
func (q *Queue) addItem(item *Item) {
    q.mu.Lock()
    defer q.mu.Unlock()
    q.items[item.ID] = item
}

This app's rule: Always use the Get*()/Set*() methods on *config.Settings. Never read or write struct fields directly across goroutines.

➡️ Full details: correctness-race-conditions.md

Maintainability

Naming and Code Style

Impact: MEDIUM | Category: maintainability | Tags: naming, go, javascript, readability

Go Conventions

// Exported: PascalCase
func BuildArgs(s *Settings) []string { ... }
type DownloadItem struct { ... }

// Unexported: camelCase (no underscores)
func parseOutput(line string) { ... }
var maxRetries = 5

// Acronyms: all caps
var httpClient *http.Client
func (d *Downloader) GetID() string { ... }
type URLParser struct { ... }

// Errors: "Err" prefix for sentinels
var ErrNotFound = errors.New("not found")

JavaScript Conventions

// Variables and functions: camelCase
const downloadQueue = [];
function updateProgressBar(item) { ... }

// Constants: UPPER_SNAKE_CASE
const MAX_CONCURRENT_DOWNLOADS = 3;

// DOM IDs in HTML: kebab-case
document.getElementById('download-progress');

// Wails event names: colon-separated, lowercase
runtime.EventsOn('download:update', callback);

Comments

// ❌ What (redundant)
// increment counter by 1
counter++

// ✅ Why (useful)
// Retry rename up to 5 times because antivirus may hold a file lock
// on the freshly downloaded binary.
for i := 0; i < 5; i++ {
    err = os.Rename(tmpPath, finalPath)
    if err == nil { break }
    time.Sleep(200 * time.Millisecond)
}

➡️ Full details: maintainability-naming.md

Quick Reference

Review Checklist

Security (CRITICAL, review first)

• No command injection in exec.Command calls
• File paths and URLs escaped in inline JS handlers
• Path traversal checked on user-supplied file paths
• Credentials (cookies, PO tokens) not logged or exposed
• SysProcAttr.CmdLine usage reviewed for injection

Performance (HIGH)

• No goroutine leaks (channels closed, contexts cancelled)
• No zombie processes (yt-dlp/FFmpeg cleaned up)
• Queue persistence not triggered on every progress tick
• Buffered channels used where appropriate

Correctness (HIGH)

• Go errors wrapped with fmt.Errorf("context: %w", err)
• JS async errors caught and surfaced to UI
• Shared state accessed through mutex or accessor methods
• New settings fields have mergeDefaults() entries
• Event names match the contract exactly

Maintainability (MEDIUM)

• Go: PascalCase exports, camelCase internal
• JS: camelCase, UPPER_SNAKE for constants
• Comments explain "why" not "what"
• frontend/wailsjs/ not manually edited

Severity Levels