Merge pull request #4023 from jooby-project/4022

fix: pac4j: Add defense-in-depth escaping and startup warning to DevLoginForm

Author: jknack

modules/jooby-pac4j/src/main/java/io/jooby/internal/pac4j/DevLoginForm.java

@@ -11,6 +11,7 @@
 import io.jooby.Context;
 import io.jooby.MediaType;
 import io.jooby.Route;
+import io.jooby.XSS;
 import io.jooby.pac4j.Pac4jContext;
 
 public class DevLoginForm implements Route.Handler {
@@ -68,8 +69,8 @@ public DevLoginForm(Config pac4j, String callbackPath) {
 
   @Override
   public Object apply(Context ctx) throws Exception {
-    String error = ctx.query("error").value("");
-    String username = ctx.query("username").value("");
+    String error = XSS.html(ctx.query("error").value(""));
+    String username = XSS.html(ctx.query("username").value(""));
 
     ctx.setAttribute("username", username);
     ctx.setAttribute("error", error);

modules/jooby-pac4j/src/main/java/io/jooby/pac4j/Pac4jModule.java

@@ -89,7 +89,7 @@ public void add(String authorizer, Object client) {
 
   private final Pac4jOptions options;
 
-  private Map<String, ProtectedPath> clientMap;
+  private @Nullable Map<String, ProtectedPath> clientMap;
 
   /** Creates a new pac4j module. */
   public Pac4jModule() {
@@ -353,9 +353,8 @@ public void install(Jooby app) throws Exception {
     Map<String, List<ClientReference>> allClients = new LinkedHashMap<>();
 
     /* Should add simple login form? */
-    var devLogin = false;
-    if (clientMap.isEmpty()) {
-      devLogin = true;
+    var devLogin = app.getEnvironment().isActive("dev", "test");
+    if (clientMap.isEmpty() && devLogin) {
       client(
           conf ->
               new FormClient(
@@ -441,6 +440,15 @@ public void install(Jooby app) throws Exception {
         ofNullable(options.getProfileManagerFactory()).orElse(ProfileManagerFactory.DEFAULT));
 
     if (devLogin) {
+      var devLoginFormWarning =
+          !conf.hasPath("pac4j.devLoginFormWarning")
+              || conf.getBoolean("pac4j.devLoginFormWarning");
+      if (devLoginFormWarning) {
+        app.getLog()
+            .warn(
+                "Pac4j module is using the fallback DevLoginForm. This component is for development"
+                    + " purposes only and should NOT be used in production.");
+      }
       app.get("/login", new DevLoginForm(options, contextPath + options.getCallbackPath()));
     }
 

modules/jooby-pac4j/src/test/java/io/jooby/internal/pac4j/DevLoginFormTest.java

@@ -5,12 +5,16 @@
  */
 package io.jooby.internal.pac4j;
 
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.*;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.mockito.ArgumentCaptor;
 import org.pac4j.core.client.Clients;
 import org.pac4j.core.config.Config;
+import org.pac4j.core.context.WebContext;
 import org.pac4j.core.http.url.UrlResolver;
 
 import io.jooby.Context;
@@ -98,4 +102,52 @@ void testApplyWithMissingQueryData() throws Exception {
                         && html.contains("action=\"/callback?client_name=FormClient\"")
                         && html.contains("value=\"\"")));
   }
+
+  @Test
+  public void shouldEscapeXssPayloads() throws Exception {
+    Config config = mock(Config.class);
+    Clients clients = mock(Clients.class);
+    UrlResolver urlResolver = mock(UrlResolver.class);
+
+    when(config.getClients()).thenReturn(clients);
+    when(clients.getUrlResolver()).thenReturn(urlResolver);
+    when(urlResolver.compute(anyString(), any(WebContext.class))).thenReturn("/callback");
+
+    Context ctx = mock(Context.class);
+    Value errorNode = mock(Value.class);
+    Value userNode = mock(Value.class);
+
+    String xssError = "<script>alert('error')</script>";
+    String xssUser = "\"><script>alert('user')</script>";
+
+    when(ctx.query("error")).thenReturn(errorNode);
+    when(errorNode.value("")).thenReturn(xssError);
+
+    when(ctx.query("username")).thenReturn(userNode);
+    when(userNode.value("")).thenReturn(xssUser);
+
+    // Mock response chain
+    when(ctx.setResponseType(MediaType.html)).thenReturn(ctx);
+
+    // Capture the HTML argument passed to ctx.send() and return the ctx mock
+    ArgumentCaptor<String> htmlCaptor = ArgumentCaptor.forClass(String.class);
+    when(ctx.send(htmlCaptor.capture())).thenReturn(ctx);
+
+    DevLoginForm form = new DevLoginForm(config, "/callback");
+    form.apply(ctx);
+
+    // Retrieve the captured HTML
+    String html = htmlCaptor.getValue();
+
+    // Assert inputs were sanitized
+    assertFalse(html.contains(xssError), "Error payload was rendered unescaped!");
+    assertFalse(html.contains(xssUser), "Username payload was rendered unescaped!");
+
+    // Verify HTML entity encoding occurred (checking < and >)
+    assertTrue(html.contains("&lt;script&gt;"), "Expected escaped <script> tag");
+
+    // Verify quotes were escaped in the value attribute injection
+    assertTrue(
+        html.contains("&quot;&gt;") || html.contains("&#34;&gt;"), "Expected escaped double quote");
+  }
 }

modules/jooby-pac4j/src/test/java/io/jooby/pac4j/Pac4jModuleTest.java

@@ -19,7 +19,9 @@
 import org.pac4j.core.client.Clients;
 import org.pac4j.core.config.Config;
 import org.pac4j.http.client.direct.DirectBasicAuthClient;
+import org.slf4j.Logger;
 
+import io.jooby.Environment;
 import io.jooby.Jooby;
 import io.jooby.ServiceRegistry;
 import io.jooby.StatusCode;
@@ -29,16 +31,22 @@
 public class Pac4jModuleTest {
 
   private Jooby app;
+  private Logger log;
+  private Environment env;
   private ServiceRegistry registry;
   private com.typesafe.config.Config config;
 
   @BeforeEach
   void setUp() {
     app = mock(Jooby.class);
+    log = mock(Logger.class);
+    env = mock(Environment.class);
     registry = mock(ServiceRegistry.class);
     config = mock(com.typesafe.config.Config.class);
 
     when(app.getServices()).thenReturn(registry);
+    when(app.getLog()).thenReturn(log);
+    when(app.getEnvironment()).thenReturn(env);
     when(app.getConfig()).thenReturn(config);
     when(app.getContextPath()).thenReturn("/");
   }
@@ -76,8 +84,38 @@ void testClientDSLVariants() {
   }
 
   @Test
-  void testInstallDefaultLogin() throws Exception {
+  void testInstallDevLoginWithWarning() throws Exception {
     Pac4jModule module = new Pac4jModule();
+    when(env.isActive("dev", "test")).thenReturn(true);
+
+    module.install(app);
+
+    verify(app).get(eq("/login"), any());
+    verify(app).get(eq("/callback"), any());
+    verify(app).post(eq("/callback"), any());
+  }
+
+  @Test
+  void testInstallDevLoginWithWarningOff() throws Exception {
+    Pac4jModule module = new Pac4jModule();
+    when(config.hasPath("pac4j.devLoginFormWarning")).thenReturn(true);
+    when(config.getBoolean("pac4j.devLoginFormWarning")).thenReturn(false);
+    when(env.isActive("dev", "test")).thenReturn(true);
+
+    module.install(app);
+
+    verify(app).get(eq("/login"), any());
+    verify(app).get(eq("/callback"), any());
+    verify(app).post(eq("/callback"), any());
+  }
+
+  @Test
+  void testInstallDevLoginWithWarningOn() throws Exception {
+    Pac4jModule module = new Pac4jModule();
+    when(config.hasPath("pac4j.devLoginFormWarning")).thenReturn(true);
+    when(config.getBoolean("pac4j.devLoginFormWarning")).thenReturn(true);
+    when(env.isActive("dev", "test")).thenReturn(true);
+
     module.install(app);
 
     verify(app).get(eq("/login"), any());