Merge pull request #356 from natalie-o-perret/fix/discovery-auth-xdg-open-linux

fix: send auth credentials during spec discovery; fix xdg-open on Linux

Author: danielgtaylor

.agents/skills/rsh-review/SKILL.md

@@ -36,7 +36,8 @@ Review code changes with a bug-finding mindset. Prioritize correctness, regressi
 4. Challenge assumptions around error paths, cancellation, timeouts, cleanup, concurrency, config precedence, and backward compatibility.
 5. Check whether tests cover intended behavior and important failure modes.
 6. Check whether `site/` docs or `docs/design/` should change.
-7. Report findings first, ordered by severity. Keep summaries brief.
+7. Keep the review scoped to the current PR unless the user asks for a broader audit. Distinguish required fixes from optional hardening or follow-on cleanup.
+8. Report findings first, ordered by severity. Keep summaries brief.
 
 ## Output Expectations
 
@@ -47,6 +48,7 @@ Review code changes with a bug-finding mindset. Prioritize correctness, regressi
 - If no findings are present, say so explicitly and call out any residual risk or untested areas.
 - Do not pad the review with praise or low-value nits unless the user asks for them.
 - Do not report speculative issues unless there is a plausible failure mode in the changed code.
+- Do not turn a scoped PR review into general codebase cleanup. If you notice unrelated risks, list them as out-of-scope follow-ups only when they are important.
 - Treat missing coverage as a test gap unless the diff shows a confirmed bug.
 
 ## Severity Guide
@@ -105,6 +107,8 @@ Intentional formatter changes should usually come with targeted regression cover
 
 Changes in these areas often regress behavior only in realistic end-to-end paths. Review interactions, not just isolated helpers.
 
+For auth, redaction, and cache metadata changes, check both positive behavior and negative leakage boundaries: where credentials are applied, where they must not be applied, what gets persisted, and what appears in errors or traces. Keep findings tied to the PR's changed paths.
+
 ### Test buffer races
 
 Tests that share a `bytes.Buffer` across concurrent writers can hide data races, especially when subprocess stderr/stdout is wired into test buffers.

.agents/skills/rsh-simplify/SKILL.md

@@ -27,6 +27,19 @@ Identify and eliminate unnecessary complexity in the Restish codebase. Fewer lin
 
 User experience comes first. Developer experience comes second. Simplifications that improve both are highest value. Simplifications that improve only developer experience at some user cost are not acceptable.
 
+## Post-Review Shrink Pass
+
+After a PR has gone through review/fix loops, do a focused simplification pass before final handoff when the diff has grown substantially. Compare `git diff --stat` or `git diff --shortstat` before and after. Target duplicated test setup, repeated assertions, over-large inline fixtures, and helper code that obscures the behavior under test. Prefer deleting ceremony over deleting coverage.
+
+Good shrink-pass moves:
+
+- Table-drive cases that share setup and assertion shape.
+- Add small helpers for repeated config/cache/server setup when they make tests read more directly.
+- Share behavioral assertions that are repeated verbatim.
+- Keep edge-case tests for auth, redaction, redirects, cache metadata, migrations, and subprocess behavior when those edge cases are the point of the PR.
+
+Avoid code golf. A shorter diff that is harder to audit, especially around security/auth/cache behavior, is not a simplification.
+
 ## Process
 
 ### 1. Analyze

.agents/skills/rsh-test/SKILL.md

@@ -59,6 +59,9 @@ Prefer plain Go tests with a small Restish-specific helper vocabulary over Gherk
 
 - One test should usually cover one behavior from input through observable result.
 - Combine cases when they differ only by inputs and expected outputs. Split cases when setup or failure meaning differs.
+- For review-driven edge cases, first preserve the behavioral boundary, then reduce scaffolding. Table-drive repeated redirect, redaction, cache, migration, or auth-origin cases only when the setup and assertion shape truly match.
+- Use focused helpers for repeated config/cache/server setup, but keep the request, persisted state, error, or output assertion visible in the test.
+- When covering credential handling, assert both where credentials are applied and where they are not applied, plus what gets persisted and what appears in diagnostics.
 - Avoid asserting private call sequences unless ordering is the contract.
 - Avoid mocks for HTTP, files, and CLI I/O when standard library fakes or temp resources are clearer.
 - Keep fixtures tiny but believable: real OpenAPI fragments, real response headers, real shorthand, real config snippets.

AGENTS.md

@@ -48,6 +48,10 @@ The core design is a `CLI` struct in `internal/cli/cli.go` that owns all state 
 
 `TODO.md` and `review.md` are local working files for planning, code reviews, and in-progress implementation notes only. They must never be committed to git history. Keep them ignored/untracked, and if they are accidentally staged, unstage them before committing.
 
