From 9264173be03f81c91a6f1442dcafa187f96a5628 Mon Sep 17 00:00:00 2001 From: Jordan Evans Date: Wed, 14 Oct 2026 11:57:05 -0700 Subject: [PATCH] fix: validate device login URLs before launching browser Require absolute HTTPS verification URLs with a hostname before invoking an OS launcher. Preserve manual instructions for rejected values and escape control characters in IdP-supplied login instructions. Add regression tests and document browser-launch restrictions. Rebase onto current main while retaining its patched Go toolchain and dependency versions. Issue: linuxfoundation/lfx-self-serve-ops#467 Co-authored-by: Copilot Signed-off-by: Jordan Evans --- README.md | 7 +++ internal/commands/auth.go | 86 ++++++++++++++++++------- internal/commands/auth_test.go | 111 +++++++++++++++++++++++++++++++++ 3 files changed, 182 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 108abe5..d4f1040 100644 --- a/README.md +++ b/README.md @@ -41,6 +41,9 @@ go install github.com/linuxfoundation/lfx-cli/cmd/lfx@latest # Log in via the Auth0 Device Code flow. lfx auth login +# Automatically open the HTTPS verification URL in your default browser. +lfx auth login --web + # Log in against a non-production environment (aliases accepted). lfx auth login --env staging # also: stage, stg lfx auth login --env dev # also: develop, development @@ -60,6 +63,10 @@ lfx api /projects --field name=example # auto-promotes to POST lfx api -X PUT /projects/123 --input - -H "If-Match: " < input.json # Content-Type: application/json is added automatically ``` +Automatic browser opening accepts only absolute HTTPS URLs with a hostname. +Other verification URI values are displayed for manual inspection without +launching them; control characters in login instructions are escaped. + Credentials (refresh token, cached access token) are stored in your operating system's credential store by default (macOS Keychain, Windows Credential Manager, Linux Secret Service/KWallet/`pass`). Which of these is diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 7a897e2..cc478d0 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -12,11 +12,14 @@ import ( "errors" "fmt" "io" + "net/url" "os" "os/exec" "runtime" + "strconv" "strings" "time" + "unicode" "github.com/linuxfoundation/lfx-cli/internal/credstore" "github.com/urfave/cli/v3" @@ -217,22 +220,8 @@ func loginWithDeviceCode( return fmt.Errorf("request device code: %w", err) } - fmt.Printf("First copy your one-time code: %s\n", resp.UserCode) - // VerificationURIComplete is optional per RFC 8628 §3.2; fall back to - // VerificationURI (always present) plus the user code if the IdP - // doesn't supply it. - verificationURI := resp.VerificationURIComplete - if verificationURI == "" { - verificationURI = resp.VerificationURI - } - if cmd.Bool(webFlagName) { - fmt.Printf("Opening %s in your browser...\n", verificationURI) - if err := openBrowser(verificationURI); err != nil { - fmt.Printf("Couldn't open browser automatically: %v\n", err) - fmt.Printf("Please visit: %s\n", verificationURI) - } - } else { - fmt.Printf("Then visit: %s\n", verificationURI) + if err := printDeviceCodeInstructions(os.Stdout, resp, cmd.Bool(webFlagName)); err != nil { + return fmt.Errorf("display login instructions: %w", err) } fmt.Println("Waiting for authentication...") @@ -756,15 +745,68 @@ func base64URLDecode(s string) ([]byte, error) { return base64.RawURLEncoding.DecodeString(s) } -// openBrowser opens url in the user's default browser, following the -// per-OS conventions used by tools like `gh`. -func openBrowser(url string) error { +// printDeviceCodeInstructions displays untrusted IdP fields safely, retaining +// manual instructions when automatic browser opening is refused or fails. +func printDeviceCodeInstructions(w io.Writer, resp *oauth2.DeviceAuthResponse, web bool) error { + if _, err := fmt.Fprintf(w, "First copy your one-time code: %s\n", terminalSafeText(resp.UserCode)); err != nil { + return err + } + // VerificationURIComplete is optional per RFC 8628 §3.2. + verificationURI := resp.VerificationURIComplete + if verificationURI == "" { + verificationURI = resp.VerificationURI + } + displayURI := terminalSafeText(verificationURI) + if web { + if _, err := fmt.Fprintf(w, "Opening %s in your browser...\n", displayURI); err != nil { + return err + } + if err := openBrowser(verificationURI); err != nil { + if _, writeErr := fmt.Fprintf(w, "Couldn't open browser automatically: %v\n", err); writeErr != nil { + return writeErr + } + _, writeErr := fmt.Fprintf(w, "Please visit: %s\n", displayURI) + return writeErr + } + } else { + _, err := fmt.Fprintf(w, "Then visit: %s\n", displayURI) + return err + } + return nil +} + +// terminalSafeText escapes control characters rather than letting IdP-supplied +// values execute terminal escape sequences or forge additional output lines. +func terminalSafeText(s string) string { + if strings.ContainsFunc(s, unicode.IsControl) { + return strconv.QuoteToASCII(s) + } + return s +} + +// validateBrowserURL restricts OS launchers to absolute HTTPS URLs. These +// launchers also accept local files and custom schemes, so exec.Command alone +// is not sufficient to make an untrusted verification URI safe to launch. +func validateBrowserURL(uri string) error { + u, err := url.Parse(uri) + if err != nil || u.Scheme != "https" || u.Hostname() == "" || u.Opaque != "" || strings.ContainsFunc(uri, unicode.IsControl) { + return errors.New("refusing to open verification URI: expected an absolute HTTPS URL with a hostname") + } + return nil +} + +// openBrowser opens a validated HTTPS URL in the user's default browser, +// following the per-OS conventions used by tools like `gh`. +func openBrowser(uri string) error { + if err := validateBrowserURL(uri); err != nil { + return err + } switch runtime.GOOS { case "darwin": - return exec.Command("open", url).Start() + return exec.Command("open", uri).Start() case "windows": - return exec.Command("rundll32", "url.dll,FileProtocolHandler", url).Start() + return exec.Command("rundll32", "url.dll,FileProtocolHandler", uri).Start() default: - return exec.Command("xdg-open", url).Start() + return exec.Command("xdg-open", uri).Start() } } diff --git a/internal/commands/auth_test.go b/internal/commands/auth_test.go index f92b64a..dfcb2ef 100644 --- a/internal/commands/auth_test.go +++ b/internal/commands/auth_test.go @@ -4,16 +4,127 @@ package commands import ( + "bytes" "context" "encoding/base64" "encoding/json" "errors" + "strconv" + "strings" "testing" "github.com/linuxfoundation/lfx-cli/internal/credstore" "github.com/urfave/cli/v3" + "golang.org/x/oauth2" ) +func TestValidateBrowserURL(t *testing.T) { + tests := []struct { + name string + uri string + valid bool + }{ + {name: "HTTPS", uri: "https://example.test/activate", valid: true}, + {name: "complete URI", uri: "https://example.test/activate?user_code=ABCD-EFGH", valid: true}, + {name: "port and fragment", uri: "https://example.test:8443/activate#code", valid: true}, + {name: "IPv6", uri: "https://[::1]/activate", valid: true}, + {name: "empty"}, + {name: "HTTP", uri: "http://example.test/activate"}, + {name: "UNC", uri: `\\example.test\share\update.exe`}, + {name: "Windows path", uri: `C:\Temp\update.exe`}, + {name: "local path", uri: "/Applications/Example.app"}, + {name: "file", uri: "file:///Applications/Example.app"}, + {name: "remote file", uri: "file://example.test/share/update.exe"}, + {name: "custom scheme", uri: "ms-msdt:/id Example"}, + {name: "javascript", uri: "javascript:alert(1)"}, + {name: "relative", uri: "/activate"}, + {name: "protocol relative", uri: "//example.test/activate"}, + {name: "missing host", uri: "https:///activate"}, + {name: "empty hostname", uri: "https://:443/activate"}, + {name: "opaque", uri: "https:example.test/activate"}, + {name: "launcher option", uri: "--help"}, + {name: "leading whitespace", uri: " https://example.test/activate"}, + {name: "invalid escape", uri: "https://example.test/%zz"}, + {name: "backslash host", uri: `https://example.test\share\update.exe`}, + {name: "terminal escape", uri: "https://example.test/\x1b[31m"}, + {name: "newline", uri: "https://example.test/\nforged output"}, + {name: "Unicode control", uri: "https://example.test/\u009b31m"}, + } + + // No launcher is available: a rejected URI must produce the validation + // error, not an exec error, proving the guard runs before process startup. + t.Setenv("PATH", t.TempDir()) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := validateBrowserURL(tc.uri) + if tc.valid { + if err != nil { + t.Fatalf("validateBrowserURL(%q): %v", tc.uri, err) + } + return + } + if err == nil { + t.Fatalf("validateBrowserURL(%q): expected rejection", tc.uri) + } + if launchErr := openBrowser(tc.uri); launchErr == nil || launchErr.Error() != err.Error() { + t.Fatalf("openBrowser(%q) = %v, want validation error %v", tc.uri, launchErr, err) + } + }) + } +} + +func TestPrintDeviceCodeInstructionsRejectsUnsafeURIs(t *testing.T) { + t.Setenv("PATH", t.TempDir()) + for _, uri := range []string{ + `\\example.test\share\update.exe`, + "file:///Applications/Example.app", + "ms-msdt:/id Example", + "https://example.test/\x1b[31m\nforged output", + } { + for _, complete := range []bool{false, true} { + t.Run(strconv.FormatBool(complete)+"/"+uri, func(t *testing.T) { + resp := &oauth2.DeviceAuthResponse{ + UserCode: "ABCD-EFGH", + VerificationURI: uri, + } + if complete { + resp.VerificationURI = "https://example.test/activate" + resp.VerificationURIComplete = uri + } + var output bytes.Buffer + if err := printDeviceCodeInstructions(&output, resp, true); err != nil { + t.Fatalf("printDeviceCodeInstructions: %v", err) + } + got := output.String() + if !strings.Contains(got, "refusing to open verification URI") { + t.Fatalf("missing browser refusal: %q", got) + } + if !strings.Contains(got, "Please visit: "+terminalSafeText(uri)+"\n") { + t.Fatalf("missing manual instructions: %q", got) + } + if strings.Contains(got, "\x1b") || strings.Contains(got, "\nforged output") { + t.Fatalf("unsafe terminal output: %q", got) + } + }) + } + } +} + +func TestPrintDeviceCodeInstructionsManual(t *testing.T) { + resp := &oauth2.DeviceAuthResponse{ + UserCode: "ABCD-EFGH\x1b[31m", + VerificationURI: "https://example.test/activate", + } + var output bytes.Buffer + if err := printDeviceCodeInstructions(&output, resp, false); err != nil { + t.Fatalf("printDeviceCodeInstructions: %v", err) + } + want := "First copy your one-time code: \"ABCD-EFGH\\x1b[31m\"\nThen visit: https://example.test/activate\n" + if got := output.String(); got != want { + t.Fatalf("instructions = %q, want %q", got, want) + } +} + // newTestCommand builds a *cli.Command with the --insecure-storage and // --backend flags registered (as the root `lfx` command does via // CredentialStoreFlags), parses args against it, and returns the parsed