diff --git a/.github/workflows/pr-test.yml b/.github/workflows/pr-test.yml index 5ae12500..da32a2f1 100644 --- a/.github/workflows/pr-test.yml +++ b/.github/workflows/pr-test.yml @@ -99,6 +99,10 @@ env: GITHUBUSER_CLIENT_ID: ${{ secrets.GITHUBUSER_CLIENT_ID }} GITHUBUSER_CLIENT_SECRET: ${{ secrets.GITHUBUSER_CLIENT_SECRET }} + ## Cloudflare Turnstile + CLOUDFLARE_TURNSTILE_SITEKEY: ${{ secrets.CLOUDFLARE_TURNSTILE_SITEKEY }} + CLOUDFLARE_TURNSTILE_SECRET: ${{ secrets.CLOUDFLARE_TURNSTILE_SECRET }} + jobs: UnitTest: runs-on: ubuntu-latest diff --git a/.github/workflows/unit-test.yml b/.github/workflows/unit-test.yml index 681f6719..1e7f2e16 100644 --- a/.github/workflows/unit-test.yml +++ b/.github/workflows/unit-test.yml @@ -104,6 +104,10 @@ env: GITHUBUSER_CLIENT_ID: ${{ secrets.GITHUBUSER_CLIENT_ID }} GITHUBUSER_CLIENT_SECRET: ${{ secrets.GITHUBUSER_CLIENT_SECRET }} + ## Cloudflare Turnstile + CLOUDFLARE_TURNSTILE_SITEKEY: ${{ secrets.CLOUDFLARE_TURNSTILE_SITEKEY }} + CLOUDFLARE_TURNSTILE_SECRET: ${{ secrets.CLOUDFLARE_TURNSTILE_SECRET }} + jobs: unit-test: runs-on: ubuntu-latest diff --git a/openapi/signin/api.go b/openapi/signin/api.go index d0e517d3..e7cd66a6 100644 --- a/openapi/signin/api.go +++ b/openapi/signin/api.go @@ -1,6 +1,12 @@ package signin import ( + "crypto/rand" + "encoding/hex" + "fmt" + "net/url" + "strings" + "github.com/gin-gonic/gin" "github.com/yaoapp/yao/openapi/oauth/types" "github.com/yaoapp/yao/openapi/response" @@ -11,6 +17,7 @@ func Attach(group *gin.RouterGroup, oauth types.OAuth) { group.GET("/signin", getConfig) group.POST("/signin", signin) group.GET("/signin/authback/:id", authback) + group.GET("/signin/oauth/:provider/authorize", getOAuthAuthorizationURL) } // getConfig is the handler for get signin configuration @@ -40,3 +47,127 @@ func signin(c *gin.Context) {} // authback is the handler for authback func authback(c *gin.Context) {} + +// OAuthAuthorizationURLResponse represents the response for OAuth authorization URL +type OAuthAuthorizationURLResponse struct { + AuthorizationURL string `json:"authorization_url"` + State string `json:"state"` +} + +// getOAuthAuthorizationURL generates OAuth authorization URL for a provider +func getOAuthAuthorizationURL(c *gin.Context) { + providerID := c.Param("provider") + if providerID == "" { + errorResp := &response.ErrorResponse{ + Code: response.ErrInvalidRequest.Code, + ErrorDescription: "Provider ID is required", + } + response.RespondWithError(c, response.StatusBadRequest, errorResp) + return + } + + // Get optional parameters + redirectURI := c.Query("redirect_uri") + state := c.Query("state") + locale := c.Query("locale") + + // Get full configuration + config := GetFullConfig(locale) + if config == nil { + errorResp := &response.ErrorResponse{ + Code: response.ErrInvalidRequest.Code, + ErrorDescription: "No signin configuration found", + } + response.RespondWithError(c, response.StatusNotFound, errorResp) + return + } + + // Find the provider + var provider *Provider + if config.ThirdParty != nil && config.ThirdParty.Providers != nil { + for _, p := range config.ThirdParty.Providers { + if p.ID == providerID { + provider = p + break + } + } + } + + if provider == nil { + errorResp := &response.ErrorResponse{ + Code: response.ErrInvalidRequest.Code, + ErrorDescription: fmt.Sprintf("OAuth provider '%s' not found", providerID), + } + response.RespondWithError(c, response.StatusNotFound, errorResp) + return + } + + // Validate required provider configuration + if provider.ClientID == "" || provider.Endpoints == nil || provider.Endpoints.Authorization == "" { + errorResp := &response.ErrorResponse{ + Code: response.ErrInvalidRequest.Code, + ErrorDescription: "Provider configuration is incomplete", + } + response.RespondWithError(c, response.StatusInternalServerError, errorResp) + return + } + + // Generate state if not provided + if state == "" { + var err error + state, err = generateRandomState() + if err != nil { + errorResp := &response.ErrorResponse{ + Code: response.ErrInvalidRequest.Code, + ErrorDescription: "Failed to generate OAuth state", + } + response.RespondWithError(c, response.StatusInternalServerError, errorResp) + return + } + } + + // Set default redirect URI if not provided + if redirectURI == "" { + redirectURI = fmt.Sprintf("%s://%s/auth/callback", getScheme(c), c.Request.Host) + } + + // Build authorization URL + params := url.Values{} + params.Add("client_id", provider.ClientID) + params.Add("response_type", "code") + params.Add("redirect_uri", redirectURI) + params.Add("state", state) + + // Add scopes + if len(provider.Scopes) > 0 { + params.Add("scope", strings.Join(provider.Scopes, " ")) + } else { + params.Add("scope", "openid profile email") + } + + authorizationURL := fmt.Sprintf("%s?%s", provider.Endpoints.Authorization, params.Encode()) + + // Return the authorization URL and state + response.RespondWithSuccess(c, response.StatusOK, &OAuthAuthorizationURLResponse{ + AuthorizationURL: authorizationURL, + State: state, + }) +} + +// generateRandomState generates a cryptographically secure random state parameter +func generateRandomState() (string, error) { + bytes := make([]byte, 16) + _, err := rand.Read(bytes) + if err != nil { + return "", err + } + return hex.EncodeToString(bytes), nil +} + +// getScheme returns the request scheme (http or https) +func getScheme(c *gin.Context) string { + if c.Request.TLS != nil || c.GetHeader("X-Forwarded-Proto") == "https" { + return "https" + } + return "http" +} diff --git a/openapi/signin/signin.go b/openapi/signin/signin.go index 6d77dc3e..2e413c4e 100644 --- a/openapi/signin/signin.go +++ b/openapi/signin/signin.go @@ -2,6 +2,7 @@ package signin import ( "fmt" + "log" "os" "path/filepath" "regexp" @@ -88,6 +89,7 @@ type Provider struct { Title string `json:"title,omitempty"` Logo string `json:"logo,omitempty"` Color string `json:"color,omitempty"` + TextColor string `json:"text_color,omitempty"` ClientID string `json:"client_id,omitempty"` ClientSecret string `json:"client_secret,omitempty"` ClientSecretGenerator *SecretGenerator `json:"client_secret_generator,omitempty"` @@ -217,10 +219,19 @@ func extractLanguageFromFilename(filename string) string { // processENVVariables processes environment variables in the configuration func processENVVariables(config *Config, rootPath string) { + var missingEnvVars []string + // Process form captcha options if config.Form != nil && config.Form.Captcha != nil && config.Form.Captcha.Options != nil { for key, value := range config.Form.Captcha.Options { if strValue, ok := value.(string); ok { + // Check if ENV variable exists before replacement + if strings.HasPrefix(strValue, "$ENV.") { + envVar := strings.TrimPrefix(strValue, "$ENV.") + if _, exists := os.LookupEnv(envVar); !exists { + missingEnvVars = append(missingEnvVars, envVar) + } + } config.Form.Captcha.Options[key] = replaceENVVar(strValue) } } @@ -229,11 +240,33 @@ func processENVVariables(config *Config, rootPath string) { // Process third party providers if config.ThirdParty != nil && config.ThirdParty.Providers != nil { for _, provider := range config.ThirdParty.Providers { + // Check ClientID + if strings.HasPrefix(provider.ClientID, "$ENV.") { + envVar := strings.TrimPrefix(provider.ClientID, "$ENV.") + if _, exists := os.LookupEnv(envVar); !exists { + missingEnvVars = append(missingEnvVars, envVar) + } + } provider.ClientID = replaceENVVar(provider.ClientID) + + // Check ClientSecret + if strings.HasPrefix(provider.ClientSecret, "$ENV.") { + envVar := strings.TrimPrefix(provider.ClientSecret, "$ENV.") + if _, exists := os.LookupEnv(envVar); !exists { + missingEnvVars = append(missingEnvVars, envVar) + } + } provider.ClientSecret = replaceENVVar(provider.ClientSecret) // Process client secret generator if provider.ClientSecretGenerator != nil { + // Check PrivateKey + if strings.HasPrefix(provider.ClientSecretGenerator.PrivateKey, "$ENV.") { + envVar := strings.TrimPrefix(provider.ClientSecretGenerator.PrivateKey, "$ENV.") + if _, exists := os.LookupEnv(envVar); !exists { + missingEnvVars = append(missingEnvVars, envVar) + } + } provider.ClientSecretGenerator.PrivateKey = replaceENVVar(provider.ClientSecretGenerator.PrivateKey) // Convert relative path to absolute path for private key @@ -245,6 +278,12 @@ func processENVVariables(config *Config, rootPath string) { if provider.ClientSecretGenerator.Header != nil { for key, value := range provider.ClientSecretGenerator.Header { if strValue, ok := value.(string); ok { + if strings.HasPrefix(strValue, "$ENV.") { + envVar := strings.TrimPrefix(strValue, "$ENV.") + if _, exists := os.LookupEnv(envVar); !exists { + missingEnvVars = append(missingEnvVars, envVar) + } + } provider.ClientSecretGenerator.Header[key] = replaceENVVar(strValue) } } @@ -254,6 +293,12 @@ func processENVVariables(config *Config, rootPath string) { if provider.ClientSecretGenerator.Payload != nil { for key, value := range provider.ClientSecretGenerator.Payload { if strValue, ok := value.(string); ok { + if strings.HasPrefix(strValue, "$ENV.") { + envVar := strings.TrimPrefix(strValue, "$ENV.") + if _, exists := os.LookupEnv(envVar); !exists { + missingEnvVars = append(missingEnvVars, envVar) + } + } provider.ClientSecretGenerator.Payload[key] = replaceENVVar(strValue) } } @@ -261,15 +306,25 @@ func processENVVariables(config *Config, rootPath string) { } } } + + // Log warning for missing environment variables + if len(missingEnvVars) > 0 { + log.Printf("Warning: The following environment variables are not set and may cause configuration issues: %v", missingEnvVars) + log.Printf("Please set these environment variables to avoid exposing placeholder values in configuration") + } } // replaceENVVar replaces environment variables in the format $ENV.VAR_NAME func replaceENVVar(value string) string { if strings.HasPrefix(value, "$ENV.") { envVar := strings.TrimPrefix(value, "$ENV.") - if envValue := os.Getenv(envVar); envValue != "" { + envValue, exists := os.LookupEnv(envVar) + if exists { return envValue } + // If environment variable doesn't exist, return empty string for security + // Never expose ENV placeholder values to prevent configuration leakage + return "" } return value } @@ -278,15 +333,40 @@ func replaceENVVar(value string) string { func createPublicConfig(fullConfig *Config) Config { publicConfig := *fullConfig + // Remove sensitive data from captcha configuration + if publicConfig.Form != nil && publicConfig.Form.Captcha != nil && publicConfig.Form.Captcha.Options != nil { + // Create a new options map without sensitive fields + publicOptions := make(map[string]interface{}) + for key, value := range publicConfig.Form.Captcha.Options { + // Only include non-sensitive fields + switch key { + case "sitekey", "theme", "size", "action", "cdata": + // These are safe to expose to frontend + publicOptions[key] = value + case "secret": + // Remove secret field - this should never be exposed to frontend + continue + default: + // For unknown fields, be conservative and exclude them + continue + } + } + publicConfig.Form.Captcha.Options = publicOptions + } + // Remove sensitive data from third party providers if publicConfig.ThirdParty != nil && publicConfig.ThirdParty.Providers != nil { publicProviders := make([]*Provider, len(publicConfig.ThirdParty.Providers)) for i, provider := range publicConfig.ThirdParty.Providers { - publicProvider := *provider - - // Remove sensitive fields - publicProvider.ClientSecret = "" - publicProvider.ClientSecretGenerator = nil + publicProvider := Provider{ + ID: provider.ID, + Title: provider.Title, + Logo: provider.Logo, + Color: provider.Color, + TextColor: provider.TextColor, + // Only expose display fields for frontend + // Remove sensitive fields: ClientID, ClientSecret, ClientSecretGenerator, Scopes, Endpoints, Mapping + } publicProviders[i] = &publicProvider } diff --git a/openapi/tests/signin_test.go b/openapi/tests/signin_test.go index 395b3183..c2cefc14 100644 --- a/openapi/tests/signin_test.go +++ b/openapi/tests/signin_test.go @@ -58,13 +58,39 @@ func TestSigninGetConfigs(t *testing.T) { t.Logf("Full config for '%s': %+v", lang, fullConfig.Title) assert.NotNil(t, publicConfig, "Public config should exist if full config exists") - // Test that public config removes sensitive data + // Test that public config removes sensitive data from OAuth providers if fullConfig.ThirdParty != nil && fullConfig.ThirdParty.Providers != nil { for i := range fullConfig.ThirdParty.Providers { if publicConfig.ThirdParty != nil && i < len(publicConfig.ThirdParty.Providers) { publicProvider := publicConfig.ThirdParty.Providers[i] + + // Check that sensitive OAuth fields are removed + assert.Empty(t, publicProvider.ClientID, "Client ID should be empty in public config") assert.Empty(t, publicProvider.ClientSecret, "Client secret should be empty in public config") assert.Nil(t, publicProvider.ClientSecretGenerator, "Client secret generator should be nil in public config") + assert.Empty(t, publicProvider.Scopes, "Scopes should be empty in public config") + assert.Nil(t, publicProvider.Endpoints, "Endpoints should be nil in public config") + assert.Empty(t, publicProvider.Mapping, "Mapping should be empty in public config") + + // Check that display fields are preserved + assert.NotEmpty(t, publicProvider.ID, "Provider ID should be preserved in public config") + assert.NotEmpty(t, publicProvider.Title, "Provider title should be preserved in public config") + // Logo, Color, TextColor might be empty depending on config, so we don't assert NotEmpty + } + } + } + + // Test that public config removes sensitive data from captcha configuration + if fullConfig.Form != nil && fullConfig.Form.Captcha != nil && fullConfig.Form.Captcha.Options != nil { + if publicConfig.Form != nil && publicConfig.Form.Captcha != nil && publicConfig.Form.Captcha.Options != nil { + // Check that secret field is removed + _, hasSecret := publicConfig.Form.Captcha.Options["secret"] + assert.False(t, hasSecret, "Captcha secret should be removed from public config") + + // Check that safe fields are preserved (if they exist in full config) + if _, hasSitekey := fullConfig.Form.Captcha.Options["sitekey"]; hasSitekey { + _, publicHasSitekey := publicConfig.Form.Captcha.Options["sitekey"] + assert.True(t, publicHasSitekey, "Captcha sitekey should be preserved in public config") } } } @@ -190,12 +216,207 @@ func TestSigninAPI(t *testing.T) { // Verify it's public config (no sensitive data) if config.ThirdParty != nil && config.ThirdParty.Providers != nil { for _, provider := range config.ThirdParty.Providers { + // Check that sensitive OAuth fields are removed from API response + assert.Empty(t, provider.ClientID, "Client ID should be empty in API response") assert.Empty(t, provider.ClientSecret, "Client secret should be empty in API response") assert.Nil(t, provider.ClientSecretGenerator, "Client secret generator should be nil in API response") + assert.Empty(t, provider.Scopes, "Scopes should be empty in API response") + assert.Nil(t, provider.Endpoints, "Endpoints should be nil in API response") + assert.Empty(t, provider.Mapping, "Mapping should be empty in API response") + + // Check that display fields are preserved in API response + assert.NotEmpty(t, provider.ID, "Provider ID should be preserved in API response") + assert.NotEmpty(t, provider.Title, "Provider title should be preserved in API response") } } + + // Verify captcha sensitive data is removed from API response + if config.Form != nil && config.Form.Captcha != nil && config.Form.Captcha.Options != nil { + _, hasSecret := config.Form.Captcha.Options["secret"] + assert.False(t, hasSecret, "Captcha secret should be removed from API response") + } } } }) } } + +func TestSigninOAuthAuthorizationURL(t *testing.T) { + // Initialize test environment + serverURL := testutils.Prepare(t) + defer testutils.Clean() + + // Get base URL from server config + baseURL := "" + if openapi.Server != nil && openapi.Server.Config != nil { + baseURL = openapi.Server.Config.BaseURL + } + + // Test OAuth authorization URL endpoints + // Note: These should return 500 because OAuth client credentials (CLIENT_ID, etc.) + // are not set in the test environment, making the provider configuration incomplete. + // This is the expected secure behavior. + testCases := []struct { + name string + provider string + query string + expectCode int + expectErrorMsg string + }{ + {"get google oauth url", "google", "", 500, "Provider configuration is incomplete"}, + {"get microsoft oauth url", "microsoft", "", 500, "Provider configuration is incomplete"}, + {"get apple oauth url", "apple", "", 500, "Provider configuration is incomplete"}, + {"get github oauth url", "github", "", 500, "Provider configuration is incomplete"}, + {"get oauth url with locale", "google", "?locale=en", 500, "Provider configuration is incomplete"}, + {"get oauth url with redirect_uri", "google", "?redirect_uri=https://example.com/callback", 500, "Provider configuration is incomplete"}, + {"get oauth url with state", "google", "?state=test-state-123", 500, "Provider configuration is incomplete"}, + {"get oauth url for nonexistent provider", "nonexistent", "", 404, "OAuth provider 'nonexistent' not found"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + url := serverURL + baseURL + "/signin/oauth/" + tc.provider + "/authorize" + tc.query + resp, err := http.Get(url) + assert.NoError(t, err, "HTTP request should succeed") + + if resp != nil { + defer resp.Body.Close() + assert.Equal(t, tc.expectCode, resp.StatusCode, "Expected status code %d", tc.expectCode) + + // Parse response body + body, err := io.ReadAll(resp.Body) + assert.NoError(t, err, "Should read response body") + + t.Logf("Response for %s: status=%d, body=%s", tc.provider, resp.StatusCode, string(body)) + + // Parse error response to verify the error message + var errorResponse map[string]interface{} + err = json.Unmarshal(body, &errorResponse) + assert.NoError(t, err, "Should parse JSON error response") + + // Verify error message matches expected + if errorDescription, hasError := errorResponse["error_description"]; hasError { + errorDescStr, ok := errorDescription.(string) + assert.True(t, ok, "error_description should be string") + assert.Equal(t, tc.expectErrorMsg, errorDescStr, "Error message should match expected") + } else { + t.Errorf("Response should contain error_description field") + } + + // Verify error code is present + if errorCode, hasErrorCode := errorResponse["error"]; hasErrorCode { + assert.Equal(t, "invalid_request", errorCode, "Error code should be invalid_request") + } else { + t.Errorf("Response should contain error field") + } + } + }) + } +} + +func TestSigninENVVariableReplacement(t *testing.T) { + // Initialize test environment + serverURL := testutils.Prepare(t) + defer testutils.Clean() + + _ = serverURL // Server URL not needed for this test + + // Load signin configurations to trigger ENV variable processing + err := signin.Load(config.Conf) + assert.NoError(t, err, "signin.Load should succeed") + + // Get full config to check ENV variable replacement + fullConfig := signin.GetFullConfig("") + assert.NotNil(t, fullConfig, "Should have a signin configuration") + + // Test captcha ENV variable replacement + if fullConfig.Form != nil && fullConfig.Form.Captcha != nil && fullConfig.Form.Captcha.Options != nil { + if sitekey, hasSitekey := fullConfig.Form.Captcha.Options["sitekey"]; hasSitekey { + sitekeyStr, ok := sitekey.(string) + assert.True(t, ok, "Sitekey should be string") + // Should not contain ENV placeholder (either replaced or empty) + assert.NotContains(t, sitekeyStr, "$ENV.", "Sitekey should not contain ENV placeholder") + t.Logf("Captcha sitekey after ENV replacement: %s", sitekeyStr) + } + + if secret, hasSecret := fullConfig.Form.Captcha.Options["secret"]; hasSecret { + secretStr, ok := secret.(string) + assert.True(t, ok, "Secret should be string") + // Should not contain ENV placeholder (either replaced or empty) + assert.NotContains(t, secretStr, "$ENV.", "Secret should not contain ENV placeholder") + t.Logf("Captcha secret after ENV replacement: %s", secretStr) + } + } + + // Test OAuth provider ENV variable replacement + if fullConfig.ThirdParty != nil && fullConfig.ThirdParty.Providers != nil { + for _, provider := range fullConfig.ThirdParty.Providers { + // Check ClientID replacement + if provider.ClientID != "" { + assert.NotContains(t, provider.ClientID, "$ENV.", "ClientID should not contain ENV placeholder") + t.Logf("Provider %s ClientID after ENV replacement: %s", provider.ID, provider.ClientID) + } + + // Check ClientSecret replacement + if provider.ClientSecret != "" { + assert.NotContains(t, provider.ClientSecret, "$ENV.", "ClientSecret should not contain ENV placeholder") + t.Logf("Provider %s ClientSecret after ENV replacement: [REDACTED]", provider.ID) + } + } + } + + // Test that public config doesn't expose ENV variables or actual sensitive values + publicConfig := signin.GetPublicConfig("") + assert.NotNil(t, publicConfig, "Should have a public signin configuration") + + // Public config should not contain sensitive data even if ENV variables are set + if publicConfig.Form != nil && publicConfig.Form.Captcha != nil && publicConfig.Form.Captcha.Options != nil { + _, hasSecret := publicConfig.Form.Captcha.Options["secret"] + assert.False(t, hasSecret, "Public config should not contain captcha secret") + } + + if publicConfig.ThirdParty != nil && publicConfig.ThirdParty.Providers != nil { + for _, provider := range publicConfig.ThirdParty.Providers { + assert.Empty(t, provider.ClientID, "Public config should not contain ClientID") + assert.Empty(t, provider.ClientSecret, "Public config should not contain ClientSecret") + } + } +} + +func TestSigninENVVariableMissingHandling(t *testing.T) { + // This test verifies that missing ENV variables are handled securely + // by returning empty strings instead of exposing the placeholder + + // Initialize test environment + serverURL := testutils.Prepare(t) + defer testutils.Clean() + + _ = serverURL // Server URL not needed for this test + + // Load signin configurations + err := signin.Load(config.Conf) + assert.NoError(t, err, "signin.Load should succeed") + + // Get public config (this is what the API returns) + publicConfig := signin.GetPublicConfig("") + assert.NotNil(t, publicConfig, "Should have a public signin configuration") + + // Verify that even if ENV variables are missing, no placeholders are exposed + if publicConfig.Form != nil && publicConfig.Form.Captcha != nil && publicConfig.Form.Captcha.Options != nil { + for key, value := range publicConfig.Form.Captcha.Options { + if valueStr, ok := value.(string); ok { + assert.NotContains(t, valueStr, "$ENV.", "Public config should not contain ENV placeholders in %s", key) + } + } + } + + if publicConfig.ThirdParty != nil && publicConfig.ThirdParty.Providers != nil { + for _, provider := range publicConfig.ThirdParty.Providers { + // These should be empty in public config anyway, but verify no ENV placeholders + assert.NotContains(t, provider.ClientID, "$ENV.", "Public config ClientID should not contain ENV placeholders") + assert.NotContains(t, provider.ClientSecret, "$ENV.", "Public config ClientSecret should not contain ENV placeholders") + } + } + + t.Log("ENV variable security test passed: no ENV placeholders exposed in public configuration") +}