From aeb9c7b510683adce278af3b9024c25dcfb04fec Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 17:30:01 +0200 Subject: [PATCH 01/14] feat(auth): answer the two-factor challenge on sign-in ExchangeToken now posts mfa=1 with the link, telling the platform this build can answer a challenge. For an account with a confirmed factor the platform answers with the __Host-nan_mfa_pending cookie and reason=mfa_required; ExchangeToken returns that as *MFARequiredError, and Complete posts the code (6 digits) or recovery code (16 characters, any case, hyphens optional) to /api/auth/mfa/challenge as JSON, with the cookie and without Origin or Sec-Fetch-Site, and returns the session. Every refusal in the API contract maps to a sentence; Retryable says whether the same challenge can take another answer (wrong, replayed or unreadable code, and mfa_totp_suspended for a recovery code). The challenge cookie stays in memory: the error's Format never prints it, and an error code is only echoed when it has the platform's shape. The sign-in calls derive from one auth.Origin, so tests point the whole flow at a fake and a QA build can point it at dev with -ldflags -X. internal/auth/authtest is that fake, written from the contract. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- internal/auth/auth.go | 52 +++- internal/auth/auth_test.go | 6 +- internal/auth/authtest/platform.go | 317 +++++++++++++++++++++++ internal/auth/mfa.go | 314 +++++++++++++++++++++++ internal/auth/mfa_test.go | 395 +++++++++++++++++++++++++++++ 5 files changed, 1072 insertions(+), 12 deletions(-) create mode 100644 internal/auth/authtest/platform.go create mode 100644 internal/auth/mfa.go create mode 100644 internal/auth/mfa_test.go diff --git a/internal/auth/auth.go b/internal/auth/auth.go index b4b61e7..25fb1e1 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -20,8 +20,7 @@ import ( ) const ( - loginRequestURL = "https://cloud-api.nan.builders/api/auth/login/request" - sessionCookie = "nan_session" + sessionCookie = "nan_session" // The domain the platform sends its sign-in links from. Subdomains count: // the link lands on the web app, the API lives next door. linkDomain = "nan.builders" @@ -34,9 +33,19 @@ const ( LinkValidity = "15 minutes" ) -// loginVerifyURL is a variable so a test can point the exchange at a server -// of its own. -var loginVerifyURL = "https://cloud-api.nan.builders/api/auth/login/verify" +// Origin is where every call of the sign-in goes. A variable and not a +// constant for two readers only: the tests, which point the flow at a server +// of their own, and a build for QA, which points it at the dev platform +// without a flag or an environment variable that a released binary would +// carry: +// +// go build -ldflags "-X github.com/nxssie/nan-cli/internal/auth.Origin=https://cloud-api-dev.nan.builders" +// +// Nothing else sets it. +var Origin = "https://cloud-api.nan.builders" + +func loginRequestURL() string { return Origin + "/api/auth/login/request" } +func loginVerifyURL() string { return Origin + "/api/auth/login/verify" } // clientKind is what this CLI declares on the verify POST, so the platform // records the session it hands back as a CLI session. A CLI session can @@ -61,7 +70,7 @@ func RequestSignInLink(email string) error { return err } client := &http.Client{Timeout: requestTimeout} - resp, err := client.Post(loginRequestURL, "application/json", bytes.NewReader(body)) + resp, err := client.Post(loginRequestURL(), "application/json", bytes.NewReader(body)) if err != nil { return fmt.Errorf("could not reach nan.builders: %w", err) } @@ -119,6 +128,10 @@ func isLinkHost(host string) bool { // cookie back. This does the same POST and keeps the cookie instead of // following the redirect, which is the whole reason the old flow had to send // people into DevTools. +// +// An account with two-factor authentication gets no session from the link +// alone: the answer is a challenge, returned here as *MFARequiredError, and +// the session comes from answering it (MFARequiredError.Complete). func ExchangeToken(token string) (string, error) { client := &http.Client{ Timeout: requestTimeout, @@ -126,23 +139,39 @@ func ExchangeToken(token string) (string, error) { return http.ErrUseLastResponse }, } - form := url.Values{"token": {token}, "client": {clientKind}} - resp, err := client.PostForm(loginVerifyURL, form) + // mfa=1 says this build can answer a two-factor challenge. Without it the + // platform refuses an account that has a factor outright, with a reason + // that tells the member to update - which is what every build before this + // one gets, since none of them could ask for the code. + form := url.Values{"token": {token}, "client": {clientKind}, "mfa": {"1"}} + resp, err := client.PostForm(loginVerifyURL(), form) if err != nil { return "", fmt.Errorf("could not reach nan.builders: %w", err) } defer resp.Body.Close() + var pending string for _, c := range resp.Cookies() { if c.Name == sessionCookie && c.Value != "" { return c.Value, nil } + if c.Name == mfaPendingCookie && c.Value != "" { + pending = c.Value + } } // A spent or expired link redirects to the platform's access-denied page // with the reason in the query, which is more useful than the status code. + // A link that worked for an account with a second factor redirects to the + // page that asks for the code, with the challenge in a cookie. if location, err := resp.Location(); err == nil { - if reason := location.Query().Get("reason"); reason != "" { + reason := location.Query().Get("reason") + switch { + case reason == mfaRequiredReason && pending != "": + return "", &MFARequiredError{pending: pending} + case reason == mfaRequiredReason: + return "", errors.New("nan.builders asked for a two-factor code but sent no sign-in to answer it — ask for a new link and sign in again") + case reason != "": return "", errors.New(linkReasonMessage(reason)) } } @@ -159,6 +188,11 @@ var linkReasons = map[string]string{ "invalid_link": "that link expired or was already used — a link works once and " + "expires " + LinkValidity + " after it is sent, so ask for another one", "missing_token": "that link is incomplete — copy the whole link out of the most recent email", + // What a build from before two-factor sign-in gets for an account that + // has it. This build says it can answer the challenge, so it should never + // be told this; if it is, the sentence is still the right one. + "mfa_enabled_update_nan_cli": "this account uses two-factor authentication, and this nan-cli cannot ask for the code — " + + "update it with the installer (https://github.com/helmcode/nan-cli#install) and ask for a new link", } func linkReasonMessage(reason string) string { diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 3edb67b..8cb3c8c 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -109,9 +109,9 @@ func TestExchangeTokenDeclaresTheCLI(t *testing.T) { http.Redirect(w, r, "/dashboard", http.StatusSeeOther) })) defer srv.Close() - old := loginVerifyURL - loginVerifyURL = srv.URL - defer func() { loginVerifyURL = old }() + old := Origin + Origin = srv.URL + defer func() { Origin = old }() got, err := ExchangeToken("tok") if err != nil || got != "sess" { diff --git a/internal/auth/authtest/platform.go b/internal/auth/authtest/platform.go new file mode 100644 index 0000000..2659bec --- /dev/null +++ b/internal/auth/authtest/platform.go @@ -0,0 +1,317 @@ +// Package authtest is a fake nan.builders sign-in for tests: the link +// exchange and the two-factor challenge, answering the way the platform's API +// contract says they answer (nan-cloud-api openspec/changes/mfa/api-contract.md, +// "POST /api/auth/login/verify", "POST /api/auth/mfa/challenge" and "CLI +// contract"). +// +// It is imported by tests only, so it never reaches the binary. +package authtest + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync" + "testing" + "time" +) + +const ( + // FrontendURL is the UI the platform redirects to, and the only origin + // the challenge route accepts. + FrontendURL = "https://cloud.nan.builders" + + // PendingCookie is the challenge cookie's name. + PendingCookie = "__Host-nan_mfa_pending" + + // Values a test can look for: in what was sent, and in what must never + // be written anywhere. + LinkToken = "link-token-for-a-test" + Pending = "fake-pending-challenge-0123456789abcdefghij" // 43 base64url characters, like the real one + Session = "fake-session-from-the-platform" + Code = "246810" +) + +// Platform is the fake. Set the exported fields before the first request. +type Platform struct { + *httptest.Server + + // Factor: the account has a confirmed authenticator. Without it the link + // alone signs in, exactly as before two-factor sign-in existed. + Factor bool + // RecoveryCodes the account has not used yet, in canonical form + // (xxxx-xxxx-xxxx-xxxx). + RecoveryCodes []string + // Next, when set, answers the next challenge POST instead of checking + // it, and is then cleared: a refusal the test wants to see handled. + Next *Refusal + + mu sync.Mutex + // What came in, for the test to look at. + verifies []url.Values + challenges []Request + + spent bool + open bool // a challenge is waiting for its code + ended string // why the last challenge ended: used, exhausted, expired + attempts int // left on the open challenge + codeUsed bool // Code's step has been spent (anti-replay) + lockedUntil time.Time +} + +// Refusal is an answer the test forces on the challenge route. +type Refusal struct { + Status int + Body string + Header map[string]string +} + +// Request is a challenge POST as it arrived. +type Request struct { + Header http.Header + Body string + // Cookie is the challenge cookie it carried, "" without one. + Cookie string +} + +// New starts the fake. It stops when the test ends. +func New(t testing.TB) *Platform { + t.Helper() + p := &Platform{} + mux := http.NewServeMux() + mux.HandleFunc("POST /api/auth/login/request", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusAccepted) + }) + mux.HandleFunc("POST /api/auth/login/verify", p.verify) + mux.HandleFunc("POST /api/auth/mfa/challenge", p.challenge) + p.Server = httptest.NewServer(mux) + t.Cleanup(p.Close) + return p +} + +// Verifies are the forms the verify route received. +func (p *Platform) Verifies() []url.Values { + p.mu.Lock() + defer p.mu.Unlock() + return append([]url.Values(nil), p.verifies...) +} + +// Challenges are the challenge POSTs the platform received. +func (p *Platform) Challenges() []Request { + p.mu.Lock() + defer p.mu.Unlock() + return append([]Request(nil), p.challenges...) +} + +// NewLink lets one more link through, as a new email would. +func (p *Platform) NewLink() { + p.mu.Lock() + defer p.mu.Unlock() + p.spent = false +} + +// Expire ends the open challenge the way ten minutes would. +func (p *Platform) Expire() { + p.mu.Lock() + defer p.mu.Unlock() + if p.open { + p.open, p.ended = false, "expired" + } +} + +// Lock pauses the account's two-factor sign-in until the given time. +func (p *Platform) Lock(until time.Time) { + p.mu.Lock() + defer p.mu.Unlock() + p.lockedUntil = until +} + +func (p *Platform) verify(w http.ResponseWriter, r *http.Request) { + _ = r.ParseForm() + p.mu.Lock() + defer p.mu.Unlock() + p.verifies = append(p.verifies, r.PostForm) + + // The link is consumed in every row of the contract's table. + if r.PostForm.Get("token") != LinkToken || p.spent { + http.Redirect(w, r, FrontendURL+"/access-denied?reason=invalid_link", http.StatusFound) + return + } + p.spent = true + + switch { + case !p.Factor: + http.SetCookie(w, &http.Cookie{Name: "nan_session", Value: Session, Path: "/", Domain: ".nan.builders", Secure: true, HttpOnly: true}) + http.Redirect(w, r, FrontendURL+"/", http.StatusFound) + case r.PostForm.Get("client") == "cli" && r.PostForm.Get("mfa") != "1": + http.Redirect(w, r, FrontendURL+"/access-denied?reason=mfa_enabled_update_nan_cli", http.StatusFound) + default: + // A new challenge replaces any earlier one (D8). + p.open, p.ended, p.attempts = true, "", 5 + http.SetCookie(w, &http.Cookie{Name: PendingCookie, Value: Pending, Path: "/", MaxAge: 600, Secure: true, HttpOnly: true, SameSite: http.SameSiteStrictMode}) + http.Redirect(w, r, FrontendURL+"/mfa?reason=mfa_required", http.StatusFound) + } +} + +func (p *Platform) challenge(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(io.LimitReader(r.Body, 1<<16)) + cookie := "" + if c, err := r.Cookie(PendingCookie); err == nil { + cookie = c.Value + } + p.mu.Lock() + defer p.mu.Unlock() + p.challenges = append(p.challenges, Request{Header: r.Header.Clone(), Body: string(body), Cookie: cookie}) + w.Header().Set("Cache-Control", "no-store") + + // The FRONTEND_URL origin only; without an Origin, refused when the + // browser says it came from another site, through otherwise (the CLI). + if o := r.Header.Get("Origin"); o != "" && o != FrontendURL { + writeJSON(w, http.StatusForbidden, map[string]any{"error": "forbidden_origin"}) + return + } + if s := r.Header.Get("Sec-Fetch-Site"); r.Header.Get("Origin") == "" && (s == "cross-site" || s == "same-site") { + writeJSON(w, http.StatusForbidden, map[string]any{"error": "forbidden_origin"}) + return + } + if !strings.HasPrefix(r.Header.Get("Content-Type"), "application/json") { + writeJSON(w, http.StatusBadRequest, refusal("invalid_request")) + return + } + + if p.Next != nil { + next := p.Next + p.Next = nil + for k, v := range next.Header { + w.Header().Set(k, v) + } + w.WriteHeader(next.Status) + _, _ = io.WriteString(w, next.Body) + return + } + + if cookie != Pending { + expire(w) + writeJSON(w, http.StatusForbidden, refusal("mfa_challenge_missing")) + return + } + if !p.open { + expire(w) + code := "mfa_challenge_" + p.ended + if p.ended == "" { + code = "mfa_challenge_missing" + } + writeJSON(w, http.StatusForbidden, refusal(code)) + return + } + if time.Now().Before(p.lockedUntil) { + p.locked(w) + return + } + + var answer map[string]string + dec := json.NewDecoder(strings.NewReader(string(body))) + dec.DisallowUnknownFields() + if err := dec.Decode(&answer); err != nil || len(answer) != 1 { + writeJSON(w, http.StatusBadRequest, refusal("invalid_request")) + return + } + + if code, ok := answer["code"]; ok { + code = strings.ReplaceAll(code, " ", "") + if len(code) != 6 { + writeJSON(w, http.StatusBadRequest, refusal("invalid_request")) + return + } + switch { + case code == Code && !p.codeUsed: + p.codeUsed = true + p.pass(w, nil) + case code == Code: + p.fail(w, "mfa_code_already_used") + default: + p.fail(w, "mfa_code_invalid") + } + return + } + if rc, ok := answer["recovery_code"]; ok { + canonical, ok := canonicalRecovery(rc) + if !ok { + writeJSON(w, http.StatusBadRequest, refusal("invalid_request")) + return + } + for i, have := range p.RecoveryCodes { + if have == canonical { + p.RecoveryCodes = append(p.RecoveryCodes[:i:i], p.RecoveryCodes[i+1:]...) + left := len(p.RecoveryCodes) + p.pass(w, &left) + return + } + } + p.fail(w, "mfa_recovery_code_invalid") + return + } + writeJSON(w, http.StatusBadRequest, refusal("invalid_request")) +} + +func (p *Platform) pass(w http.ResponseWriter, recoveryLeft *int) { + p.open, p.ended = false, "used" + expire(w) + http.SetCookie(w, &http.Cookie{Name: "nan_session", Value: Session, Path: "/", Domain: ".nan.builders", Secure: true, HttpOnly: true}) + writeJSON(w, http.StatusOK, map[string]any{"redirect": "/", "recovery_codes_remaining": recoveryLeft}) +} + +func (p *Platform) fail(w http.ResponseWriter, code string) { + p.attempts-- + if p.attempts <= 0 { + p.open, p.ended = false, "exhausted" + expire(w) + writeJSON(w, http.StatusForbidden, refusal("mfa_challenge_exhausted")) + return + } + body := refusal(code) + body["attempts_remaining"] = p.attempts + writeJSON(w, http.StatusBadRequest, body) +} + +func (p *Platform) locked(w http.ResponseWriter) { + secs := int(time.Until(p.lockedUntil).Seconds()) + 1 + w.Header().Set("Retry-After", fmt.Sprint(secs)) + body := refusal("mfa_locked") + body["locked_until"] = p.lockedUntil.UTC().Format(time.RFC3339) + body["retry_after"] = secs + writeJSON(w, http.StatusTooManyRequests, body) +} + +// canonicalRecovery is the platform's reading of a recovery code: sixteen +// base32 characters in any case, with or without hyphens and spaces. +func canonicalRecovery(s string) (string, bool) { + s = strings.ToLower(strings.NewReplacer("-", "", " ", "").Replace(s)) + if len(s) != 16 { + return "", false + } + for _, r := range s { + if !(r >= 'a' && r <= 'z' || r >= '2' && r <= '7') { + return "", false + } + } + return s[0:4] + "-" + s[4:8] + "-" + s[8:12] + "-" + s[12:16], true +} + +func refusal(code string) map[string]any { + return map[string]any{"error": code, "message": "A sentence the client must not branch on."} +} + +func expire(w http.ResponseWriter) { + http.SetCookie(w, &http.Cookie{Name: PendingCookie, Value: "", Path: "/", MaxAge: -1, Secure: true, HttpOnly: true, SameSite: http.SameSiteStrictMode}) +} + +func writeJSON(w http.ResponseWriter, status int, v any) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(v) +} diff --git a/internal/auth/mfa.go b/internal/auth/mfa.go new file mode 100644 index 0000000..fb0c418 --- /dev/null +++ b/internal/auth/mfa.go @@ -0,0 +1,314 @@ +package auth + +// Two-factor sign-in. +// +// A member who has turned on two-factor authentication does not get a session +// from the link alone. The verify POST answers with a challenge instead - a +// cookie that names it, and a redirect whose reason says so - and the session +// comes back only once a code from the member's authenticator app, or one of +// their recovery codes, is posted against that challenge. +// +// The challenge cookie is a credential for the next ten minutes, so it lives +// in memory and nowhere else: not in session.json, not in an error message, +// not in a log. A process that exits halfway leaves it to expire on the +// platform, and the member asks for a new link. + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "strconv" + "strings" + "time" +) + +const ( + mfaPendingCookie = "__Host-nan_mfa_pending" + mfaRequiredReason = "mfa_required" + + // What ends every message about a sign-in that cannot go on. The link was + // spent when the challenge was opened, so a new one is the only way back. + newLinkHint = "ask for a new link and sign in again" +) + +func mfaChallengeURL() string { return Origin + "/api/auth/mfa/challenge" } + +// MFARequiredError is what ExchangeToken returns when the link was good and +// the account asks for a second factor. Answer it with Complete. +type MFARequiredError struct { + pending string +} + +func (e *MFARequiredError) Error() string { + return "this account uses two-factor authentication: enter the 6-digit code from your authenticator app, or one of your recovery codes" +} + +// Format prints the message whatever the verb. Without it %#v - or %d, or +// anything else fmt does not route to Error - prints the struct, and the +// struct is the challenge. +func (e *MFARequiredError) Format(f fmt.State, _ rune) { _, _ = io.WriteString(f, e.Error()) } + +// SignIn is a challenge answered. +type SignIn struct { + Session string + // UsedRecoveryCode is set when the answer was a recovery code, and + // RecoveryCodesLeft is then how many the member still has. + UsedRecoveryCode bool + RecoveryCodesLeft int +} + +// ErrNotACode is input that is neither shape the platform takes, refused here +// without spending one of the sign-in's attempts on it. +var ErrNotACode = errors.New("that is neither a 6-digit code nor a recovery code (xxxx-xxxx-xxxx-xxxx) — " + + "type the code your authenticator app shows for nan.builders") + +// MFAError is the platform refusing a code, or the challenge it was posted to. +type MFAError struct { + // The platform's code for it, e.g. "mfa_code_invalid". Empty when the + // answer carried none. + Code string + Status int + Attempts int // attempts left on this sign-in, for a wrong code + LockedTill time.Time + RetryAfter time.Duration +} + +// Retryable reports whether the same challenge can take another answer: the +// code was wrong, already used or not a code at all, and the sign-in is still +// open. Anything else ends it, and the member needs a new link. +func Retryable(err error) bool { + if errors.Is(err, ErrNotACode) { + return true + } + var e *MFAError + if !errors.As(err, &e) { + return false + } + switch e.Code { + case "mfa_code_invalid", "mfa_recovery_code_invalid", "mfa_code_already_used", + // Nothing was spent: the platform could not read what was sent. + "invalid_request", + // Authenticator codes refused for good; a recovery code still works. + "mfa_totp_suspended": + return true + } + return false +} + +func (e *MFAError) Error() string { + switch e.Code { + case "mfa_code_invalid": + return "that code is not right" + attemptsLeft(e.Attempts) + case "mfa_recovery_code_invalid": + return "that recovery code is not right, or it was already used" + attemptsLeft(e.Attempts) + case "mfa_code_already_used": + return "that code was already used — wait for the next one in your authenticator app" + attemptsLeft(e.Attempts) + case "invalid_request": + return "nan.builders could not read that — type the 6-digit code from your authenticator app, or a recovery code (xxxx-xxxx-xxxx-xxxx)" + case "mfa_totp_suspended": + return "authenticator codes are suspended on this account after too many wrong ones — " + + "use one of your recovery codes, or ask NaN to reset your two-factor authentication" + case "mfa_challenge_exhausted": + return "too many wrong codes for this sign-in — " + newLinkHint + case "mfa_challenge_expired": + return "this sign-in timed out: the code has to be entered within 10 minutes of using the link — " + newLinkHint + case "mfa_challenge_used": + return "this sign-in is already over (it was finished, or a newer sign-in replaced it) — " + newLinkHint + case "mfa_challenge_missing": + return "nan.builders has no record of this sign-in — " + newLinkHint + case "login_refused": + return "nan.builders refused the sign-in for this account — " + newLinkHint + case "mfa_locked": + return "too many wrong codes on this account: two-factor sign-in is paused " + e.pausedFor() + " — then " + newLinkHint + case "rate_limited": + return "too many sign-in attempts from this network — wait " + e.wait() + " and " + newLinkHint + } + if e.Code == "" || e.Status >= 500 { + return fmt.Sprintf("nan.builders could not check the code (HTTP %d) — %s", e.Status, newLinkHint) + } + // A code from a newer platform than this build still gets said, rather + // than swallowed. + return fmt.Sprintf("nan.builders refused the code: %s (HTTP %d) — %s", + strings.ReplaceAll(e.Code, "_", " "), e.Status, newLinkHint) +} + +// attemptsLeft is the tail of a wrong-code message. The platform always says +// how many are left on those; a body without the number says nothing rather +// than "0 attempts left", which would read as the end of the sign-in. +func attemptsLeft(n int) string { + switch { + case n <= 0: + return "" + case n == 1: + return " (1 attempt left)" + } + return fmt.Sprintf(" (%d attempts left)", n) +} + +// now is the clock the lock message is read against. A variable for the tests. +var now = time.Now + +// pausedFor says when the lock ends. locked_until is the platform's clock and +// retry_after the seconds it counts from now; the second is what the +// countdown uses, so a machine whose clock is off still waits the right time. +func (e *MFAError) pausedFor() string { + if e.LockedTill.IsZero() { + return "for " + e.wait() + } + until, today := e.LockedTill.Local(), now().Local() + layout := "15:04" + if until.YearDay() != today.YearDay() || until.Year() != today.Year() { + layout = "Mon 2 Jan 15:04" + } + if e.RetryAfter <= 0 { + return "until " + until.Format(layout) + } + return "until " + until.Format(layout) + " (" + e.wait() + " from now)" +} + +func (e *MFAError) wait() string { + d := e.RetryAfter + if d <= 0 { + return "a few minutes" + } + if d < time.Minute { + return fmt.Sprintf("%d seconds", int(d.Seconds())) + } + mins := int((d + time.Minute - 1) / time.Minute) + if mins == 1 { + return "1 minute" + } + return fmt.Sprintf("%d minutes", mins) +} + +// normalizeAnswer decides what the member typed, the way the platform does: six +// digits once the spaces are out is an authenticator code, sixteen letters and +// digits a recovery code - in any case, with or without its hyphens. +func normalizeAnswer(input string) (field, value string, err error) { + compact := strings.Map(func(r rune) rune { + switch r { + case ' ', '\t', '-': + return -1 + } + return r + }, strings.TrimSpace(input)) + + if len(compact) == 6 && allASCII(compact, isDigit) { + return "code", compact, nil + } + if len(compact) == 16 && allASCII(compact, isAlnum) { + c := strings.ToLower(compact) + return "recovery_code", c[0:4] + "-" + c[4:8] + "-" + c[8:12] + "-" + c[12:16], nil + } + return "", "", ErrNotACode +} + +// isErrorCode is the platform's error-code shape, ^[a-z0-9_]+$. +func isErrorCode(s string) bool { + return s != "" && len(s) <= 64 && allASCII(s, func(b byte) bool { + return isDigit(b) || (b >= 'a' && b <= 'z') || b == '_' + }) +} + +func allASCII(s string, ok func(byte) bool) bool { + for i := 0; i < len(s); i++ { + if !ok(s[i]) { + return false + } + } + return true +} + +func isDigit(b byte) bool { return b >= '0' && b <= '9' } +func isAlnum(b byte) bool { + return isDigit(b) || (b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') +} + +// Complete answers the challenge with what the member typed: the code their +// authenticator app shows, or one of their recovery codes. It returns the +// session on success, and an *MFAError (or ErrNotACode) otherwise; Retryable +// says whether this same challenge can take another try. +func (e *MFARequiredError) Complete(input string) (*SignIn, error) { + field, value, err := normalizeAnswer(input) + if err != nil { + return nil, err + } + body, err := json.Marshal(map[string]string{field: value}) + if err != nil { + return nil, err + } + req, err := http.NewRequest(http.MethodPost, mfaChallengeURL(), bytes.NewReader(body)) + if err != nil { + return nil, err + } + // JSON is what the platform requires here: a text/plain POST would skip a + // browser's preflight, so anything else is refused before the code is + // looked at. No Origin and no Sec-Fetch-Site - this is not a browser, and + // the platform lets a request through without them only because the + // challenge cookie can be held by nothing but the client that opened it. + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json") + req.AddCookie(&http.Cookie{Name: mfaPendingCookie, Value: e.pending}) + + client := &http.Client{ + Timeout: requestTimeout, + CheckRedirect: func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + }, + } + resp, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("could not reach nan.builders: %w", err) + } + defer resp.Body.Close() + raw, _ := io.ReadAll(io.LimitReader(resp.Body, 64<<10)) + + if resp.StatusCode == http.StatusOK { + var ok struct { + RecoveryCodesRemaining *int `json:"recovery_codes_remaining"` + } + _ = json.Unmarshal(raw, &ok) + for _, c := range resp.Cookies() { + if c.Name == sessionCookie && c.Value != "" { + in := &SignIn{Session: c.Value} + if ok.RecoveryCodesRemaining != nil { + in.UsedRecoveryCode = true + in.RecoveryCodesLeft = *ok.RecoveryCodesRemaining + } + return in, nil + } + } + return nil, errors.New("nan.builders accepted the code but sent no session back — " + newLinkHint) + } + + var refusal struct { + Error string `json:"error"` + AttemptsRemaining int `json:"attempts_remaining"` + LockedUntil string `json:"locked_until"` + RetryAfter int `json:"retry_after"` + } + _ = json.Unmarshal(raw, &refusal) + out := &MFAError{Status: resp.StatusCode, Attempts: refusal.AttemptsRemaining} + // Only something shaped like the platform's codes is taken as one: an + // unknown code is printed, and a body that is not ours - a proxy's error + // page, a 404 from a platform with two-factor sign-in turned off - should + // not put whatever it carries on the member's terminal. + if isErrorCode(refusal.Error) { + out.Code = refusal.Error + } + if t, err := time.Parse(time.RFC3339, refusal.LockedUntil); err == nil { + out.LockedTill = t + } + switch { + case refusal.RetryAfter > 0: + out.RetryAfter = time.Duration(refusal.RetryAfter) * time.Second + default: + if s, err := strconv.Atoi(resp.Header.Get("Retry-After")); err == nil && s > 0 { + out.RetryAfter = time.Duration(s) * time.Second + } + } + return nil, out +} diff --git a/internal/auth/mfa_test.go b/internal/auth/mfa_test.go new file mode 100644 index 0000000..1e05cda --- /dev/null +++ b/internal/auth/mfa_test.go @@ -0,0 +1,395 @@ +package auth + +import ( + "encoding/json" + "errors" + "fmt" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/nxssie/nan-cli/internal/auth/authtest" +) + +// fakePlatform starts the contract's fake and points the sign-in at it. +func fakePlatform(t *testing.T) *authtest.Platform { + t.Helper() + p := authtest.New(t) + old := Origin + Origin = p.URL + t.Cleanup(func() { Origin = old }) + return p +} + +// challengeFor runs the link exchange against an account with a factor and +// returns the challenge it comes back with. +func challengeFor(t *testing.T, p *authtest.Platform) *MFARequiredError { + t.Helper() + p.Factor = true + _, err := ExchangeToken(authtest.LinkToken) + var ch *MFARequiredError + if !errors.As(err, &ch) { + t.Fatalf("ExchangeToken = %v, want a two-factor challenge", err) + } + return ch +} + +// An account without a factor signs in exactly as before: one POST, the +// session from its cookie. The POST now says this build can answer a +// challenge, and that is the only thing that changed. +func TestNoFactorSignsInAsBefore(t *testing.T) { + p := fakePlatform(t) + got, err := ExchangeToken(authtest.LinkToken) + if err != nil || got != authtest.Session { + t.Fatalf("ExchangeToken = %q, %v", got, err) + } + forms := p.Verifies() + if len(forms) != 1 { + t.Fatalf("%d verify POSTs", len(forms)) + } + f := forms[0] + if f.Get("token") != authtest.LinkToken || f.Get("client") != "cli" || f.Get("mfa") != "1" || f.Has("flow") { + t.Errorf("verify form = %v", f) + } + if n := len(p.Challenges()); n != 0 { + t.Errorf("%d challenge POSTs for an account without a factor", n) + } +} + +// The new CLI announces it can answer, so a platform that follows the +// contract never sends it the "update nan-cli" refusal that older builds get. +func TestTheNewCLINeverMeetsTheUpdateRefusal(t *testing.T) { + p := fakePlatform(t) + ch := challengeFor(t, p) + if ch == nil || p.Verifies()[0].Get("mfa") != "1" { + t.Fatal("no challenge") + } + // And should it meet it anyway, it says what to do in a sentence. + msg := linkReasonMessage("mfa_enabled_update_nan_cli") + for _, want := range []string{"two-factor", "update", "new link"} { + if !strings.Contains(msg, want) { + t.Errorf("%q does not mention %q", msg, want) + } + } +} + +func TestRightCodeSignsIn(t *testing.T) { + p := fakePlatform(t) + ch := challengeFor(t, p) + + in, err := ch.Complete(" 246 810 ") + if err != nil { + t.Fatal(err) + } + if in.Session != authtest.Session || in.UsedRecoveryCode { + t.Errorf("SignIn = %+v", in) + } + + reqs := p.Challenges() + if len(reqs) != 1 { + t.Fatalf("%d challenge POSTs", len(reqs)) + } + r := reqs[0] + if r.Cookie != authtest.Pending { + t.Errorf("challenge cookie = %q", r.Cookie) + } + if ct := r.Header.Get("Content-Type"); ct != "application/json" { + t.Errorf("Content-Type = %q", ct) + } + // Not a browser, and must not pass for one: the platform lets an + // Origin-less request through only because the cookie proves who opened + // the challenge. + for _, h := range []string{"Origin", "Sec-Fetch-Site", "Referer"} { + if v := r.Header.Get(h); v != "" { + t.Errorf("sent %s: %q", h, v) + } + } + var body map[string]string + if err := json.Unmarshal([]byte(r.Body), &body); err != nil || len(body) != 1 || body["code"] != "246810" { + t.Errorf("body = %s", r.Body) + } +} + +func TestWrongCodeThenRight(t *testing.T) { + p := fakePlatform(t) + ch := challengeFor(t, p) + + _, err := ch.Complete("111111") + var refused *MFAError + if !errors.As(err, &refused) || refused.Code != "mfa_code_invalid" { + t.Fatalf("err = %v", err) + } + if !Retryable(err) { + t.Error("a wrong code ends the sign-in") + } + if !strings.Contains(err.Error(), "4 attempts left") { + t.Errorf("%q does not say how many tries are left", err) + } + + in, err := ch.Complete(authtest.Code) + if err != nil || in.Session != authtest.Session { + t.Fatalf("second try: %+v %v", in, err) + } +} + +// A recovery code is read the way the platform reads it - any case, with or +// without hyphens and spaces - and sent as one, and the answer says how many +// are left. +func TestRecoveryCodeSignsInAndCountsWhatIsLeft(t *testing.T) { + for _, typed := range []string{"q7wa-z2xd-pl4r-6wze", "Q7WA Z2XD PL4R 6WZE", "q7waz2xdpl4r6wze"} { + t.Run(typed, func(t *testing.T) { + p := fakePlatform(t) + p.RecoveryCodes = []string{"aaaa-bbbb-cccc-dddd", "q7wa-z2xd-pl4r-6wze"} + ch := challengeFor(t, p) + + in, err := ch.Complete(typed) + if err != nil { + t.Fatal(err) + } + if !in.UsedRecoveryCode || in.RecoveryCodesLeft != 1 || in.Session != authtest.Session { + t.Errorf("SignIn = %+v", in) + } + var body map[string]string + _ = json.Unmarshal([]byte(p.Challenges()[0].Body), &body) + if len(body) != 1 || body["recovery_code"] != "q7wa-z2xd-pl4r-6wze" { + t.Errorf("body = %s", p.Challenges()[0].Body) + } + }) + } +} + +func TestUsedRecoveryCodeIsRefused(t *testing.T) { + p := fakePlatform(t) + ch := challengeFor(t, p) + _, err := ch.Complete("zzzz-zzzz-zzzz-zzzz") + var refused *MFAError + if !errors.As(err, &refused) || refused.Code != "mfa_recovery_code_invalid" || !Retryable(err) { + t.Fatalf("err = %v", err) + } + if !strings.Contains(err.Error(), "already used") { + t.Errorf("%q", err) + } +} + +// The same 30-second code twice: the platform keeps one use per step, and the +// member is told to wait for the next. +func TestCodeAlreadyUsed(t *testing.T) { + p := fakePlatform(t) + ch := challengeFor(t, p) + if _, err := ch.Complete(authtest.Code); err != nil { + t.Fatal(err) + } + p.NewLink() + ch = challengeFor(t, p) + _, err := ch.Complete(authtest.Code) + var refused *MFAError + if !errors.As(err, &refused) || refused.Code != "mfa_code_already_used" || !Retryable(err) { + t.Fatalf("err = %v", err) + } + if !strings.Contains(err.Error(), "wait for the next one") { + t.Errorf("%q", err) + } +} + +// Five wrong codes end the sign-in, and the last answer says so. +func TestFifthWrongCodeEndsTheSignIn(t *testing.T) { + p := fakePlatform(t) + ch := challengeFor(t, p) + var err error + for i := 0; i < 5; i++ { + _, err = ch.Complete("000000") + } + var refused *MFAError + if !errors.As(err, &refused) || refused.Code != "mfa_challenge_exhausted" || Retryable(err) { + t.Fatalf("err = %v", err) + } + if !strings.Contains(err.Error(), "new link") { + t.Errorf("%q", err) + } +} + +func TestExpiredChallenge(t *testing.T) { + p := fakePlatform(t) + ch := challengeFor(t, p) + p.Expire() + _, err := ch.Complete(authtest.Code) + var refused *MFAError + if !errors.As(err, &refused) || refused.Code != "mfa_challenge_expired" || Retryable(err) { + t.Fatalf("err = %v", err) + } + if !strings.Contains(err.Error(), "10 minutes") || !strings.Contains(err.Error(), "new link") { + t.Errorf("%q", err) + } +} + +// A lock is said with its end, by the platform's clock and the seconds it +// counts, and it ends this sign-in. +func TestLockedAccount(t *testing.T) { + p := fakePlatform(t) + ch := challengeFor(t, p) + until := time.Now().Add(14*time.Minute + 30*time.Second) + p.Lock(until) + + _, err := ch.Complete(authtest.Code) + var refused *MFAError + if !errors.As(err, &refused) || refused.Code != "mfa_locked" || Retryable(err) { + t.Fatalf("err = %v", err) + } + if refused.Status != http.StatusTooManyRequests || refused.RetryAfter < 14*time.Minute { + t.Errorf("refusal = %+v", refused) + } + msg := err.Error() + for _, want := range []string{"paused", until.Local().Format("15:04"), "15 minutes from now"} { + if !strings.Contains(msg, want) { + t.Errorf("%q does not say %q", msg, want) + } + } +} + +// Every refusal in the contract, as the platform sends it: whether the same +// sign-in can take another answer, and what the member is told. +func TestEveryRefusalInTheContract(t *testing.T) { + for _, c := range []struct { + name string + refusal authtest.Refusal + code string + retryable bool + want []string + }{ + {"wrong code", r(400, `{"error":"mfa_code_invalid","message":"x","attempts_remaining":1}`), "mfa_code_invalid", true, []string{"not right", "1 attempt left"}}, + {"wrong recovery code", r(400, `{"error":"mfa_recovery_code_invalid","message":"x","attempts_remaining":3}`), "mfa_recovery_code_invalid", true, []string{"recovery code", "3 attempts left"}}, + {"replayed code", r(400, `{"error":"mfa_code_already_used","message":"x","attempts_remaining":2}`), "mfa_code_already_used", true, []string{"next one", "2 attempts left"}}, + {"unreadable", r(400, `{"error":"invalid_request","message":"x"}`), "invalid_request", true, []string{"6-digit"}}, + {"exhausted", r(403, `{"error":"mfa_challenge_exhausted","message":"x"}`), "mfa_challenge_exhausted", false, []string{"too many wrong codes", "new link"}}, + {"expired", r(403, `{"error":"mfa_challenge_expired","message":"x"}`), "mfa_challenge_expired", false, []string{"timed out", "new link"}}, + {"used", r(403, `{"error":"mfa_challenge_used","message":"x"}`), "mfa_challenge_used", false, []string{"already over", "new link"}}, + {"missing", r(403, `{"error":"mfa_challenge_missing","message":"x"}`), "mfa_challenge_missing", false, []string{"no record", "new link"}}, + {"refused account", r(403, `{"error":"login_refused","message":"x"}`), "login_refused", false, []string{"refused the sign-in", "new link"}}, + {"locked, body only", r(429, `{"error":"mfa_locked","message":"x","retry_after":600}`), "mfa_locked", false, []string{"paused for 10 minutes"}}, + {"rate limited", rh(429, `{"error":"rate_limited","message":"x","retry_after":60}`, "Retry-After", "60"), "rate_limited", false, []string{"wait 1 minute", "new link"}}, + {"rate limited, header only", rh(429, `{"error":"rate_limited","message":"x"}`, "Retry-After", "45"), "rate_limited", false, []string{"wait 45 seconds"}}, + {"another origin", r(403, `{"error":"forbidden_origin"}`), "forbidden_origin", false, []string{"forbidden origin", "HTTP 403"}}, + {"codes suspended", r(403, `{"error":"mfa_totp_suspended","message":"x"}`), "mfa_totp_suspended", true, []string{"recovery codes"}}, + {"server error", r(500, `{"error":"internal_error","message":"x"}`), "internal_error", false, []string{"could not check the code", "HTTP 500"}}, + {"two-factor turned off", r(404, "404 page not found\n"), "", false, []string{"HTTP 404", "new link"}}, + {"not our body", r(502, ""+authtest.Pending+"\x1b[31m"), "", false, []string{"HTTP 502"}}, + {"a code that is not one", r(403, `{"error":"Bad \u001b[2J`+authtest.Pending+`"}`), "", false, []string{"HTTP 403"}}, + {"a code from tomorrow", r(403, `{"error":"mfa_something_new","message":"x"}`), "mfa_something_new", false, []string{"mfa something new"}}, + } { + t.Run(c.name, func(t *testing.T) { + p := fakePlatform(t) + ch := challengeFor(t, p) + p.Next = &c.refusal + _, err := ch.Complete(authtest.Code) + var refused *MFAError + if !errors.As(err, &refused) { + t.Fatalf("err = %v (%T)", err, err) + } + if refused.Code != c.code { + t.Errorf("code = %q, want %q", refused.Code, c.code) + } + if Retryable(err) != c.retryable { + t.Errorf("Retryable = %v, want %v", !c.retryable, c.retryable) + } + for _, want := range c.want { + if !strings.Contains(err.Error(), want) { + t.Errorf("%q does not say %q", err, want) + } + } + if strings.Contains(err.Error(), authtest.Pending) || strings.ContainsRune(err.Error(), '\x1b') { + t.Errorf("the message carries what the body did: %q", err) + } + }) + } +} + +func r(status int, body string) authtest.Refusal { + return authtest.Refusal{Status: status, Body: body, Header: map[string]string{"Content-Type": "application/json"}} +} + +func rh(status int, body, k, v string) authtest.Refusal { + out := r(status, body) + out.Header[k] = v + return out +} + +// Something that is neither a code nor a recovery code is refused here, and +// spends nothing: no request, no attempt. +func TestNotACodeIsNeverSent(t *testing.T) { + p := fakePlatform(t) + ch := challengeFor(t, p) + for _, typed := range []string{"", "12345", "1234567", "abcdef", "q7wa-z2xd-pl4r", "q7wa-z2xd-pl4r-6wze-xxxx", "246810\x00", "２４６８１０"} { + if _, err := ch.Complete(typed); !errors.Is(err, ErrNotACode) || !Retryable(err) { + t.Errorf("Complete(%q) = %v", typed, err) + } + } + if n := len(p.Challenges()); n != 0 { + t.Errorf("%d requests for input that is not a code", n) + } +} + +func TestNormalizeAnswer(t *testing.T) { + for _, c := range []struct{ in, field, value string }{ + {"123456", "code", "123456"}, + {" 123 456 ", "code", "123456"}, + {"123-456", "code", "123456"}, + {"q7wa-z2xd-pl4r-6wze", "recovery_code", "q7wa-z2xd-pl4r-6wze"}, + {"Q7WAZ2XDPL4R6WZE", "recovery_code", "q7wa-z2xd-pl4r-6wze"}, + {"q7wa z2xd\tpl4r 6wze", "recovery_code", "q7wa-z2xd-pl4r-6wze"}, + } { + field, value, err := normalizeAnswer(c.in) + if err != nil || field != c.field || value != c.value { + t.Errorf("normalizeAnswer(%q) = %q %q %v", c.in, field, value, err) + } + } +} + +// A redirect that asks for the code without the challenge to answer it with +// is a dead end, and says so instead of prompting for nothing. +func TestMFAReasonWithoutTheCookie(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, authtest.FrontendURL+"/mfa?reason=mfa_required", http.StatusFound) + })) + defer srv.Close() + old := Origin + Origin = srv.URL + defer func() { Origin = old }() + + _, err := ExchangeToken(authtest.LinkToken) + var ch *MFARequiredError + if err == nil || errors.As(err, &ch) { + t.Fatalf("err = %v", err) + } + if !strings.Contains(err.Error(), "new link") { + t.Errorf("%q", err) + } +} + +// The challenge cookie is a credential for ten minutes. However the +// challenge is printed or logged, the value is not in it. +func TestTheChallengeNeverPrintsItsCookie(t *testing.T) { + p := fakePlatform(t) + ch := challengeFor(t, p) + + var out []string + for _, verb := range []string{"%v", "%+v", "%#v", "%s", "%q", "%d", "%x"} { + out = append(out, fmt.Sprintf(verb, ch), fmt.Sprintf(verb, error(ch))) + } + out = append(out, ch.Error(), fmt.Errorf("wrapped: %w", ch).Error()) + if b, err := json.Marshal(ch); err == nil { + out = append(out, string(b)) + } + var logged strings.Builder + slog.New(slog.NewTextHandler(&logged, nil)).Info("sign-in", "err", ch, "challenge", ch) + slog.New(slog.NewJSONHandler(&logged, nil)).Info("sign-in", "err", ch, "challenge", ch) + out = append(out, logged.String()) + + for _, s := range out { + if strings.Contains(s, authtest.Pending) { + t.Errorf("the challenge cookie is in %q", s) + } + } +} From 12b663dd136ec7917682f652c85109b028729c49 Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 17:32:38 +0200 Subject: [PATCH 02/14] feat(login): ask for the two-factor code after the link When the link exchange comes back with a challenge, nan auth login asks "Authenticator code:" without echo on a terminal, and asks again after a wrong, replayed or unreadable code until the platform takes one or ends the sign-in. --totp - reads one line of stdin instead, with one try, for a shell that cannot answer a prompt; with neither, the command says the link is spent and names the flag. A recovery code that gets the member in prints how many are left and where to make new ones. --totp takes only "-" (a recovery code on the command line would sit in the shell history), and is refused next to --link -, --token and --api-token before anything is posted, so a flag mistake never spends a link. The session is saved as before, and the challenge cookie is never written: the tests walk HOME for it after success and failure. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- cmd/auth.go | 165 ++++++++++++++++++-- cmd/auth_mfa_test.go | 364 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 520 insertions(+), 9 deletions(-) create mode 100644 cmd/auth_mfa_test.go diff --git a/cmd/auth.go b/cmd/auth.go index a9f0e10..2537f82 100644 --- a/cmd/auth.go +++ b/cmd/auth.go @@ -23,6 +23,7 @@ var ( linkFlag string keepToolsFlag bool apiTokenFlag bool + totpFlag string ) var authCmd = &cobra.Command{ @@ -35,6 +36,10 @@ var loginCmd = &cobra.Command{ Short: "Log in by email, or save a platform token with --api-token", Long: `Log in with a sign-in link sent to your email. +With two-factor authentication on, the link is followed by the code from +your authenticator app, or one of your recovery codes. It is asked for +without echo; with no terminal to ask on, pipe it in with --totp -. + With --api-token, save a platform token instead, for a machine with nobody at the keyboard (a server, a CI runner). Create the token in Settings > Tokens at https://cloud.nan.builders. It is read from stdin only, so it @@ -45,7 +50,8 @@ need the email sign-in. An API key (sk-...) can be saved the same way; it replaces the one the Setup tab saved.`, Example: ` nan auth login --email you@example.com nan auth login --api-token # paste the token; it is not shown - nan auth login --api-token < token.txt`, + nan auth login --api-token < token.txt + echo 123456 | nan auth login --link "" --totp - # two-factor, no prompt`, RunE: runLogin, } @@ -63,6 +69,7 @@ func init() { loginCmd.Flags().StringVar(&linkFlag, "link", "", `Finish the login with the link from the email ("-" reads it from stdin, keeping the token out of your shell history)`) loginCmd.Flags().StringVar(&tokenFlag, "token", "", `Save a nan_session token directly, skipping the email ("-" reads it from stdin, keeping it out of your shell history)`) loginCmd.Flags().BoolVar(&apiTokenFlag, "api-token", false, "Save a platform token (nan_pat_...) or API key (sk-...) for nan run and nan runs, read from stdin") + loginCmd.Flags().StringVar(&totpFlag, "totp", "", `With two-factor authentication on, read the code (or a recovery code) from stdin instead of asking: "-" is the only value`) logoutCmd.Flags().BoolVar(&keepToolsFlag, "keep-tools", false, "Leave the API key in the tools this CLI configured") } @@ -72,11 +79,14 @@ func init() { // and the command then asked for a cookie that no longer existed. func runLogin(cmd *cobra.Command, args []string) error { if apiTokenFlag { - if tokenFlag != "" || linkFlag != "" || emailFlag != "" { - return fmt.Errorf("--api-token cannot be combined with --email, --link or --token") + if tokenFlag != "" || linkFlag != "" || emailFlag != "" || totpFlag != "" { + return fmt.Errorf("--api-token cannot be combined with --email, --link, --token or --totp") } return loginWithAPIToken(cmd.Context(), newTokenInput(os.Stdin), os.Stdout, os.Stderr) } + if err := checkTOTPFlag(); err != nil { + return err + } if tokenFlag != "" { token, err := flagValue(tokenFlag) if err != nil { @@ -97,11 +107,11 @@ func runLogin(cmd *cobra.Command, args []string) error { if err != nil { return err } - sessionToken, err := auth.ExchangeToken(token) - if err != nil { - return err - } - return saveToken(sessionToken) + // A code is a line of a few characters; nothing past this is one. + stdin := bufio.NewReader(io.LimitReader(os.Stdin, 1024)) + return finishSignIn(token, newCodeInput(os.Stdin, func() (string, error) { + return stdin.ReadString('\n') + }), os.Stdout, os.Stderr) } in := bufio.NewScanner(os.Stdin) @@ -169,11 +179,148 @@ func runLogin(cmd *cobra.Command, args []string) error { return err } + // The code comes from the same scanner as the two answers before it: a + // pipe that carries all three has already been read into its buffer. + return finishSignIn(token, newCodeInput(os.Stdin, func() (string, error) { + if in.Scan() { + return in.Text(), nil + } + if err := in.Err(); err != nil { + return "", err + } + return "", io.EOF + }), os.Stdout, os.Stderr) +} + +// checkTOTPFlag refuses the --totp spellings that cannot work. +// +// Only "-": a recovery code is good until it is used, and one written on the +// command line is in the shell's history and in `ps` for as long as that +// lasts. And not next to another flag that reads stdin, because the first one +// would take everything and leave the code nothing. +func checkTOTPFlag() error { + switch { + case totpFlag == "": + return nil + case totpFlag != "-": + return fmt.Errorf(`--totp only takes "-", reading the code from stdin, so a recovery code never lands in your shell history:%s`, totpHint) + case tokenFlag != "": + return fmt.Errorf("--totp cannot be combined with --token: a saved session token skips the sign-in, and its code") + case linkFlag == "-": + return fmt.Errorf("--link - and --totp - cannot both read stdin: pass the link as an argument (it works once, and is spent by this command):%s", totpHint) + } + return nil +} + +const totpHint = "\n\n echo | nan auth login --link \"\" --totp -" + +// finishSignIn spends the link, answers the two-factor challenge when the +// account has one, and saves the session. +func finishSignIn(token string, codes codeInput, out, prompt io.Writer) error { sessionToken, err := auth.ExchangeToken(token) + var challenge *auth.MFARequiredError + if !errors.As(err, &challenge) { + if err != nil { + return err + } + return saveToken(sessionToken) + } + + signIn, err := answerChallenge(challenge, codes, prompt) if err != nil { return err } - return saveToken(sessionToken) + if err := saveToken(signIn.Session); err != nil { + return err + } + if signIn.UsedRecoveryCode { + fmt.Fprintln(out, recoveryCodesLeft(signIn.RecoveryCodesLeft)) + } + return nil +} + +// answerChallenge asks for the code until the platform takes one or the +// sign-in ends. Asking again is for a member at a terminal: a code piped in +// with --totp - gets one try, and anything else ends with the refusal. +func answerChallenge(challenge *auth.MFARequiredError, codes codeInput, prompt io.Writer) (*auth.SignIn, error) { + if !codes.tty && !codes.fromStdin { + // The link is spent by now - the platform consumed it to open the + // challenge - so this cannot just say "run it again with the flag". + return nil, fmt.Errorf("this account uses two-factor authentication, and there is no terminal here to ask for the code. "+ + "The link is spent: ask for a new one, and pipe the code in with --totp -:%s", totpHint) + } + interactive := codes.tty && !codes.fromStdin + if interactive { + fmt.Fprintln(prompt, "This account uses two-factor authentication. Enter the 6-digit code from your") + fmt.Fprintln(prompt, "authenticator app, or one of your recovery codes.") + } + for { + input, err := codes.read(prompt) + if err != nil { + return nil, err + } + signIn, err := challenge.Complete(input) + if err == nil { + return signIn, nil + } + if !interactive || !auth.Retryable(err) { + return nil, err + } + fmt.Fprintln(prompt, "error: "+err.Error()) + } +} + +// recoveryCodesLeft is what the login says after a recovery code got the +// member in: each works once, and running out is the way to be locked out. +func recoveryCodesLeft(n int) string { + const where = "cloud.nan.builders, Settings > Account > Two-factor authentication" + switch n { + case 0: + return "That was your last recovery code. Make new ones before you need one: " + where + "." + case 1: + return "You signed in with a recovery code: 1 left. Make new ones in the browser: " + where + "." + } + return fmt.Sprintf("You signed in with a recovery code: %d left. You can make new ones in the browser: %s.", n, where) +} + +// codeInput is where the second factor comes from: a hidden prompt on a +// terminal, or one line of stdin with --totp -. +type codeInput struct { + tty bool + fromStdin bool + readHidden func() ([]byte, error) + readLine func() (string, error) +} + +// newCodeInput asks f, hidden when it is a terminal; readLine is the next +// line of the same stdin, for --totp -. +func newCodeInput(f *os.File, readLine func() (string, error)) codeInput { + return codeInput{ + tty: term.IsTerminal(f.Fd()), + fromStdin: totpFlag == "-", + readHidden: func() ([]byte, error) { return term.ReadPassword(f.Fd()) }, + readLine: readLine, + } +} + +func (in codeInput) read(prompt io.Writer) (string, error) { + if in.tty { + fmt.Fprint(prompt, "Authenticator code: ") + raw, err := in.readHidden() + fmt.Fprintln(prompt) + if err != nil { + return "", fmt.Errorf("could not read the code: %w", err) + } + return strings.TrimSpace(string(raw)), nil + } + line, err := in.readLine() + if err != nil && !errors.Is(err, io.EOF) { + return "", fmt.Errorf("could not read the code from stdin: %w", err) + } + if line = strings.TrimSpace(line); line == "" { + return "", fmt.Errorf("no code arrived on stdin. The link is spent: ask for a new one, and pipe the code in:%s", totpHint) + } + return line, nil } // A flag whose value is a credential, with "-" meaning stdin. diff --git a/cmd/auth_mfa_test.go b/cmd/auth_mfa_test.go new file mode 100644 index 0000000..64641ff --- /dev/null +++ b/cmd/auth_mfa_test.go @@ -0,0 +1,364 @@ +package cmd + +import ( + "bytes" + "errors" + "io" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/nxssie/nan-cli/internal/auth" + "github.com/nxssie/nan-cli/internal/auth/authtest" +) + +// Two-factor sign-in from `nan auth login`, against a fake platform written +// from the MFA API contract (internal/auth/authtest). + +const testLink = "https://cloud-api.nan.builders/api/auth/login/verify?token=" + authtest.LinkToken + +// mfaPlatform points the sign-in at the fake and gives the login flags back +// their defaults when the test ends: cobra writes them into package vars. +func mfaPlatform(t *testing.T, factor bool) *authtest.Platform { + t.Helper() + p := authtest.New(t) + p.Factor = factor + swap(t, &auth.Origin, p.URL) + for _, f := range []*string{&linkFlag, &totpFlag, &tokenFlag, &emailFlag} { + swap(t, f, "") + } + swap(t, &apiTokenFlag, false) + return p +} + +// typed is a terminal on which the member types these, one per prompt. +func typed(answers ...string) (codeInput, *int) { + asked := new(int) + return codeInput{tty: true, readHidden: func() ([]byte, error) { + if *asked >= len(answers) { + return nil, io.EOF + } + *asked++ + return []byte(answers[*asked-1] + "\n"), nil + }}, asked +} + +// piped is stdin that is not a terminal: what --totp - reads. +func piped(stdin string, totp bool) codeInput { + r := strings.NewReader(stdin) + line := func() (string, error) { + var b strings.Builder + buf := make([]byte, 1) + for { + n, err := r.Read(buf) + if n == 1 { + if buf[0] == '\n' { + return b.String(), nil + } + b.WriteByte(buf[0]) + } + if err != nil { + return b.String(), err + } + } + } + return codeInput{fromStdin: totp, readLine: line} +} + +func TestLoginWithoutAFactorIsUnchanged(t *testing.T) { + home := fakeHome(t, "") + p := mfaPlatform(t, false) + var prompt bytes.Buffer + codes, asked := typed() + if err := finishSignIn(authtest.LinkToken, codes, io.Discard, &prompt); err != nil { + t.Fatal(err) + } + if sess, _ := readSession(t, home); sess["token"] != authtest.Session { + t.Errorf("session.json = %v", sess) + } + if *asked != 0 || prompt.Len() != 0 || len(p.Challenges()) != 0 { + t.Errorf("asked for a code an account without a factor does not have: %d prompts, %q", *asked, prompt.String()) + } +} + +func TestLoginAsksForTheCodeOnATerminal(t *testing.T) { + home := fakeHome(t, `{"token":"","apiKey":"sk-kept"}`) + mfaPlatform(t, true) + var prompt bytes.Buffer + codes, asked := typed(authtest.Code) + if err := finishSignIn(authtest.LinkToken, codes, io.Discard, &prompt); err != nil { + t.Fatal(err) + } + sess, _ := readSession(t, home) + if sess["token"] != authtest.Session || sess["apiKey"] != "sk-kept" { + t.Errorf("session.json = %v", sess) + } + if *asked != 1 || !strings.Contains(prompt.String(), "Authenticator code: ") { + t.Errorf("%d prompts: %q", *asked, prompt.String()) + } + if !strings.Contains(prompt.String(), "recovery codes") { + t.Errorf("the prompt does not say a recovery code works too: %q", prompt.String()) + } +} + +// A wrong code, a replayed one and something that is not a code at all are +// all asked again, on the same sign-in, with what went wrong. +func TestLoginAsksAgainAfterAWrongCode(t *testing.T) { + home := fakeHome(t, "") + p := mfaPlatform(t, true) + var prompt bytes.Buffer + codes, asked := typed("000000", "not a code", authtest.Code) + if err := finishSignIn(authtest.LinkToken, codes, io.Discard, &prompt); err != nil { + t.Fatal(err) + } + if *asked != 3 { + t.Errorf("%d prompts, want 3", *asked) + } + for _, want := range []string{"not right (4 attempts left)", "neither a 6-digit code"} { + if !strings.Contains(prompt.String(), want) { + t.Errorf("%q does not say %q", prompt.String(), want) + } + } + // "not a code" never left the machine. + if n := len(p.Challenges()); n != 2 { + t.Errorf("%d challenge POSTs, want 2", n) + } + if sess, _ := readSession(t, home); sess["token"] != authtest.Session { + t.Errorf("session.json = %v", sess) + } +} + +func TestLoginWithARecoveryCodeSaysHowManyAreLeft(t *testing.T) { + for _, c := range []struct { + codes []string + want string + }{ + {[]string{"q7wa-z2xd-pl4r-6wze", "aaaa-bbbb-cccc-dddd", "eeee-ffff-gggg-hhhh"}, "2 left"}, + {[]string{"q7wa-z2xd-pl4r-6wze"}, "last recovery code"}, + } { + t.Run(c.want, func(t *testing.T) { + fakeHome(t, "") + p := mfaPlatform(t, true) + p.RecoveryCodes = c.codes + var out bytes.Buffer + codes, _ := typed("Q7WA Z2XD PL4R 6WZE") + if err := finishSignIn(authtest.LinkToken, codes, &out, io.Discard); err != nil { + t.Fatal(err) + } + if !strings.Contains(out.String(), c.want) || !strings.Contains(out.String(), "Two-factor authentication") { + t.Errorf("%q does not say %q and where to make more", out.String(), c.want) + } + }) + } +} + +// Each way the sign-in ends saves nothing and says what to do next. +func TestLoginThatCannotFinishSavesNothing(t *testing.T) { + for _, c := range []struct { + name string + setup func(*authtest.Platform) + codes []string + want string + }{ + {"five wrong codes", nil, []string{"000000", "000001", "000002", "000003", "000004"}, "too many wrong codes"}, + {"expired", func(p *authtest.Platform) { p.Expire() }, []string{authtest.Code}, "timed out"}, + {"refused", func(p *authtest.Platform) { + p.Next = &authtest.Refusal{Status: 403, Body: `{"error":"login_refused","message":"x"}`} + }, []string{authtest.Code}, "refused the sign-in"}, + {"nothing typed at all", nil, nil, "could not read the code"}, + } { + t.Run(c.name, func(t *testing.T) { + home := fakeHome(t, "") + p := mfaPlatform(t, true) + codes, _ := typed(c.codes...) + // The challenge opens on the verify POST, which finishSignIn + // makes; the fake is changed as soon as it has. + if c.setup != nil { + inner := codes.readHidden + first := true + codes.readHidden = func() ([]byte, error) { + if first { + first = false + c.setup(p) + } + return inner() + } + } + err := finishSignIn(authtest.LinkToken, codes, io.Discard, io.Discard) + if err == nil || !strings.Contains(err.Error(), c.want) { + t.Fatalf("err = %v, want %q", err, c.want) + } + if sess, _ := readSession(t, home); sess != nil { + t.Errorf("saved %v", sess) + } + }) + } +} + +// --totp - reads one line of stdin, with no prompt and no second try: a +// script cannot answer one. +func TestTOTPFromStdin(t *testing.T) { + t.Run("right", func(t *testing.T) { + home := fakeHome(t, "") + mfaPlatform(t, true) + var prompt bytes.Buffer + if err := finishSignIn(authtest.LinkToken, piped(authtest.Code+"\n", true), io.Discard, &prompt); err != nil { + t.Fatal(err) + } + if prompt.Len() != 0 { + t.Errorf("prompted a pipe: %q", prompt.String()) + } + if sess, _ := readSession(t, home); sess["token"] != authtest.Session { + t.Errorf("session.json = %v", sess) + } + }) + t.Run("wrong, then nothing", func(t *testing.T) { + home := fakeHome(t, "") + p := mfaPlatform(t, true) + err := finishSignIn(authtest.LinkToken, piped("000000\n"+authtest.Code+"\n", true), io.Discard, io.Discard) + if err == nil || !strings.Contains(err.Error(), "not right") { + t.Fatalf("err = %v", err) + } + if n := len(p.Challenges()); n != 1 { + t.Errorf("%d tries from a pipe, want 1", n) + } + if sess, _ := readSession(t, home); sess != nil { + t.Errorf("saved %v", sess) + } + }) + t.Run("empty", func(t *testing.T) { + fakeHome(t, "") + mfaPlatform(t, true) + err := finishSignIn(authtest.LinkToken, piped("", true), io.Discard, io.Discard) + if err == nil || !strings.Contains(err.Error(), "no code arrived") || !strings.Contains(err.Error(), "--totp -") { + t.Fatalf("err = %v", err) + } + }) +} + +// No terminal and no --totp: the link is already spent, so the message says +// to ask for another and names the flag that would have worked. +func TestNoTerminalAndNoFlag(t *testing.T) { + home := fakeHome(t, "") + p := mfaPlatform(t, true) + err := finishSignIn(authtest.LinkToken, piped(authtest.Code+"\n", false), io.Discard, io.Discard) + if err == nil { + t.Fatal("signed in with no way to ask for the code") + } + for _, want := range []string{"two-factor", "new one", "--totp -"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("%q does not say %q", err, want) + } + } + if n := len(p.Challenges()); n != 0 { + t.Errorf("read a code nobody asked to be read: %d POSTs", n) + } + if sess, _ := readSession(t, home); sess != nil { + t.Errorf("saved %v", sess) + } +} + +// The whole command, as a script runs it: link as an argument, code piped in. +func TestLoginCommandWithTOTPFromStdin(t *testing.T) { + home := fakeHome(t, "") + p := mfaPlatform(t, true) + withStdin(t, authtest.Code+"\n") + rootCmd.SetArgs([]string{"auth", "login", "--link", testLink, "--totp", "-"}) + rootCmd.SetOut(io.Discard) + rootCmd.SetErr(io.Discard) + if err := rootCmd.Execute(); err != nil { + t.Fatal(err) + } + if sess, _ := readSession(t, home); sess["token"] != authtest.Session { + t.Errorf("session.json = %v", sess) + } + if f := p.Verifies(); len(f) != 1 || f[0].Get("mfa") != "1" { + t.Errorf("verify forms = %v", f) + } + assertNoChallengeOnDisk(t, home) +} + +func TestTOTPFlagSpellings(t *testing.T) { + for _, c := range []struct { + name string + args []string + want string + }{ + {"a code on the command line", []string{"--link", testLink, "--totp", authtest.Code}, `only takes "-"`}, + {"two flags reading stdin", []string{"--link", "-", "--totp", "-"}, "cannot both read stdin"}, + {"with a saved token", []string{"--token", "x", "--totp", "-"}, "cannot be combined with --token"}, + {"with --api-token", []string{"--api-token", "--totp", "-"}, "cannot be combined"}, + } { + t.Run(c.name, func(t *testing.T) { + home := fakeHome(t, "") + p := mfaPlatform(t, true) + rootCmd.SetArgs(append([]string{"auth", "login"}, c.args...)) + rootCmd.SetOut(io.Discard) + rootCmd.SetErr(io.Discard) + err := rootCmd.Execute() + if err == nil || !strings.Contains(err.Error(), c.want) { + t.Fatalf("err = %v, want %q", err, c.want) + } + if strings.Contains(err.Error(), authtest.Code) { + t.Errorf("the refusal repeats the code: %v", err) + } + if n := len(p.Verifies()); n != 0 { + t.Errorf("spent the link on flags that cannot work: %d verify POSTs", n) + } + if sess, _ := readSession(t, home); sess != nil { + t.Errorf("saved %v", sess) + } + }) + } +} + +// The challenge cookie is a credential for ten minutes and is kept in memory +// only. Whatever the sign-in wrote under HOME - on success and on failure - +// does not contain it, and neither does anything it printed. +func TestTheChallengeCookieNeverReachesDiskOrOutput(t *testing.T) { + for _, c := range []struct { + name string + codes []string + }{ + {"signed in", []string{"000000", authtest.Code}}, + {"gave up", []string{"000000"}}, + } { + t.Run(c.name, func(t *testing.T) { + home := fakeHome(t, `{"token":"","apiKey":"sk-kept"}`) + mfaPlatform(t, true) + var out, prompt bytes.Buffer + codes, _ := typed(c.codes...) + err := finishSignIn(authtest.LinkToken, codes, &out, &prompt) + printed := out.String() + prompt.String() + if err != nil { + printed += err.Error() + } + if strings.Contains(printed, authtest.Pending) { + t.Errorf("printed the challenge cookie: %q", printed) + } + assertNoChallengeOnDisk(t, home) + }) + } +} + +func assertNoChallengeOnDisk(t *testing.T, home string) { + t.Helper() + err := filepath.WalkDir(home, func(path string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return err + } + data, err := os.ReadFile(path) + if err != nil { + return err + } + if bytes.Contains(data, []byte(authtest.Pending)) { + t.Errorf("%s holds the challenge cookie", path) + } + return nil + }) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + t.Fatal(err) + } +} From c504b02536637dfac1f23a2ff4db88108670ee68 Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 17:35:15 +0200 Subject: [PATCH 03/14] feat(tui): ask for the two-factor code in the panel's sign-in The panel shares internal/auth with nan auth login, so a link that opens a challenge now leads to a third question, "Authenticator code:", in the same step of the guided setup and without echo. A wrong, replayed or unreadable code keeps the question, empty, with the attempts left. A sign-in that cannot go on (timed out, exhausted, locked, refused) spent its link, so the panel goes back to the address, filled in, and enter asks for a new one. Esc forgets the challenge, and an exchange that answers after the member left does not reopen it. A recovery code that gets the member in is said on the next step, with how many are left and where to make new ones. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- internal/tui/config_test.go | 4 +- internal/tui/mfa_test.go | 199 ++++++++++++++++++++++++++++++++++++ internal/tui/tui.go | 115 +++++++++++++++++++-- 3 files changed, 306 insertions(+), 12 deletions(-) create mode 100644 internal/tui/mfa_test.go diff --git a/internal/tui/config_test.go b/internal/tui/config_test.go index 0f56b83..581bbbe 100644 --- a/internal/tui/config_test.go +++ b/internal/tui/config_test.go @@ -1980,7 +1980,7 @@ func TestSigningInGivesTheClientTheNewToken(t *testing.T) { t.Fatal(err) } - updated, _ := m.Update(signedInMsg{nil}) + updated, _ := m.Update(signedInMsg{}) after := updated.(model) if after.sess.Token != "a-fresh-session-token" { @@ -2247,7 +2247,7 @@ func TestSigningInLeadsStraightToTheKey(t *testing.T) { t.Fatal(err) } - updated, _ := m.Update(signedInMsg{nil}) + updated, _ := m.Update(signedInMsg{}) after := updated.(model) if !after.editingKey { t.Error("after signing in the member is left to work out that `e` is next") diff --git a/internal/tui/mfa_test.go b/internal/tui/mfa_test.go new file mode 100644 index 0000000..322781d --- /dev/null +++ b/internal/tui/mfa_test.go @@ -0,0 +1,199 @@ +package tui + +import ( + "bytes" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + + tea "github.com/charmbracelet/bubbletea" + + "github.com/nxssie/nan-cli/internal/auth" + "github.com/nxssie/nan-cli/internal/auth/authtest" + "github.com/nxssie/nan-cli/internal/session" +) + +// Two-factor sign-in from the panel: the same flow as `nan auth login`, +// through internal/auth, with the code asked for as a third question. + +const mfaTestLink = "https://cloud-api.nan.builders/api/auth/login/verify?token=" + authtest.LinkToken + +// atTheCode drives the panel up to the code question against a fake platform +// whose account has a factor: the address, the link, and the exchange that +// comes back with a challenge. +func atTheCode(t *testing.T) (model, *authtest.Platform) { + t.Helper() + p := authtest.New(t) + p.Factor = true + old := auth.Origin + auth.Origin = p.URL + t.Cleanup(func() { auth.Origin = old }) + + m := setupModel(t, &session.Session{}) + m.lay = newLayout(100, 40) + m.startLogin() + m.loginEmail = "member@example.com" + mod, _ := m.Update(linkSentMsg{}) + m = mod.(model) + + msg := signIn(mfaTestLink)() + if _, ok := msg.(mfaChallengeMsg); !ok { + t.Fatalf("the link came back as %#v, not a challenge", msg) + } + mod, _ = m.Update(msg) + return mod.(model), p +} + +// enter types value into the field and presses enter, running what that +// starts and handing its answer back to the panel. +func enter(t *testing.T, m model, value string) model { + t.Helper() + m.loginInput.SetValue(value) + mod, cmd := m.Update(tea.KeyMsg{Type: tea.KeyEnter}) + if cmd == nil { + t.Fatal("enter started nothing") + } + mod, _ = mod.(model).Update(cmd()) + return mod.(model) +} + +func TestThePanelAsksForTheCode(t *testing.T) { + m, _ := atTheCode(t) + if m.loginStage != loginAskCode || m.wizard != wizardLink { + t.Fatalf("stage %v, wizard %v", m.loginStage, m.wizard) + } + if m.loginInput.EchoMode == 0 { + t.Error("the code field echoes: a recovery code works until it is used") + } + flat := strings.Join(strings.Fields(m.View()), " ") + for _, want := range []string{"Authenticator code", "two-factor", "recovery codes"} { + if !strings.Contains(flat, want) { + t.Errorf("the screen does not say %q:\n%s", want, m.View()) + } + } + + m = enter(t, m, authtest.Code) + if m.loginStage != loginOff || m.loginChallenge != nil { + t.Errorf("still signing in after the right code: stage %v", m.loginStage) + } + sess, err := session.Load() + if err != nil || sess.Token != authtest.Session { + t.Fatalf("session = %+v, %v", sess, err) + } + if m.client.Token() != authtest.Session { + t.Error("the panel's client does not carry the new session") + } + assertNoChallengeUnderHome(t) +} + +// A wrong code keeps the same question, empty, with what went wrong and how +// many tries are left. +func TestThePanelAsksAgainAfterAWrongCode(t *testing.T) { + m, p := atTheCode(t) + m = enter(t, m, "000000") + if m.loginStage != loginAskCode { + t.Fatalf("a wrong code left the question: stage %v", m.loginStage) + } + if m.loginInput.Value() != "" { + t.Error("the wrong code is still in the field") + } + if !strings.Contains(m.loginMsg, "4 attempts left") { + t.Errorf("loginMsg = %q", m.loginMsg) + } + m = enter(t, m, authtest.Code) + if m.loginStage != loginOff { + t.Errorf("the right code after a wrong one did not sign in: stage %v", m.loginStage) + } + if n := len(p.Challenges()); n != 2 { + t.Errorf("%d challenge POSTs", n) + } +} + +// A sign-in that cannot go on - timed out, too many wrong codes, refused - +// spent its link, so the panel goes back to the address, filled in: enter +// asks for a new one. +func TestThePanelStartsAgainWhenTheSignInEnds(t *testing.T) { + m, p := atTheCode(t) + p.Expire() + m = enter(t, m, authtest.Code) + if m.loginStage != loginAskEmail || m.wizard != wizardEmail { + t.Fatalf("stage %v, wizard %v", m.loginStage, m.wizard) + } + if m.loginChallenge != nil { + t.Error("the panel keeps a challenge that is over") + } + if m.loginInput.Value() != "member@example.com" || m.loginInput.Prompt != "Email: " { + t.Errorf("field = %q %q", m.loginInput.Prompt, m.loginInput.Value()) + } + if !strings.HasPrefix(m.loginMsg, "error: ") || !strings.Contains(m.loginMsg, "new link") { + t.Errorf("loginMsg = %q", m.loginMsg) + } + if s, err := session.Load(); err == nil && s.Token != "" { + t.Errorf("saved a session: %+v", s) + } +} + +func TestThePanelSaysHowManyRecoveryCodesAreLeft(t *testing.T) { + m, p := atTheCode(t) + p.RecoveryCodes = []string{"q7wa-z2xd-pl4r-6wze", "aaaa-bbbb-cccc-dddd"} + m = enter(t, m, "Q7WA-Z2XD-PL4R-6WZE") + if m.loginStage != loginOff || m.wizard != wizardKey { + t.Fatalf("stage %v, wizard %v", m.loginStage, m.wizard) + } + flat := strings.Join(strings.Fields(m.View()), " ") + if !strings.Contains(flat, "1 left") || !strings.Contains(flat, "Two-factor authentication") { + t.Errorf("the next step does not say how many are left:\n%s", m.View()) + } +} + +// Esc leaves the code question and forgets the challenge; an exchange that +// answers after the member left does not bring the question back. +func TestEscapeForgetsTheChallenge(t *testing.T) { + m, _ := atTheCode(t) + mod, _ := m.Update(tea.KeyMsg{Type: tea.KeyEsc}) + m = mod.(model) + if m.loginStage != loginOff || m.loginChallenge != nil { + t.Errorf("stage %v, challenge kept: %v", m.loginStage, m.loginChallenge != nil) + } + + m2, _ := atTheCode(t) + challenge := m2.loginChallenge + m2.cancelLogin() + mod, _ = m2.Update(mfaChallengeMsg{challenge}) + if got := mod.(model); got.loginStage != loginOff || got.loginChallenge != nil { + t.Errorf("a late challenge reopened the sign-in: stage %v", got.loginStage) + } +} + +// The screen never shows the challenge cookie, and nothing under HOME holds +// it. +func TestThePanelKeepsTheChallengeToItself(t *testing.T) { + m, _ := atTheCode(t) + m = enter(t, m, "000000") + if strings.Contains(m.View(), authtest.Pending) || strings.Contains(m.loginMsg, authtest.Pending) { + t.Error("the challenge cookie is on screen") + } + assertNoChallengeUnderHome(t) +} + +func assertNoChallengeUnderHome(t *testing.T) { + t.Helper() + err := filepath.WalkDir(os.Getenv("HOME"), func(path string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return err + } + data, err := os.ReadFile(path) + if err != nil { + return err + } + if bytes.Contains(data, []byte(authtest.Pending)) { + t.Errorf("%s holds the challenge cookie", path) + } + return nil + }) + if err != nil { + t.Fatal(err) + } +} diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 581fae1..b390dbd 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -153,7 +153,17 @@ type configuredMsg struct { type firstRunMsg struct{} type linkSentMsg struct{ err error } -type signedInMsg struct{ err error } + +// signedInMsg ends the sign-in: err when it failed, and notice for something +// the member should read once it worked (a recovery code spent). +type signedInMsg struct { + err error + notice string +} + +// mfaChallengeMsg is a link that worked for an account with two-factor +// authentication: the session comes once the challenge is answered. +type mfaChallengeMsg struct{ challenge *auth.MFARequiredError } // signedOutMsg is the platform's answer to ending the session (err nil when it // confirmed). @@ -205,6 +215,11 @@ type model struct { loginEmail string loginMsg string loginBusy bool + // The two-factor challenge the link opened, while the code is asked + // for. In memory only, like everything else about it. + loginChallenge *auth.MFARequiredError + // Said on the steps after the sign-in, once: a recovery code was spent. + loginNotice string } // Where a member is in the guided setup. The inner inputs still drive @@ -262,13 +277,15 @@ func (w wizardStep) mood() mascotMood { } // Where a member is in the sign-in flow. It is two questions - an address and -// the link that arrives at it. +// the link that arrives at it - and a third for an account with two-factor +// authentication: the code from the authenticator app. type loginStage int const ( loginOff loginStage = iota loginAskEmail loginAskLink + loginAskCode ) func newModel(client *api.Client, sess *session.Session) model { @@ -425,10 +442,44 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.loginInput.EchoCharacter = '•' return m, m.loginInput.Focus() + case mfaChallengeMsg: + m.loginBusy = false + // Left with esc while the link was being exchanged: nothing to ask. + if m.loginStage != loginAskLink { + return m, nil + } + m.loginChallenge = msg.challenge + m.loginStage = loginAskCode + m.loginMsg = "this account uses two-factor authentication — enter the 6-digit code from your" + + " authenticator app, or one of your recovery codes" + m.loginInput.SetValue("") + m.loginInput.Placeholder = "123456" + m.loginInput.Prompt = "Authenticator code: " + // A recovery code works until it is used, so it is not echoed either. + m.loginInput.EchoMode = textinput.EchoPassword + m.loginInput.EchoCharacter = '•' + return m, m.loginInput.Focus() + case signedInMsg: m.loginBusy = false if msg.err != nil { m.loginMsg = "error: " + msg.err.Error() + if m.loginStage == loginAskCode { + if auth.Retryable(msg.err) { + m.loginInput.SetValue("") + } else { + // The challenge is over, and the link that opened it was + // spent opening it: back to the address, filled in, so + // enter asks for a new one. + m.loginChallenge = nil + m.wizard = wizardEmail + m.loginStage = loginAskEmail + m.loginInput.SetValue(m.loginEmail) + m.loginInput.Placeholder = "you@example.com" + m.loginInput.Prompt = "Email: " + m.loginInput.EchoMode = textinput.EchoNormal + } + } return m, m.loginInput.Focus() } // Reload the session so the token is the one just written, and drop @@ -444,6 +495,7 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.client = api.New(m.sess.Token) wasGuided := m.wizard != wizardOff m.cancelLogin() + m.loginNotice = msg.notice if wasGuided { m.wizard = wizardKey } @@ -507,6 +559,10 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.loginMsg = "sending a link to " + value + "…" return m, sendLink(value) } + if m.loginStage == loginAskCode { + m.loginMsg = "checking the code…" + return m, answerChallenge(m.loginChallenge, value) + } m.loginMsg = "signing in…" return m, signIn(value) case "esc": @@ -771,6 +827,8 @@ func (m *model) startLogin() tea.Cmd { m.wizard = wizardEmail m.loginStage = loginAskEmail m.loginMsg = "" + m.loginNotice = "" + m.loginChallenge = nil // A machine set up with a platform token looks signed in to whoever set // it up. The dashboard cannot use one, and asking for an email with no // word about it reads as the token having been lost. @@ -832,6 +890,7 @@ func (m *model) resumeSetup() tea.Cmd { func (m *model) cancelLogin() { m.wizard = wizardOff m.loginStage = loginOff + m.loginChallenge = nil m.loginBusy = false m.loginInput.Blur() m.loginInput.SetValue("") @@ -846,24 +905,57 @@ func signIn(pasted string) tea.Cmd { return func() tea.Msg { token, err := auth.TokenFromLink(strings.TrimSpace(pasted)) if err != nil { - return signedInMsg{err} + return signedInMsg{err: err} } sessionToken, err := auth.ExchangeToken(token) + var challenge *auth.MFARequiredError + if errors.As(err, &challenge) { + return mfaChallengeMsg{challenge} + } if err != nil { - return signedInMsg{err} + return signedInMsg{err: err} } - current, err := session.Load() + return signedInMsg{err: saveSessionToken(sessionToken)} + } +} + +// answerChallenge posts the code the member typed against the challenge the +// link opened. +func answerChallenge(challenge *auth.MFARequiredError, code string) tea.Cmd { + return func() tea.Msg { + signIn, err := challenge.Complete(code) if err != nil { - current = &session.Session{} + return signedInMsg{err: err} } - current.Token = sessionToken - if err := session.Save(current); err != nil { - return signedInMsg{err} + if err := saveSessionToken(signIn.Session); err != nil { + return signedInMsg{err: err} } - return signedInMsg{nil} + if !signIn.UsedRecoveryCode { + return signedInMsg{} + } + return signedInMsg{notice: recoveryCodesNotice(signIn.RecoveryCodesLeft)} } } +// recoveryCodesNotice is what the panel says after a recovery code got the +// member in: each works once, and running out is the way to be locked out. +func recoveryCodesNotice(left int) string { + const where = "cloud.nan.builders, Settings > Account > Two-factor authentication" + if left == 0 { + return "that was your last recovery code — make new ones before you need one: " + where + } + return fmt.Sprintf("signed in with a recovery code, %d left — you can make new ones at %s", left, where) +} + +func saveSessionToken(token string) error { + current, err := session.Load() + if err != nil { + current = &session.Session{} + } + current.Token = token + return session.Save(current) +} + func (m model) fetchKeyStatus() tea.Cmd { client := m.client return func() tea.Msg { @@ -3380,6 +3472,9 @@ func (m model) renderWizard(l layout) string { b.WriteString("\n" + m.wrapped(l, m.loginMsg) + "\n") } case wizardKey: + if m.loginNotice != "" { + b.WriteString(m.wrapped(l, m.loginNotice) + "\n\n") + } if m.editingKey { b.WriteString(l.indent + m.keyInput.View() + "\n") } From f5dda2965802b23c793ddb56666b9fdbee83c39e Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 17:35:33 +0200 Subject: [PATCH 04/14] docs(readme): two-factor sign-in; bump version to 0.1.35 Merging this to main publishes v0.1.35: tag.yml tags any commit on main whose internal/tui.Version has no tag yet, and calls release.yml. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- README.md | 14 ++++++++++++++ internal/tui/tui.go | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 3d34166..4f153d4 100644 --- a/README.md +++ b/README.md @@ -330,6 +330,20 @@ nan auth login --api-token < token.txt # or pipe it in Never put a token on the command line itself: it would end up in your shell history and in `ps`. +**Two-factor authentication.** With it turned on for your account (Settings > +Account > Two-factor authentication at ), +`nan auth login` and the dashboard's sign-in ask for the code from your +authenticator app after the link, without echo. One of your recovery codes +works in its place. With no terminal to ask on, pipe the code in: + +```bash +echo 123456 | nan auth login --link "" --totp - +``` + +`--totp` takes only `-`, so a recovery code never ends up in your shell +history. Versions before 0.1.35 cannot ask for the code: the platform refuses +the link with `mfa enabled update nan cli`. Update, then ask for a new link. + ## Build from source Requires Go 1.26.10+ ([mise](https://mise.jdx.dev/) recommended) — the version diff --git a/internal/tui/tui.go b/internal/tui/tui.go index b390dbd..561325d 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -3756,7 +3756,7 @@ func (m model) renderSetup(l layout) string { // ── about renderer ─────────────────────────────────────────────────────────── -const Version = "0.1.34" +const Version = "0.1.35" func renderAbout(l layout) string { var b strings.Builder From 468614473672ede73c9ad76f992cdff9f8f08669 Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 18:14:23 +0200 Subject: [PATCH 05/14] fix(auth): take only base32 as a recovery code Recovery codes are 16 characters of the RFC 4648 base32 alphabet (A-Z, 2-7), which is what the platform makes them from and all it accepts (nan-cloud-api internal/mfa/recovery.go NormalizeRecoveryCode). The local check took any 16 letters and digits, so a 0, 1, 8 or 9 typo was posted and came back as invalid_request instead of being refused here. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- internal/auth/mfa.go | 15 ++++++++++----- internal/auth/mfa_test.go | 5 ++++- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/internal/auth/mfa.go b/internal/auth/mfa.go index fb0c418..e6ef9e0 100644 --- a/internal/auth/mfa.go +++ b/internal/auth/mfa.go @@ -185,8 +185,11 @@ func (e *MFAError) wait() string { } // normalizeAnswer decides what the member typed, the way the platform does: six -// digits once the spaces are out is an authenticator code, sixteen letters and -// digits a recovery code - in any case, with or without its hyphens. +// digits once the spaces are out is an authenticator code, and sixteen +// characters of the RFC 4648 base32 alphabet (A-Z and 2-7) a recovery code - in +// any case, with or without its hyphens. That is the alphabet the platform +// makes them in, so a 0, 1, 8 or 9 is a typo, refused before it costs one of +// the sign-in's attempts. func normalizeAnswer(input string) (field, value string, err error) { compact := strings.Map(func(r rune) rune { switch r { @@ -199,7 +202,7 @@ func normalizeAnswer(input string) (field, value string, err error) { if len(compact) == 6 && allASCII(compact, isDigit) { return "code", compact, nil } - if len(compact) == 16 && allASCII(compact, isAlnum) { + if len(compact) == 16 && allASCII(compact, isBase32) { c := strings.ToLower(compact) return "recovery_code", c[0:4] + "-" + c[4:8] + "-" + c[8:12] + "-" + c[12:16], nil } @@ -223,8 +226,10 @@ func allASCII(s string, ok func(byte) bool) bool { } func isDigit(b byte) bool { return b >= '0' && b <= '9' } -func isAlnum(b byte) bool { - return isDigit(b) || (b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') + +// isBase32 is the RFC 4648 alphabet, in either case. +func isBase32(b byte) bool { + return (b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') || (b >= '2' && b <= '7') } // Complete answers the challenge with what the member typed: the code their diff --git a/internal/auth/mfa_test.go b/internal/auth/mfa_test.go index 1e05cda..7d9abb4 100644 --- a/internal/auth/mfa_test.go +++ b/internal/auth/mfa_test.go @@ -321,7 +321,10 @@ func rh(status int, body, k, v string) authtest.Refusal { func TestNotACodeIsNeverSent(t *testing.T) { p := fakePlatform(t) ch := challengeFor(t, p) - for _, typed := range []string{"", "12345", "1234567", "abcdef", "q7wa-z2xd-pl4r", "q7wa-z2xd-pl4r-6wze-xxxx", "246810\x00", "２４６８１０"} { + for _, typed := range []string{"", "12345", "1234567", "abcdef", "q7wa-z2xd-pl4r", "q7wa-z2xd-pl4r-6wze-xxxx", "246810\x00", "２４６８１０", + // Sixteen characters, but not the alphabet recovery codes are made + // in: RFC 4648 base32 has no 0, 1, 8 or 9. + "q7wa-z2xd-pl4r-6wz0", "q7wa-z2xd-pl4r-6wz1", "Q7WA Z2XD PL4R 6WZ8", "q7waz2xdpl4r6wz9"} { if _, err := ch.Complete(typed); !errors.Is(err, ErrNotACode) || !Retryable(err) { t.Errorf("Complete(%q) = %v", typed, err) } From 4d81856c151e0bb0ad9cb429df076af717e80c6d Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 18:14:49 +0200 Subject: [PATCH 06/14] fix(auth): drop mfa_totp_suspended, which the contract no longer has The user accepted OQ-11 (escalating lock, capped at 24 h), so the 100-failure suspension and its mfa_totp_suspended code were removed from the API contract (nan-cloud-api 980a2ec) and are not implemented. The refusal mapping is now exactly the contract's; an unknown code still gets said, and ends the sign-in. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- internal/auth/mfa.go | 7 +------ internal/auth/mfa_test.go | 1 - 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/internal/auth/mfa.go b/internal/auth/mfa.go index e6ef9e0..a22ee89 100644 --- a/internal/auth/mfa.go +++ b/internal/auth/mfa.go @@ -90,9 +90,7 @@ func Retryable(err error) bool { switch e.Code { case "mfa_code_invalid", "mfa_recovery_code_invalid", "mfa_code_already_used", // Nothing was spent: the platform could not read what was sent. - "invalid_request", - // Authenticator codes refused for good; a recovery code still works. - "mfa_totp_suspended": + "invalid_request": return true } return false @@ -108,9 +106,6 @@ func (e *MFAError) Error() string { return "that code was already used — wait for the next one in your authenticator app" + attemptsLeft(e.Attempts) case "invalid_request": return "nan.builders could not read that — type the 6-digit code from your authenticator app, or a recovery code (xxxx-xxxx-xxxx-xxxx)" - case "mfa_totp_suspended": - return "authenticator codes are suspended on this account after too many wrong ones — " + - "use one of your recovery codes, or ask NaN to reset your two-factor authentication" case "mfa_challenge_exhausted": return "too many wrong codes for this sign-in — " + newLinkHint case "mfa_challenge_expired": diff --git a/internal/auth/mfa_test.go b/internal/auth/mfa_test.go index 7d9abb4..059dce0 100644 --- a/internal/auth/mfa_test.go +++ b/internal/auth/mfa_test.go @@ -272,7 +272,6 @@ func TestEveryRefusalInTheContract(t *testing.T) { {"rate limited", rh(429, `{"error":"rate_limited","message":"x","retry_after":60}`, "Retry-After", "60"), "rate_limited", false, []string{"wait 1 minute", "new link"}}, {"rate limited, header only", rh(429, `{"error":"rate_limited","message":"x"}`, "Retry-After", "45"), "rate_limited", false, []string{"wait 45 seconds"}}, {"another origin", r(403, `{"error":"forbidden_origin"}`), "forbidden_origin", false, []string{"forbidden origin", "HTTP 403"}}, - {"codes suspended", r(403, `{"error":"mfa_totp_suspended","message":"x"}`), "mfa_totp_suspended", true, []string{"recovery codes"}}, {"server error", r(500, `{"error":"internal_error","message":"x"}`), "internal_error", false, []string{"could not check the code", "HTTP 500"}}, {"two-factor turned off", r(404, "404 page not found\n"), "", false, []string{"HTTP 404", "new link"}}, {"not our body", r(502, ""+authtest.Pending+"\x1b[31m"), "", false, []string{"HTTP 502"}}, From 44d5f41ea86e5dee18e0f9c59de741f0a95706a7 Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 18:15:33 +0200 Subject: [PATCH 07/14] refactor(auth): SignInBaseURL, and what a QA build must know auth.Origin read like the Origin header, which this same package takes care never to send. Renamed to SignInBaseURL, and its comment now says what it reaches: the link request, the exchange and the challenge, and nothing else - internal/api (nan me, logout, runs, the panel's tabs) stays on production. So a QA build pointed at dev has to run with a HOME of its own: it writes the dev session over the member's real one, and every command after the sign-in sends it to production. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- cmd/auth_mfa_test.go | 2 +- internal/auth/auth.go | 29 ++++++++++++++++++++--------- internal/auth/auth_test.go | 6 +++--- internal/auth/mfa.go | 2 +- internal/auth/mfa_test.go | 12 ++++++------ internal/tui/mfa_test.go | 6 +++--- 6 files changed, 34 insertions(+), 23 deletions(-) diff --git a/cmd/auth_mfa_test.go b/cmd/auth_mfa_test.go index 64641ff..48e81ff 100644 --- a/cmd/auth_mfa_test.go +++ b/cmd/auth_mfa_test.go @@ -25,7 +25,7 @@ func mfaPlatform(t *testing.T, factor bool) *authtest.Platform { t.Helper() p := authtest.New(t) p.Factor = factor - swap(t, &auth.Origin, p.URL) + swap(t, &auth.SignInBaseURL, p.URL) for _, f := range []*string{&linkFlag, &totpFlag, &tokenFlag, &emailFlag} { swap(t, f, "") } diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 25fb1e1..809db67 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -33,19 +33,30 @@ const ( LinkValidity = "15 minutes" ) -// Origin is where every call of the sign-in goes. A variable and not a -// constant for two readers only: the tests, which point the flow at a server -// of their own, and a build for QA, which points it at the dev platform -// without a flag or an environment variable that a released binary would -// carry: +// SignInBaseURL is where the three calls of the sign-in go: the link +// request, the link exchange and the two-factor challenge. Nothing else: the +// rest of the CLI (internal/api - nan me, logout, runs, the panel's tabs) +// keeps its own production URLs. // -// go build -ldflags "-X github.com/nxssie/nan-cli/internal/auth.Origin=https://cloud-api-dev.nan.builders" +// A variable and not a constant for two readers only: the tests, which point +// the sign-in at a server of their own, and a build for QA, which points it at +// the dev platform without a flag or an environment variable that a released +// binary would carry: +// +// go build -ldflags "-X github.com/nxssie/nan-cli/internal/auth.SignInBaseURL=https://cloud-api-dev.nan.builders" -o nan-qa . +// HOME=$(mktemp -d) ./nan-qa auth login --email you@example.com +// +// Run a QA build with a HOME of its own. It saves the dev session in +// ~/.config/nan/session.json, over the member's production one, and every +// command after the sign-in sends that dev session to production: the panel +// takes the refusal for an expired session and clears it, and logout asks +// production, not dev, to end it. // // Nothing else sets it. -var Origin = "https://cloud-api.nan.builders" +var SignInBaseURL = "https://cloud-api.nan.builders" -func loginRequestURL() string { return Origin + "/api/auth/login/request" } -func loginVerifyURL() string { return Origin + "/api/auth/login/verify" } +func loginRequestURL() string { return SignInBaseURL + "/api/auth/login/request" } +func loginVerifyURL() string { return SignInBaseURL + "/api/auth/login/verify" } // clientKind is what this CLI declares on the verify POST, so the platform // records the session it hands back as a CLI session. A CLI session can diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 8cb3c8c..b057b2e 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -109,9 +109,9 @@ func TestExchangeTokenDeclaresTheCLI(t *testing.T) { http.Redirect(w, r, "/dashboard", http.StatusSeeOther) })) defer srv.Close() - old := Origin - Origin = srv.URL - defer func() { Origin = old }() + old := SignInBaseURL + SignInBaseURL = srv.URL + defer func() { SignInBaseURL = old }() got, err := ExchangeToken("tok") if err != nil || got != "sess" { diff --git a/internal/auth/mfa.go b/internal/auth/mfa.go index a22ee89..34a2d37 100644 --- a/internal/auth/mfa.go +++ b/internal/auth/mfa.go @@ -34,7 +34,7 @@ const ( newLinkHint = "ask for a new link and sign in again" ) -func mfaChallengeURL() string { return Origin + "/api/auth/mfa/challenge" } +func mfaChallengeURL() string { return SignInBaseURL + "/api/auth/mfa/challenge" } // MFARequiredError is what ExchangeToken returns when the link was good and // the account asks for a second factor. Answer it with Complete. diff --git a/internal/auth/mfa_test.go b/internal/auth/mfa_test.go index 059dce0..a0d1144 100644 --- a/internal/auth/mfa_test.go +++ b/internal/auth/mfa_test.go @@ -18,9 +18,9 @@ import ( func fakePlatform(t *testing.T) *authtest.Platform { t.Helper() p := authtest.New(t) - old := Origin - Origin = p.URL - t.Cleanup(func() { Origin = old }) + old := SignInBaseURL + SignInBaseURL = p.URL + t.Cleanup(func() { SignInBaseURL = old }) return p } @@ -356,9 +356,9 @@ func TestMFAReasonWithoutTheCookie(t *testing.T) { http.Redirect(w, r, authtest.FrontendURL+"/mfa?reason=mfa_required", http.StatusFound) })) defer srv.Close() - old := Origin - Origin = srv.URL - defer func() { Origin = old }() + old := SignInBaseURL + SignInBaseURL = srv.URL + defer func() { SignInBaseURL = old }() _, err := ExchangeToken(authtest.LinkToken) var ch *MFARequiredError diff --git a/internal/tui/mfa_test.go b/internal/tui/mfa_test.go index 322781d..702a1fb 100644 --- a/internal/tui/mfa_test.go +++ b/internal/tui/mfa_test.go @@ -27,9 +27,9 @@ func atTheCode(t *testing.T) (model, *authtest.Platform) { t.Helper() p := authtest.New(t) p.Factor = true - old := auth.Origin - auth.Origin = p.URL - t.Cleanup(func() { auth.Origin = old }) + old := auth.SignInBaseURL + auth.SignInBaseURL = p.URL + t.Cleanup(func() { auth.SignInBaseURL = old }) m := setupModel(t, &session.Session{}) m.lay = newLayout(100, 40) From fc3833278a94775d8c9458389ac118e628bacf02 Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 18:15:57 +0200 Subject: [PATCH 08/14] docs(login): --totp - on a terminal asks without echo, once With --totp - the code gets a single try, and when stdin is a terminal it is still read there without echo rather than echoed back. Said in nan auth login --help and the README, and pinned by a test. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- README.md | 3 ++- cmd/auth.go | 4 +++- cmd/auth_mfa_test.go | 23 +++++++++++++++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 4f153d4..890d0e2 100644 --- a/README.md +++ b/README.md @@ -341,7 +341,8 @@ echo 123456 | nan auth login --link "" --totp - ``` `--totp` takes only `-`, so a recovery code never ends up in your shell -history. Versions before 0.1.35 cannot ask for the code: the platform refuses +history. With `--totp -` the code gets one try, where the prompt lets you try +again; if stdin is a terminal it is still asked for there, without echo. Versions before 0.1.35 cannot ask for the code: the platform refuses the link with `mfa enabled update nan cli`. Update, then ask for a new link. ## Build from source diff --git a/cmd/auth.go b/cmd/auth.go index 2537f82..2aa2c27 100644 --- a/cmd/auth.go +++ b/cmd/auth.go @@ -38,7 +38,9 @@ var loginCmd = &cobra.Command{ With two-factor authentication on, the link is followed by the code from your authenticator app, or one of your recovery codes. It is asked for -without echo; with no terminal to ask on, pipe it in with --totp -. +without echo, again after a wrong one; with no terminal to ask on, pipe it +in with --totp -. With --totp - the code gets one try, and if stdin is a +terminal it is still asked for there without echo. With --api-token, save a platform token instead, for a machine with nobody at the keyboard (a server, a CI runner). Create the token in Settings > diff --git a/cmd/auth_mfa_test.go b/cmd/auth_mfa_test.go index 48e81ff..82edc45 100644 --- a/cmd/auth_mfa_test.go +++ b/cmd/auth_mfa_test.go @@ -238,6 +238,29 @@ func TestTOTPFromStdin(t *testing.T) { }) } +// --totp - with a terminal on stdin: the code is still read without echo, +// and it still gets one try, as the help says. +func TestTOTPFromATerminalStdin(t *testing.T) { + home := fakeHome(t, "") + p := mfaPlatform(t, true) + codes, asked := typed("000000", authtest.Code) + codes.fromStdin = true + var prompt bytes.Buffer + err := finishSignIn(authtest.LinkToken, codes, io.Discard, &prompt) + if err == nil || !strings.Contains(err.Error(), "not right") { + t.Fatalf("err = %v", err) + } + if *asked != 1 || len(p.Challenges()) != 1 { + t.Errorf("%d prompts, %d POSTs: --totp - gets one try", *asked, len(p.Challenges())) + } + if !strings.Contains(prompt.String(), "Authenticator code: ") { + t.Errorf("not asked on the terminal: %q", prompt.String()) + } + if sess, _ := readSession(t, home); sess != nil { + t.Errorf("saved %v", sess) + } +} + // No terminal and no --totp: the link is already spent, so the message says // to ask for another and names the flag that would have worked. func TestNoTerminalAndNoFlag(t *testing.T) { From a055ed133493a81868e9c0b6e627ab03d5502fae Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 18:18:03 +0200 Subject: [PATCH 09/14] fix(login): put the terminal back on ctrl-c at a hidden prompt term.ReadPassword turns echo off and only restores it when it returns; ctrl-c in between killed the process first and left the shell without echo. While "Authenticator code:" or the --api-token prompt is up, the interrupt is now caught: the terminal state is restored, then the command exits 130 as an interrupted one does. Checked in a pty with script(1): after ctrl-c at the --api-token prompt, a build of main leaves the terminal at -echo, this one at echo. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- cmd/auth.go | 49 +++++++++++++++++++++++++++++++++++++++-- cmd/hidden_unix_test.go | 35 +++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 cmd/hidden_unix_test.go diff --git a/cmd/auth.go b/cmd/auth.go index 2aa2c27..28d9d2c 100644 --- a/cmd/auth.go +++ b/cmd/auth.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "os" + "os/signal" "strings" "github.com/charmbracelet/x/term" @@ -285,6 +286,50 @@ func recoveryCodesLeft(n int) string { return fmt.Sprintf("You signed in with a recovery code: %d left. You can make new ones in the browser: %s.", n, where) } +// readHiddenLine reads a line from the terminal f without echo. +// +// term.ReadPassword turns echo off and only puts it back when it returns. A +// ctrl-c in between ends the process first, and the shell it hands back to is +// left without echo: whatever is typed next is invisible until `reset`. So for +// as long as the prompt is up, an interrupt puts the terminal back before the +// command exits. +func readHiddenLine(f *os.File) ([]byte, error) { + fd := f.Fd() + if state, err := term.GetState(fd); err == nil { + release := onInterrupt(func() { + _ = term.Restore(fd, state) + fmt.Fprintln(os.Stderr) + }) + defer release() + } + return term.ReadPassword(fd) +} + +// interrupted ends the command after ctrl-c, with the status a shell gives an +// interrupted one. A variable for the tests. +var interrupted = func() { os.Exit(130) } + +// onInterrupt runs undo and then interrupted if ctrl-c arrives before release +// is called. Until then the interrupt is this function's to handle, not the +// runtime's, which would exit without undoing anything. +func onInterrupt(undo func()) (release func()) { + sig := make(chan os.Signal, 1) + signal.Notify(sig, os.Interrupt) + done := make(chan struct{}) + go func() { + select { + case <-sig: + undo() + interrupted() + case <-done: + } + }() + return func() { + signal.Stop(sig) + close(done) + } +} + // codeInput is where the second factor comes from: a hidden prompt on a // terminal, or one line of stdin with --totp -. type codeInput struct { @@ -300,7 +345,7 @@ func newCodeInput(f *os.File, readLine func() (string, error)) codeInput { return codeInput{ tty: term.IsTerminal(f.Fd()), fromStdin: totpFlag == "-", - readHidden: func() ([]byte, error) { return term.ReadPassword(f.Fd()) }, + readHidden: func() ([]byte, error) { return readHiddenLine(f) }, readLine: readLine, } } @@ -415,7 +460,7 @@ func newTokenInput(f *os.File) tokenInput { return tokenInput{ r: f, tty: term.IsTerminal(f.Fd()), - readHidden: func() ([]byte, error) { return term.ReadPassword(f.Fd()) }, + readHidden: func() ([]byte, error) { return readHiddenLine(f) }, } } diff --git a/cmd/hidden_unix_test.go b/cmd/hidden_unix_test.go new file mode 100644 index 0000000..2c5c93e --- /dev/null +++ b/cmd/hidden_unix_test.go @@ -0,0 +1,35 @@ +//go:build unix + +package cmd + +import ( + "os" + "sync/atomic" + "syscall" + "testing" + "time" +) + +// Ctrl-c at a hidden prompt used to end the process while echo was off, and +// the shell stayed that way. While the prompt is up, the interrupt is caught: +// the terminal is put back first, and only then does the command exit. +func TestCtrlCAtAHiddenPromptPutsTheTerminalBackFirst(t *testing.T) { + var restored atomic.Bool + exited := make(chan bool, 1) + swap(t, &interrupted, func() { exited <- restored.Load() }) + + release := onInterrupt(func() { restored.Store(true) }) + defer release() + + if err := syscall.Kill(os.Getpid(), syscall.SIGINT); err != nil { + t.Fatal(err) + } + select { + case restoredFirst := <-exited: + if !restoredFirst { + t.Error("exited before putting the terminal back") + } + case <-time.After(5 * time.Second): + t.Fatal("ctrl-c at the prompt was not handled") + } +} From 68752f7b686a2c75b908d97a053f3ae2048a05d5 Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 18:54:56 +0200 Subject: [PATCH 10/14] docs(login): keep the code off the command line in --totp examples `echo | nan auth login ... --totp -` put the code in the shell history all the same, which is what --totp only taking "-" was meant to prevent. The help, the hint the errors print and the README now read it into a variable without echo (read -rs) or redirect it from a file or a password manager, and the promise says what holds: --totp never takes the code as an argument. A test fails if any --totp - example puts a code on the command line again. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- README.md | 20 ++++++++++++++------ cmd/auth.go | 17 +++++++++++------ cmd/auth_mfa_test.go | 29 +++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 890d0e2..579f787 100644 --- a/README.md +++ b/README.md @@ -334,16 +334,24 @@ history and in `ps`. Account > Two-factor authentication at ), `nan auth login` and the dashboard's sign-in ask for the code from your authenticator app after the link, without echo. One of your recovery codes -works in its place. With no terminal to ask on, pipe the code in: +works in its place. When nan cannot ask, pipe the code in. Never type the code +into the command itself, where it would stay in your shell history: read it +into a variable without echo, or redirect it from a file or a password +manager's command line. ```bash -echo 123456 | nan auth login --link "" --totp - +read -rs code # type or paste it; it is not shown +printf '%s\n' "$code" | nan auth login --link "" --totp - +nan auth login --link "" --totp - < code.txt ``` -`--totp` takes only `-`, so a recovery code never ends up in your shell -history. With `--totp -` the code gets one try, where the prompt lets you try -again; if stdin is a terminal it is still asked for there, without echo. Versions before 0.1.35 cannot ask for the code: the platform refuses -the link with `mfa enabled update nan cli`. Update, then ask for a new link. +`--totp` never takes the code as an argument: its only value is `-`, which +reads one line of stdin. With `--totp -` the code gets one try, where the +prompt lets you try again; if stdin is a terminal it is still asked for there, +without echo. + +Versions before 0.1.35 cannot ask for the code: the platform refuses the link +with `mfa enabled update nan cli`. Update, then ask for a new link. ## Build from source diff --git a/cmd/auth.go b/cmd/auth.go index 28d9d2c..52dbf96 100644 --- a/cmd/auth.go +++ b/cmd/auth.go @@ -54,7 +54,7 @@ replaces the one the Setup tab saved.`, Example: ` nan auth login --email you@example.com nan auth login --api-token # paste the token; it is not shown nan auth login --api-token < token.txt - echo 123456 | nan auth login --link "" --totp - # two-factor, no prompt`, + read -rs code; printf '%s\n' "$code" | nan auth login --link "" --totp - # two-factor, no prompt from nan`, RunE: runLogin, } @@ -197,16 +197,18 @@ func runLogin(cmd *cobra.Command, args []string) error { // checkTOTPFlag refuses the --totp spellings that cannot work. // -// Only "-": a recovery code is good until it is used, and one written on the -// command line is in the shell's history and in `ps` for as long as that -// lasts. And not next to another flag that reads stdin, because the first one +// Only "-": --totp never takes the code as an argument. A recovery code is +// good until it is used, and one written on the command line is in the +// shell's history and in `ps` for as long as that lasts. The examples keep it +// off the command line too - read into a variable, or from a file - because +// `echo |` would put it in the history just the same. And not next to another flag that reads stdin, because the first one // would take everything and leave the code nothing. func checkTOTPFlag() error { switch { case totpFlag == "": return nil case totpFlag != "-": - return fmt.Errorf(`--totp only takes "-", reading the code from stdin, so a recovery code never lands in your shell history:%s`, totpHint) + return fmt.Errorf(`--totp only takes "-": it never takes the code as an argument, where it would stay in your shell history and in ps. Read it from stdin:%s`, totpHint) case tokenFlag != "": return fmt.Errorf("--totp cannot be combined with --token: a saved session token skips the sign-in, and its code") case linkFlag == "-": @@ -215,7 +217,10 @@ func checkTOTPFlag() error { return nil } -const totpHint = "\n\n echo | nan auth login --link \"\" --totp -" +// totpHint is how to pipe the code in without typing it into the command: +// the variable is filled without echo and never appears in the history. +const totpHint = "\n\n read -rs code; printf '%s\\n' \"$code\" | nan auth login --link \"\" --totp -\n\n" + + "or redirect it from a file or a password manager: --totp - < code.txt" // finishSignIn spends the link, answers the two-factor challenge when the // account has one, and saves the session. diff --git a/cmd/auth_mfa_test.go b/cmd/auth_mfa_test.go index 82edc45..6db2c91 100644 --- a/cmd/auth_mfa_test.go +++ b/cmd/auth_mfa_test.go @@ -337,6 +337,35 @@ func TestTOTPFlagSpellings(t *testing.T) { } } +// No example of --totp - shows the code typed into the command, which would +// put it in the shell history the flag exists to keep it out of: not the +// help, not the hint the errors print, not the README. +func TestTOTPExamplesKeepTheCodeOffTheCommandLine(t *testing.T) { + readme, err := os.ReadFile(filepath.Join("..", "README.md")) + if err != nil { + t.Fatal(err) + } + for name, text := range map[string]string{ + "nan auth login --help": loginCmd.Example + "\n" + loginCmd.Long, + "the errors' hint": totpHint, + "README.md": string(readme), + } { + var examples int + for _, line := range strings.Split(text, "\n") { + if !strings.Contains(line, "--totp -") { + continue + } + examples++ + if strings.Contains(line, "echo ") || strings.Contains(line, "") || strings.Contains(line, authtest.Code) { + t.Errorf("%s puts the code on the command line: %q", name, strings.TrimSpace(line)) + } + } + if examples == 0 { + t.Errorf("%s has no --totp - example left to check", name) + } + } +} + // The challenge cookie is a credential for ten minutes and is kept in memory // only. Whatever the sign-in wrote under HOME - on success and on failure - // does not contain it, and neither does anything it printed. From 88e9b9eaa02ee39250c9548f4da66ee31e7a9fc9 Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 18:56:23 +0200 Subject: [PATCH 11/14] test(auth): the challenge POST never follows a redirect The client refused redirects with nothing to pin it. A 302, 303, 307 or 308 from the challenge route now gets a test: the server it points at answers like a successful challenge, and must get no request. With the guard removed it gets the __Host-nan_mfa_pending cookie (Go forwards Cookie to the same host on another port) and, on a 307 or 308, the code in the replayed body, and the CLI signs in with its session. The reason is now written next to the guard. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- internal/auth/mfa.go | 5 +++++ internal/auth/mfa_test.go | 41 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/internal/auth/mfa.go b/internal/auth/mfa.go index 34a2d37..d6844b1 100644 --- a/internal/auth/mfa.go +++ b/internal/auth/mfa.go @@ -253,6 +253,11 @@ func (e *MFARequiredError) Complete(input string) (*SignIn, error) { req.Header.Set("Accept", "application/json") req.AddCookie(&http.Cookie{Name: mfaPendingCookie, Value: e.pending}) + // Never a redirect. Go forwards the Cookie header to a redirect on the + // same host, whatever the port, and a 307 or 308 replays the body with the + // code in it: following one would hand the challenge and its answer to + // wherever it points. The contract has no redirect here, so one is an + // answer like any other refusal. client := &http.Client{ Timeout: requestTimeout, CheckRedirect: func(*http.Request, []*http.Request) error { diff --git a/internal/auth/mfa_test.go b/internal/auth/mfa_test.go index a0d1144..1abc0e1 100644 --- a/internal/auth/mfa_test.go +++ b/internal/auth/mfa_test.go @@ -4,10 +4,12 @@ import ( "encoding/json" "errors" "fmt" + "io" "log/slog" "net/http" "net/http/httptest" "strings" + "sync" "testing" "time" @@ -349,6 +351,45 @@ func TestNormalizeAnswer(t *testing.T) { } } +// A redirect from the challenge route is not followed. Go would forward the +// Cookie header to it - same host, other port - and a 307 or 308 would replay +// the body with the code, so the server it points at answers like a +// successful challenge: following it would also look like a sign-in. +func TestTheChallengeDoesNotFollowRedirects(t *testing.T) { + for _, status := range []int{http.StatusFound, http.StatusSeeOther, http.StatusTemporaryRedirect, http.StatusPermanentRedirect} { + t.Run(http.StatusText(status), func(t *testing.T) { + var reached []string + var mu sync.Mutex + elsewhere := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + mu.Lock() + reached = append(reached, fmt.Sprintf("%s cookie=%q body=%q", r.Method, r.Header.Get("Cookie"), body)) + mu.Unlock() + http.SetCookie(w, &http.Cookie{Name: "nan_session", Value: "a-session-from-elsewhere"}) + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"redirect":"/","recovery_codes_remaining":null}`) + })) + defer elsewhere.Close() + + p := fakePlatform(t) + ch := challengeFor(t, p) + p.Next = &authtest.Refusal{Status: status, Header: map[string]string{"Location": elsewhere.URL + "/api/auth/mfa/challenge"}} + + in, err := ch.Complete(authtest.Code) + if err == nil { + t.Errorf("signed in through a %d: %+v", status, in) + } else if Retryable(err) || !strings.Contains(err.Error(), fmt.Sprintf("HTTP %d", status)) { + t.Errorf("err = %v", err) + } + mu.Lock() + defer mu.Unlock() + if len(reached) != 0 { + t.Errorf("followed the %d: the server it pointed at got %v", status, reached) + } + }) + } +} + // A redirect that asks for the code without the challenge to answer it with // is a dead end, and says so instead of prompting for nothing. func TestMFAReasonWithoutTheCookie(t *testing.T) { From 51cf584dc5a9c6f6c909392237bfc39ccbd1cba3 Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 18:57:31 +0200 Subject: [PATCH 12/14] test(login): ctrl-c while reading restores the state it saved Nothing checked that readHiddenLine hands term.Restore, with the state term.GetState saved, to the interrupt handler. The three terminal calls are now package variables, so a test stands in for a terminal it does not have: the stand-in ReadPassword sends SIGINT to the process, and the test asserts Restore ran on the same descriptor with the saved state before the command exited. Removing the restore, restoring another state, or dropping the handler each fails it. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- cmd/auth.go | 15 ++++++++++--- cmd/hidden_unix_test.go | 47 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/cmd/auth.go b/cmd/auth.go index 52dbf96..ef6ccee 100644 --- a/cmd/auth.go +++ b/cmd/auth.go @@ -300,16 +300,25 @@ func recoveryCodesLeft(n int) string { // command exits. func readHiddenLine(f *os.File) ([]byte, error) { fd := f.Fd() - if state, err := term.GetState(fd); err == nil { + if state, err := termGetState(fd); err == nil { release := onInterrupt(func() { - _ = term.Restore(fd, state) + _ = termRestore(fd, state) fmt.Fprintln(os.Stderr) }) defer release() } - return term.ReadPassword(fd) + return termReadPassword(fd) } +// The terminal calls readHiddenLine makes, as variables so a test can stand +// in for a terminal it does not have and see what the interrupt handler puts +// back. +var ( + termGetState = term.GetState + termRestore = term.Restore + termReadPassword = term.ReadPassword +) + // interrupted ends the command after ctrl-c, with the status a shell gives an // interrupted one. A variable for the tests. var interrupted = func() { os.Exit(130) } diff --git a/cmd/hidden_unix_test.go b/cmd/hidden_unix_test.go index 2c5c93e..35ee927 100644 --- a/cmd/hidden_unix_test.go +++ b/cmd/hidden_unix_test.go @@ -8,6 +8,8 @@ import ( "syscall" "testing" "time" + + "github.com/charmbracelet/x/term" ) // Ctrl-c at a hidden prompt used to end the process while echo was off, and @@ -33,3 +35,48 @@ func TestCtrlCAtAHiddenPromptPutsTheTerminalBackFirst(t *testing.T) { t.Fatal("ctrl-c at the prompt was not handled") } } + +// readHiddenLine hands the interrupt handler the terminal's own state: a +// ctrl-c while the password is being read restores exactly what GetState +// saved, on the same descriptor, before the command exits. Without a real +// terminal, the three calls are stood in for. +func TestCtrlCWhileReadingRestoresTheSavedState(t *testing.T) { + saved := &term.State{} + f, err := os.Open(os.DevNull) + if err != nil { + t.Fatal(err) + } + defer f.Close() + + type restore struct { + fd uintptr + state *term.State + } + restored := make(chan restore, 1) + exited := make(chan struct{}) + swap(t, &termGetState, func(uintptr) (*term.State, error) { return saved, nil }) + swap(t, &termRestore, func(fd uintptr, s *term.State) error { restored <- restore{fd, s}; return nil }) + swap(t, &interrupted, func() { close(exited) }) + // The member presses ctrl-c while the prompt is waiting for them. + swap(t, &termReadPassword, func(uintptr) ([]byte, error) { + if err := syscall.Kill(os.Getpid(), syscall.SIGINT); err != nil { + t.Error(err) + } + select { + case <-exited: + case <-time.After(5 * time.Second): + t.Error("ctrl-c while reading was not handled") + } + return nil, os.ErrClosed + }) + + _, _ = readHiddenLine(f) + select { + case got := <-restored: + if got.fd != f.Fd() || got.state != saved { + t.Errorf("restored fd %d state %p, want fd %d state %p", got.fd, got.state, f.Fd(), saved) + } + default: + t.Error("ctrl-c exited without restoring the terminal") + } +} From 93db4af14bcfbc1449585ce702019ff7d942fe4e Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 20:47:59 +0200 Subject: [PATCH 13/14] feat(auth): say what to do when sign-up has to finish in the browser An invited or registered member with a factor who signs in from the CLI now gets a 302 to /access-denied?reason=finish_onboarding_in_the_browser and no cookie at all (API contract, "CLI contract" step 8): the onboarding token is behind the second factor, and that flow is browser only. The fallback printed "the link did not work: finish onboarding in the browser", which leaves out that the link is spent. It is now mapped to a sentence that says where to ask for a new one and what comes after. The fake platform answers both onboarding rows: with a factor and client=cli, the reason and no cookie; without a factor, the unchanged nan_onboarding token, which the CLI does not take for a session. Tests pin the text in nan auth login and in the panel, and that no code is asked for. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- cmd/auth_mfa_test.go | 23 ++++++++++++++++++++ internal/auth/auth.go | 6 +++++ internal/auth/authtest/platform.go | 15 ++++++++++++- internal/auth/mfa_test.go | 35 ++++++++++++++++++++++++++++++ internal/tui/mfa_test.go | 34 +++++++++++++++++++++++++++++ 5 files changed, 112 insertions(+), 1 deletion(-) diff --git a/cmd/auth_mfa_test.go b/cmd/auth_mfa_test.go index 6db2c91..953877b 100644 --- a/cmd/auth_mfa_test.go +++ b/cmd/auth_mfa_test.go @@ -261,6 +261,29 @@ func TestTOTPFromATerminalStdin(t *testing.T) { } } +// An account that has not finished signing up and has a factor finishes in +// the browser: nan auth login asks for no code and saves nothing, and says +// what to do next. +func TestLoginForAnUnfinishedSignUpSendsItToTheBrowser(t *testing.T) { + home := fakeHome(t, "") + p := mfaPlatform(t, true) + p.Onboarding = true + var prompt bytes.Buffer + codes, asked := typed(authtest.Code) + err := finishSignIn(authtest.LinkToken, codes, io.Discard, &prompt) + const want = "your NaN account is not set up yet — finish it in the browser: ask for a new link at " + + "https://cloud.nan.builders (this one is spent), complete the setup there, then sign in here again" + if err == nil || err.Error() != want { + t.Fatalf("err = %v", err) + } + if *asked != 0 || prompt.Len() != 0 || len(p.Challenges()) != 0 { + t.Errorf("asked for a code with no challenge: %d prompts, %q", *asked, prompt.String()) + } + if sess, _ := readSession(t, home); sess != nil { + t.Errorf("saved %v", sess) + } +} + // No terminal and no --totp: the link is already spent, so the message says // to ask for another and names the flag that would have worked. func TestNoTerminalAndNoFlag(t *testing.T) { diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 809db67..1d24603 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -204,6 +204,12 @@ var linkReasons = map[string]string{ // be told this; if it is, the sentence is still the right one. "mfa_enabled_update_nan_cli": "this account uses two-factor authentication, and this nan-cli cannot ask for the code — " + "update it with the installer (https://github.com/helmcode/nan-cli#install) and ask for a new link", + // An account that has not finished signing up and has two-factor + // authentication on: the setup, behind the code, happens in the browser + // only. The link this was is spent. + "finish_onboarding_in_the_browser": "your NaN account is not set up yet — finish it in the browser: " + + "ask for a new link at https://cloud.nan.builders (this one is spent), complete the setup there, " + + "then sign in here again", } func linkReasonMessage(reason string) string { diff --git a/internal/auth/authtest/platform.go b/internal/auth/authtest/platform.go index 2659bec..c747cb2 100644 --- a/internal/auth/authtest/platform.go +++ b/internal/auth/authtest/platform.go @@ -43,6 +43,9 @@ type Platform struct { // Factor: the account has a confirmed authenticator. Without it the link // alone signs in, exactly as before two-factor sign-in existed. Factor bool + // Onboarding: the member is invited or registered and has not finished + // signing up, so a link gets them the onboarding token, never a session. + Onboarding bool // RecoveryCodes the account has not used yet, in canonical form // (xxxx-xxxx-xxxx-xxxx). RecoveryCodes []string @@ -143,11 +146,21 @@ func (p *Platform) verify(w http.ResponseWriter, r *http.Request) { } p.spent = true + cli := r.PostForm.Get("client") == "cli" switch { + case p.Onboarding && !p.Factor: + // Unchanged by two-factor sign-in: the onboarding token, which is not + // a session, and the page that uses it. + http.SetCookie(w, &http.Cookie{Name: "nan_onboarding", Value: "fake-onboarding-token", Path: "/", Secure: true, HttpOnly: true}) + http.Redirect(w, r, FrontendURL+"/onboarding", http.StatusFound) + case p.Onboarding && cli: + // "CLI contract" step 8: with a factor, onboarding is finished in the + // browser, whatever mfa says. No cookie of any kind. + http.Redirect(w, r, FrontendURL+"/access-denied?reason=finish_onboarding_in_the_browser", http.StatusFound) case !p.Factor: http.SetCookie(w, &http.Cookie{Name: "nan_session", Value: Session, Path: "/", Domain: ".nan.builders", Secure: true, HttpOnly: true}) http.Redirect(w, r, FrontendURL+"/", http.StatusFound) - case r.PostForm.Get("client") == "cli" && r.PostForm.Get("mfa") != "1": + case cli && r.PostForm.Get("mfa") != "1": http.Redirect(w, r, FrontendURL+"/access-denied?reason=mfa_enabled_update_nan_cli", http.StatusFound) default: // A new challenge replaces any earlier one (D8). diff --git a/internal/auth/mfa_test.go b/internal/auth/mfa_test.go index 1abc0e1..48797ea 100644 --- a/internal/auth/mfa_test.go +++ b/internal/auth/mfa_test.go @@ -436,3 +436,38 @@ func TestTheChallengeNeverPrintsItsCookie(t *testing.T) { } } } + +// An account that has not finished signing up and has a factor is sent to the +// browser, with or without mfa=1 ("CLI contract" step 8): no challenge, no +// cookie, and a sentence that says what to do with a link that is spent. +func TestUnfinishedSignUpWithAFactorIsSentToTheBrowser(t *testing.T) { + p := fakePlatform(t) + p.Factor, p.Onboarding = true, true + got, err := ExchangeToken(authtest.LinkToken) + var ch *MFARequiredError + if err == nil || got != "" || errors.As(err, &ch) { + t.Fatalf("ExchangeToken = %q, %v", got, err) + } + if err.Error() != linkReasons["finish_onboarding_in_the_browser"] { + t.Errorf("message = %q", err) + } + for _, want := range []string{"browser", "new link", "cloud.nan.builders", "spent", "sign in here again"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("%q does not say %q", err, want) + } + } + if n := len(p.Challenges()); n != 0 { + t.Errorf("%d challenge POSTs with no challenge to answer", n) + } +} + +// Without a factor nothing changed: the onboarding token is not a session, +// and the CLI does not take it for one. +func TestUnfinishedSignUpWithoutAFactorGetsNoSession(t *testing.T) { + p := fakePlatform(t) + p.Onboarding = true + got, err := ExchangeToken(authtest.LinkToken) + if err == nil || got != "" || !strings.Contains(err.Error(), "no session came back") { + t.Fatalf("ExchangeToken = %q, %v", got, err) + } +} diff --git a/internal/tui/mfa_test.go b/internal/tui/mfa_test.go index 702a1fb..1422161 100644 --- a/internal/tui/mfa_test.go +++ b/internal/tui/mfa_test.go @@ -197,3 +197,37 @@ func assertNoChallengeUnderHome(t *testing.T) { t.Fatal(err) } } + +// An account that has not finished signing up and has a factor: the panel +// stays on the link question with the sentence that sends it to the browser, +// and asks for no code. +func TestThePanelSendsAnUnfinishedSignUpToTheBrowser(t *testing.T) { + p := authtest.New(t) + p.Factor, p.Onboarding = true, true + old := auth.SignInBaseURL + auth.SignInBaseURL = p.URL + t.Cleanup(func() { auth.SignInBaseURL = old }) + + m := setupModel(t, &session.Session{}) + m.lay = newLayout(100, 40) + m.startLogin() + mod, _ := m.Update(linkSentMsg{}) + m = mod.(model) + m = enter(t, m, mfaTestLink) + + if m.loginStage != loginAskLink || m.loginChallenge != nil { + t.Fatalf("stage %v, challenge %v", m.loginStage, m.loginChallenge != nil) + } + const want = "error: your NaN account is not set up yet — finish it in the browser: ask for a new link at " + + "https://cloud.nan.builders (this one is spent), complete the setup there, then sign in here again" + if m.loginMsg != want { + t.Errorf("loginMsg = %q", m.loginMsg) + } + flat := strings.Join(strings.Fields(m.View()), " ") + if !strings.Contains(flat, "finish it in the browser") { + t.Errorf("the screen does not say it:\n%s", m.View()) + } + if s, err := session.Load(); err == nil && s.Token != "" { + t.Errorf("saved a session: %+v", s) + } +} From ce62cd160b8772c327090d5115bc5768e05a3984 Mon Sep 17 00:00:00 2001 From: Pau Morillas Date: Fri, 23 Oct 2026 20:48:00 +0200 Subject: [PATCH 14/14] fix(auth): show a refused link's reason only when it is shaped like one The reason of a refused link comes out of a redirect's query, decoded, and an unknown one was printed with its underscores turned to spaces: an escape sequence, a new line or a paragraph in it reached the terminal as is. It is now echoed only when it has the platform's shape, ^[a-z0-9_]{1,64}$ - the filter the challenge's error codes already go through - and anything else gets a sentence of our own. Tested through ExchangeToken with escapes, a new line, capitals, spaces, 65 characters and non-ASCII, and at the 64-character edge. Refs helmcode/nan-cloud-api#389 Co-Authored-By: Claude Opus 5.5 --- internal/auth/auth.go | 6 +++++ internal/auth/mfa.go | 4 +++- internal/auth/mfa_test.go | 46 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 1 deletion(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 1d24603..57930cc 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -216,5 +216,11 @@ func linkReasonMessage(reason string) string { if message, ok := linkReasons[reason]; ok { return message } + // The reason comes out of a redirect's query, decoded, so it can carry + // anything - an escape sequence, a paragraph. Only something shaped like + // the platform's reasons is put on the terminal. + if !isErrorCode(reason) { + return "the link did not work, and nan.builders did not say why in a way this version can show — ask for a new link" + } return "the link did not work: " + strings.ReplaceAll(reason, "_", " ") } diff --git a/internal/auth/mfa.go b/internal/auth/mfa.go index d6844b1..e0c8f5a 100644 --- a/internal/auth/mfa.go +++ b/internal/auth/mfa.go @@ -204,7 +204,9 @@ func normalizeAnswer(input string) (field, value string, err error) { return "", "", ErrNotACode } -// isErrorCode is the platform's error-code shape, ^[a-z0-9_]+$. +// isErrorCode is the shape of the platform's codes, ^[a-z0-9_]{1,64}$: the +// error codes of the challenge and the reasons of a refused link alike. Only +// those are echoed to the terminal. func isErrorCode(s string) bool { return s != "" && len(s) <= 64 && allASCII(s, func(b byte) bool { return isDigit(b) || (b >= 'a' && b <= 'z') || b == '_' diff --git a/internal/auth/mfa_test.go b/internal/auth/mfa_test.go index 48797ea..14392cd 100644 --- a/internal/auth/mfa_test.go +++ b/internal/auth/mfa_test.go @@ -8,6 +8,7 @@ import ( "log/slog" "net/http" "net/http/httptest" + "net/url" "strings" "sync" "testing" @@ -471,3 +472,48 @@ func TestUnfinishedSignUpWithoutAFactorGetsNoSession(t *testing.T) { t.Fatalf("ExchangeToken = %q, %v", got, err) } } + +// The reason of a refused link is read out of a redirect's query, decoded, so +// it can be anything. Only the platform's shape of reason is put on the +// terminal; anything else gets a sentence of our own. +func TestOddReasonsNeverReachTheTerminal(t *testing.T) { + for _, c := range []struct { + name, reason string + echoed bool + }{ + {"one of ours, unknown to this build", "a_reason_from_tomorrow", true}, + {"the longest that is still one", strings.Repeat("a", 64), true}, + {"an escape sequence", "\x1b[2J\x1b]0;owned\x07", false}, + {"a new line", "invalid\nlink", false}, + {"capitals", "Invalid_Link", false}, + {"spaces", "please paste your token here", false}, + {"too long", strings.Repeat("a", 65), false}, + {"not ASCII", "enlace_inválido", false}, + } { + t.Run(c.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, authtest.FrontendURL+"/access-denied?reason="+url.QueryEscape(c.reason), http.StatusFound) + })) + defer srv.Close() + old := SignInBaseURL + SignInBaseURL = srv.URL + defer func() { SignInBaseURL = old }() + + _, err := ExchangeToken(authtest.LinkToken) + if err == nil { + t.Fatal("no error") + } + msg := err.Error() + shown := strings.ReplaceAll(c.reason, "_", " ") + if c.echoed != strings.Contains(msg, shown) { + t.Errorf("echoed = %v, want %v: %q", !c.echoed, c.echoed, msg) + } + if strings.ContainsAny(msg, "\x1b\x07\n") { + t.Errorf("control characters reached the message: %q", msg) + } + if !c.echoed && !strings.Contains(msg, "ask for a new link") { + t.Errorf("%q does not say what to do", msg) + } + }) + } +}