+## PR Review and Cleanup Discipline
+
+Keep review and fix loops scoped to the current PR's behavioral surface unless the user explicitly asks for broader cleanup. After substantial review loops, do a simplification pass before final handoff: reduce repeated scaffolding, table-drive genuinely similar cases, and consolidate helpers where that preserves readability. Do not shrink a PR by deleting important coverage or making security/auth/cache behavior harder to audit.
+
 ## Commit Messages
 
 Use [Conventional Commits](https://www.conventionalcommits.org/) for all commits:

internal/auth/oauth_authcode.go

@@ -776,7 +776,11 @@ func DefaultOpenBrowser(rawURL string) error {
 var openBrowserCommand = defaultOpenBrowserCommand
 
 func defaultOpenBrowserCommand(rawURL string) *exec.Cmd {
-	switch runtime.GOOS {
+	return defaultOpenBrowserCommandForGOOS(runtime.GOOS, rawURL)
+}
+
+func defaultOpenBrowserCommandForGOOS(goos, rawURL string) *exec.Cmd {
+	switch goos {
 	case "darwin":
 		return exec.Command("open", "--", rawURL)
 	case "windows":
@@ -785,7 +789,7 @@ func defaultOpenBrowserCommand(rawURL string) *exec.Cmd {
 		// cmd /c start flags.
 		return exec.Command("cmd", "/c", "start", "", "--", rawURL)
 	default:
-		return exec.Command("xdg-open", "--", rawURL)
+		return exec.Command("xdg-open", rawURL)
 	}
 }
 

internal/auth/oauth_authcode_test.go

@@ -19,6 +19,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"reflect"
 	"runtime"
 	"strings"
 	"sync"
@@ -1199,13 +1200,26 @@ func TestDefaultOpenBrowserReturnsAfterStart(t *testing.T) {
 }
 
 func TestDefaultOpenBrowserCommandUsesArgumentSeparator(t *testing.T) {
+	if runtime.GOOS == "linux" {
+		// xdg-open does not support --, so we skip the separator check on Linux.
+		// Real OAuth URLs always start with https://, so this is safe in practice.
+		t.Skip("xdg-open does not accept --")
+	}
 	cmd := defaultOpenBrowserCommand("-https://example.com")
 	args := strings.Join(cmd.Args, "\x00")
 	if !strings.Contains(args, "\x00--\x00-https://example.com") {
 		t.Fatalf("browser command should pass -- before URL, got %#v", cmd.Args)
 	}
 }
 
+func TestDefaultOpenBrowserCommandLinuxUsesXDGOpenWithoutSeparator(t *testing.T) {
+	cmd := defaultOpenBrowserCommandForGOOS("linux", "https://example.com/callback?code=abc")
+	want := []string{"xdg-open", "https://example.com/callback?code=abc"}
+	if !reflect.DeepEqual(cmd.Args, want) {
+		t.Fatalf("linux browser command args = %#v, want %#v", cmd.Args, want)
+	}
+}
+
 func mustCallbackURL(t *testing.T, rawAuthorizeURL string) (string, string) {
 	t.Helper()
 	u, err := url.Parse(rawAuthorizeURL)

internal/cli/api.go

@@ -5,7 +5,9 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"net/http"
+	"net/url"
 	"os"
 	"reflect"
 	"sort"
@@ -222,6 +224,7 @@ func (c *CLI) runAPISync(cmd *cobra.Command, args []string) error {
 	if closer != nil {
 		defer closer.Close()
 	}
+	fetch := c.discoveryFetcher(requestContext(cmd), apiName, apiCfg, profileName)
 	discCfg := spec.DiscoverConfig{
 		APIName:          c.apiStateName(apiName),
 		BaseURL:          effectiveProfileBaseURL(apiCfg, profileName),
@@ -232,6 +235,7 @@ func (c *CLI) runAPISync(cmd *cobra.Command, args []string) error {
 		ServerVariables:  effectiveServerVariables(apiCfg, profileName),
 		Version:          Version,
 		Transport:        transport,
+		Fetch:            fetch,
 		AllowCrossOrigin: apiCfg.AllowCrossOriginSpec || allowCrossOrigin,
 		ForceRefresh:     true,
 		Trace:            c.discoveryTrace(cmd),
@@ -336,6 +340,7 @@ func (c *CLI) runAPIConnect(cmd *cobra.Command, args []string) error {
 		if closer != nil {
 			defer closer.Close()
 		}
+		fetch := c.discoveryFetcher(requestContext(cmd), apiName, apiCfg, "default")
 		discCfg := spec.DiscoverConfig{
 			APIName:          apiName,
 			BaseURL:          baseURL,
@@ -345,6 +350,7 @@ func (c *CLI) runAPIConnect(cmd *cobra.Command, args []string) error {
 			ServerVariables:  nil,
 			Version:          Version,
 			Transport:        transport,
+			Fetch:            fetch,
 			AllowCrossOrigin: allowCrossOrigin,
 			ForceRefresh:     true,
 			Trace:            c.discoveryTrace(cmd),
@@ -1114,6 +1120,127 @@ func (c *CLI) discoveryTransport(ctx context.Context, apiCfg *config.APIConfig,
 	return transport, closer, nil
 }
 
+func (c *CLI) discoveryFetcher(ctx context.Context, apiName string, apiCfg *config.APIConfig, profileName string) spec.HTTPFetcher {
+	authOrigins := discoveryAuthOrigins(apiCfg, profileName)
+	return func(ctx context.Context, rawURL string, transport http.RoundTripper) (*http.Response, error) {
+		baseOpts, authOpts := c.discoveryRequestOptions(ctx, apiName, apiCfg, profileName, transport)
+		opts := baseOpts
+		if discoveryAuthAllowed(authOrigins, rawURL) {
+			opts = authOpts
+		}
+		return c.doDiscoveryRequest(ctx, http.MethodGet, rawURL, opts)
+	}
+}
+
+func (c *CLI) discoveryRequestOptions(ctx context.Context, apiName string, apiCfg *config.APIConfig, profileName string, transport http.RoundTripper) (request.Options, request.Options) {
+	if profileName == "" {
+		profileName = "default"
+	}
+	baseOpts := request.Options{
+		Transport: transport,
+		UserAgent: "restish/" + Version,
+	}
+	authOpts := baseOpts
+	if apiCfg != nil {
+		if apiCfg.PreserveHeaderCase {
+			authOpts.PreserveHeaderCase = true
+		}
+		prof := profileForName(apiCfg, profileName)
+		if prof != nil {
+			authOpts.Headers = append([]string(nil), prof.Headers...)
+			authOpts.Query = append([]string(nil), prof.Query...)
+		}
+		callbacks := c.authOnRequest(apiName, profileName, prof, authHandlerOptionsFromContext(ctx))
+		authOpts.OnRequest = callbacks.OnRequest
+		authOpts.OnUnauthorized = callbacks.OnUnauthorized
+	}
+	if authOpts.OnRequest != nil || len(c.pluginsByHook["request-middleware"]) > 0 {
+		origOnRequest := authOpts.OnRequest
+		authOpts.OnRequest = func(req *http.Request) error {
+			if origOnRequest != nil {
+				if err := origOnRequest(req); err != nil {
+					return err
+				}
+			}
+			return c.runRequestMiddlewarePlugins(req)
+		}
+	}
+	return baseOpts, authOpts
+}
+
+func (c *CLI) doDiscoveryRequest(ctx context.Context, method, rawURL string, opts request.Options) (*http.Response, error) {
+	resp, err := request.Do(ctx, method, rawURL, nil, opts)
+	if err != nil || resp == nil || resp.StatusCode != http.StatusUnauthorized || opts.OnUnauthorized == nil {
+		return resp, err
+	}
+	if resp.Body != nil {
+		_, _ = io.Copy(io.Discard, resp.Body)
+		_ = resp.Body.Close()
+	}
+	retryOpts := opts
+	onUnauthorized := retryOpts.OnUnauthorized
+	retryOpts.OnRequest = func(req *http.Request) error {
+		if err := onUnauthorized(req); err != nil {
+			return err
+		}
+		return c.runRequestMiddlewarePlugins(req)
+	}
+	retryOpts.OnUnauthorized = nil
+	return request.Do(ctx, method, rawURL, nil, retryOpts)
+}
+
+func authHandlerOptionsFromContext(ctx context.Context) authHandlerOptions {
+	gf := globalFlagsFromContext(ctx)
+	return authHandlerOptions{NoBrowser: gf.NoBrowser, Verbose: gf.Verbose > 0}
+}
+
+func discoveryAuthOrigins(apiCfg *config.APIConfig, profileName string) []*url.URL {
+	if apiCfg == nil {
+		return nil
+	}
+	var origins []*url.URL
+	addNormalized := func(raw string) {
+		if raw == "" {
+			return
+		}
+		if normalized, err := request.Normalize(raw, ""); err == nil {
+			raw = normalized
+		}
+		u, err := url.Parse(raw)
+		if err == nil && u.IsAbs() && (u.Scheme == "http" || u.Scheme == "https") {
+			origins = append(origins, u)
+		}
+	}
+	addExplicitURL := func(raw string) {
+		u, err := url.Parse(raw)
+		if err == nil && u.IsAbs() && (u.Scheme == "http" || u.Scheme == "https") {
+			origins = append(origins, u)
+		}
+	}
+	addNormalized(effectiveProfileBaseURL(apiCfg, profileName))
+	addNormalized(apiCfg.SpecURL)
+	for _, src := range apiCfg.SpecFiles {
+		addExplicitURL(src)
+	}
+	return origins
+}
+
+func discoveryAuthAllowed(origins []*url.URL, rawURL string) bool {
+	if normalized, err := request.Normalize(rawURL, ""); err == nil {
+		rawURL = normalized
+	}
+	u, err := url.Parse(rawURL)
+	if err != nil || !u.IsAbs() {
+		return false
+	}
+	for _, origin := range origins {
+		if request.SameOrigin(origin, u) {
+			return true
+		}
+	}
+	return false
+}
+
 // runAPIInspect prints the config for a named API as indented JSON,
 // with secret auth params replaced by "***".
 func (c *CLI) runAPIInspect(cmd *cobra.Command, args []string) error {