Skip to content

Commit 9e7bb58

Browse files
committed
Harden OAuth discovery and token endpoints
- Scheme-validate the discovery authorization_endpoint (https, or http on loopback) before it's dispatched to the OS browser launcher, so a hostile discovery doc can't hand the OS a file:// (or other) URL. - Enforce RequireSecureURL on the DCR registration_endpoint. - Set CheckRedirect on the OAuth HTTP client so token exchange/refresh bodies (auth code, refresh_token) aren't replayed to a redirect target.
1 parent 61aea03 commit 9e7bb58

3 files changed

Lines changed: 64 additions & 2 deletions

File tree

internal/appctx/context.go

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,18 @@ func (a *authAdapter) AccessToken(ctx context.Context) (string, error) {
8383

8484
// NewApp creates a new App with the given configuration.
8585
func NewApp(cfg *config.Config) *App {
86-
// Create HTTP client for auth manager (OAuth discovery, token refresh)
87-
httpClient := &http.Client{Timeout: 30 * time.Second}
86+
// Create HTTP client for auth manager (OAuth discovery, token refresh).
87+
// Refuse to follow redirects: RFC 6749 token endpoints don't legitimately
88+
// 3xx-redirect POSTs, and because the exchange/refresh requests set GetBody,
89+
// Go would replay the auth code / refresh_token to the redirect target —
90+
// only the initial endpoint is origin-validated. (Mirrors the SDK API
91+
// client's redirect guard.)
92+
httpClient := &http.Client{
93+
Timeout: 30 * time.Second,
94+
CheckRedirect: func(*http.Request, []*http.Request) error {
95+
return http.ErrUseLastResponse
96+
},
97+
}
8898
authMgr := auth.NewManager(cfg, httpClient)
8999

90100
// Create observability components

internal/auth/auth.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -622,6 +622,13 @@ func (m *Manager) loadBC3Client() (*ClientCredentials, error) {
622622
}
623623

624624
func (m *Manager) registerBC3Client(ctx context.Context, registrationEndpoint string, opts *LoginOptions) (*ClientCredentials, error) {
625+
// The registration endpoint comes from the server-controlled discovery
626+
// document. Refuse to POST the DCR request over plaintext http to a
627+
// non-localhost host (matches the token-exchange and launchpad paths).
628+
if err := hostutil.RequireSecureURL(registrationEndpoint); err != nil {
629+
return nil, output.ErrAuth(fmt.Sprintf("registration endpoint: %v", err))
630+
}
631+
625632
customRedirect := opts.RedirectURI != defaultRedirectURI
626633
regReq := map[string]any{
627634
"client_name": "basecamp-cli",
@@ -704,6 +711,15 @@ func (m *Manager) buildAuthURL(cfg *oauth.Config, oauthType, scope, state, codeC
704711
return "", err
705712
}
706713

714+
// The authorization endpoint comes from the server-controlled discovery
715+
// document and is later dispatched to the OS browser handler (xdg-open /
716+
// open). Restrict it to https (or http on loopback for local development)
717+
// so a hostile discovery doc can't hand the OS a file:// (or other) URL.
718+
secureEndpoint := u.Scheme == "https" || (u.Scheme == "http" && hostutil.IsLocalhost(u.Host))
719+
if !secureEndpoint {
720+
return "", output.ErrAuth(fmt.Sprintf("invalid authorization endpoint %q: scheme must be https (or http on loopback)", cfg.AuthorizationEndpoint))
721+
}
722+
707723
q := u.Query()
708724
q.Set("response_type", "code")
709725
q.Set("client_id", clientID)

internal/auth/auth_test.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -592,6 +592,42 @@ func TestBuildAuthURL_UsesResolvedRedirectURI(t *testing.T) {
592592
assert.Contains(t, authURL, "redirect_uri=http%3A%2F%2Flocalhost%3A9999%2Fmy-callback")
593593
}
594594

595+
// TestBuildAuthURL_RejectsUnsafeScheme guards against a hostile discovery
596+
// document handing the OS browser launcher a non-https authorization endpoint
597+
// (e.g. file://). https and http-on-loopback are accepted; everything else
598+
// must error before reaching OpenBrowser.
599+
func TestBuildAuthURL_RejectsUnsafeScheme(t *testing.T) {
600+
m := &Manager{cfg: config.Default(), httpClient: http.DefaultClient}
601+
opts := &LoginOptions{RedirectURI: "http://localhost:9999/callback"}
602+
603+
accepted := []string{
604+
"https://auth.example.com/authorize",
605+
"http://localhost:3000/authorize",
606+
"http://127.0.0.1:3000/authorize",
607+
}
608+
for _, endpoint := range accepted {
609+
t.Run("accepts "+endpoint, func(t *testing.T) {
610+
oauthCfg := &oauth.Config{AuthorizationEndpoint: endpoint}
611+
_, err := m.buildAuthURL(oauthCfg, "bc3", "read", "state", "challenge", "cid", opts)
612+
require.NoError(t, err)
613+
})
614+
}
615+
616+
rejected := []string{
617+
"file:///etc/passwd",
618+
"http://evil.example.com/authorize",
619+
"javascript:alert(1)",
620+
"-flag",
621+
}
622+
for _, endpoint := range rejected {
623+
t.Run("rejects "+endpoint, func(t *testing.T) {
624+
oauthCfg := &oauth.Config{AuthorizationEndpoint: endpoint}
625+
_, err := m.buildAuthURL(oauthCfg, "bc3", "read", "state", "challenge", "cid", opts)
626+
require.Error(t, err)
627+
})
628+
}
629+
}
630+
595631
func TestExchangeCode_UsesResolvedRedirectURI(t *testing.T) {
596632
// Capture the request body sent to the token endpoint
597633
var receivedBody string

0 commit comments

Comments
 (0)