sec(cli): scheme-allowlist openBrowser + atomic tokens.Save (F-17, F-18)

cmd/login.go

@@ -2,9 +2,11 @@ package cmd
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"os"
 	"os/exec"
 	"runtime"
@@ -299,18 +301,87 @@ func loadAnonymousTokens() []string {
 	return out
 }
 
-// openBrowser opens url in the user's default browser, best-effort.
-func openBrowser(url string) {
-	var err error
-	switch runtime.GOOS {
+// safeBrowserURL validates that raw is a well-formed http(s) URL whose first
+// character is not '-' (so the URL can never be interpreted as a flag by the
+// helper binary we exec). SEC-CLI FINDING-17.
+//
+// This is defense-in-depth: a hostile API server returning
+// `{"auth_url":"-Fpath"}` would otherwise have `open -F path` invoked on
+// macOS, exposing a local file in Finder. The CLI talks to TLS-protected
+// instanode.dev today so the threat model is narrow — but the cost of
+// hardening is < 20 LOC.
+func safeBrowserURL(raw string) (string, error) {
+	raw = strings.TrimSpace(raw)
+	if raw == "" {
+		return "", errors.New("empty URL")
+	}
+	// Reject leading dash so the URL can't be parsed as a flag by the
+	// underlying open/xdg-open/rundll32 helper.
+	if raw[0] == '-' {
+		return "", fmt.Errorf("refusing to open URL with leading '-': %q", raw)
+	}
+	u, err := url.Parse(raw)
+	if err != nil {
+		return "", fmt.Errorf("parsing URL: %w", err)
+	}
+	scheme := strings.ToLower(u.Scheme)
+	if scheme != "http" && scheme != "https" {
+		return "", fmt.Errorf("refusing to open URL with scheme %q (only http/https allowed)", u.Scheme)
+	}
+	if u.Host == "" {
+		return "", fmt.Errorf("refusing to open URL with empty host: %q", raw)
+	}
+	return raw, nil
+}
+
+// browserLauncherForGOOS returns the helper binary + arg list that opens a
+// URL in the user's default browser on the given GOOS. Extracted so the
+// per-platform fan-out is testable from a single-OS CI runner (the variant
+// not matching runtime.GOOS would otherwise be uncovered, which is what
+// our 100%-patch-coverage gate cares about).
+//
+// nil result means "no known helper for this GOOS"; caller should skip the
+// exec attempt and tell the user to open the URL manually.
+func browserLauncherForGOOS(goos, safeURL string) (name string, args []string) {
+	switch goos {
 	case "darwin":
-		err = exec.Command("open", url).Start()
+		return "open", []string{safeURL}
 	case "linux":
-		err = exec.Command("xdg-open", url).Start()
+		return "xdg-open", []string{safeURL}
 	case "windows":
-		err = exec.Command("rundll32", "url.dll,FileProtocolHandler", url).Start()
+		return "rundll32", []string{"url.dll,FileProtocolHandler", safeURL}
 	}
-	if err != nil {
+	return "", nil
+}
+
+// openBrowserOn is the GOOS-injectable core of openBrowser; the public
+// wrapper passes runtime.GOOS but tests can drive every per-OS branch
+// (including the unknown-GOOS fallback and the exec-failure path) from a
+// single CI runner. Returns "ok" / "refused" / "no-helper" / "exec-failed"
+// so a test can assert outcome without parsing stderr.
+func openBrowserOn(goos, rawURL string) string {
+	safe, verr := safeBrowserURL(rawURL)
+	if verr != nil {
+		fmt.Fprintf(os.Stderr, "Refusing to open URL: %v\n", verr)
+		return "refused"
+	}
+	name, args := browserLauncherForGOOS(goos, safe)
+	if name == "" {
+		fmt.Fprintf(os.Stderr, "Could not open browser automatically. Visit the URL above manually.\n")
+		return "no-helper"
+	}
+	if err := exec.Command(name, args...).Start(); err != nil {
 		fmt.Fprintf(os.Stderr, "Could not open browser automatically. Visit the URL above manually.\n")
+		return "exec-failed"
 	}
+	return "ok"
+}
+
+// openBrowser opens url in the user's default browser, best-effort.
+//
+// The url is validated by safeBrowserURL before being passed to any helper
+// binary; a server-controlled URL with a hostile scheme or leading-dash
+// payload is refused with a clear stderr message rather than executed.
+func openBrowser(rawURL string) {
+	_ = openBrowserOn(runtime.GOOS, rawURL)
 }

cmd/login_safe_url_test.go

@@ -0,0 +1,163 @@
+package cmd
+
+import (
+	"strings"
+	"testing"
+)
+
+// SEC-CLI FINDING-17 — defense-in-depth around openBrowser.
+//
+// A hostile API server returning an auth_url like "-FattackerFile" would
+// otherwise cause `open -F path` to execute on macOS, surfacing a local file
+// in Finder. We refuse anything that:
+//   - has a leading '-' (so it can't be parsed as a helper-binary flag)
+//   - has a scheme other than http/https
+//   - has an empty host
+//   - is empty
+func TestSafeBrowserURL(t *testing.T) {
+	cases := []struct {
+		name     string
+		in       string
+		wantErr  bool
+		errMatch string
+	}{
+		{name: "https ok", in: "https://instanode.dev/auth/cli/abc"},
+		{name: "http ok", in: "http://localhost:8080/auth"},
+		{name: "leading dash macOS exploit", in: "-Fattacker", wantErr: true, errMatch: "leading '-'"},
+		{name: "javascript scheme", in: "javascript:alert(1)", wantErr: true, errMatch: "scheme"},
+		{name: "file scheme", in: "file:///etc/passwd", wantErr: true, errMatch: "scheme"},
+		{name: "ftp scheme", in: "ftp://x.example/", wantErr: true, errMatch: "scheme"},
+		{name: "empty", in: "", wantErr: true, errMatch: "empty"},
+		{name: "whitespace-only", in: "   ", wantErr: true, errMatch: "empty"},
+		{name: "https no host", in: "https://", wantErr: true, errMatch: "host"},
+		{name: "garbage", in: "::not a url", wantErr: true},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := safeBrowserURL(tc.in)
+			if tc.wantErr {
+				if err == nil {
+					t.Fatalf("safeBrowserURL(%q) = %q, want error", tc.in, got)
+				}
+				if tc.errMatch != "" && !strings.Contains(err.Error(), tc.errMatch) {
+					t.Errorf("safeBrowserURL(%q) err = %v, want substring %q", tc.in, err, tc.errMatch)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("safeBrowserURL(%q) unexpected error: %v", tc.in, err)
+			}
+			if got == "" {
+				t.Errorf("safeBrowserURL(%q) returned empty", tc.in)
+			}
+		})
+	}
+}
+
+// TestOpenBrowser_RefuseDoesNotPanic verifies the wrapper safely skips the
+// exec.Command path when the URL is refused — we don't crash and we don't
+// pass user-controlled bytes to a helper binary.
+func TestOpenBrowser_RefuseDoesNotPanic(t *testing.T) {
+	defer func() {
+		if r := recover(); r != nil {
+			t.Fatalf("openBrowser panicked on bad URL: %v", r)
+		}
+	}()
+	openBrowser("-FattackerPath")
+	openBrowser("javascript:alert(1)")
+	openBrowser("")
+}
+
+// TestOpenBrowser_HappyPathDoesNotPanic hits the real-runtime path on a
+// known-good URL on whatever runtime.GOOS the test runs on — we don't
+// assert on stderr (the helper may or may not be installed in CI), only
+// that the wrapper returns cleanly.
+func TestOpenBrowser_HappyPathDoesNotPanic(t *testing.T) {
+	defer func() {
+		if r := recover(); r != nil {
+			t.Fatalf("openBrowser panicked: %v", r)
+		}
+	}()
+	// A reachable-looking URL — the exec may fail in CI (no $DISPLAY,
+	// no `open`, no `rundll32`), and that's fine: openBrowser is
+	// best-effort and just writes to stderr.
+	openBrowser("https://instanode.dev/")
+}
+
+// TestOpenBrowserOn_AllBranches drives openBrowserOn across every outcome
+// the function can return, satisfying the 100%-patch-coverage gate on a
+// single-OS CI runner.
+func TestOpenBrowserOn_AllBranches(t *testing.T) {
+	// Refused (bad URL).
+	if got := openBrowserOn("linux", "-FattackerPath"); got != "refused" {
+		t.Errorf("bad URL: got %q, want refused", got)
+	}
+	// No helper for the GOOS.
+	if got := openBrowserOn("plan9", "https://instanode.dev/"); got != "no-helper" {
+		t.Errorf("unknown GOOS: got %q, want no-helper", got)
+	}
+	// Exec-failed: pretend the OS is linux but use a URL that's valid AND
+	// pass a GOOS string we map to a binary that doesn't exist on PATH.
+	// browserLauncherForGOOS returns "xdg-open" for linux; in CI it may
+	// or may not exist. We force the exec-failed branch by mocking via
+	// the unknown-GOOS path… actually the cleanest forcing function is
+	// to verify the function returns SOMETHING from the known set on a
+	// real OS. The "exec-failed" branch is covered indirectly when the
+	// helper is missing from $PATH on the CI runner. We assert only that
+	// the return is in the valid set.
+	got := openBrowserOn("linux", "https://instanode.dev/")
+	switch got {
+	case "ok", "exec-failed":
+		// either is fine — both exercise the launcher path
+	default:
+		t.Errorf("real-helper path: got %q, want ok or exec-failed", got)
+	}
+}
+
+// TestOpenBrowserOn_ExecFailedForcedViaWindows forces the exec-failed
+// branch on a Linux CI runner by asking for the windows launcher
+// ("rundll32") which is guaranteed not to exist on PATH there.
+func TestOpenBrowserOn_ExecFailedForced(t *testing.T) {
+	// On any non-windows host, rundll32 is missing → exec.Start() returns
+	// ErrNotExist → openBrowserOn returns "exec-failed". On a windows
+	// host this test will accept "ok" too (no harm).
+	got := openBrowserOn("windows", "https://instanode.dev/")
+	if got != "exec-failed" && got != "ok" {
+		t.Errorf("windows launcher: got %q, want exec-failed (or ok on a real windows runner)", got)
+	}
+}
+
+// TestBrowserLauncherForGOOS asserts the per-OS helper choice across every
+// branch (darwin / linux / windows / unknown). Decoupling from runtime.GOOS
+// lets a Linux CI runner cover the macOS + Windows + fallback branches too
+// — required for the 100%-patch-coverage gate.
+func TestBrowserLauncherForGOOS(t *testing.T) {
+	cases := []struct {
+		goos     string
+		wantName string
+		wantArg0 string
+	}{
+		{goos: "darwin", wantName: "open", wantArg0: "https://x.example/"},
+		{goos: "linux", wantName: "xdg-open", wantArg0: "https://x.example/"},
+		{goos: "windows", wantName: "rundll32", wantArg0: "url.dll,FileProtocolHandler"},
+		{goos: "plan9", wantName: "", wantArg0: ""},
+		{goos: "", wantName: "", wantArg0: ""},
+	}
+	for _, tc := range cases {
+		t.Run(tc.goos, func(t *testing.T) {
+			name, args := browserLauncherForGOOS(tc.goos, "https://x.example/")
+			if name != tc.wantName {
+				t.Fatalf("name = %q, want %q", name, tc.wantName)
+			}
+			if name == "" {
+				if args != nil {
+					t.Fatalf("args should be nil on unknown GOOS, got %v", args)
+				}
+				return
+			}
+			if len(args) == 0 || args[0] != tc.wantArg0 {
+				t.Fatalf("args[0] = %v, want %q", args, tc.wantArg0)
+			}
+		})
+	}
+}

internal/tokens/store.go

@@ -107,13 +107,27 @@ func Load() (*Store, error) {
 	return s, nil
 }
 
-// Save writes the store back to disk.
+// Save writes the store back to disk atomically — temp file + rename — so a
+// crash mid-write can never truncate ~/.instant-tokens and lose every
+// recorded anonymous token. SEC-CLI FINDING-18.
+//
+// The pattern mirrors cliconfig.Save (already in this repo).
 func (s *Store) Save() error {
 	data, err := json.MarshalIndent(s, "", "  ")
 	if err != nil {
 		return err
 	}
-	return os.WriteFile(s.path, data, 0600)
+	tmp := s.path + ".tmp"
+	if err := os.WriteFile(tmp, data, 0600); err != nil {
+		return err
+	}
+	if err := os.Rename(tmp, s.path); err != nil {
+		// Best-effort cleanup of the temp file so a half-written sibling
+		// doesn't linger after a failed rename.
+		_ = os.Remove(tmp)
+		return err
+	}
+	return nil
 }
 
 // Add appends a new entry and saves.

internal/tokens/store_atomic_test.go

@@ -0,0 +1,89 @@
+package tokens
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+// TestSave_AtomicNoTempLeak ensures Save() leaves no .tmp file on disk on a
+// successful write. SEC-CLI FINDING-18.
+func TestSave_AtomicNoTempLeak(t *testing.T) {
+	dir := setupTempHome(t)
+	s, err := Load()
+	if err != nil {
+		t.Fatalf("Load: %v", err)
+	}
+	if err := s.Add(Entry{Token: "tok-atomic", Name: "x", Type: "postgres", URL: "postgres://x"}); err != nil {
+		t.Fatalf("Add: %v", err)
+	}
+	tmpPath := filepath.Join(dir, ".instant-tokens.tmp")
+	if _, err := os.Stat(tmpPath); !os.IsNotExist(err) {
+		t.Errorf(".tmp file should not linger after successful Save, got err=%v", err)
+	}
+	// And the real file should exist + be readable.
+	if _, err := os.Stat(filepath.Join(dir, ".instant-tokens")); err != nil {
+		t.Errorf("expected .instant-tokens to exist, got: %v", err)
+	}
+}
+
+// TestSave_RenameFailureReturnsError exercises the rename-failure branch.
+// Pointing the store at a path whose parent directory is a regular file
+// makes os.WriteFile of the .tmp succeed (we're writing inside the parent
+// of `path`, which is the temp dir) BUT — wait, no: WriteFile would fail
+// upstream. To hit the rename-failure branch specifically we craft a path
+// where the .tmp can be written but the rename target is a directory: on
+// POSIX, os.Rename(file, existingDir) returns ENOTEMPTY / EISDIR, which is
+// exactly the failure mode our cleanup branch handles.
+func TestSave_RenameFailureReturnsError(t *testing.T) {
+	dir := setupTempHome(t)
+	storePath := filepath.Join(dir, ".instant-tokens")
+	// Make storePath a non-empty directory so os.Rename(tmp, storePath)
+	// returns an error and we exercise the cleanup branch.
+	if err := os.Mkdir(storePath, 0700); err != nil {
+		t.Fatalf("mkdir: %v", err)
+	}
+	if err := os.WriteFile(filepath.Join(storePath, "block"), []byte("x"), 0600); err != nil {
+		t.Fatalf("seed inside dir: %v", err)
+	}
+
+	s := &Store{path: storePath}
+	err := s.Add(Entry{Token: "tok-rename-fail", Name: "x", Type: "postgres", URL: "postgres://x"})
+	if err == nil {
+		t.Fatalf("expected Save to return error when target path is a non-empty dir")
+	}
+	// The .tmp sibling MUST be cleaned up by the failure-cleanup branch.
+	if _, statErr := os.Stat(storePath + ".tmp"); !os.IsNotExist(statErr) {
+		t.Errorf(".tmp file should have been removed after rename failure, got err=%v", statErr)
+	}
+}
+
+// TestSave_RenameOverwritesExistingFile verifies the rename idiom replaces
+// an existing file (not appended). Cross-platform — works on Linux + macOS.
+func TestSave_RenameOverwritesExistingFile(t *testing.T) {
+	dir := setupTempHome(t)
+	path := filepath.Join(dir, ".instant-tokens")
+
+	// Pre-seed the path with garbage so Save() must clobber it.
+	if err := os.WriteFile(path, []byte("GARBAGE_PRESEED"), 0600); err != nil {
+		t.Fatalf("pre-seed: %v", err)
+	}
+
+	s, err := Load()
+	if err == nil {
+		t.Fatalf("Load should have failed on garbage seed, got s=%+v", s)
+	}
+	// Build a fresh store and Save — overwrites the bad bytes.
+	fresh := &Store{path: path}
+	if err := fresh.Add(Entry{Token: "real", Name: "n", Type: "postgres", URL: "postgres://x"}); err != nil {
+		t.Fatalf("Add: %v", err)
+	}
+
+	got, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatalf("ReadFile: %v", err)
+	}
+	if string(got) == "GARBAGE_PRESEED" {
+		t.Errorf("Save did not overwrite seeded garbage")
+	}
+}