fix: comprehensive code review fixes across security, correctness, and robustness

[tim-thacker-nullify]

Summary

Addresses 34 issues identified in a senior-level code review of the CLI repository (5 critical, 6 high, 11 medium, 7 low, 5 nit). Key highlights:

Critical & Security Fixes

• Fix broken refresh token flow: Send refresh token as HTTP cookie instead of query parameter to match backend (security-droid) expectation
• Fix GITHUB_ACTION_REPOSITORY → GITHUB_REPOSITORY: The old env var name is non-standard and not set by GitHub Actions
• Add url.PathEscape for finding IDs in all URL path interpolations across MCP tools and CLI commands, preventing path traversal
• Replace bare http.Get with http.NewRequestWithContext for proper timeout and context cancellation in GitHub token exchange
• URL-encode GitHub token and owner in token exchange request
• Fix retry transport to respect context cancellation via select on ctx.Done()
• Fix file handle leak in openapi.go (missing defer Close)
• Add severity-threshold validation in ci gate to reject invalid values

Correctness Fixes

• SanitizeNullifyHost now delegates to ParseCustomerDomain, accepting acme, acme.nullify.ai, and api.acme.nullify.ai formats (strips path/query, validates hostname chars)
• Fix openapi.go nil, nil return → descriptive error
• Fix ci report limit from 1 → 1000 for accurate finding counts
• Mark pentest --spec-path as required flag (removes manual check)
• Add missing scanner types (pentest, bughunt, cspm) to MCP get_findings_for_repo and get_critical_path

Exit Codes & UX

• Consistent exit codes across all commands: ExitAuthError(2), ExitNetworkError(3), ExitFindings(1) — CI pipelines can now distinguish auth failures from findings
• Fix auth token to print trailing newline (was breaking piping)
• Fix "1 findings" → "1 finding" singular form
• Add periodic "still waiting..." message during login (every 30s)
• Improve auth config error message when no config exists

Architecture & Code Quality

• Deduplicate buildQueryString in MCP package → delegates to lib.BuildQueryString
• Add ClientOption/WithHTTPClient to generated API client; wire retry transport into it
• Export NewRetryTransport for reuse across client types
• Parallelize status command scanner queries with errgroup
• Use promptResult helper consistently in all MCP prompts
• Fix wizard to use absolute paths via os.Getwd() instead of relative paths
• Handle output.Print errors with stderr fallback
• Remove unused --auth-config global flag (pentest-specific flag remains)
• Clear stale Token field on refreshing transport client
• Log warning when NULLIFY_HOST env var is invalid

Files Changed (25)

internal/auth/login.go, internal/lib/get_token.go, internal/lib/urls.go, internal/lib/openapi.go, internal/client/retry.go, internal/client/client.go, internal/client/refreshing_transport.go, internal/api/client.go, internal/mcp/server.go, internal/mcp/tools_unified.go, internal/mcp/tools_composite.go, internal/mcp/prompts.go, internal/wizard/steps.go, cmd/cli/cmd/root.go, cmd/cli/cmd/ci.go, cmd/cli/cmd/status.go, cmd/cli/cmd/findings.go, cmd/cli/cmd/fix.go, cmd/cli/cmd/auth.go, cmd/cli/cmd/chat.go, cmd/cli/cmd/mcp.go, cmd/cli/cmd/pentest.go, cmd/cli/cmd/repos.go, cmd/cli/cmd/status_test.go, scripts/generate/main.go

Test plan

• go build ./... — compiles cleanly
• go vet ./... — no warnings
• go test ./... — all tests pass (updated test expectations for singular form and new URL parsing behavior)
• Manual: nullify auth login --host acme accepts short-form host
• Manual: nullify ci gate --severity-threshold invalid rejects with error
• Manual: nullify auth token outputs trailing newline
• CI: verify refresh token flow works end-to-end

🤖 Generated with Claude Code

[jonathanlam]

conflict

[nullify-reviewer]

Code review

Found 3 issues:

1. time.After inside waitLoop resets on every ticker tick — the 10-minute timeout never fires

time.After(10 * time.Minute) is evaluated on every iteration of the for { select { ... } } loop. The 30-second ticker.C case fires every 30 seconds, causing a new time.After timer to be created each time. This means the effective timeout is reset every 30 seconds and will never fire, leaving authentication waiting indefinitely. The fix is to create the timer once outside the loop: timeout := time.NewTimer(10 * time.Minute).

cli/internal/auth/login.go

Lines 118 to 136 in 731fd38

	ticker := time.NewTicker(30 * time.Second)
	defer ticker.Stop()
	var sessionID string
	waitLoop:
	for {
	select {
	case sessionID = <-sessionCh:
	break waitLoop
	case err := <-errCh:
	return fmt.Errorf("local server error: %w", err)
	case <-ctx.Done():
	return fmt.Errorf("authentication cancelled")
	case <-time.After(10 * time.Minute):
	return fmt.Errorf("authentication timed out — the session has expired")
	case <-ticker.C:
	fmt.Println("Still waiting for authentication...")
	}
	}

2. resolveHost config-file path skips SanitizeNullifyHost, causing inconsistent host format

Paths 1 (flag) and 2 (env var) in resolveHost both call lib.SanitizeNullifyHost, which now prepends api. to bare hosts (e.g. acme.nullify.ai → api.acme.nullify.ai). Path 3 (config file) returns cfg.Host directly without sanitizing. This means the same host value resolves differently depending on how it was provided: --host acme.nullify.ai yields api.acme.nullify.ai, but a config file containing acme.nullify.ai yields the bare form. The inconsistency causes lookup mismatches in GetValidToken (keyed by host) and sends HTTP requests to the wrong host in authTransport.

cli/cmd/cli/cmd/root.go

Lines 160 to 164 in 731fd38

	// 3. Read from config file
	cfg, err := auth.LoadConfig()
	if err == nil && cfg.Host != "" {
	return cfg.Host
	}

3. SanitizeNullifyHost semantic change breaks existing credentials for users with pre-existing configs

SanitizeNullifyHost now delegates to ParseCustomerDomain, which adds an api. prefix where it previously did not (e.g. acme.nullify.ai now becomes api.acme.nullify.ai). Credentials saved by prior versions are keyed under "acme.nullify.ai" in the credentials file. After this change, GetValidToken is called with "api.acme.nullify.ai" (the newly sanitized form), which finds no match and returns "not authenticated", silently breaking authentication for any existing user whose credentials were stored under the bare-host key.

cli/internal/lib/urls.go

Lines 51 to 53 in 731fd38

	func SanitizeNullifyHost(nullifyHost string) (string, error) {
	return ParseCustomerDomain(nullifyHost)
	}

Generated by the Platform-Tools/code-review lambda using Claude Code Opus 4.6.