Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions cmd/thv/app/auth_flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ type RemoteAuthFlags struct {
RemoteAuthClientSecret string
RemoteAuthClientSecretFile string
RemoteAuthScopes []string
RemoteAuthScopeParamName string
RemoteAuthSkipBrowser bool
RemoteAuthTimeout time.Duration
RemoteAuthCallbackPort int
Expand Down Expand Up @@ -163,6 +164,8 @@ func AddRemoteAuthFlags(cmd *cobra.Command, config *RemoteAuthFlags) {
"authorization server supports dynamic client registration (RFC 7591) or if using PKCE)")
cmd.Flags().StringSliceVar(&config.RemoteAuthScopes, "remote-auth-scopes", []string{},
"OAuth scopes to request for remote server authentication (defaults: OIDC uses 'openid,profile,email')")
cmd.Flags().StringVar(&config.RemoteAuthScopeParamName, "remote-auth-scope-param-name", "",
"Override the query parameter name for scopes in the authorization URL (e.g., 'user_scope' for Slack OAuth)")
cmd.Flags().BoolVar(&config.RemoteAuthSkipBrowser, "remote-auth-skip-browser", false,
"Skip opening browser for remote server OAuth flow (default false)")
cmd.Flags().DurationVar(&config.RemoteAuthTimeout, "remote-auth-timeout", 30*time.Second,
Expand Down
34 changes: 18 additions & 16 deletions cmd/thv/app/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -360,14 +360,15 @@ func handleOutgoingAuthentication(ctx context.Context) (*discovery.OAuthFlowResu
}

flowConfig := &discovery.OAuthFlowConfig{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: duplicate OAuthFlowConfig construction

The two OAuthFlowConfig struct literals (here and at line 393) are identical — the only difference is the first argument to PerformOAuthFlow. Pre-existing, but this PR extends it by one more field. Consider extracting a helper to avoid future drift.

ClientID: remoteAuthFlags.RemoteAuthClientID,
ClientSecret: clientSecret,
AuthorizeURL: remoteAuthFlags.RemoteAuthAuthorizeURL,
TokenURL: remoteAuthFlags.RemoteAuthTokenURL,
Scopes: remoteAuthFlags.RemoteAuthScopes,
CallbackPort: remoteAuthFlags.RemoteAuthCallbackPort,
Timeout: remoteAuthFlags.RemoteAuthTimeout,
SkipBrowser: remoteAuthFlags.RemoteAuthSkipBrowser,
ClientID: remoteAuthFlags.RemoteAuthClientID,
ClientSecret: clientSecret,
AuthorizeURL: remoteAuthFlags.RemoteAuthAuthorizeURL,
TokenURL: remoteAuthFlags.RemoteAuthTokenURL,
Scopes: remoteAuthFlags.RemoteAuthScopes,
CallbackPort: remoteAuthFlags.RemoteAuthCallbackPort,
Timeout: remoteAuthFlags.RemoteAuthTimeout,
SkipBrowser: remoteAuthFlags.RemoteAuthSkipBrowser,
ScopeParamName: remoteAuthFlags.RemoteAuthScopeParamName,
}

result, err := discovery.PerformOAuthFlow(ctx, remoteAuthFlags.RemoteAuthIssuer, flowConfig)
Expand All @@ -390,14 +391,15 @@ func handleOutgoingAuthentication(ctx context.Context) (*discovery.OAuthFlowResu

// Perform OAuth flow with discovered configuration
flowConfig := &discovery.OAuthFlowConfig{
ClientID: remoteAuthFlags.RemoteAuthClientID,
ClientSecret: clientSecret,
AuthorizeURL: remoteAuthFlags.RemoteAuthAuthorizeURL,
TokenURL: remoteAuthFlags.RemoteAuthTokenURL,
Scopes: remoteAuthFlags.RemoteAuthScopes,
CallbackPort: remoteAuthFlags.RemoteAuthCallbackPort,
Timeout: remoteAuthFlags.RemoteAuthTimeout,
SkipBrowser: remoteAuthFlags.RemoteAuthSkipBrowser,
ClientID: remoteAuthFlags.RemoteAuthClientID,
ClientSecret: clientSecret,
AuthorizeURL: remoteAuthFlags.RemoteAuthAuthorizeURL,
TokenURL: remoteAuthFlags.RemoteAuthTokenURL,
Scopes: remoteAuthFlags.RemoteAuthScopes,
CallbackPort: remoteAuthFlags.RemoteAuthCallbackPort,
Timeout: remoteAuthFlags.RemoteAuthTimeout,
SkipBrowser: remoteAuthFlags.RemoteAuthSkipBrowser,
ScopeParamName: remoteAuthFlags.RemoteAuthScopeParamName,
}

result, err := discovery.PerformOAuthFlow(ctx, authInfo.Realm, flowConfig)
Expand Down
4 changes: 4 additions & 0 deletions cmd/thv/app/run_flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -911,6 +911,9 @@ func getRemoteAuthFromRemoteServerMetadata(
authCfg.OAuthParams = oc.OAuthParams
}

// ScopeParamName: from CLI flag only (not yet supported in registry metadata)
authCfg.ScopeParamName = f.RemoteAuthScopeParamName

// Resolve bearer token from multiple sources (flag, file, environment variable)
resolvedBearerToken, err := resolveSecret(
f.RemoteAuthBearerToken,
Expand Down Expand Up @@ -972,6 +975,7 @@ func getRemoteAuthFromRunFlags(runFlags *RunFlags) (*remote.Config, error) {
ClientID: runFlags.RemoteAuthFlags.RemoteAuthClientID,
ClientSecret: clientSecret,
Scopes: runFlags.RemoteAuthFlags.RemoteAuthScopes,
ScopeParamName: runFlags.RemoteAuthFlags.RemoteAuthScopeParamName,
SkipBrowser: runFlags.RemoteAuthFlags.RemoteAuthSkipBrowser,
Timeout: runFlags.RemoteAuthFlags.RemoteAuthTimeout,
CallbackPort: runFlags.RemoteAuthFlags.RemoteAuthCallbackPort,
Expand Down
1 change: 1 addition & 0 deletions docs/cli/thv_proxy.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions docs/cli/thv_run.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions pkg/auth/discovery/discovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,7 @@ type OAuthFlowConfig struct {
SkipBrowser bool
Resource string // RFC 8707 resource indicator (optional)
OAuthParams map[string]string
ScopeParamName string // Override scope query parameter name (e.g., "user_scope" for Slack)
}

// OAuthFlowResult contains the result of an OAuth flow
Expand Down Expand Up @@ -645,6 +646,7 @@ func createOAuthConfig(ctx context.Context, issuer string, config *OAuthFlowConf
config.CallbackPort,
config.Resource,
config.OAuthParams,
config.ScopeParamName,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ScopeParamName silently ignored on OIDC discovery fallback path

ScopeParamName is correctly passed here on the manual-endpoints path, but the OIDC discovery fallback below (line ~655, CreateOAuthConfigFromOIDC) does not accept or propagate ScopeParamName. A user who sets --remote-auth-scope-param-name with --remote-auth-issuer but without explicit endpoint URLs will get standard scope= behavior with no warning.

Could set it on the returned config after the OIDC call:

cfg, err := oauth.CreateOAuthConfigFromOIDC(ctx, issuer, ...)
if err != nil { return nil, err }
cfg.ScopeParamName = config.ScopeParamName
return cfg, nil

)
}

Expand Down
20 changes: 20 additions & 0 deletions pkg/auth/oauth/flow.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"net/http"
"os"
"os/signal"
"strings"
"syscall"
"time"

Expand Down Expand Up @@ -60,6 +61,13 @@ type Config struct {

// OAuthParams are additional parameters to pass to the authorization URL
OAuthParams map[string]string

// ScopeParamName overrides the query parameter name used to send scopes in the
// authorization URL. When empty (default), the standard "scope" parameter is used.
// Some providers use non-standard parameter names (e.g., Slack uses "user_scope"
// for user-token scopes). When set, scopes are sent under this parameter name
// instead of "scope", and the standard "scope" parameter is cleared.
ScopeParamName string
}

// Flow handles the OAuth authentication flow
Expand Down Expand Up @@ -267,6 +275,18 @@ func (f *Flow) buildAuthURL() string {
}
}

// When a custom scope parameter name is configured, move scopes from the
// standard "scope" parameter to the custom one. This supports OAuth providers
// that use non-standard parameter names (e.g., Slack's "user_scope").
// The standard "scope" is cleared by setting it to empty; oauth2Config.Scopes
// is preserved so token refresh requests still include scopes correctly.
if f.config.ScopeParamName != "" && len(f.oauth2Config.Scopes) > 0 {
opts = append(opts,
oauth2.SetAuthURLParam("scope", ""),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

scope= empty parameter persists in the authorization URL

oauth2.SetAuthURLParam("scope", "") does not remove the scope parameter — it sets it to an empty string, producing scope= in the query string. RFC 6749 §3.3 requires scope values to have at least one character, so an explicit scope= is syntactically invalid. Some OAuth providers may reject it.

The test at flow_test.go:274 uses query.Get("scope") which returns "" for both absent and empty-valued params, masking the issue.

One approach: temporarily nil out f.oauth2Config.Scopes before calling AuthCodeURL so the library never adds scope, then restore it:

if f.config.ScopeParamName != "" && len(f.oauth2Config.Scopes) > 0 {
    scopeValue := strings.Join(f.oauth2Config.Scopes, " ")
    savedScopes := f.oauth2Config.Scopes
    f.oauth2Config.Scopes = nil
    defer func() { f.oauth2Config.Scopes = savedScopes }()
    opts = append(opts,
        oauth2.SetAuthURLParam(f.config.ScopeParamName, scopeValue),
    )
}

And update the test assertion to verify truly absent:

_, has := query["scope"]
assert.False(t, has, "scope parameter should be absent, not empty")

oauth2.SetAuthURLParam(f.config.ScopeParamName, strings.Join(f.oauth2Config.Scopes, " ")),
)
}

// Add PKCE parameters if enabled
if f.config.UsePKCE {
opts = append(opts,
Expand Down
42 changes: 42 additions & 0 deletions pkg/auth/oauth/flow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,48 @@ func TestBuildAuthURL(t *testing.T) {
assert.Equal(t, "S256", query.Get("code_challenge_method"))
},
},
{
name: "auth URL with custom scope parameter name",
config: &Config{
ClientID: "test-client",
AuthURL: "https://example.com/auth",
TokenURL: "https://example.com/token",
Scopes: []string{"search:read", "chat:write"},
ScopeParamName: "user_scope",
},
validate: func(t *testing.T, authURL string, _ *Flow) {
t.Helper()
parsedURL, err := url.Parse(authURL)
require.NoError(t, err)

query := parsedURL.Query()
// Standard "scope" should be cleared
assert.Empty(t, query.Get("scope"))
// Scopes should appear under the custom parameter name
assert.Contains(t, query.Get("user_scope"), "search:read")
assert.Contains(t, query.Get("user_scope"), "chat:write")
},
},
{
name: "auth URL with scope param name but no scopes",
config: &Config{
ClientID: "test-client",
AuthURL: "https://example.com/auth",
TokenURL: "https://example.com/token",
Scopes: []string{},
ScopeParamName: "user_scope",
},
validate: func(t *testing.T, authURL string, _ *Flow) {
t.Helper()
parsedURL, err := url.Parse(authURL)
require.NoError(t, err)

query := parsedURL.Query()
// Neither scope nor user_scope should be present
assert.Empty(t, query.Get("scope"))
assert.Empty(t, query.Get("user_scope"))
},
},
}

for _, tt := range tests {
Expand Down
20 changes: 11 additions & 9 deletions pkg/auth/oauth/manual.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ func CreateOAuthConfigManual(
callbackPort int,
resource string,
oauthParams map[string]string,
scopeParamName string,
) (*Config, error) {
if clientID == "" {
return nil, fmt.Errorf("client ID is required")
Expand All @@ -44,14 +45,15 @@ func CreateOAuthConfigManual(
}

return &Config{
ClientID: clientID,
ClientSecret: clientSecret,
AuthURL: authURL,
TokenURL: tokenURL,
Scopes: scopes,
UsePKCE: usePKCE,
CallbackPort: callbackPort,
Resource: resource,
OAuthParams: oauthParams,
ClientID: clientID,
ClientSecret: clientSecret,
AuthURL: authURL,
TokenURL: tokenURL,
Scopes: scopes,
UsePKCE: usePKCE,
CallbackPort: callbackPort,
Resource: resource,
OAuthParams: oauthParams,
ScopeParamName: scopeParamName,
}, nil
}
5 changes: 5 additions & 0 deletions pkg/auth/oauth/manual_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ func TestCreateOAuthConfigManual(t *testing.T) {
tt.callbackPort,
tt.resource,
oauthParams,
"",
)

if tt.expectError {
Expand Down Expand Up @@ -335,6 +336,7 @@ func TestCreateOAuthConfigManual_ScopeDefaultBehavior(t *testing.T) {
8080,
"",
nil, // No OAuth params for basic tests
"", // No scope param name override
)

require.NoError(t, err)
Expand Down Expand Up @@ -378,6 +380,7 @@ func TestCreateOAuthConfigManual_PKCEBehavior(t *testing.T) {
8080,
"",
nil, // No OAuth params for basic tests
"", // No scope param name override
)

require.NoError(t, err)
Expand Down Expand Up @@ -426,6 +429,7 @@ func TestCreateOAuthConfigManual_CallbackPortBehavior(t *testing.T) {
tt.port,
"",
nil, // No OAuth params for basic tests
"", // No scope param name override
)

require.NoError(t, err)
Expand Down Expand Up @@ -491,6 +495,7 @@ func TestCreateOAuthConfigManual_OAuthParamsBehavior(t *testing.T) {
8080,
"",
tt.oauthParams,
"",
)

require.NoError(t, err)
Expand Down
5 changes: 5 additions & 0 deletions pkg/auth/remote/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ type Config struct {
// OAuth parameters for server-specific customization
OAuthParams map[string]string `json:"oauth_params,omitempty" yaml:"oauth_params,omitempty"`

// ScopeParamName overrides the query parameter name used to send scopes in the
// authorization URL. When empty, the standard "scope" parameter is used.
// Some providers require a non-standard name (e.g., Slack uses "user_scope").
ScopeParamName string `json:"scope_param_name,omitempty" yaml:"scope_param_name,omitempty"`

// Bearer token configuration (alternative to OAuth)
BearerToken string `json:"bearer_token,omitempty" yaml:"bearer_token,omitempty"` //nolint:gosec // G117
BearerTokenFile string `json:"bearer_token_file,omitempty" yaml:"bearer_token_file,omitempty"`
Expand Down
21 changes: 11 additions & 10 deletions pkg/auth/remote/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,16 +146,17 @@ func (h *Handler) performOAuthFlow(
// buildOAuthFlowConfig creates the OAuth flow configuration
func (h *Handler) buildOAuthFlowConfig(scopes []string, authServerInfo *discovery.AuthServerInfo) *discovery.OAuthFlowConfig {
flowConfig := &discovery.OAuthFlowConfig{
ClientID: h.config.ClientID,
ClientSecret: h.config.ClientSecret,
AuthorizeURL: h.config.AuthorizeURL,
TokenURL: h.config.TokenURL,
Scopes: scopes,
CallbackPort: h.config.CallbackPort,
Timeout: h.config.Timeout,
SkipBrowser: h.config.SkipBrowser,
Resource: h.config.Resource,
OAuthParams: h.config.OAuthParams,
ClientID: h.config.ClientID,
ClientSecret: h.config.ClientSecret,
AuthorizeURL: h.config.AuthorizeURL,
TokenURL: h.config.TokenURL,
Scopes: scopes,
CallbackPort: h.config.CallbackPort,
Timeout: h.config.Timeout,
SkipBrowser: h.config.SkipBrowser,
Resource: h.config.Resource,
OAuthParams: h.config.OAuthParams,
ScopeParamName: h.config.ScopeParamName,
}

// If we have discovered endpoints from the authorization server metadata,
Expand Down