From 0dfa2979bba92a14f30fa0ec922fae0e64528c47 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 13 Aug 2026 18:46:53 +0100 Subject: [PATCH] R83: DCR require client_secret for confidential auth, validate redirect_uri scheme; reject callback query/fragment --- dynamic_client_registration.go | 22 ++++++++ review_r36_regression_test.go | 8 +-- review_r83_test.go | 97 ++++++++++++++++++++++++++++++++++ settings.go | 8 +++ 4 files changed, 131 insertions(+), 4 deletions(-) create mode 100644 review_r83_test.go diff --git a/dynamic_client_registration.go b/dynamic_client_registration.go index a37151a..995df13 100644 --- a/dynamic_client_registration.go +++ b/dynamic_client_registration.go @@ -207,6 +207,19 @@ func (r *DynamicClientRegistrar) RegisterClient(ctx context.Context, registratio return nil, fmt.Errorf("registration response missing client_id") } + // The plugin's runtime client-auth default is client_secret_post + // (settings.go Config.Validate); RFC 7591 registers a confidential + // client. For a secret-based auth method the response must carry a + // client_secret, otherwise every token exchange fails. Adopting a + // secret-less client would silently break all auth. + authMethod := "client_secret_post" + if r.config != nil && r.config.ClientMetadata != nil && r.config.ClientMetadata.TokenEndpointAuthMethod != "" { + authMethod = r.config.ClientMetadata.TokenEndpointAuthMethod + } + if (authMethod == "client_secret_post" || authMethod == "client_secret_basic") && regResp.ClientSecret == "" { + return nil, fmt.Errorf("registration response missing client_secret for auth method %q", authMethod) + } + r.logger.Infof("Successfully registered client with ID: %s", regResp.ClientID) // Cache the response @@ -237,6 +250,15 @@ func (r *DynamicClientRegistrar) buildRegistrationRequest() ([]byte, error) { // Required: redirect_uris if len(metadata.RedirectURIs) > 0 { + // Reject malformed or wrong-scheme entries before they reach the IdP: + // RFC 7591 expects absolute http/https redirect URIs, and an invalid + // one sent to the IdP would corrupt the registered callback. + for _, uri := range metadata.RedirectURIs { + parsed, perr := url.ParseRequestURI(uri) + if perr != nil || (parsed.Scheme != "http" && parsed.Scheme != "https") || parsed.Host == "" { + return nil, fmt.Errorf("redirect_uri %q must be an absolute http(s) URL", uri) + } + } reqData["redirect_uris"] = metadata.RedirectURIs } else { return nil, fmt.Errorf("redirect_uris is required for client registration") diff --git a/review_r36_regression_test.go b/review_r36_regression_test.go index 40f77ce..6d99626 100644 --- a/review_r36_regression_test.go +++ b/review_r36_regression_test.go @@ -16,10 +16,10 @@ import ( // combination is still validated as before. func TestConfigValidate_DCRSkipsCredentialCheck(t *testing.T) { base := Config{ - ProviderURL: "https://issuer.example.com", - CallbackURL: "/callback", + ProviderURL: "https://issuer.example.com", + CallbackURL: "/callback", SessionEncryptionKey: "0123456789abcdef0123456789abcdef", // >= MinSessionEncryptionKeyLength - RateLimit: 100, // >= MinRateLimit (10) + RateLimit: 100, // >= MinRateLimit (10) } // Control: without DCR, an empty clientID must still be rejected. @@ -54,7 +54,7 @@ func TestDCRBuildRegistrationRequestDefaultAuthMethod(t *testing.T) { cfg := &DynamicClientRegistrationConfig{ Enabled: true, ClientMetadata: &ClientRegistrationMetadata{ - RedirectURIs: []string{"/callback"}, + RedirectURIs: []string{"https://example.com/callback"}, }, } r := NewDynamicClientRegistrar(nil, nil, cfg, "https://issuer.example.com") diff --git a/review_r83_test.go b/review_r83_test.go new file mode 100644 index 0000000..383a54a --- /dev/null +++ b/review_r83_test.go @@ -0,0 +1,97 @@ +package traefikoidc + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" +) + +// TestDCR_MissingClientSecretRejected is a regression: a registration +// response carrying client_id but no client_secret was accepted and adopted, +// although the plugin's client-auth default is client_secret_post (RFC 7591 +// confidential client) — every subsequent token exchange would fail. The fix +// rejects a secret-less response for secret-based auth methods. +func TestDCR_MissingClientSecretRejected(t *testing.T) { + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(ClientRegistrationResponse{ClientID: "client-123"}) + })) + defer server.Close() + + registrar := NewDynamicClientRegistrar( + server.Client(), + NewLogger("DEBUG"), + &DynamicClientRegistrationConfig{ + Enabled: true, + ClientMetadata: &ClientRegistrationMetadata{ + RedirectURIs: []string{"https://example.com/callback"}, + }, + }, + server.URL, + ) + + _, err := registrar.RegisterClient(context.Background(), server.URL+"/register") + if err == nil { + t.Fatalf("expected registration to fail when the response lacks a client_secret for client_secret_post") + } +} + +// TestConfig_RejectsCallbackQueryOrFragment is a regression: a callbackURL +// containing a query string or fragment passed config validation but can +// never match the request path (req.URL.Path == redirURLPath) and breaks +// redirect_uri, causing an endless re-auth loop. The fix rejects it. +func TestConfig_RejectsCallbackQueryOrFragment(t *testing.T) { + tests := []string{ + "/callback?from=idp", + "/callback#frag", + } + for _, cb := range tests { + cfg := &Config{ + ProviderURL: "https://issuer.example.com", + CallbackURL: cb, + ClientID: "client-id", + ClientSecret: "client-secret", + SessionEncryptionKey: "0123456789abcdef0123456789abcdef", + ClientAuthMethod: "client_secret_post", + RateLimit: 100, + } + if err := cfg.Validate(); err == nil { + t.Errorf("expected Validate to reject callbackURL %q with query/fragment", cb) + } + } +} + +// TestDCR_InvalidRedirectURIScheme is a regression: redirect_uris entries +// were sent to the IdP without validating scheme/host, so a malformed or +// wrong-scheme URI reached the provider and corrupted the registered +// callback. The fix rejects non-absolute http(s) entries before the POST. +func TestDCR_InvalidRedirectURIScheme(t *testing.T) { + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(ClientRegistrationResponse{ + ClientID: "client-123", + ClientSecret: "secret-456", + }) + })) + defer server.Close() + + registrar := NewDynamicClientRegistrar( + server.Client(), + NewLogger("DEBUG"), + &DynamicClientRegistrationConfig{ + Enabled: true, + ClientMetadata: &ClientRegistrationMetadata{ + RedirectURIs: []string{"ftp://example.com/not-a-web-callback"}, + }, + }, + server.URL, + ) + + if _, err := registrar.RegisterClient(context.Background(), server.URL+"/register"); err == nil { + t.Fatalf("expected registration to reject a non-http(s) redirect_uri") + } +} diff --git a/settings.go b/settings.go index b5e42d3..a7bed05 100644 --- a/settings.go +++ b/settings.go @@ -421,6 +421,14 @@ func (c *Config) Validate() error { if !strings.HasPrefix(c.CallbackURL, "/") { return fmt.Errorf("callbackURL must start with /") } + // The callback is matched by exact request-path comparison + // (req.URL.Path == redirURLPath) and used verbatim as redirect_uri. A + // query string or fragment in the path config part never matches the + // request path and corrupts redirect_uri (buildFullURL), causing an + // endless re-auth loop. Reject it at config time. + if strings.ContainsAny(c.CallbackURL, "?#") { + return fmt.Errorf("callbackURL must not contain a query string or fragment") + } // Validate client credentials. With Dynamic Client Registration (RFC 7591) // enabled, clientID (and typically clientSecret) are not known at