auth: address PR #1377 review — preserve ErrNotLoggedIn, refuse discovery redirects

f883714→main·

toothbrush·1mo ago·4 files·+96 added/-8 removed

Two reviewer findings:

- ErrNotLoggedIn lost after discovery (cursor + Copilot): contextReauthError returned a plain string, so callers that branch on errors.Is(err, ErrNotLoggedIn) (NewAuthenticatedAPIClient/search/dispatch) fell through to their generic error — a regression vs the pre-discovery TokenForResource path. Wrap the sentinel via a reauthError type that keeps the friendly context-named message while unwrapping to the tokenmanager sentinel.

- Redirect-following in fetchWellKnownJSON (Copilot): a trust-root fetch must not follow a 3xx to another origin/plaintext. Refuse redirects on a shallow-copied client (so the caller's redirect policy is untouched). Low real exploitability — the token is never sent to the redirect target — but cheap hardening that covers the cluster path too.

Tests: provider error unwraps to ErrNotLoggedIn; cross-origin redirect (to a server serving a valid doc) is refused rather than followed.

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

Sessions

d24466d6862bView transcript

Changes

4

• cmd/entire/cli/auth

• Mdata_api_test.go+19

• Mrefresh.go+29/-7

• internal/entireclient/clusterdiscovery

• Mapi_discovery_test.go+35

• Mdiscovery.go+13/-1

154 unmodified lines

...

// When the selected context has no stored token, the provider's error must // still unwrap to ErrNotLoggedIn so callers (NewAuthenticatedAPIClient, search, // dispatch) that branch on errors.Is render their login guidance — the // regression the PR review flagged on the discovery path. func TestNewRefreshingResourceProvider_NotLoggedInPreservesSentinel(t *testing.T) { restore := tokenstore.UseFileBackendForTesting(filepath.Join(t.TempDir(), "tokens.json")) t.Cleanup(restore)

c := &contexts.Context{Name: "me@core", CoreURL: "https://core.example", Handle: "me", KeychainService: "kc:me"} provider, err := NewRefreshingResourceProvider(c, "https://data.example", "https://data.example", nil, false) if err != nil { t.Fatalf("NewRefreshingResourceProvider: %v", err) } _, err = provider(context.Background()) if !errors.Is(err, ErrNotLoggedIn) { t.Fatalf("provider error must unwrap to ErrNotLoggedIn, got %v", err) } }

func mustOrigin(t *testing.T, raw string) string { t.Helper() u, err := url.Parse(raw)

Mcmd/entire/cli/auth/data_api_test.go+19

139 unmodified lines

...

// reauthError carries a friendly, context-named re-login message while still
// unwrapping to the underlying tokenmanager sentinel. Callers that branch on
// errors.Is(err, ErrNotLoggedIn) (NewAuthenticatedAPIClient, search, dispatch)
// keep matching — without this, the discovery path turned a missing keyring
// token into an opaque string and those callers fell through to their generic
// error, a regression vs the pre-discovery TokenForResource path. Error()
// returns only msg so the sentinel's terse text ("not logged in") doesn't leak
// into the rendered message.
type reauthError struct {
    msg      string
    sentinel error
}

func (e *reauthError) Error() string { return e.msg }
func (e *reauthError) Unwrap() error { return e.sentinel }

// contextReauthError maps the two re-auth sentinels a per-context manager can
// return into a single friendly message that names the context and its core
// (so a multi-core user logs back into the right one — matching
// clusterdiscovery.RenderLoginHint's idiom). Returns nil when err is neither
// sentinel, leaving the caller to wrap the residual error in its own terms
// (refresh vs exchange).
// return into a friendly message that names the context and its core (so a
// multi-core user logs back into the right one — matching
// clusterdiscovery.RenderLoginHint's idiom), preserving the sentinel for
// errors.Is. Returns nil when err is neither sentinel, leaving the caller to
// wrap the residual error in its own terms (refresh vs exchange).
func contextReauthError(c *contexts.Context, err error) error {
    coreURL := strings.TrimRight(c.CoreURL, "/")
    switch {
    case errors.Is(err, tokenmanager.ErrReauthRequired):
        return fmt.Errorf("login session for %q (%s) expired; run `entire login` to re-authenticate", c.Name, coreURL)
    case errors.Is(err, tokenmanager.ErrNotLoggedIn):
        return fmt.Errorf("no usable login for %q (%s); run `entire login`, c.Name, coreURL)
    }
    return nil
}

Mcmd/entire/cli/auth/refresh.go+29/-7

3 unmodified lines

...

// schemeRewriteTransport rewrites the scheme to http (DiscoverAPI hard-codes // https://) while leaving the host untouched, so a cross-origin redirect // reaches its real target rather than being pinned back to the first server. type schemeRewriteTransport struct{ base http.RoundTripper }

func (s schemeRewriteTransport) RoundTrip(req *http.Request) (*http.Response, error) { req.URL.Scheme = "http" return s.base.RoundTrip(req) }

const apiDiscoveryBody = `{ "issuer": "https://us.auth.partial.to", "trusted_issuers": ["https://us.auth.partial.to", "https://eu.auth.partial.to"],

Minternal/entireclient/clusterdiscovery/api_discovery_test.go+35

76 unmodified lines

...

if err != nil {
    return fmt.Errorf("build discovery request: %w", err)
}
resp, err := c.Do(req)
// Refuse redirects. This is a trust-root fetch — the response decides which
// login servers we honour — so a 3xx to another origin (or a plaintext
// downgrade) from a hostile/misconfigured host must not be followed.
// Shallow-copy the caller's client so we don't mutate its redirect policy
// (it's reused for other operations); the copy shares Transport/TLS config.
if c == nil {
    c = http.DefaultClient
}
noRedirect := *c
noRedirect.CheckRedirect = func(*http.Request, []*http.Request) error {
    return errors.New("discovery does not follow redirects (trust root)")
}
resp, err := noRedirect.Do(req)
if err != nil {
    debugf("discovery: %v", err)
    return fmt.Errorf("%w: %w", ErrUnreachable, err)
}