fix(auth): address LDAP security and stability issues from code review

- Prevent spring-boot-starter-data-ldap AutoConfiguration side effects
  by setting spring.ldap.urls="" to avoid connection attempts when disabled
- Add conditional LdapAutoConfiguration that only creates LdapTemplate
  when skillhub.ldap.enabled=true
- Fix resource leaks in LdapAuthService (DirContext, NamingEnumeration)
- Add safeLogHost() to prevent credential exposure in logs
- Add connection timeouts (5s connect, 10s read) to prevent hangs
- Add LDAP injection prevention via isValidUsername() validation

Signed-off-by: jangrui <admin@jangrui.com>

Author: jangrui

server/skillhub-app/src/main/resources/application.yml

@@ -15,6 +15,10 @@ spring:
     basename: messages
   application:
     name: skillhub
+  ldap:
+    # Prevent spring-boot-starter-data-ldap AutoConfiguration from attempting
+    # to initialize LDAP connections when LDAP is not configured
+    urls: ""
   lifecycle:
     timeout-per-shutdown-phase: 30s
   jpa:
@@ -105,6 +109,8 @@ skillhub:
       email-from-address: ${SKILLHUB_AUTH_PASSWORD_RESET_FROM_ADDRESS:noreply@skillhub.local}
       email-from-name: ${SKILLHUB_AUTH_PASSWORD_RESET_FROM_NAME:SkillHub}
   ldap:
+    # LDAP authentication configuration
+    # Set enabled to true and configure url/base/username/password to enable LDAP authentication
     enabled: ${SKILLHUB_LDAP_ENABLED:false}
     url: ${SKILLHUB_LDAP_URL:}
     base: ${SKILLHUB_LDAP_BASE:}

server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/LdapAutoConfiguration.java

@@ -0,0 +1,40 @@
+package com.iflytek.skillhub.auth.config;
+
+import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.ldap.core.LdapTemplate;
+import org.springframework.ldap.core.support.LdapContextSource;
+
+/**
+ * LDAP auto-configuration that creates LdapTemplate only when LDAP is enabled.
+ * This avoids the startup side effects of spring-boot-starter-data-ldap's
+ * auto-configuration when LDAP is disabled.
+ */
+@Configuration
+@ConditionalOnProperty(name = "skillhub.ldap.enabled", havingValue = "true")
+public class LdapAutoConfiguration {
+
+    /**
+     * Creates an LdapContextSource configured from skillhub.ldap properties.
+     */
+    @Bean
+    public LdapContextSource ldapContextSource(LdapProperties ldapProperties) {
+        LdapContextSource contextSource = new LdapContextSource();
+        contextSource.setUrl(ldapProperties.getUrl());
+        contextSource.setBase(ldapProperties.getBase());
+        if (ldapProperties.getUsername() != null && !ldapProperties.getUsername().isEmpty()) {
+            contextSource.setUserDn(ldapProperties.getUsername());
+            contextSource.setPassword(ldapProperties.getPassword());
+        }
+        return contextSource;
+    }
+
+    /**
+     * Creates an LdapTemplate for LDAP operations.
+     */
+    @Bean
+    public LdapTemplate ldapTemplate(LdapContextSource ldapContextSource) {
+        return new LdapTemplate(ldapContextSource);
+    }
+}

server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/ldap/LdapAuthService.java

@@ -74,8 +74,8 @@ public PlatformPrincipal login(String username, String password) {
             throw new AuthFlowException(HttpStatus.SERVICE_UNAVAILABLE, "error.auth.ldap.disabled");
         }
 
