From 0fcce5f8c25e6da6b5a33b04677c5a4ac3cb95b2 Mon Sep 17 00:00:00 2001 From: Eric Paulsen Date: Thu, 27 Mar 2025 12:19:20 +0000 Subject: [PATCH 1/5] fix: conceal sensitive domain information in auth error messages Remove exposure of allowed domain list in OIDC authentication error messages to enhance security. Third-party contractors no longer see internal domain lists when accessing Coder with unauthorized email addresses. --- coderd/userauth.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index ba57a1dff4580..f08e126208d3c 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1358,7 +1358,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { emailSp := strings.Split(email, "@") if len(emailSp) == 1 { httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ - Message: fmt.Sprintf("Your email %q is not in domains %q!", email, api.OIDCConfig.EmailDomain), + Message: fmt.Sprintf("Your email %q is not from an authorized domain! Please contact your administrator.", email), }) return } @@ -1373,7 +1373,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { } if !ok { httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ - Message: fmt.Sprintf("Your email %q is not in domains %q!", email, api.OIDCConfig.EmailDomain), + Message: fmt.Sprintf("Your email %q is not from an authorized domain! Please contact your administrator.", email), }) return } From 949ca4dad66c54a53b840370a2101a1aacb60f42 Mon Sep 17 00:00:00 2001 From: Eric Paulsen Date: Thu, 27 Mar 2025 12:37:08 +0000 Subject: [PATCH 2/5] add unit tests for unauth'd email domains --- coderd/domain_error_test.go | 121 ++++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 coderd/domain_error_test.go diff --git a/coderd/domain_error_test.go b/coderd/domain_error_test.go new file mode 100644 index 0000000000000..fb6fe07500ede --- /dev/null +++ b/coderd/domain_error_test.go @@ -0,0 +1,121 @@ +package coderd_test + +import ( + "context" + "io" + "net/http" + "testing" + + "github.com/golang-jwt/jwt/v4" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd" + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/coderdtest/oidctest" +) + +// TestOIDCDomainErrorMessage ensures that when a user with an unauthorized domain +// attempts to login, the error message doesn't expose the list of authorized domains. +func TestOIDCDomainErrorMessage(t *testing.T) { + t.Parallel() + + // Setup OIDC fake provider + fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) + + // Configure OIDC provider with domain restrictions + allowedDomains := []string{"allowed1.com", "allowed2.org", "company.internal"} + cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { + cfg.EmailDomain = allowedDomains + cfg.AllowSignups = true + }) + + // Create a Coder server with OIDC enabled + server := coderdtest.New(t, &coderdtest.Options{ + OIDCConfig: cfg, + }) + + // Test case 1: Email domain not in allowed list + t.Run("ErrorMessageOmitsDomains", func(t *testing.T) { + t.Parallel() + + // Prepare claims with email from unauthorized domain + claims := jwt.MapClaims{ + "email": "user@unauthorized.com", + "email_verified": true, + "sub": uuid.NewString(), + } + + // Attempt login and check for failure + _, resp := fake.AttemptLogin(t, server, claims) + defer resp.Body.Close() + + // Verify the status code + require.Equal(t, http.StatusForbidden, resp.StatusCode) + + // Check the response content + data, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + // Verify the message contains the generic text + require.Contains(t, string(data), "is not from an authorized domain") + require.Contains(t, string(data), "Please contact your administrator") + + // Verify it doesn't contain any of the allowed domains + for _, domain := range allowedDomains { + require.NotContains(t, string(data), domain) + } + }) + + // Test case 2: Malformed email without @ symbol + t.Run("MalformedEmailErrorOmitsDomains", func(t *testing.T) { + t.Parallel() + + // Prepare claims with an invalid email format (no @ symbol) + claims := jwt.MapClaims{ + "email": "invalid-email-without-domain", + "email_verified": true, + "sub": uuid.NewString(), + } + + // Attempt login and check for failure + _, resp := fake.AttemptLogin(t, server, claims) + defer resp.Body.Close() + + // Verify the status code + require.Equal(t, http.StatusForbidden, resp.StatusCode) + + // Check the response content + data, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + // Verify the message contains the generic text + require.Contains(t, string(data), "is not from an authorized domain") + require.Contains(t, string(data), "Please contact your administrator") + + // Verify it doesn't contain any of the allowed domains + for _, domain := range allowedDomains { + require.NotContains(t, string(data), domain) + } + }) + + // Test case 3: Authorized domain (should succeed) + t.Run("AuthorizedDomainSucceeds", func(t *testing.T) { + t.Parallel() + + // Prepare claims with an authorized domain + claims := jwt.MapClaims{ + "email": "user@allowed1.com", + "email_verified": true, + "sub": uuid.NewString(), + } + + // Attempt login and expect success + client, _ := fake.Login(t, server, claims) + + // Verify the user was created correctly + user, err := client.User(context.Background(), "me") + require.NoError(t, err) + require.Equal(t, "user", user.Username) + }) +} \ No newline at end of file From 75bb4cd43d6e7d7a591d9e78314b6f5517f8125e Mon Sep 17 00:00:00 2001 From: Eric Paulsen Date: Thu, 27 Mar 2025 12:41:50 +0000 Subject: [PATCH 3/5] make fmt --- coderd/domain_error_test.go | 44 ++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/coderd/domain_error_test.go b/coderd/domain_error_test.go index fb6fe07500ede..e2e6c8879af92 100644 --- a/coderd/domain_error_test.go +++ b/coderd/domain_error_test.go @@ -19,103 +19,103 @@ import ( // attempts to login, the error message doesn't expose the list of authorized domains. func TestOIDCDomainErrorMessage(t *testing.T) { t.Parallel() - + // Setup OIDC fake provider fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) - + // Configure OIDC provider with domain restrictions allowedDomains := []string{"allowed1.com", "allowed2.org", "company.internal"} cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { cfg.EmailDomain = allowedDomains cfg.AllowSignups = true }) - + // Create a Coder server with OIDC enabled server := coderdtest.New(t, &coderdtest.Options{ OIDCConfig: cfg, }) - + // Test case 1: Email domain not in allowed list t.Run("ErrorMessageOmitsDomains", func(t *testing.T) { t.Parallel() - + // Prepare claims with email from unauthorized domain claims := jwt.MapClaims{ "email": "user@unauthorized.com", "email_verified": true, "sub": uuid.NewString(), } - + // Attempt login and check for failure _, resp := fake.AttemptLogin(t, server, claims) defer resp.Body.Close() - + // Verify the status code require.Equal(t, http.StatusForbidden, resp.StatusCode) - + // Check the response content data, err := io.ReadAll(resp.Body) require.NoError(t, err) - + // Verify the message contains the generic text require.Contains(t, string(data), "is not from an authorized domain") require.Contains(t, string(data), "Please contact your administrator") - + // Verify it doesn't contain any of the allowed domains for _, domain := range allowedDomains { require.NotContains(t, string(data), domain) } }) - + // Test case 2: Malformed email without @ symbol t.Run("MalformedEmailErrorOmitsDomains", func(t *testing.T) { t.Parallel() - + // Prepare claims with an invalid email format (no @ symbol) claims := jwt.MapClaims{ "email": "invalid-email-without-domain", "email_verified": true, "sub": uuid.NewString(), } - + // Attempt login and check for failure _, resp := fake.AttemptLogin(t, server, claims) defer resp.Body.Close() - + // Verify the status code require.Equal(t, http.StatusForbidden, resp.StatusCode) - + // Check the response content data, err := io.ReadAll(resp.Body) require.NoError(t, err) - + // Verify the message contains the generic text require.Contains(t, string(data), "is not from an authorized domain") require.Contains(t, string(data), "Please contact your administrator") - + // Verify it doesn't contain any of the allowed domains for _, domain := range allowedDomains { require.NotContains(t, string(data), domain) } }) - + // Test case 3: Authorized domain (should succeed) t.Run("AuthorizedDomainSucceeds", func(t *testing.T) { t.Parallel() - + // Prepare claims with an authorized domain claims := jwt.MapClaims{ "email": "user@allowed1.com", "email_verified": true, "sub": uuid.NewString(), } - + // Attempt login and expect success client, _ := fake.Login(t, server, claims) - + // Verify the user was created correctly user, err := client.User(context.Background(), "me") require.NoError(t, err) require.Equal(t, "user", user.Username) }) -} \ No newline at end of file +} From bbdd048969bb5c609566f7b77ab83789256a1a05 Mon Sep 17 00:00:00 2001 From: Eric Paulsen Date: Thu, 27 Mar 2025 12:51:25 +0000 Subject: [PATCH 4/5] test: add test cases for domain concealment in auth error messages - Verifies the error message no longer shows domain list - Adds tests for both invalid domain and malformed email cases - Includes test for successful login with allowed domain - Fixes response body closing in test --- coderd/domain_error_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/domain_error_test.go b/coderd/domain_error_test.go index e2e6c8879af92..c7b5c629d9d10 100644 --- a/coderd/domain_error_test.go +++ b/coderd/domain_error_test.go @@ -111,7 +111,8 @@ func TestOIDCDomainErrorMessage(t *testing.T) { } // Attempt login and expect success - client, _ := fake.Login(t, server, claims) + client, resp := fake.Login(t, server, claims) + defer resp.Body.Close() // Verify the user was created correctly user, err := client.User(context.Background(), "me") From 4fe7e668ce41bf8e56c469d601c89e7ecc3269b2 Mon Sep 17 00:00:00 2001 From: Eric Paulsen Date: Thu, 27 Mar 2025 13:24:27 +0000 Subject: [PATCH 5/5] move tests to userauth_test.go & reduce comments --- coderd/domain_error_test.go | 122 ------------------------------------ coderd/userauth_test.go | 73 +++++++++++++++++++++ 2 files changed, 73 insertions(+), 122 deletions(-) delete mode 100644 coderd/domain_error_test.go diff --git a/coderd/domain_error_test.go b/coderd/domain_error_test.go deleted file mode 100644 index c7b5c629d9d10..0000000000000 --- a/coderd/domain_error_test.go +++ /dev/null @@ -1,122 +0,0 @@ -package coderd_test - -import ( - "context" - "io" - "net/http" - "testing" - - "github.com/golang-jwt/jwt/v4" - "github.com/google/uuid" - "github.com/stretchr/testify/require" - - "github.com/coder/coder/v2/coderd" - "github.com/coder/coder/v2/coderd/coderdtest" - "github.com/coder/coder/v2/coderd/coderdtest/oidctest" -) - -// TestOIDCDomainErrorMessage ensures that when a user with an unauthorized domain -// attempts to login, the error message doesn't expose the list of authorized domains. -func TestOIDCDomainErrorMessage(t *testing.T) { - t.Parallel() - - // Setup OIDC fake provider - fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) - - // Configure OIDC provider with domain restrictions - allowedDomains := []string{"allowed1.com", "allowed2.org", "company.internal"} - cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { - cfg.EmailDomain = allowedDomains - cfg.AllowSignups = true - }) - - // Create a Coder server with OIDC enabled - server := coderdtest.New(t, &coderdtest.Options{ - OIDCConfig: cfg, - }) - - // Test case 1: Email domain not in allowed list - t.Run("ErrorMessageOmitsDomains", func(t *testing.T) { - t.Parallel() - - // Prepare claims with email from unauthorized domain - claims := jwt.MapClaims{ - "email": "user@unauthorized.com", - "email_verified": true, - "sub": uuid.NewString(), - } - - // Attempt login and check for failure - _, resp := fake.AttemptLogin(t, server, claims) - defer resp.Body.Close() - - // Verify the status code - require.Equal(t, http.StatusForbidden, resp.StatusCode) - - // Check the response content - data, err := io.ReadAll(resp.Body) - require.NoError(t, err) - - // Verify the message contains the generic text - require.Contains(t, string(data), "is not from an authorized domain") - require.Contains(t, string(data), "Please contact your administrator") - - // Verify it doesn't contain any of the allowed domains - for _, domain := range allowedDomains { - require.NotContains(t, string(data), domain) - } - }) - - // Test case 2: Malformed email without @ symbol - t.Run("MalformedEmailErrorOmitsDomains", func(t *testing.T) { - t.Parallel() - - // Prepare claims with an invalid email format (no @ symbol) - claims := jwt.MapClaims{ - "email": "invalid-email-without-domain", - "email_verified": true, - "sub": uuid.NewString(), - } - - // Attempt login and check for failure - _, resp := fake.AttemptLogin(t, server, claims) - defer resp.Body.Close() - - // Verify the status code - require.Equal(t, http.StatusForbidden, resp.StatusCode) - - // Check the response content - data, err := io.ReadAll(resp.Body) - require.NoError(t, err) - - // Verify the message contains the generic text - require.Contains(t, string(data), "is not from an authorized domain") - require.Contains(t, string(data), "Please contact your administrator") - - // Verify it doesn't contain any of the allowed domains - for _, domain := range allowedDomains { - require.NotContains(t, string(data), domain) - } - }) - - // Test case 3: Authorized domain (should succeed) - t.Run("AuthorizedDomainSucceeds", func(t *testing.T) { - t.Parallel() - - // Prepare claims with an authorized domain - claims := jwt.MapClaims{ - "email": "user@allowed1.com", - "email_verified": true, - "sub": uuid.NewString(), - } - - // Attempt login and expect success - client, resp := fake.Login(t, server, claims) - defer resp.Body.Close() - - // Verify the user was created correctly - user, err := client.User(context.Background(), "me") - require.NoError(t, err) - require.Equal(t, "user", user.Username) - }) -} diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 38356eb19fdd6..ad8e126706dd1 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1982,6 +1982,79 @@ func TestUserLogout(t *testing.T) { // - JWT with issuer https://secondary.com // // Without this security check disabled, all three above would have to match. + +// TestOIDCDomainErrorMessage ensures that when a user with an unauthorized domain +// attempts to login, the error message doesn't expose the list of authorized domains. +func TestOIDCDomainErrorMessage(t *testing.T) { + t.Parallel() + + fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) + + allowedDomains := []string{"allowed1.com", "allowed2.org", "company.internal"} + cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { + cfg.EmailDomain = allowedDomains + cfg.AllowSignups = true + }) + + server := coderdtest.New(t, &coderdtest.Options{ + OIDCConfig: cfg, + }) + + // Test case 1: Email domain not in allowed list + t.Run("ErrorMessageOmitsDomains", func(t *testing.T) { + t.Parallel() + + // Prepare claims with email from unauthorized domain + claims := jwt.MapClaims{ + "email": "user@unauthorized.com", + "email_verified": true, + "sub": uuid.NewString(), + } + + _, resp := fake.AttemptLogin(t, server, claims) + defer resp.Body.Close() + + require.Equal(t, http.StatusForbidden, resp.StatusCode) + + data, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + require.Contains(t, string(data), "is not from an authorized domain") + require.Contains(t, string(data), "Please contact your administrator") + + for _, domain := range allowedDomains { + require.NotContains(t, string(data), domain) + } + }) + + // Test case 2: Malformed email without @ symbol + t.Run("MalformedEmailErrorOmitsDomains", func(t *testing.T) { + t.Parallel() + + // Prepare claims with an invalid email format (no @ symbol) + claims := jwt.MapClaims{ + "email": "invalid-email-without-domain", + "email_verified": true, + "sub": uuid.NewString(), + } + + _, resp := fake.AttemptLogin(t, server, claims) + defer resp.Body.Close() + + require.Equal(t, http.StatusForbidden, resp.StatusCode) + + data, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + require.Contains(t, string(data), "is not from an authorized domain") + require.Contains(t, string(data), "Please contact your administrator") + + for _, domain := range allowedDomains { + require.NotContains(t, string(data), domain) + } + }) +} + func TestOIDCSkipIssuer(t *testing.T) { t.Parallel() const primaryURLString = "https://primary.com"