Merge pull request #11 from nemozak1/main

simonredfern · web-flow · commit 9bc078c7f8b7 · 2026-03-11T15:12:15.000+01:00
Claude Security Review fixes, add claude workflow
diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml
@@ -0,0 +1,50 @@
+name: Claude Code
+
+on:
+  issue_comment:
+    types: [created]
+  pull_request_review_comment:
+    types: [created]
+  issues:
+    types: [opened, assigned]
+  pull_request_review:
+    types: [submitted]
+
+jobs:
+  claude:
+    if: |
+      (github.event_name == 'issue_comment' && contains(github.event.comment.body, '@claude')) ||
+      (github.event_name == 'pull_request_review_comment' && contains(github.event.comment.body, '@claude')) ||
+      (github.event_name == 'pull_request_review' && contains(github.event.review.body, '@claude')) ||
+      (github.event_name == 'issues' && (contains(github.event.issue.body, '@claude') || contains(github.event.issue.title, '@claude')))
+    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+      pull-requests: read
+      issues: read
+      id-token: write
+      actions: read # Required for Claude to read CI results on PRs
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v4
+        with:
+          fetch-depth: 1
+
+      - name: Run Claude Code
+        id: claude
+        uses: anthropics/claude-code-action@v1
+        with:
+          claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}
+
+          # This is an optional setting that allows Claude to read CI results on PRs
+          additional_permissions: |
+            actions: read
+
+          # Optional: Give a custom prompt to Claude. If this is not specified, Claude will perform the instructions specified in the comment that tagged it.
+          # prompt: 'Update the pull request description to include a summary of changes.'
+
+          # Optional: Add claude_args to customize behavior and configuration
+          # See https://github.com/anthropics/claude-code-action/blob/main/docs/usage.md
+          # or https://code.claude.com/docs/en/cli-reference for available options
+          # claude_args: '--allowed-tools Bash(gh pr:*)'
+
diff --git a/src/main/scala/com/tesobe/oidc/auth/HybridAuthService.scala b/src/main/scala/com/tesobe/oidc/auth/HybridAuthService.scala
@@ -1007,9 +1007,7 @@ class HybridAuthService(
               Left(
                 OidcError(
                   "server_error",
-                  Some(
-                    s"Database error: ${error.getMessage}. Client may already exist or database constraint violation."
-                  )
+                  Some("Failed to create client. Please try again later.")
                 )
               )
             )
diff --git a/src/main/scala/com/tesobe/oidc/endpoints/AuthEndpoint.scala b/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
@@ -324,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(
@@ -359,31 +360,31 @@ 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            ")
 
         clientName = clientOpt.map(_.client_name).getOrElse("Unknown Client")
         consumerId = clientOpt.map(_.consumer_id).getOrElse("Unknown Consumer")
 
         // Format client name for production display: replace dashes with spaces and convert to proper case
-        formattedClientName = clientName
+        formattedClientName = htmlEncode(clientName
           .replace("-", " ")
           .split(" ")
           .map(word =>
             if (word.isEmpty) ""
             else word.charAt(0).toUpper + word.substring(1).toLowerCase
           )
           .mkString(" ")
-          .replace("Obp ", "OBP ")
+          .replace("Obp ", "OBP "))
 
         errorHtml = errorMessage
           .map(msg => s"""<div class="error">$msg</div>""")
@@ -434,10 +435,10 @@ class AuthEndpoint(
           $errorHtml
           ${if (config.localDevelopmentMode) {
             s"""<div class="info">
-            <strong>Consumer ID:</strong> $consumerId<br>
-            <strong>Client Name:</strong> $clientName<br>
-            <strong>Client ID:</strong> $clientId<br>
-            <strong>Requested Scopes:</strong> $scope
+            <strong>Consumer ID:</strong> ${htmlEncode(consumerId)}<br>
+            <strong>Client Name:</strong> ${htmlEncode(clientName)}<br>
+            <strong>Client ID:</strong> ${htmlEncode(clientId)}<br>
+            <strong>Requested Scopes:</strong> ${htmlEncode(scope)}
           </div>"""
           } else {
             ""
@@ -469,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">
@@ -480,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
 
@@ -617,7 +618,7 @@ class AuthEndpoint(
       redirectUri: String,
       error: OidcError
   ): IO[Response[IO]] = {
-    val stateParam = error.state.map(s => s"&state=$s").getOrElse("")
+    val stateParam = error.state.map(s => s"&state=${java.net.URLEncoder.encode(s, "UTF-8")}").getOrElse("")
     val descriptionParam = error.error_description
       .map(d => s"&error_description=${java.net.URLEncoder.encode(d, "UTF-8")}")
       .getOrElse("")
diff --git a/src/main/scala/com/tesobe/oidc/endpoints/ClientsEndpoint.scala b/src/main/scala/com/tesobe/oidc/endpoints/ClientsEndpoint.scala
@@ -21,6 +21,7 @@ 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._
@@ -205,30 +206,30 @@ class ClientsEndpoint(authService: HybridAuthService) {
       "<em>None</em>"
     } else {
       "<ul>" + client.redirect_uris
-        .map(uri => s"<li>$uri</li>")
+        .map(uri => s"<li>${htmlEncode(uri)}</li>")
         .mkString("") + "</ul>"
     }
 
     val scopesList = client.scopes
-      .map(scope => s"""<span class="scope">$scope</span>""")
+      .map(scope => s"""<span class="scope">${htmlEncode(scope)}</span>""")
       .mkString(" ")
 
     val clientSecretDisplay = client.client_secret match {
-      case Some(secret) => s"""<span class="client-secret">$secret</span>"""
+      case Some(secret) => s"""<span class="client-secret">${htmlEncode(secret)}</span>"""
       case None         => "<em>Not set</em>"
     }
 
     s"""<tr>
-       |    <td><span class="consumer-id">${client.consumer_id}</span></td>
-       |    <td><span class="client-name">${client.client_name}</span></td>
-       |    <td><span class="client-id">${client.client_id}</span></td>
+       |    <td><span class="consumer-id">${htmlEncode(client.consumer_id)}</span></td>
+       |    <td><span class="client-name">${htmlEncode(client.client_name)}</span></td>
+       |    <td><span class="client-id">${htmlEncode(client.client_id)}</span></td>
        |    <td>$clientSecretDisplay</td>
        |    <td class="redirect-uris">$redirectUrisList</td>
        |    <td class="scopes">$scopesList</td>
-       |    <td>${client.token_endpoint_auth_method}</td>
-       |    <td class="created-at">${client.created_at.getOrElse(
+       |    <td>${htmlEncode(client.token_endpoint_auth_method)}</td>
+       |    <td class="created-at">${htmlEncode(client.created_at.getOrElse(
         "Unknown"
-      )}</td>
+      ))}</td>
        |</tr>""".stripMargin
   }
 }
diff --git a/src/main/scala/com/tesobe/oidc/endpoints/HtmlUtils.scala b/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;")
+}
diff --git a/src/main/scala/com/tesobe/oidc/endpoints/RegistrationEndpoint.scala b/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)
       }
