diff --git a/cmd/login.go b/cmd/login.go index 39cebed..734a8e3 100644 --- a/cmd/login.go +++ b/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) } diff --git a/cmd/login_safe_url_test.go b/cmd/login_safe_url_test.go new file mode 100644 index 0000000..a554433 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/internal/tokens/store.go b/internal/tokens/store.go index 03a2335..bd95a64 100644 --- a/internal/tokens/store.go +++ b/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. diff --git a/internal/tokens/store_atomic_test.go b/internal/tokens/store_atomic_test.go new file mode 100644 index 0000000..3a21f04 --- /dev/null +++ b/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") + } +}