Skip to content

feat: add UNCOMPACT_CLI_AUTH_URL env override for local testing #566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
greynewell wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add UNCOMPACT_CLI_AUTH_URL env override for local testing #566

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion cmd/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,11 @@ func authLoginBrowser(cfg *config.Config) (string, error) {
_ = server.Shutdown(ctx)
}()

dashURL := fmt.Sprintf("%s?port=%d&state=%s", config.DashboardCLIAuthURL, port, state)
cliAuthURL := config.DashboardCLIAuthURL
if override := os.Getenv("UNCOMPACT_CLI_AUTH_URL"); override != "" {
cliAuthURL = override
}
dashURL := fmt.Sprintf("%s?port=%d&state=%s", cliAuthURL, port, state)
Comment on lines +173 to +177
Copy link

@coderabbitai coderabbitai bot Mar 4, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's check if cmd/auth.go exists and read the relevant section
find . -name "auth.go" -path "*/cmd/*" | head -5

Repository: supermodeltools/Uncompact

Length of output: 82

🏁 Script executed:

# Read the relevant section of cmd/auth.go around lines 173-177
sed -n '160,185p' ./cmd/auth.go

Repository: supermodeltools/Uncompact

Length of output: 911

🏁 Script executed:

# Let's see the function signature and return type
sed -n '1,180p' ./cmd/auth.go | grep -A 20 "^func.*getAuthURL\|^func.*Auth\|^func" | head -30

Repository: supermodeltools/Uncompact

Length of output: 943

🏁 Script executed:

# Let's find the function that contains the auth URL logic
sed -n '100,180p' ./cmd/auth.go | head -80

Repository: supermodeltools/Uncompact

Length of output: 2376

🏁 Script executed:

# Check the imports at the top of cmd/auth.go
head -30 ./cmd/auth.go

Repository: supermodeltools/Uncompact

Length of output: 593

🏁 Script executed:

# Let's verify the issue scenario mentioned in the review by checking what happens
# if someone sets an override URL that already has query params
python3 << 'EOF'
from urllib.parse import urlparse, parse_qs

# Simulate the current code behavior
port = 43210
state = "abcd1234"

test_cases = [
    ("http://localhost:3000/cli-auth/", "Normal case (no existing params)"),
    ("http://localhost:3000/cli-auth/?existing=value", "Edge case (has existing param)"),
]

print("=" * 70)
print("CURRENT CODE: fmt.Sprintf behavior")
print("=" * 70)

for override, desc in test_cases:
    # Current code does: fmt.Sprintf("%s?port=%d&state=%s", cliAuthURL, port, state)
    dashURL = f"{override}?port={port}&state={state}"
    parsed = urlparse(dashURL)
    
    print(f"\n{desc}")
    print(f"Override URL:  {override}")
    print(f"Result URL:    {dashURL}")
    print(f"Query string:  {parsed.query}")
    try:
        params = parse_qs(parsed.query)
        print(f"Parsed params: {params}")
    except Exception as e:
        print(f"ERROR parsing:  {e}")

print("\n" + "=" * 70)
print("WITH FIX: Using net/url Parse + Query().Set()")
print("=" * 70)

from urllib.parse import urlencode

for override, desc in test_cases:
    # Simulating the fixed code
    parsed = urlparse(override)
    params = parse_qs(parsed.query, keep_blank_values=True)
    
    # Add new params
    params['port'] = [str(port)]
    params['state'] = [state]
    
    # Rebuild query
    query_str = urlencode(params, doseq=True)
    fixed_url = f"{parsed.scheme}://{parsed.netloc}{parsed.path}?{query_str}"
    
    print(f"\n{desc}")
    print(f"Override URL:  {override}")
    print(f"Result URL:    {fixed_url}")
    print(f"Query string:  {query_str}")
    print(f"Parsed params: {params}")

EOF

Repository: supermodeltools/Uncompact

Length of output: 1570

🏁 Script executed:

# Let's also check if there are any tests for the auth login functionality
find . -name "*test*.go" -o -name "*_test.go" | xargs grep -l "authLoginBrowser\|UNCOMPACT_CLI_AUTH_URL" 2>/dev/null | head -5

Repository: supermodeltools/Uncompact

Length of output: 51

🏁 Script executed:

# Let's check the config to see what the default DashboardCLIAuthURL is
grep -n "DashboardCLIAuthURL" ./internal/config/config.go

Repository: supermodeltools/Uncompact

Length of output: 144

🏁 Script executed:

# Quick check: verify the fix would compile by checking if the function
# can actually handle the error cases properly
sed -n '119,130p' ./cmd/auth.go

Repository: supermodeltools/Uncompact

Length of output: 363

Use net/url to safely build the auth callback URL.

Nice feature! But there's a subtle URL-building issue on line 177. Right now, the code just concatenates the override URL directly with ?port=...&state=.... This works fine if the override is a simple URL like http://localhost:3000/cli-auth/, but if someone passes an override that already has query params—like http://localhost:3000/cli-auth/?existing=value—you end up with a malformed URL: http://localhost:3000/cli-auth/?existing=value?port=43210&state=abcd1234 (notice the double ?). That breaks the query parsing and kills the auth callback.

The fix is straightforward: use net/url.Parse() to safely merge query params, validate the scheme and host, and then encode. This way, existing params get properly combined with the new ones.

Suggested fix 
 import (
 	"context"
 	"crypto/rand"
 	"encoding/hex"
 	"fmt"
 	"net"
 	"net/http"
+	"net/url"
 	"os"
+	"strconv"
 	"strings"
 	"time"
@@
 	cliAuthURL := config.DashboardCLIAuthURL
 	if override := os.Getenv("UNCOMPACT_CLI_AUTH_URL"); override != "" {
 		cliAuthURL = override
 	}
-	dashURL := fmt.Sprintf("%s?port=%d&state=%s", cliAuthURL, port, state)
+	parsedURL, err := url.Parse(cliAuthURL)
+	if err != nil {
+		return "", fmt.Errorf("invalid UNCOMPACT_CLI_AUTH_URL: %w", err)
+	}
+	if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" {
+		return "", fmt.Errorf("UNCOMPACT_CLI_AUTH_URL must use http or https")
+	}
+	if parsedURL.Host == "" {
+		return "", fmt.Errorf("UNCOMPACT_CLI_AUTH_URL must include a host")
+	}
+	q := parsedURL.Query()
+	q.Set("port", strconv.Itoa(port))
+	q.Set("state", state)
+	parsedURL.RawQuery = q.Encode()
+	dashURL := parsedURL.String()
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@cmd/auth.go` around lines 173 - 177, The current construction of dashURL by
concatenating cliAuthURL with query params can break when the override
(UNCOMPACT_CLI_AUTH_URL) already contains query parameters; instead parse
cliAuthURL using net/url.Parse(), validate the parsed URL has a scheme and host,
then merge params by getting q := u.Query(), set q.Set("port", fmt.Sprint(port))
and q.Set("state", state), assign u.RawQuery = q.Encode(), and use u.String() as
dashURL; update the code references around cliAuthURL, override, dashURL, and
the UNCOMPACT_CLI_AUTH_URL handling to use this safe URL-building approach.

fmt.Println("Opening your browser to sign in...")
fmt.Printf(" %s\n\n", dashURL)
fmt.Println("Waiting for authentication (this will timeout in 2 minutes)...")
Expand Down