R83: DCR require client_secret for confidential auth, validate redirect_uri scheme; reject callback query/fragment

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")

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",
+		ProviderURL:          "https://issuer.example.com",
-		CallbackURL:         "/callback",
+		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")

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")
+	}
+}

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