-        log.debug("LDAP URL: {}, Base: {}, SearchBase: {}, SearchAttr: {}", 
-            ldapProperties.getUrl(), 
+        log.debug("LDAP host: {}, base: {}, searchBase: {}, searchAttr: {}",
+            safeLogHost(ldapProperties.getUrl()),
             ldapProperties.getBase(),
             ldapProperties.getUserSearchBase(),
             ldapProperties.getUserSearchAttribute());
@@ -138,6 +138,12 @@ private void ensureUserCanLogin(UserAccount user) {
      * Finds the DN (Distinguished Name) of a user in LDAP.
      */
     private String findUserDn(String username) {
+        // LDAP injection prevention: validate username before search
+        if (!isValidUsername(username)) {
+            log.warn("Invalid username format for LDAP search: {}", username);
+            return null;
+        }
+
         DirContext ctx = null;
         javax.naming.NamingEnumeration<SearchResult> results = null;
         try {
@@ -181,12 +187,16 @@ private DirContext createLdapContext() throws NamingException {
         env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");
         env.put(Context.PROVIDER_URL, ldapProperties.getUrl());
         env.put(Context.SECURITY_AUTHENTICATION, "simple");
-        
+
+        // Connection timeout: 5 seconds for connect, 10 seconds for read
+        env.put("com.sun.jndi.ldap.connect.timeout", "5000");
+        env.put("com.sun.jndi.ldap.read.timeout", "10000");
+
         if (ldapProperties.getUsername() != null && !ldapProperties.getUsername().isEmpty()) {
             env.put(Context.SECURITY_PRINCIPAL, ldapProperties.getUsername());
             env.put(Context.SECURITY_CREDENTIALS, ldapProperties.getPassword());
         }
-        
+
         return new InitialDirContext(env);
     }
 
@@ -203,48 +213,79 @@ private void closeContext(DirContext ctx) {
         }
     }
 
+    /**
+     * Safely extracts host from LDAP URL for logging, avoiding credential exposure.
+     * Handles formats like: ldap://host:389, ldap://user:pass@host:389, ldaps://host
+     */
+    private String safeLogHost(String url) {
+        if (url == null || url.isEmpty()) {
+            return "";
+        }
+        try {
+            // Remove protocol prefix
+            String withoutProtocol = url.replaceFirst("^ldaps?://", "");
+            // Extract host:port or just host
+            int atIndex = withoutProtocol.indexOf('@');
+            if (atIndex > 0) {
+                withoutProtocol = withoutProtocol.substring(atIndex + 1);
+            }
+            int colonIndex = withoutProtocol.indexOf(':');
+            return colonIndex > 0 ? withoutProtocol.substring(0, colonIndex) : withoutProtocol;
+        } catch (Exception e) {
+            return "[url-parse-error]";
+        }
+    }
+
     /**
      * Authenticates a user against the LDAP server using their DN and password.
      */
     private boolean authenticateLdap(String userDn, String password) {
+        DirContext ctx = null;
         try {
             Hashtable<String, String> env = new Hashtable<>();
             env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");
             env.put(Context.PROVIDER_URL, ldapProperties.getUrl());
             env.put(Context.SECURITY_AUTHENTICATION, "simple");
             env.put(Context.SECURITY_PRINCIPAL, userDn);
             env.put(Context.SECURITY_CREDENTIALS, password);
+            env.put("com.sun.jndi.ldap.connect.timeout", "5000");
+            env.put("com.sun.jndi.ldap.read.timeout", "10000");
 
-            DirContext ctx = new InitialDirContext(env);
-            ctx.close();
+            ctx = new InitialDirContext(env);
             return true;
         } catch (NamingException e) {
             return false;
+        } finally {
+            closeContext(ctx);
         }
     }
 
     /**
      * Retrieves user attributes from LDAP.
      */
     private Attributes getUserAttributes(String userDn) {
+        DirContext ctx = null;
         try {
             Hashtable<String, String> env = new Hashtable<>();
             env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");
             env.put(Context.PROVIDER_URL, ldapProperties.getUrl());
             env.put(Context.SECURITY_AUTHENTICATION, "simple");
-            
+            env.put("com.sun.jndi.ldap.connect.timeout", "5000");
+            env.put("com.sun.jndi.ldap.read.timeout", "10000");
+
             // Use bind DN if configured, otherwise anonymous bind
             if (ldapProperties.getUsername() != null && !ldapProperties.getUsername().isEmpty()) {
                 env.put(Context.SECURITY_PRINCIPAL, ldapProperties.getUsername());
                 env.put(Context.SECURITY_CREDENTIALS, ldapProperties.getPassword());
             }
-            
-            DirContext ctx = new InitialDirContext(env);
+
+            ctx = new InitialDirContext(env);
             Attributes attrs = ctx.getAttributes(new LdapName(userDn));
-            ctx.close();
             return attrs;
         } catch (Exception e) {
             return null;
+        } finally {
+            closeContext(ctx);
         }
     }
 
@@ -272,8 +313,11 @@ private UserAccount findOrCreateLdapUser(String username, Attributes attributes)
 
         // If not found, create a new user
         if (user == null) {
-            // Use a unique identifier based on username if email is missing
-            // This prevents creating duplicate accounts for users without email
+            // For LDAP users without email, use "ldap:{username}@internal" as a unique identifier.
+            // This format:
+            // 1. Prevents duplicate accounts when email attribute is missing
+            // 2. Clearly identifies the account origin (LDAP vs local)
+            // 3. Follows email format to satisfy the email NOT NULL constraint
             String normalizedEmail = email != null ? email.toLowerCase() : "ldap:" + username + "@internal";
 
             user = new UserAccount(
@@ -322,4 +366,16 @@ private PlatformPrincipal buildPrincipal(UserAccount user) {
             roles
         );
     }
+
+    /**
+     * Validates username to prevent LDAP injection attacks.
+     * Allows only alphanumeric characters and underscores, 3-64 characters.
+     */
+    private boolean isValidUsername(String username) {
+        if (username == null || username.isEmpty()) {
+            return false;
+        }
+        // Allow alphanumeric, underscore, hyphen, dot, and @ for UPN formats
+        return username.matches("^[A-Za-z0-9_@.\\-]{3,64}$");
+    }
 }

server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/local/LocalAuthService.java

@@ -134,9 +134,9 @@ public PlatformPrincipal login(String username, String password) {
             // Fallback to LDAP authentication if enabled
             if (ldapProperties.isEnabled()) {
                 log.info("Local user not found, attempting LDAP authentication for username: {}", username);
-                log.debug("LDAP enabled: {}, URL: {}, Base: {}", 
-                    ldapProperties.isEnabled(), 
-                    ldapProperties.getUrl(), 
+                log.debug("LDAP enabled: {}, host: {}, base: {}",
+                    ldapProperties.isEnabled(),
+                    safeLogHost(ldapProperties.getUrl()),
                     ldapProperties.getBase());
                 try {
                     PlatformPrincipal ldapPrincipal = ldapAuthService.login(username, password);
@@ -271,4 +271,24 @@ private void validateEmail(String email) {
             throw new AuthFlowException(HttpStatus.BAD_REQUEST, "validation.auth.local.email.invalid");
         }
     }
+
+    /**
+     * Safely extracts host from LDAP URL for logging, avoiding credential exposure.
+     */
+    private static String safeLogHost(String url) {
+        if (url == null || url.isEmpty()) {
+            return "";
+        }
+        try {
+            String withoutProtocol = url.replaceFirst("^ldaps?://", "");
+            int atIndex = withoutProtocol.indexOf('@');
+            if (atIndex > 0) {
+                withoutProtocol = withoutProtocol.substring(atIndex + 1);
+            }
+            int colonIndex = withoutProtocol.indexOf(':');
+            return colonIndex > 0 ? withoutProtocol.substring(0, colonIndex) : withoutProtocol;
+        } catch (Exception e) {
+            return "[url-parse-error]";
+        }
+    }
 }