@@ -175,7 +175,7 @@ class RegistrationEndpoint(
             InternalServerError(
               ClientRegistrationError(
                 "server_error",
-                Some(s"Failed to register client: ${error.error_description.getOrElse("Unknown error")}")
+                Some("Registration failed. Please try again later.")
               ).asJson
             ).map(addNoCacheHeaders)
         }
diff --git a/src/main/scala/com/tesobe/oidc/endpoints/StaticFilesEndpoint.scala b/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.")
     }
   }
 }
diff --git a/src/main/scala/com/tesobe/oidc/server/OidcServer.scala b/src/main/scala/com/tesobe/oidc/server/OidcServer.scala
@@ -42,6 +42,8 @@ import scala.concurrent.duration._
 
 object OidcServer extends IOApp {
 
+  import HtmlUtils.htmlEncode
+
   private def retryWithBackoff(
       action: IO[Either[String, String]],
       description: String,
@@ -540,10 +542,10 @@ object OidcServer extends IOApp {
                                 }
                                 .distinct
                                 .map(url =>
-                                  s"""<a href="$url" target="_blank">$url</a>"""
+                                  s"""<a href="${htmlEncode(url)}" target="_blank" rel="noopener noreferrer">${htmlEncode(url)}</a>"""
                                 )
                                 .mkString(", ")
-                            s"""<div class="app">${client.client_name}: $redirectUrlsList</div>"""
+                            s"""<div class="app">${htmlEncode(client.client_name)}: $redirectUrlsList</div>"""
                           }
                           .mkString("")
                         s"""
diff --git a/src/test/scala/com/tesobe/oidc/endpoints/HtmlUtilsTest.scala b/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"
+  }
+}

Original file line number	Diff line number	Diff line change
`@@ -1007,9 +1007,7 @@ class HybridAuthService(`
`1007`	`1007`	`Left(`
`1008`	`1008`	`OidcError(`
`1009`	`1009`	`"server_error",`
`1010`		`- Some(`
`1011`		`- s"Database error: ${error.getMessage}. Client may already exist or database constraint violation."`
`1012`		`- )`
	`1010`	`+ Some("Failed to create client. Please try again later.")`
`1013`	`1011`	`)`
`1014`	`1012`	`)`
`1015`	`1013`	`)`
Original file line number	Diff line number	Diff line change
`@@ -42,6 +42,8 @@ import scala.concurrent.duration._`
`42`	`42`
`43`	`43`	`object OidcServer extends IOApp {`
`44`	`44`
	`45`	`+ import HtmlUtils.htmlEncode`
	`46`	`+`
`45`	`47`	`private def retryWithBackoff(`
`46`	`48`	`action: IO[Either[String, String]],`
`47`	`49`	`description: String,`
`@@ -540,10 +542,10 @@ object OidcServer extends IOApp {`
`540`	`542`	`}`
`541`	`543`	`.distinct`
`542`	`544`	`.map(url =>`
`543`		`- s"""<a href="$url" target="_blank">$url</a>"""`
	`545`	`+ s"""<a href="${htmlEncode(url)}" target="_blank" rel="noopener noreferrer">${htmlEncode(url)}</a>"""`
`544`	`546`	`)`
`545`	`547`	`.mkString(", ")`
`546`		`- s"""<div class="app">${client.client_name}: $redirectUrlsList</div>"""`
	`548`	`+ s"""<div class="app">${htmlEncode(client.client_name)}: $redirectUrlsList</div>"""`
`547`	`549`	`}`
`548`	`550`	`.mkString("")`
`549`	`551`	`s"""`