From a5ff7f996b3a077fdc7d8bca516292e2086377c8 Mon Sep 17 00:00:00 2001 From: zingzy Date: Sat, 18 Jul 2026 06:23:40 +0530 Subject: [PATCH 1/2] feat(auth): send PKCE S256 challenge and verifier in device flow The backend now requires code_challenge on /auth/device/login and code_verifier on /auth/device/token. Generate a 43-char base64url verifier per login and thread it through the token exchange. --- internal/api/auth.go | 7 +++--- internal/api/auth_test.go | 13 ++++++++++- internal/auth/device.go | 43 ++++++++++++++++++++++++++++-------- internal/auth/device_test.go | 33 +++++++++++++++++++++++++-- internal/cmd/auth.go | 4 ++-- 5 files changed, 83 insertions(+), 17 deletions(-) diff --git a/internal/api/auth.go b/internal/api/auth.go index 65d0157..5f7d732 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -20,10 +20,11 @@ type DeviceTokens struct { } // ExchangeDeviceCode trades a one-time device-auth code for a JWT pair. -// The code is the credential — no prior auth is required. -func (c *Client) ExchangeDeviceCode(ctx context.Context, code string) (*DeviceTokens, error) { +// The code is the credential — no prior auth is required. The verifier is +// the PKCE code verifier whose S256 challenge was sent on the login URL. +func (c *Client) ExchangeDeviceCode(ctx context.Context, code, verifier string) (*DeviceTokens, error) { var out DeviceTokens - if err := c.do(ctx, http.MethodPost, "/auth/device/token", nil, map[string]string{"code": code}, &out); err != nil { + if err := c.do(ctx, http.MethodPost, "/auth/device/token", nil, map[string]string{"code": code, "code_verifier": verifier}, &out); err != nil { return nil, err } return &out, nil diff --git a/internal/api/auth_test.go b/internal/api/auth_test.go index 54b6d95..f5b026e 100644 --- a/internal/api/auth_test.go +++ b/internal/api/auth_test.go @@ -2,6 +2,7 @@ package api import ( "context" + "encoding/json" "net/http" "net/http/httptest" "testing" @@ -12,12 +13,22 @@ func TestExchangeDeviceCode(t *testing.T) { if r.URL.Path != "/auth/device/token" || r.Method != http.MethodPost { t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) } + var body map[string]string + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Errorf("decode body: %v", err) + } + if body["code"] != "onetimecode" { + t.Errorf("code = %q, want onetimecode", body["code"]) + } + if body["code_verifier"] != "theverifier" { + t.Errorf("code_verifier = %q, want theverifier", body["code_verifier"]) + } w.Write([]byte(`{"access_token":"at","refresh_token":"rt","user":{"id":"1","email":"a@b.c","email_verified":true,"name":"A","plan":"free"}}`)) })) defer srv.Close() c := New(srv.URL, newTestStore(t, nil)) - tok, err := c.ExchangeDeviceCode(context.Background(), "onetimecode") + tok, err := c.ExchangeDeviceCode(context.Background(), "onetimecode", "theverifier") if err != nil { t.Fatal(err) } diff --git a/internal/auth/device.go b/internal/auth/device.go index b0b06df..ce74a48 100644 --- a/internal/auth/device.go +++ b/internal/auth/device.go @@ -3,6 +3,8 @@ package auth import ( "context" "crypto/rand" + "crypto/sha256" + "encoding/base64" "encoding/hex" "errors" "fmt" @@ -34,16 +36,21 @@ type DeviceFlow struct { } // Run blocks until the consent callback delivers a code, the context -// expires, or the callback is invalid. Returns the one-time code. -func (f *DeviceFlow) Run(ctx context.Context) (string, error) { +// expires, or the callback is invalid. Returns the one-time code and the +// PKCE verifier that must accompany it in the token exchange. +func (f *DeviceFlow) Run(ctx context.Context) (string, string, error) { state, err := randomState() if err != nil { - return "", err + return "", "", err + } + verifier, err := codeVerifier() + if err != nil { + return "", "", err } ln, err := net.Listen("tcp", CallbackAddr) if err != nil { - return "", fmt.Errorf("cannot listen on %s (is another spoo login running?): %w", CallbackAddr, err) + return "", "", fmt.Errorf("cannot listen on %s (is another spoo login running?): %w", CallbackAddr, err) } codeCh := make(chan string, 1) @@ -72,9 +79,10 @@ func (f *DeviceFlow) Run(ctx context.Context) (string, error) { go srv.Serve(ln) defer srv.Shutdown(context.Background()) - authURL := fmt.Sprintf("%s/auth/device/login?app_id=%s&redirect_uri=%s&state=%s", + authURL := fmt.Sprintf("%s/auth/device/login?app_id=%s&redirect_uri=%s&state=%s&code_challenge=%s&code_challenge_method=S256", f.APIBase, AppID, - url.QueryEscape("http://"+CallbackAddr+CallbackPath), state) + url.QueryEscape("http://"+CallbackAddr+CallbackPath), state, + codeChallengeS256(verifier)) fmt.Fprintln(f.Out, "Opening your browser to authorize spoo CLI…") fmt.Fprintf(f.Out, "If it doesn't open automatically, visit:\n\n %s\n\n", authURL) @@ -84,11 +92,11 @@ func (f *DeviceFlow) Run(ctx context.Context) (string, error) { select { case code := <-codeCh: - return code, nil + return code, verifier, nil case err := <-errCh: - return "", err + return "", "", err case <-ctx.Done(): - return "", fmt.Errorf("login timed out: %w", ctx.Err()) + return "", "", fmt.Errorf("login timed out: %w", ctx.Err()) } } @@ -99,3 +107,20 @@ func randomState() (string, error) { } return hex.EncodeToString(b), nil } + +// codeVerifier returns a PKCE code verifier: 32 random bytes encoded as +// unpadded base64url, always 43 characters (RFC 7636 §4.1). +func codeVerifier() (string, error) { + b := make([]byte, 32) + if _, err := rand.Read(b); err != nil { + return "", err + } + return base64.RawURLEncoding.EncodeToString(b), nil +} + +// codeChallengeS256 derives the S256 challenge for a verifier: +// BASE64URL(SHA256(verifier)) without padding (RFC 7636 §4.2). +func codeChallengeS256(verifier string) string { + sum := sha256.Sum256([]byte(verifier)) + return base64.RawURLEncoding.EncodeToString(sum[:]) +} diff --git a/internal/auth/device_test.go b/internal/auth/device_test.go index 684537b..3a1890f 100644 --- a/internal/auth/device_test.go +++ b/internal/auth/device_test.go @@ -6,14 +6,18 @@ import ( "io" "net/http" "net/url" + "regexp" "strings" "testing" "time" ) +var challengeRe = regexp.MustCompile(`^[A-Za-z0-9_-]{43}$`) + // Simulates the browser leg: the flow opens a URL; we parse state and // redirect_uri out of it and hit the loopback callback like spoo.me would. func TestDeviceFlowReturnsCode(t *testing.T) { + challengeCh := make(chan string, 1) flow := &DeviceFlow{ APIBase: "https://spoo.example", Out: io.Discard, @@ -32,6 +36,14 @@ func TestDeviceFlowReturnsCode(t *testing.T) { if !strings.HasPrefix(cb, "http://127.0.0.1:53682/callback") { t.Errorf("redirect_uri = %q", cb) } + if q.Get("code_challenge_method") != "S256" { + t.Errorf("code_challenge_method = %q, want S256", q.Get("code_challenge_method")) + } + challenge := q.Get("code_challenge") + if !challengeRe.MatchString(challenge) { + t.Errorf("code_challenge = %q, want 43 base64url chars", challenge) + } + challengeCh <- challenge time.Sleep(50 * time.Millisecond) // let the server start resp, err := http.Get(fmt.Sprintf("%s?code=thecode&state=%s", cb, q.Get("state"))) if err != nil { @@ -46,13 +58,30 @@ func TestDeviceFlowReturnsCode(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - code, err := flow.Run(ctx) + code, verifier, err := flow.Run(ctx) if err != nil { t.Fatal(err) } if code != "thecode" { t.Fatalf("code = %q, want thecode", code) } + if len(verifier) != 43 { + t.Fatalf("verifier length = %d, want 43", len(verifier)) + } + if got := codeChallengeS256(verifier); got != <-challengeCh { + t.Fatalf("code_challenge on auth URL does not match S256(verifier): %q", got) + } +} + +// RFC 7636 Appendix B test vector. +func TestCodeChallengeS256Vector(t *testing.T) { + const ( + verifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" + want = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" + ) + if got := codeChallengeS256(verifier); got != want { + t.Fatalf("codeChallengeS256 = %q, want %q", got, want) + } } func TestDeviceFlowRejectsStateMismatch(t *testing.T) { @@ -72,7 +101,7 @@ func TestDeviceFlowRejectsStateMismatch(t *testing.T) { } ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - if _, err := flow.Run(ctx); err == nil { + if _, _, err := flow.Run(ctx); err == nil { t.Fatal("expected state-mismatch error, got nil") } } diff --git a/internal/cmd/auth.go b/internal/cmd/auth.go index 2ae6690..269448b 100644 --- a/internal/cmd/auth.go +++ b/internal/cmd/auth.go @@ -61,11 +61,11 @@ func loginWithBrowser(cmd *cobra.Command, d *deps) error { ctx, cancel := context.WithTimeout(cmd.Context(), 5*time.Minute) defer cancel() - code, err := flow.Run(ctx) + code, verifier, err := flow.Run(ctx) if err != nil { return err } - tokens, err := d.client.ExchangeDeviceCode(ctx, code) + tokens, err := d.client.ExchangeDeviceCode(ctx, code, verifier) if err != nil { return fmt.Errorf("token exchange failed: %w", err) } From f9968518a9784264afe7c713e35c0ec0e61ddbe6 Mon Sep 17 00:00:00 2001 From: zingzy Date: Sat, 18 Jul 2026 14:18:29 +0530 Subject: [PATCH 2/2] feat(keys): remove 'keys create' subcommand Key creation now requires an interactive session on spoo.me, so an app token cannot mint keys. Rather than ship a subcommand that always fails, drop it: 'keys list' and 'keys revoke' stay, and the empty-state hint points to the dashboard. Removes the dead CreateKey client method and --scopes completion. --- internal/api/keys.go | 18 ---------- internal/api/keys_test.go | 23 ------------ internal/cmd/completion.go | 29 --------------- internal/cmd/completion_test.go | 21 ----------- internal/cmd/keys.go | 63 ++------------------------------- internal/cmd/keys_test.go | 30 +++------------- 6 files changed, 7 insertions(+), 177 deletions(-) diff --git a/internal/api/keys.go b/internal/api/keys.go index 693f0dc..a0acf79 100644 --- a/internal/api/keys.go +++ b/internal/api/keys.go @@ -16,24 +16,6 @@ type APIKey struct { ExpiresAt int64 `json:"expires_at"` Revoked bool `json:"revoked"` TokenPrefix string `json:"token_prefix"` - Token string `json:"token,omitempty"` // full token, present only on create -} - -type CreateKeyRequest struct { - Name string `json:"name"` - Description string `json:"description,omitempty"` - Scopes []string `json:"scopes"` - ExpiresAt string `json:"expires_at,omitempty"` // ISO 8601 or epoch seconds -} - -// CreateKey mints a new API key. Requires a device-flow (JWT) session; -// the backend refuses key creation authenticated by another API key. -func (c *Client) CreateKey(ctx context.Context, req CreateKeyRequest) (*APIKey, error) { - var out APIKey - if err := c.do(ctx, http.MethodPost, "/api/v1/keys", nil, req, &out); err != nil { - return nil, err - } - return &out, nil } func (c *Client) ListKeys(ctx context.Context) ([]APIKey, error) { diff --git a/internal/api/keys_test.go b/internal/api/keys_test.go index 161b558..40b3822 100644 --- a/internal/api/keys_test.go +++ b/internal/api/keys_test.go @@ -2,34 +2,11 @@ package api import ( "context" - "encoding/json" "net/http" "net/http/httptest" "testing" ) -func TestCreateKeyReturnsTokenOnce(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - var req CreateKeyRequest - json.NewDecoder(r.Body).Decode(&req) - if req.Name != "CI" || len(req.Scopes) != 1 || req.Scopes[0] != "shorten:create" { - t.Errorf("unexpected body: %+v", req) - } - w.WriteHeader(http.StatusCreated) - w.Write([]byte(`{"id":"k1","name":"CI","scopes":["shorten:create"],"token_prefix":"abc12345","token":"spoo_secret"}`)) - })) - defer srv.Close() - - c := New(srv.URL, newTestStore(t, nil)) - key, err := c.CreateKey(context.Background(), CreateKeyRequest{Name: "CI", Scopes: []string{"shorten:create"}}) - if err != nil { - t.Fatal(err) - } - if key.Token != "spoo_secret" { - t.Fatalf("token = %q", key.Token) - } -} - func TestListKeys(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte(`{"keys":[{"id":"k1","name":"CI","scopes":["shorten:create"],"token_prefix":"abc12345","revoked":false}]}`)) diff --git a/internal/cmd/completion.go b/internal/cmd/completion.go index 27029f6..ba7a402 100644 --- a/internal/cmd/completion.go +++ b/internal/cmd/completion.go @@ -102,35 +102,6 @@ func completeKeyID(cmd *cobra.Command, args []string, toComplete string) ([]stri return out, cobra.ShellCompDirectiveNoFileComp } -// apiScopes are the permission scopes accepted by `keys create --scopes`, -// mirroring the set documented in that command's help. -var apiScopes = []string{ - "shorten:create", "urls:read", "urls:manage", "stats:read", - "domains:read", "domains:manage", "admin:all", -} - -// completeScopes completes the comma-separated --scopes value: it keeps the -// scopes already typed and offers the rest. cobra hands the whole value as -// toComplete for slice flags, so we split on the last comma ourselves and -// use NoSpace so the user can keep appending. -func completeScopes(_ *cobra.Command, _ []string, toComplete string) ([]string, cobra.ShellCompDirective) { - prefix, cur := "", toComplete - if i := strings.LastIndex(toComplete, ","); i >= 0 { - prefix, cur = toComplete[:i+1], toComplete[i+1:] - } - chosen := make(map[string]bool) - for _, s := range strings.Split(prefix, ",") { - chosen[s] = true - } - var out []string - for _, s := range apiScopes { - if !chosen[s] && strings.HasPrefix(s, cur) { - out = append(out, prefix+s) - } - } - return out, cobra.ShellCompDirectiveNoSpace | cobra.ShellCompDirectiveNoFileComp -} - // completeDomain completes --domain with the distinct custom domains that // already appear on your links. There's no domains-list endpoint, so this // is the practical best-effort source (a domain you've never used yet diff --git a/internal/cmd/completion_test.go b/internal/cmd/completion_test.go index 435ceb9..33e131e 100644 --- a/internal/cmd/completion_test.go +++ b/internal/cmd/completion_test.go @@ -110,27 +110,6 @@ func TestCompleteFixedFlags(t *testing.T) { } } -func TestCompleteScopesCommaAware(t *testing.T) { - pointDepsAt(t, "http://unused.invalid") // fixed list; no API call - - // bare: offers the full scope set - out := complete(t, "keys", "create", "--scopes", "") - for _, want := range []string{"shorten:create", "stats:read", "admin:all"} { - if !strings.Contains(out, want) { - t.Fatalf("--scopes missing %q:\n%s", want, out) - } - } - // mid-list: keeps the typed prefix, drops the already-chosen scope, - // and narrows by the partial after the last comma - out = complete(t, "keys", "create", "--scopes", "shorten:create,sta") - if !strings.Contains(out, "shorten:create,stats:read") { - t.Fatalf("comma-aware completion should append stats:read:\n%s", out) - } - if strings.Contains(out, "shorten:create,shorten:create") { - t.Fatalf("already-chosen scope should not be re-offered:\n%s", out) - } -} - func TestCompleteDomainFromLinks(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Write([]byte(`{"items":[ diff --git a/internal/cmd/keys.go b/internal/cmd/keys.go index 9922008..cc0f707 100644 --- a/internal/cmd/keys.go +++ b/internal/cmd/keys.go @@ -9,7 +9,6 @@ import ( "github.com/spf13/cobra" - "github.com/spoo-me/spoo-cli/internal/api" "github.com/spoo-me/spoo-cli/internal/ui" ) @@ -21,7 +20,7 @@ func newKeysCmd() *cobra.Command { return runKeysList(cmd) }, } - cmd.AddCommand(newKeysCreateCmd(), newKeysRevokeCmd()) + cmd.AddCommand(newKeysRevokeCmd()) return cmd } @@ -40,7 +39,7 @@ func runKeysList(cmd *cobra.Command) error { return enc.Encode(keys) } if len(keys) == 0 { - fmt.Fprintln(prettyOut(cmd), ui.Dim.Render("no API keys — create one with `spoo keys create --name my-key`")) + fmt.Fprintln(prettyOut(cmd), ui.Dim.Render("no API keys — create one at https://spoo.me/dashboard/keys")) return nil } // cells stay unstyled: ANSI codes would skew tabwriter's column math @@ -61,64 +60,6 @@ func runKeysList(cmd *cobra.Command) error { return w.Flush() } -func newKeysCreateCmd() *cobra.Command { - var ( - name, description, expires string - scopes []string - ) - cmd := &cobra.Command{ - Use: "create", - Short: "Create an API key", - Long: `Create an API key. - -Scopes: shorten:create, urls:read, urls:manage, stats:read, -domains:read, domains:manage, admin:all. - -Requires a browser login (spoo auth login) — the API refuses key -creation authenticated by another API key. The token is shown ONCE.`, - Example: ` spoo keys create --name ci --scopes shorten:create - spoo keys create --name bot --scopes shorten:create,stats:read --expires 720h`, - RunE: func(cmd *cobra.Command, args []string) error { - if name == "" { - return fmt.Errorf("--name is required") - } - if len(scopes) == 0 { - return fmt.Errorf("--scopes is required (e.g. --scopes shorten:create,stats:read)") - } - d, err := newDeps() - if err != nil { - return err - } - exp, err := parseExpiry(expires, timeNow()) - if err != nil { - return err - } - key, err := d.client.CreateKey(cmd.Context(), api.CreateKeyRequest{ - Name: name, Description: description, Scopes: scopes, ExpiresAt: exp, - }) - if err != nil { - return err - } - if asJSON, _ := cmd.Flags().GetBool("json"); asJSON { - enc := json.NewEncoder(cmd.OutOrStdout()) - enc.SetIndent("", " ") - return enc.Encode(key) - } - body := ui.OK.Render("✓ key created: ") + key.Name + "\n\n" + - ui.Title.Render(key.Token) + "\n\n" + - ui.Err.Render("save it now — it cannot be shown again") - fmt.Fprintln(prettyOut(cmd), ui.Box.Render(body)) - return nil - }, - } - cmd.Flags().StringVar(&name, "name", "", "key name (required)") - cmd.Flags().StringVar(&description, "description", "", "what this key is for") - cmd.Flags().StringSliceVar(&scopes, "scopes", nil, "comma-separated scopes (required)") - cmd.Flags().StringVar(&expires, "expires", "", "expiry: ISO 8601, epoch, or duration like 720h") - flagComp(cmd, "scopes", completeScopes) - return cmd -} - func newKeysRevokeCmd() *cobra.Command { var hard bool cmd := &cobra.Command{ diff --git a/internal/cmd/keys_test.go b/internal/cmd/keys_test.go index 40d073a..74fee38 100644 --- a/internal/cmd/keys_test.go +++ b/internal/cmd/keys_test.go @@ -8,35 +8,15 @@ import ( "testing" ) -func TestKeysCreateShowsTokenOnce(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusCreated) - w.Write([]byte(`{"id":"k1","name":"ci","scopes":["shorten:create"],"token_prefix":"abc12345","token":"spoo_secret_token"}`)) - })) - defer srv.Close() - pointDepsAt(t, srv.URL) - - root := NewRootCmd() - var out bytes.Buffer - root.SetOut(&out) - root.SetErr(&out) - root.SetArgs([]string{"keys", "create", "--name", "ci", "--scopes", "shorten:create"}) - if err := root.Execute(); err != nil { - t.Fatal(err) - } - if !strings.Contains(out.String(), "spoo_secret_token") || !strings.Contains(out.String(), "cannot be shown again") { - t.Fatalf("unexpected output:\n%s", out.String()) - } -} - -func TestKeysCreateRequiresNameAndScopes(t *testing.T) { +func TestKeysCreateCommandRemoved(t *testing.T) { pointDepsAt(t, "http://unused.invalid") root := NewRootCmd() root.SetOut(new(bytes.Buffer)) root.SetErr(new(bytes.Buffer)) - root.SetArgs([]string{"keys", "create"}) - if err := root.Execute(); err == nil || !strings.Contains(err.Error(), "--name") { - t.Fatalf("err = %v, want --name guard", err) + root.SetArgs([]string{"keys", "create", "--name", "ci"}) + // Creation is dashboard-only; the subcommand must not exist. + if err := root.Execute(); err == nil { + t.Fatal("expected error for removed `keys create` subcommand") } }