feat: browser-based login with automatic API key creation#43
Conversation
supermodel login now opens the dashboard in the browser, which auto-creates an API key and sends it back to a localhost callback server. Zero copy-paste. Falls back to manual paste if the browser can't open or the flow times out (5 minutes). New: --token flag for CI/headless: supermodel login --token smsk_live_... Closes supermodeltools#35
WalkthroughBrowser-based login added with a localhost callback and a new non-interactive Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI (login cmd)
participant Local as Local HTTP<br/>(127.0.0.1:randomPort)
participant Browser as Browser / Dashboard
User->>CLI: run `supermodel login`
activate CLI
CLI->>Local: start HTTP server on random port
CLI->>CLI: generate random state
CLI->>Browser: open dashboard URL (port + state)
Browser->>Browser: user authenticates & approves
Browser->>Local: GET /callback?key=...&state=...
Local->>Local: validate state, respond 200 "Authenticated"
Local->>CLI: deliver key via channel
CLI->>CLI: trim, validate, save key
CLI->>User: show success
deactivate CLI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/login.go`:
- Around line 19-23: The RunE handler currently checks token != "" which misses
the case where the user passed --token "" (empty) from CI; change the condition
to inspect whether the flag was explicitly provided using
cmd.Flags().Changed("token") and call auth.LoginWithToken(token) when true,
otherwise call auth.Login(cmd.Context()); update the anonymous RunE func in
cmd/login.go (the closure referencing the token variable) to use
cmd.Flags().Changed("token") so empty-but-provided tokens are forwarded to
auth.LoginWithToken for proper validation.
In `@internal/auth/handler.go`:
- Around line 49-61: The callback currently accepts a real API key via GET query
param ("key") and sends it over keyCh, which exposes the bearer token in browser
history; change the flow to accept a one-time code/session id instead (e.g.,
query param "code") and send that code over a new channel (e.g., codeCh) instead
of keyCh; implement a separate secure exchange endpoint (e.g., POST "/exchange")
or have the CLI poll a localhost endpoint to exchange that one-time code for the
real API key over HTTPS, and update any references to keyCh/"/callback" handler
to use the new codeCh and exchange flow so the actual bearer token is never
placed in a URL or browser history.
- Around line 64-69: The http.Server instance created as srv (currently
&http.Server{Handler: mux}) lacks a ReadHeaderTimeout, making it vulnerable to
Slowloris; update the server construction in the auth callback code to include
ReadHeaderTimeout: 5 * time.Second (e.g., &http.Server{Handler: mux,
ReadHeaderTimeout: 5 * time.Second}), ensure the time package is imported, and
keep existing behavior around listener and errCh/Srv.Serve unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2a05b23-d177-403d-8d7f-ae6d80b7934c
📒 Files selected for processing (2)
cmd/login.gointernal/auth/handler.go
| RunE: func(cmd *cobra.Command, _ []string) error { | ||
| if token != "" { | ||
| return auth.LoginWithToken(token) | ||
| } | ||
| return auth.Login(cmd.Context()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and inspect the login.go file
find . -name "login.go" -type f | head -20Repository: supermodeltools/cli
Length of output: 77
🏁 Script executed:
# Check the structure of cmd directory
git ls-files cmd/ | head -20Repository: supermodeltools/cli
Length of output: 303
🏁 Script executed:
# Read the login.go file to see the actual code
cat -n cmd/login.goRepository: supermodeltools/cli
Length of output: 939
🏁 Script executed:
# Search for usage of Flags().Changed() in the codebase to confirm it's a valid API
rg "Changed\(" --type go | head -20Repository: supermodeltools/cli
Length of output: 45
🏁 Script executed:
# Look at the auth package to see how LoginWithToken handles empty tokens
find . -path "*/internal/auth*" -type f | head -20Repository: supermodeltools/cli
Length of output: 112
🏁 Script executed:
# Check the auth handler to see how LoginWithToken validates tokens
cat -n ./internal/auth/handler.go | head -100Repository: supermodeltools/cli
Length of output: 3729
🏁 Script executed:
# Check if there's any token validation
rg "LoginWithToken|Login\(" --type go -A 5 | head -50Repository: supermodeltools/cli
Length of output: 1031
🌐 Web query:
Cobra command line Changed() flag method documentation
💡 Result:
The Changed flag method in Cobra command line applications refers to functionality from the underlying pflag library (used by Cobra for flag parsing). There are two related features: 1. Flag.Changed field: A boolean field on the *pflag.Flag struct indicating whether the flag was explicitly set by the user during parsing (via command line arguments). It is set to true when FlagSet.Parse processes the flag, even if the provided value matches the default. Direct assignments to the bound variable or calls to flag.Value.Set do not set it. Initial value is false if not set during parse. Usage example (in a Cobra command's Run function, after parsing): flag := cmd.Flags.Lookup("myflag") if flag != nil && flag.Changed { // Flag was explicitly provided on command line } 2. FlagSet.Changed(name string) bool method: A method on *pflag.FlagSet (accessible via cmd.Flags or cmd.PersistentFlags) that returns true if the named flag was explicitly set during Parse, false otherwise. It looks up the flag and returns its Changed field. Usage: if cmd.Flags.Changed("myflag") { // Flag was set } This is useful for distinguishing user-provided flags from defaults, e.g., in conditional logic like printing only requested version fields (as discussed in Cobra issue #434). Note: Documentation comment "If the user set the value (or if left to default)" is ambiguous/misleading; behavior is "true if explicitly set during Parse". Official sources: pflag source code (flag.go) confirms flag.Changed = true is set during parsing when the flag is matched. pkg.go.dev/github.com/spf13/pflag documents the API. Cobra uses pflag internally, so this applies directly to cmd.Flags. Latest pflag v1.0.10 (2025), compatible with Cobra v1.10+.
Citations:
- 1: question: how to find what flags is been called in the command spf13/cobra#434
- 2: https://pkg.go.dev/github.com/spf13/pflag
- 3: Confusing semantics of Flag.Changed spf13/pflag#308
- 4: https://chromium.googlesource.com/external/github.com/spf13/pflag/+/252d3ee662414967926c300a620758cb1dc452e2/flag.go
- 5: https://chromium.googlesource.com/external/github.com/spf13/pflag/+/refs/heads/master/flag.go
- 6: https://github.com/spf13/pflag/blob/master/flag.go
Check the flag was explicitly provided, not just that the string isn't empty.
Here's the thing: when you run supermodel login --token "$SUPERMODEL_API_KEY" in CI and that env var happens to be empty (or undefined), bash expands it to "". Your code sees token != "" as false, so it skips LoginWithToken() and tries to open a browser instead. In a headless CI box with no browser, this just hangs forever waiting for something that'll never happen—super confusing for whoever's debugging the CI job.
The auth package is actually ready for this—LoginWithToken() already checks for empty tokens and returns a clear error saying "API key cannot be empty". But your flag check never gets there.
Use cmd.Flags().Changed("token") to check whether the user actually provided the flag (even if it's empty). This way, if someone passes --token "", it'll hit LoginWithToken(), which rejects it with a helpful error message instead of silently hanging.
Suggested fix
RunE: func(cmd *cobra.Command, _ []string) error {
- if token != "" {
+ if cmd.Flags().Changed("token") {
return auth.LoginWithToken(token)
}
return auth.Login(cmd.Context())
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RunE: func(cmd *cobra.Command, _ []string) error { | |
| if token != "" { | |
| return auth.LoginWithToken(token) | |
| } | |
| return auth.Login(cmd.Context()) | |
| RunE: func(cmd *cobra.Command, _ []string) error { | |
| if cmd.Flags().Changed("token") { | |
| return auth.LoginWithToken(token) | |
| } | |
| return auth.Login(cmd.Context()) | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/login.go` around lines 19 - 23, The RunE handler currently checks token
!= "" which misses the case where the user passed --token "" (empty) from CI;
change the condition to inspect whether the flag was explicitly provided using
cmd.Flags().Changed("token") and call auth.LoginWithToken(token) when true,
otherwise call auth.Login(cmd.Context()); update the anonymous RunE func in
cmd/login.go (the closure referencing the token variable) to use
cmd.Flags().Changed("token") so empty-but-provided tokens are forwarded to
auth.LoginWithToken for proper validation.
| mux.HandleFunc("/callback", func(w http.ResponseWriter, r *http.Request) { | ||
| if r.URL.Query().Get("state") != state { | ||
| http.Error(w, "Invalid state parameter", http.StatusBadRequest) | ||
| return | ||
| } | ||
| key := r.URL.Query().Get("key") | ||
| if key == "" { | ||
| http.Error(w, "Missing key", http.StatusBadRequest) | ||
| return | ||
| } | ||
| w.Header().Set("Content-Type", "text/html") | ||
| fmt.Fprint(w, `<!DOCTYPE html><html><body style="font-family:system-ui;display:flex;justify-content:center;align-items:center;height:100vh;margin:0;background:#0a0a0a;color:#fff"><div style="text-align:center"><h2>✓ Authenticated</h2><p style="color:#888">You can close this tab and return to your terminal.</p></div></body></html>`) | ||
| keyCh <- key |
There was a problem hiding this comment.
Don’t round-trip the real API key through GET /callback?key=....
Accepting the key from the query string means the dashboard is putting a live bearer token into browser history and any local URL logging on the machine. That should be a one-time code/session ID that the CLI exchanges for the real key over HTTPS, or a POST body to localhost if you want to keep the direct callback shape.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/handler.go` around lines 49 - 61, The callback currently
accepts a real API key via GET query param ("key") and sends it over keyCh,
which exposes the bearer token in browser history; change the flow to accept a
one-time code/session id instead (e.g., query param "code") and send that code
over a new channel (e.g., codeCh) instead of keyCh; implement a separate secure
exchange endpoint (e.g., POST "/exchange") or have the CLI poll a localhost
endpoint to exchange that one-time code for the real API key over HTTPS, and
update any references to keyCh/"/callback" handler to use the new codeCh and
exchange flow so the actual bearer token is never placed in a URL or browser
history.
| case key := <-keyCh: | ||
| fmt.Println() | ||
| cfg.APIKey = strings.TrimSpace(key) | ||
| if err := cfg.Save(); err != nil { | ||
| return err | ||
| } | ||
| ui.Success("Authenticated — key saved to %s", config.Path()) | ||
| return nil |
There was a problem hiding this comment.
Validate the key before saving it and printing “Authenticated”.
All three paths only do trim/non-empty checks before cfg.Save(). internal/config/config.go:57-71 writes cfg.APIKey straight to disk, and internal/api/client.go:27-34 consumes it as-is on later requests, so a bad callback payload or pasted typo becomes a persisted broken login. Please funnel these through a shared validate-and-save step so login only succeeds once the key is known-good.
Also applies to: 109-123, 145-163
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/auth/handler_test.go (3)
49-49: Consider not ignoring the error fromconfig.Load().I know this is test code and it'll probably be fine, but ignoring errors can hide issues when debugging flaky tests later. A quick check would make this more robust:
- cfg, _ := config.Load() + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + }Same thing applies to line 181.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/handler_test.go` at line 49, The test currently ignores the error returned by config.Load(); update the test to check the returned error (from config.Load()) and fail the test if non-nil—e.g., capture (cfg, err := config.Load()) and use t.Fatalf/t.Fatal or a test assertion helper (require.NoError/require.Nil) to surface failures; apply the same change for the second ignored call at the other occurrence so both places properly validate the load error before proceeding.
67-80: This duplicates the callback handler logic instead of testing the real one.So right now you're defining a fresh handler inline that looks like the real
/callbackhandler inhandler.go, but it's actually a separate copy. If someone tweaks the real handler (say, adds a new validation), this test would still pass even if the real code is broken.Ideally, you'd extract the callback handler logic into a reusable function or test the actual
Login()flow end-to-end. I get that testing the full browser flow is tricky, but maybe you could export the handler setup or use a factory function?Something like:
// In handler.go func newCallbackHandler(expectedState string, keyCh chan<- string) http.HandlerFunc { ... } // In test mux.HandleFunc("/callback", newCallbackHandler(state, keyCh))This way you're testing the actual code path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/handler_test.go` around lines 67 - 80, The test duplicates the /callback logic instead of exercising the real handler; extract the callback logic from handler.go into a reusable function (e.g., newCallbackHandler(expectedState string, keyCh chan<- string) http.HandlerFunc) and have the production Login() setup use that factory, then replace the inline handler in internal/auth/handler_test.go with mux.HandleFunc("/callback", newCallbackHandler(state, keyCh)) so the test exercises the actual code path.
174-175: These errors should probably be checked.I know it's test code, but if the directory can't be created or config can't be saved, the test will fail in a confusing way later. Quick fix:
- os.MkdirAll(filepath.Join(tmp, ".supermodel"), 0o700) - cfg.Save() + if err := os.MkdirAll(filepath.Join(tmp, ".supermodel"), 0o700); err != nil { + t.Fatal(err) + } + if err := cfg.Save(); err != nil { + t.Fatal(err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/handler_test.go` around lines 174 - 175, The test currently ignores errors from os.MkdirAll(filepath.Join(tmp, ".supermodel"), 0o700) and cfg.Save(); capture and check both return values and fail the test immediately on error (e.g., use t.Fatalf or testing helper like require.NoError) so failures to create the directory or persist config produce a clear error at this point; update the call sites around os.MkdirAll and cfg.Save() in handler_test.go to handle their errors and abort the test on failure.internal/auth/handler.go (1)
64-70: The//nolint:goseccomment is probably unnecessary now.Since you've got
ReadHeaderTimeout: 10 * time.Secondset, the Slowloris attack warning (G112) that gosec would normally raise should pass without any suppression. The nolint comment might be leftover from before the timeout was added.Two options:
- Remove it if gosec is happy with the timeout (which it should be)
- Keep it and be specific with a rule ID like
//nolint:gosec // G112if there's a specific reason it needs to stayGeneric
//nolint:goseccomments without a rule ID make it harder for future readers to understand why the suppression is there, so either approach is better than leaving it as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/handler.go` around lines 64 - 70, The generic suppression comment "//nolint:gosec" next to the server construction is likely unnecessary now that ReadHeaderTimeout is set; remove the generic nolint or make it rule-specific. Update the srv creation in internal/auth/handler.go (the http.Server value assigned to variable srv with ReadHeaderTimeout: 10 * time.Second) by either deleting the "//nolint:gosec" comment entirely or replacing it with a rule-specific suppression like "//nolint:gosec // G112" if you have a documented reason to silence the Slowloris check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/auth/handler_test.go`:
- Around line 17-33: The tests TestLoginWithToken,
TestLoginWithToken_Whitespace, and TestLogout currently call t.Setenv("HOME",
tmp) which doesn't isolate the home directory on Windows; update each test to
also set USERPROFILE to the same tmp (i.e., call t.Setenv("USERPROFILE", tmp)
right after t.Setenv("HOME", tmp)) so os.UserHomeDir() will point to the temp
dir on Windows and avoid touching real config during LoginWithToken and Logout
flows.
In `@internal/auth/handler.go`:
- Around line 179-183: The randomState function currently ignores the error from
rand.Read causing possible non-random state; update randomState to check the
error returned by crypto/rand.Read (in function randomState), and if err != nil
panic with a clear message including the error so the process fails fast rather
than returning a weak state string.
---
Nitpick comments:
In `@internal/auth/handler_test.go`:
- Line 49: The test currently ignores the error returned by config.Load();
update the test to check the returned error (from config.Load()) and fail the
test if non-nil—e.g., capture (cfg, err := config.Load()) and use
t.Fatalf/t.Fatal or a test assertion helper (require.NoError/require.Nil) to
surface failures; apply the same change for the second ignored call at the other
occurrence so both places properly validate the load error before proceeding.
- Around line 67-80: The test duplicates the /callback logic instead of
exercising the real handler; extract the callback logic from handler.go into a
reusable function (e.g., newCallbackHandler(expectedState string, keyCh chan<-
string) http.HandlerFunc) and have the production Login() setup use that
factory, then replace the inline handler in internal/auth/handler_test.go with
mux.HandleFunc("/callback", newCallbackHandler(state, keyCh)) so the test
exercises the actual code path.
- Around line 174-175: The test currently ignores errors from
os.MkdirAll(filepath.Join(tmp, ".supermodel"), 0o700) and cfg.Save(); capture
and check both return values and fail the test immediately on error (e.g., use
t.Fatalf or testing helper like require.NoError) so failures to create the
directory or persist config produce a clear error at this point; update the call
sites around os.MkdirAll and cfg.Save() in handler_test.go to handle their
errors and abort the test on failure.
In `@internal/auth/handler.go`:
- Around line 64-70: The generic suppression comment "//nolint:gosec" next to
the server construction is likely unnecessary now that ReadHeaderTimeout is set;
remove the generic nolint or make it rule-specific. Update the srv creation in
internal/auth/handler.go (the http.Server value assigned to variable srv with
ReadHeaderTimeout: 10 * time.Second) by either deleting the "//nolint:gosec"
comment entirely or replacing it with a rule-specific suppression like
"//nolint:gosec // G112" if you have a documented reason to silence the
Slowloris check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33ffa448-3e7c-41ad-a336-2cab4024b691
📒 Files selected for processing (2)
internal/auth/handler.gointernal/auth/handler_test.go
| func TestLoginWithToken(t *testing.T) { | ||
| // Point config to a temp dir so we don't touch real config. | ||
| tmp := t.TempDir() | ||
| t.Setenv("HOME", tmp) | ||
|
|
||
| if err := LoginWithToken("smsk_live_test123"); err != nil { | ||
| t.Fatalf("LoginWithToken: %v", err) | ||
| } | ||
|
|
||
| cfg, err := config.Load() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if cfg.APIKey != "smsk_live_test123" { | ||
| t.Errorf("expected key smsk_live_test123, got %q", cfg.APIKey) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What environment variables does Go's os.UserHomeDir() check on Windows?
💡 Result:
On Windows, Go's os.UserHomeDir checks the USERPROFILE environment variable. If USERPROFILE is set and non-empty, it returns its value. If not set, it returns an error indicating that %USERPROFILE% is not defined. This is confirmed by the official Go documentation and the source code implementation in src/os/file.go: func UserHomeDir (string, error) { ... switch runtime.GOOS { case "windows": env, enverr = "USERPROFILE", "%userprofile%" } if v := Getenv(env); v != "" { return v, nil } ... return "", errors.New(enverr + " is not defined") } Note that older versions (pre-Go 1.12.1) used HOMEDRIVE + HOMEPATH, but since Go 1.12.1, it uses USERPROFILE exclusively. As of 2026, all maintained Go versions use USERPROFILE.
Citations:
Fix Windows test isolation by setting USERPROFILE env var.
The test uses t.Setenv("HOME", tmp) to isolate config, but this doesn't work on Windows. The Go standard library's os.UserHomeDir() function reads HOME on Unix-like systems, but on Windows it only reads the USERPROFILE environment variable. So on Windows, the test would still touch the real config file even though we're trying to prevent it.
The fix is straightforward—just set both variables:
t.Setenv("HOME", tmp)
t.Setenv("USERPROFILE", tmp)This applies to TestLoginWithToken, TestLoginWithToken_Whitespace, and TestLogout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/handler_test.go` around lines 17 - 33, The tests
TestLoginWithToken, TestLoginWithToken_Whitespace, and TestLogout currently call
t.Setenv("HOME", tmp) which doesn't isolate the home directory on Windows;
update each test to also set USERPROFILE to the same tmp (i.e., call
t.Setenv("USERPROFILE", tmp) right after t.Setenv("HOME", tmp)) so
os.UserHomeDir() will point to the temp dir on Windows and avoid touching real
config during LoginWithToken and Logout flows.
| func randomState() string { | ||
| b := make([]byte, 16) | ||
| _, _ = rand.Read(b) | ||
| return hex.EncodeToString(b) | ||
| } |
There was a problem hiding this comment.
The error from rand.Read is ignored.
crypto/rand.Read can fail in edge cases (like entropy pool exhaustion on some systems). If it fails, you'd get a zero or partially-filled buffer, which means your state parameter wouldn't be random at all. That could weaken the CSRF protection.
func randomState() string {
b := make([]byte, 16)
- _, _ = rand.Read(b)
+ if _, err := rand.Read(b); err != nil {
+ panic("failed to generate random state: " + err.Error())
+ }
return hex.EncodeToString(b)
}A panic is fine here because if you can't get random bytes, something is seriously wrong with the system and you shouldn't proceed with auth anyway.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/handler.go` around lines 179 - 183, The randomState function
currently ignores the error from rand.Read causing possible non-random state;
update randomState to check the error returned by crypto/rand.Read (in function
randomState), and if err != nil panic with a clear message including the error
so the process fails fast rather than returning a weak state string.
This reverts commit 9d9b5cf.
…)" This reverts commit bd5a15d.
1 participant
Closes #35
What
supermodel loginnow opens the dashboard in the browser, which auto-creates an API key and sends it back to a localhost callback. Zero copy-paste needed.Flow
How it works
dashboard.supermodeltools.com/cli-auth?port={port}&state={state}localhost:{port}/callback?key=...&state=...Fallbacks
supermodel login --token smsk_live_...Dashboard change needed
The
cli-authpage insupermodel-public-apihas references to "uncompact" that need to be changed to "supermodel" (already changed locally, will PR separately).Test plan
go test ./...— all 14 packages pass--tokenflag works for CISummary by CodeRabbit
New Features
Tests