[jordane]

Summary

• Require absolute HTTPS verification URLs with a hostname before invoking OS browser launchers; display rejected values without launching them.
• Escape control characters in IdP-supplied login instructions and add regression coverage.
• Document --web behavior and refresh dependencies while retaining Go 1.25 compatibility.

Fixes linuxfoundation/lfx-self-serve-ops#467

[Copilot]

🟢 Approval recommended

Validation precedes browser launch, manual instructions remain available, and no blocking issues were identified.

0 open findings

What changed in this PR

Hardens device-code login by validating verification URLs before launching a browser and safely displaying identity-provider instructions.

Changes:

• Restricts automatic browser opening to absolute HTTPS URLs with a hostname.
• Escapes control characters and adds regression tests.
• Documents --web behavior and refreshes dependencies without changing Go compatibility.

File	Description
README.md	Documents browser-opening restrictions and escaped output.
internal/​commands/​auth.go	Validates URLs and safely renders login instructions.
internal/​commands/​auth_test.go	Tests URL rejection and safe instruction output.
go.sum	Updates dependency checksums.
go.mod	Updates dependencies while retaining Go 1.25.14.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[andrest50]

This PR hardens lfx auth login --web against malicious IdP verification URIs by requiring an absolute HTTPS URL with a hostname before invoking any OS browser launcher, and escapes control characters in all IdP-supplied text before printing it to the terminal. Both changes directly address the reported issue (linuxfoundation/lfx-self-serve-ops#467) and the implementation is sound: the URL validator correctly rejects file URIs, custom schemes, opaque URIs, control-character injection, and the classic browser-quirk tricks (UNC paths, backslash-in-host, launcher-option strings). The test coverage is thorough across all dimensions.

What's done well

• validateBrowserURL layers five orthogonal guards in a single condition (url.Parse error, scheme, empty hostname, opaque flag, control-char scan) with no redundancy — each guard catches a distinct class of URI the others do not.
• t.Setenv("PATH", t.TempDir()) in the validation tests is the right technique: it ensures rejected URIs fail with the validation error rather than an exec error, proving the guard runs before any process is started.
• terminalSafeText using strconv.QuoteToASCII is conservative: it wraps the entire value in Go-style double-quotes when any control character is present, eliminating ambiguity about partial escaping.
• Extracting printDeviceCodeInstructions into an io.Writer-accepting function makes it directly testable without spawning real processes or capturing os.Stdout.
• Error propagation from printDeviceCodeInstructions through fmt.Errorf("display login instructions: %w", err) is a genuine improvement; the original fmt.Printf calls silently swallowed write errors.
• The test for TestPrintDeviceCodeInstructionsRejectsUnsafeURIs exercises both VerificationURI and VerificationURIComplete paths for every unsafe URI, so the if verificationURI == "" { verificationURI = resp.VerificationURI } fallback is covered.

Reconciliation with existing reviews

• ✅ Copilot "approval recommended, 0 open findings" — no prior threads to address.

Issue count

• 🔴 Blocking: 0
• 🟡 Major: 0
• 🔵 Minor: 1
• ⚪ Nit: 0
• ❔ Question: 0

Decision

Approving — one minor UX gap noted below as a PR comment; no code changes required before merge.

[andrest50]

Minor — "Opening…" message is printed before the URL is validated

In printDeviceCodeInstructions, when web=true, "Opening %s in your browser...\n" is printed before openBrowser runs its validateBrowserURL guard. When a malicious URI is rejected, the user sees this sequence:

Opening ms-msdt:/id Example in your browser...
Couldn't open browser automatically: refusing to open verification URI: expected an absolute HTTPS URL with a hostname
Please visit: ms-msdt:/id Example

The browser is not opened (security goal achieved), but the "Opening…" message implies an attempt was made, which is both false and potentially alarming.

Fix: validate first, print only on success:

if web {
    if err := validateBrowserURL(verificationURI); err != nil {
        if _, writeErr := fmt.Fprintf(w, "Couldn't open browser automatically: %v\n", err); writeErr != nil {
            return writeErr
        }
    } else {
        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
            }
        } else {
            return nil
        }
    }
    _, writeErr := fmt.Fprintf(w, "Please visit: %s\n", displayURI)
    return writeErr
}

Alternatively, pass the pre-validated URL into openBrowser and move the "Opening…" print there, keeping a single fallback path in printDeviceCodeInstructions.

---

Nit — non-stdlib minor version bumps not mentioned in the PR description

go.mod also bumps tidwall/gjson v1.19→v1.20, urfave/cli/v3 v3.13→v3.14, and golang.org/x/oauth2 v0.36→v0.37. These are minor version changes unrelated to the OSV findings being fixed. Worth a single sentence in the PR description so reviewers know they were intentional: e.g. "Non-stdlib transitive bumps (gjson, urfave/cli/v3, oauth2) are incidental version drift, not CVE-driven."

[Copilot]

🟢 Approval recommended

Static review found no blocking issues in the supplied changes; tests were not executed.

0 open findings

🧠 Review effort: Balanced