Merge pull request #2 from nemozak1/claude/fix-xss-client-registration-nXuBU

Address PR review feedback: centralize htmlEncode, escape form inputs…

Author: nemozak1

src/main/scala/com/tesobe/oidc/endpoints/AuthEndpoint.scala

@@ -21,6 +21,7 @@ package com.tesobe.oidc.endpoints
 
 import cats.effect.IO
 import com.tesobe.oidc.auth.{AuthService, CodeService}
+import com.tesobe.oidc.endpoints.HtmlUtils.htmlEncode
 import com.tesobe.oidc.models.{OidcError, User}
 import com.tesobe.oidc.ratelimit.RateLimitService
 import com.tesobe.oidc.config.OidcConfig
@@ -40,10 +41,6 @@ class AuthEndpoint(
 
   private val logger = LoggerFactory.getLogger(getClass)
 
-  private def htmlEncode(s: String): String =
-    s.replace("&", "&amp;").replace("<", "&lt;")
-     .replace(">", "&gt;").replace("\"", "&quot;").replace("'", "&#x27;")
-
   // Test logging immediately when class is created
   logger.info("AuthEndpoint created - logging is working!")
   println("AuthEndpoint created - logging is working!")
@@ -328,7 +325,7 @@ class AuthEndpoint(
       s"Error handling login submission: ${error.getMessage}",
       error
     )
-    BadRequest(s"Invalid form data: ${error.getMessage}")
+    BadRequest("Invalid form data. Please try again.")
   }
 
   private def generateCodeForUser(
@@ -363,15 +360,15 @@ class AuthEndpoint(
         clientOpt <- authService.findClientByClientIdThatIsKey(clientId)
 
         stateParam = state
-          .map(s => s"""<input type="hidden" name="state" value="$s">""")
+          .map(s => s"""<input type="hidden" name="state" value="${htmlEncode(s)}">""")
           .getOrElse("")
         nonceParam = nonce
-          .map(n => s"""<input type="hidden" name="nonce" value="$n">""")
+          .map(n => s"""<input type="hidden" name="nonce" value="${htmlEncode(n)}">""")
           .getOrElse("")
 
         providerOptions = providers
           .map { provider =>
-            s"""<option value="$provider">$provider</option>"""
+            s"""<option value="${htmlEncode(provider)}">${htmlEncode(provider)}</option>"""
           }
           .mkString("\n            ")
 
@@ -473,7 +470,7 @@ class AuthEndpoint(
             </div>"""
           } else if (providers.length == 1) {
             // Single provider in production: use hidden field
-            s"""<input type="hidden" name="provider" value="${providers.head}">"""
+            s"""<input type="hidden" name="provider" value="${htmlEncode(providers.head)}">"""
           } else {
             // No providers - shouldn't happen but handle gracefully
             s"""<div class="form-group">
@@ -484,9 +481,9 @@ class AuthEndpoint(
             </div>"""
           }}
 
-            <input type="hidden" name="client_id" value="$clientId">
-            <input type="hidden" name="redirect_uri" value="$redirectUri">
-            <input type="hidden" name="scope" value="$scope">
+            <input type="hidden" name="client_id" value="${htmlEncode(clientId)}">
+            <input type="hidden" name="redirect_uri" value="${htmlEncode(redirectUri)}">
+            <input type="hidden" name="scope" value="${htmlEncode(scope)}">
             $stateParam
             $nonceParam
 

src/main/scala/com/tesobe/oidc/endpoints/ClientsEndpoint.scala

@@ -21,16 +21,13 @@ package com.tesobe.oidc.endpoints
 
 import cats.effect.IO
 import com.tesobe.oidc.auth.HybridAuthService
+import com.tesobe.oidc.endpoints.HtmlUtils.htmlEncode
 import com.tesobe.oidc.models.OidcClient
 import org.http4s._
 import org.http4s.dsl.io._
 
 class ClientsEndpoint(authService: HybridAuthService) {
 
-  private def htmlEncode(s: String): String =
-    s.replace("&", "&amp;").replace("<", "&lt;")
-     .replace(">", "&gt;").replace("\"", "&quot;").replace("'", "&#x27;")
-
   val routes: HttpRoutes[IO] = HttpRoutes.of[IO] {
     case GET -> Root / "clients" =>
       handleClientsListRequest()

src/main/scala/com/tesobe/oidc/endpoints/HtmlUtils.scala

@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2025 TESOBE
+ *
+ * This file is part of OBP-OIDC.
+ *
+ * OBP-OIDC is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * OBP-OIDC is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with OBP-OIDC. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package com.tesobe.oidc.endpoints
+
+object HtmlUtils {
+
+  /** Encode a string for safe interpolation into HTML content and attributes.
+    *
+    * Replaces the five characters that have special meaning in HTML/XML
+    * (&, <, >, ", ') with their corresponding character entity references.
+    */
+  def htmlEncode(s: String): String =
+    s.replace("&", "&amp;")
+     .replace("<", "&lt;")
+     .replace(">", "&gt;")
+     .replace("\"", "&quot;")
+     .replace("'", "&#x27;")
+}

src/main/scala/com/tesobe/oidc/endpoints/RegistrationEndpoint.scala

@@ -97,7 +97,7 @@ class RegistrationEndpoint(
           BadRequest(
             ClientRegistrationError(
               ClientRegistrationError.INVALID_CLIENT_METADATA,
-              Some(s"Invalid JSON request body: ${error.getMessage}")
+              Some("Invalid JSON request body. Please check the request format.")
             ).asJson
           ).map(addNoCacheHeaders)
       }

src/main/scala/com/tesobe/oidc/endpoints/StaticFilesEndpoint.scala

@@ -23,10 +23,13 @@ import cats.effect.IO
 import org.http4s._
 import org.http4s.dsl.io._
 import org.http4s.headers.`Content-Type`
+import org.slf4j.LoggerFactory
 import scala.io.Source
 
 class StaticFilesEndpoint {
 
+  private val logger = LoggerFactory.getLogger(getClass)
+
   val routes: HttpRoutes[IO] = HttpRoutes.of[IO] {
     case GET -> Root / "static" / "css" / fileName if fileName.endsWith(".css") =>
       serveStaticFile(s"static/css/$fileName", MediaType.text.css)
@@ -50,7 +53,8 @@ class StaticFilesEndpoint {
       case None =>
         NotFound(s"Static file not found: $resourcePath")
     }.handleErrorWith { error =>
-      InternalServerError(s"Error serving static file: ${error.getMessage}")
+      logger.error(s"Error serving static file $resourcePath: ${error.getMessage}", error)
+      InternalServerError("Error serving static file.")
     }
   }
 }

src/main/scala/com/tesobe/oidc/server/OidcServer.scala

@@ -42,9 +42,7 @@ import scala.concurrent.duration._
 
 object OidcServer extends IOApp {
 
-  private def htmlEncode(s: String): String =
-    s.replace("&", "&amp;").replace("<", "&lt;")
-     .replace(">", "&gt;").replace("\"", "&quot;").replace("'", "&#x27;")
+  import HtmlUtils.htmlEncode
 
   private def retryWithBackoff(
       action: IO[Either[String, String]],
@@ -544,7 +542,7 @@ object OidcServer extends IOApp {
                                 }
                                 .distinct
                                 .map(url =>
-                                  s"""<a href="${htmlEncode(url)}" target="_blank">${htmlEncode(url)}</a>"""
+                                  s"""<a href="${htmlEncode(url)}" target="_blank" rel="noopener noreferrer">${htmlEncode(url)}</a>"""
                                 )
                                 .mkString(", ")
                             s"""<div class="app">${htmlEncode(client.client_name)}: $redirectUrlsList</div>"""

src/test/scala/com/tesobe/oidc/endpoints/HtmlUtilsTest.scala

@@ -0,0 +1,85 @@
+/*
+ * Copyright (c) 2025 TESOBE
+ *
+ * This file is part of OBP-OIDC.
+ *
+ * OBP-OIDC is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * OBP-OIDC is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with OBP-OIDC. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package com.tesobe.oidc.endpoints
+
+import org.scalatest.funsuite.AnyFunSuite
+import org.scalatest.matchers.should.Matchers
+
+class HtmlUtilsTest extends AnyFunSuite with Matchers {
+
+  // Individual character escaping
+  test("htmlEncode should escape ampersand to &amp;") {
+    HtmlUtils.htmlEncode("&") shouldBe "&amp;"
+  }
+
+  test("htmlEncode should escape less-than to &lt;") {
+    HtmlUtils.htmlEncode("<") shouldBe "&lt;"
+  }
+
+  test("htmlEncode should escape greater-than to &gt;") {
+    HtmlUtils.htmlEncode(">") shouldBe "&gt;"
+  }
+
+  test("htmlEncode should escape double-quote to &quot;") {
+    HtmlUtils.htmlEncode("\"") shouldBe "&quot;"
+  }
+
+  test("htmlEncode should escape single-quote to &#x27;") {
+    HtmlUtils.htmlEncode("'") shouldBe "&#x27;"
+  }
+
+  // Edge cases
+  test("htmlEncode should return empty string unchanged") {
+    HtmlUtils.htmlEncode("") shouldBe ""
+  }
+
+  test("htmlEncode should return plain text unchanged") {
+    HtmlUtils.htmlEncode("hello world") shouldBe "hello world"
+  }
+
+  // Mixed strings
+  test("htmlEncode should escape all five special characters in one string") {
+    HtmlUtils.htmlEncode("&<>\"'") shouldBe "&amp;&lt;&gt;&quot;&#x27;"
+  }
+
+  test("htmlEncode should escape special characters embedded in plain text") {
+    HtmlUtils.htmlEncode("hello <world> & \"everyone\"") shouldBe
+      "hello &lt;world&gt; &amp; &quot;everyone&quot;"
+  }
+
+  test("htmlEncode should escape an HTML tag to prevent injection") {
+    HtmlUtils.htmlEncode("<script>alert('xss')</script>") shouldBe
+      "&lt;script&gt;alert(&#x27;xss&#x27;)&lt;/script&gt;"
+  }
+
+  test("htmlEncode should escape HTML attribute injection attempt") {
+    HtmlUtils.htmlEncode("\" onmouseover=\"alert(1)") shouldBe
+      "&quot; onmouseover=&quot;alert(1)"
+  }
+
+  // Ampersand-first ordering: ensures existing text is not double-escaped
+  test("htmlEncode should not double-escape an already-escaped entity") {
+    HtmlUtils.htmlEncode("&amp;") shouldBe "&amp;amp;"
+  }
+
+  test("htmlEncode should escape multiple ampersands") {
+    HtmlUtils.htmlEncode("a && b") shouldBe "a &amp;&amp; b"
+  }
+}