Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions cmd/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,23 @@ import (
)

func init() {
rootCmd.AddCommand(&cobra.Command{
var token string

c := &cobra.Command{
Use: "login",
Short: "Authenticate with your Supermodel account",
Long: `Prompts for an API key and saves it to ~/.supermodel/config.yaml.
Long: `Opens your browser to create an API key and automatically saves it.

Get a key at https://supermodeltools.com/dashboard`,
For CI or headless environments, pass the key directly:
supermodel login --token smsk_live_...`,
RunE: func(cmd *cobra.Command, _ []string) error {
if token != "" {
return auth.LoginWithToken(token)
}
return auth.Login(cmd.Context())
Comment on lines 19 to 23
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, find and inspect the login.go file
find . -name "login.go" -type f | head -20

Repository: supermodeltools/cli

Length of output: 77


🏁 Script executed:

# Check the structure of cmd directory
git ls-files cmd/ | head -20

Repository: supermodeltools/cli

Length of output: 303


🏁 Script executed:

# Read the login.go file to see the actual code
cat -n cmd/login.go

Repository: 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 -20

Repository: 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 -20

Repository: 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 -100

Repository: supermodeltools/cli

Length of output: 3729


🏁 Script executed:

# Check if there's any token validation
rg "LoginWithToken|Login\(" --type go -A 5 | head -50

Repository: 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:


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.

Suggested change
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.

},
})
}

c.Flags().StringVar(&token, "token", "", "API key for non-interactive login (CI)")
rootCmd.AddCommand(c)
}
149 changes: 135 additions & 14 deletions internal/auth/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,41 +3,122 @@ package auth
import (
"bufio"
"context"
"crypto/rand"
"encoding/hex"
"fmt"
"net"
"net/http"
"os"
"os/exec"
"runtime"
"strings"
"syscall"
"time"

"golang.org/x/term"

"github.com/supermodeltools/cli/internal/config"
"github.com/supermodeltools/cli/internal/ui"
)

// Login prompts the user for an API key and saves it to the config file.
// Input is read without echo when a terminal is attached.
func Login(_ context.Context) error {
fmt.Println("Get your API key at https://supermodeltools.com/dashboard")
fmt.Print("Paste your API key: ")
const dashboardBase = "https://dashboard.supermodeltools.com"

key, err := readSecret()
// Login runs the browser-based login flow. Opens the dashboard to create an
// API key, receives it via localhost callback, validates, and saves it.
// Falls back to manual paste if the browser flow fails.
func Login(ctx context.Context) error {
cfg, err := config.Load()
if err != nil {
return fmt.Errorf("read input: %w", err)
return err
}
key = strings.TrimSpace(key)
if key == "" {
return fmt.Errorf("API key cannot be empty")

// Start localhost server on a random port.
listener, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
fmt.Fprintln(os.Stderr, "Could not start local server — falling back to manual login.")
return loginManual(cfg)
}
port := listener.Addr().(*net.TCPAddr).Port
state := randomState()

// Channel to receive the API key from the callback.
keyCh := make(chan string, 1)
errCh := make(chan error, 1)

mux := http.NewServeMux()
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>&#10003; Authenticated</h2><p style="color:#888">You can close this tab and return to your terminal.</p></div></body></html>`)
keyCh <- key
Comment on lines +49 to +61
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

})

srv := &http.Server{Handler: mux, ReadHeaderTimeout: 10 * time.Second} //nolint:gosec // localhost-only server
go func() {
if err := srv.Serve(listener); err != nil && err != http.ErrServerClosed {
errCh <- err
}
}()
defer srv.Close()

// Build the dashboard URL and open the browser.
authURL := fmt.Sprintf("%s/cli-auth?port=%d&state=%s", dashboardBase, port, state)
fmt.Println("Opening browser to log in...")
fmt.Printf("If the browser doesn't open, visit:\n %s\n\n", authURL)

if err := openBrowser(authURL); err != nil {
fmt.Fprintln(os.Stderr, "Could not open browser — falling back to manual login.")
srv.Close()
return loginManual(cfg)
}

// Wait for callback or timeout.
fmt.Print("Waiting for authentication...")
select {
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
Comment on lines +86 to +93
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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

case err := <-errCh:
fmt.Println()
return fmt.Errorf("local server error: %w", err)
case <-time.After(5 * time.Minute):
fmt.Println()
fmt.Fprintln(os.Stderr, "Timed out waiting for browser login — falling back to manual login.")
srv.Close()
return loginManual(cfg)
case <-ctx.Done():
fmt.Println()
return ctx.Err()
}
}

// LoginWithToken saves an API key directly (for CI/headless use).
func LoginWithToken(token string) error {
token = strings.TrimSpace(token)
if token == "" {
return fmt.Errorf("API key cannot be empty")
}
cfg, err := config.Load()
if err != nil {
return err
}
cfg.APIKey = key
cfg.APIKey = token
if err := cfg.Save(); err != nil {
return err
}

ui.Success("Authenticated — key saved to %s", config.Path())
return nil
}
Expand All @@ -60,18 +141,58 @@ func Logout(_ context.Context) error {
return nil
}

// loginManual is the fallback paste-based login.
func loginManual(cfg *config.Config) error {
fmt.Println("Get your API key at https://dashboard.supermodeltools.com/api-keys")
fmt.Print("Paste your API key: ")

key, err := readSecret()
if err != nil {
return fmt.Errorf("read input: %w", err)
}
key = strings.TrimSpace(key)
if key == "" {
return fmt.Errorf("API key cannot be empty")
}

cfg.APIKey = key
if err := cfg.Save(); err != nil {
return err
}
ui.Success("Authenticated — key saved to %s", config.Path())
return nil
}

func openBrowser(url string) error {
switch runtime.GOOS {
case "darwin":
return exec.Command("open", url).Start()
case "linux":
return exec.Command("xdg-open", url).Start()
case "windows":
return exec.Command("rundll32", "url.dll,FileProtocolHandler", url).Start()
default:
return fmt.Errorf("unsupported platform: %s", runtime.GOOS)
}
}

func randomState() string {
b := make([]byte, 16)
_, _ = rand.Read(b)
return hex.EncodeToString(b)
}
Comment on lines +179 to +183
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


// readSecret reads a line from stdin, suppressing echo when a TTY is attached.
func readSecret() (string, error) {
fd := int(syscall.Stdin) //nolint:unconvert // syscall.Stdin is uintptr on Windows
if term.IsTerminal(fd) {
b, err := term.ReadPassword(fd)
fmt.Println() // restore newline after hidden input
fmt.Println()
if err != nil {
return "", err
}
return string(b), nil
}
// Non-TTY (pipe, CI): read as plain text
scanner := bufio.NewScanner(os.Stdin)
if scanner.Scan() {
return scanner.Text(), nil
Expand Down
Loading
Loading