refactor: consolidate Slim middleware to reduce code duplication (#8166)

DawoudIO · claude · Copilot · web-flow · commit 4f52bc71ef45 · 2026-03-03T17:52:35.000-08:00
## Summary - **Extract `BrowserRequestTrait`** — removes 30-line `isBrowserRequest()` method that was copy-pasted identically into both `AuthMiddleware` and `BaseAuthRoleMiddleware` - **Add `RequestParameterValidationMiddleware`** — declarative enum/required-field validation; replaces inline `depositType` validation block in `POST /deposits` - **Add `InputSanitizationMiddleware`** — declarative `InputUtils` sanitization mapped by field name; replaces scattered `sanitizeText()` calls in group create/update and deposit create routes - **Fix `GroupMiddleware` + wire to routes** — corrected route param name (`groupId` → `groupID`); added to 8 group write routes that were previously loading the entity inline (the middleware was defined but never used) ## Net result - 127 lines removed, 315 added (315 includes 3 new reusable middleware classes + Cypress tests) - Route handlers are now lean: no inline entity loading or sanitization boilerplate - Consistent 400/412 error responses enforced at the middleware layer ## Test plan - [x] Added `Middleware Validation Tests` block to `finance-deposits.spec.js`: - Missing `depositType` → 400 with `allowed` list - Invalid `depositType` → 400 with error message containing the bad value - XSS in `depositComment` → `Comment` field in response contains no `<script>` - [x] Added `Middleware Validation Tests` block to `private.people.groups.spec.js`: - Non-existent `groupID` on update/delete/addperson → 412 - XSS in `groupName` on create and update → `Name` field stripped of `<script>` / `onerror` - XSS in `groupRoleName` → sanitized before save - [ ] Full Cypress suite (Cypress 15.11.0 has a macOS Sequoia path bug locally; CI will validate) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com>
diff --git a/cypress/e2e/api/private/finance/finance-deposits.spec.js b/cypress/e2e/api/private/finance/finance-deposits.spec.js
@@ -222,6 +222,53 @@ describe("API Private Deposit Operations", () => {
         });
     });
 
+    describe("Middleware Validation Tests", () => {
+        it("Rejects missing depositType with 400", () => {
+            const today = new Date().toISOString().split("T")[0];
+            cy.makePrivateAdminAPICall(
+                "POST",
+                `/api/deposits`,
+                { depositComment: "No type given", depositDate: today },
+                400
+            ).then((resp) => {
+                expect(resp.body).to.have.property("error");
+                expect(resp.body).to.have.property("allowed");
+                expect(resp.body.allowed).to.include("Bank");
+            });
+        });
+
+        it("Rejects invalid depositType with 400", () => {
+            const today = new Date().toISOString().split("T")[0];
+            cy.makePrivateAdminAPICall(
+                "POST",
+                `/api/deposits`,
+                { depositType: "Cash", depositDate: today },
+                400
+            ).then((resp) => {
+                expect(resp.body).to.have.property("error");
+                expect(resp.body.error).to.include("Cash");
+                expect(resp.body).to.have.property("allowed");
+            });
+        });
+
+        it("Sanitizes XSS in depositComment before saving", () => {
+            const today = new Date().toISOString().split("T")[0];
+            cy.makePrivateAdminAPICall(
+                "POST",
+                `/api/deposits`,
+                {
+                    depositType: "Bank",
+                    depositComment: "<script>alert('xss')</script>Test",
+                    depositDate: today,
+                },
+                200
+            ).then((resp) => {
+                expect(resp.body).to.have.property("Comment");
+                expect(resp.body.Comment).to.not.include("<script>");
+            });
+        });
+    });
+
     describe("Data Structure Validation", () => {
         it("Verify deposit data structure", () => {
             // Ensure deposit operations return proper structure
diff --git a/cypress/e2e/api/private/standard/private.people.groups.spec.js b/cypress/e2e/api/private/standard/private.people.groups.spec.js
@@ -202,6 +202,79 @@ describe("API Private Group Operations", () => {
         });
     });
 
+    describe("Middleware Validation Tests", () => {
+        it("Returns 412 when updating a non-existent group", () => {
+            cy.makePrivateAdminAPICall(
+                "POST",
+                `/api/groups/999999`,
+                { groupName: "Ghost Group", groupType: 0, description: "" },
+                412
+            );
+        });
+
+        it("Returns 412 when deleting a non-existent group", () => {
+            cy.makePrivateAdminAPICall(
+                "DELETE",
+                `/api/groups/999999`,
+                null,
+                412
+            );
+        });
+
+        it("Returns 412 when adding a person to a non-existent group", () => {
+            cy.makePrivateAdminAPICall(
+                "POST",
+                `/api/groups/999999/addperson/1`,
+                { RoleID: 1 },
+                412
+            );
+        });
+
+        it("Sanitizes XSS in groupName when creating a group", () => {
+            cy.makePrivateAdminAPICall(
+                "POST",
+                `/api/groups/`,
+                {
+                    groupName: "<script>alert('xss')</script>TestGroup",
+                    description: "safe description",
+                },
+                200
+            ).then((resp) => {
+                expect(resp.body).to.have.property("Name");
+                expect(resp.body.Name).to.not.include("<script>");
+            });
+        });
+
+        it("Sanitizes XSS in groupName when updating a group", () => {
+            cy.makePrivateAdminAPICall(
+                "POST",
+                `/api/groups/${groupID}`,
+                {
+                    groupName: "<img src=x onerror=alert(1)>CleanName",
+                    groupType: 0,
+                    description: "",
+                },
+                200
+            ).then((resp) => {
+                expect(resp.body).to.have.property("Name");
+                expect(resp.body.Name).to.not.include("onerror");
+            });
+        });
+
+        it("Sanitizes XSS in role name when updating group role", () => {
+            cy.makePrivateAdminAPICall(
+                "POST",
+                `/api/groups/${groupID}/roles/1`,
+                { groupRoleName: "<b>Bold</b>RoleName" },
+                200
+            ).then((resp) => {
+                expect(resp.body).to.have.property("OptionName");
+                expect(resp.body.OptionName).to.not.include("<b>");
+                expect(resp.body.OptionName).to.include("RoleName");
+            });
+        });
+    });
+
     describe("Authorization Tests - Non-Admin Users", () => {
         it("Non-admin should be denied adding group members", () => {
             // Test that a user without bManageGroups permission is denied
diff --git a/src/ChurchCRM/Slim/Middleware/Api/GroupMiddleware.php b/src/ChurchCRM/Slim/Middleware/Api/GroupMiddleware.php
@@ -16,14 +16,14 @@ class GroupMiddleware implements MiddlewareInterface
     public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
     {
         $response = new Response();
-        $groupId = SlimUtils::getRouteArgument($request, 'groupId');
+        $groupId = SlimUtils::getRouteArgument($request, 'groupID');
         if (empty(trim($groupId))) {
-            return $response->withStatus(412, gettext('Missing') . ' GroupId');
+            return SlimUtils::renderErrorJSON($response, gettext('Missing') . ' GroupId', [], 412);
         }
 
         $group = GroupQuery::create()->findPk($groupId);
         if (empty($group)) {
-            return $response->withStatus(412, 'GroupId: ' . $groupId . ' ' . gettext('not found'));
+            return SlimUtils::renderErrorJSON($response, 'GroupId: ' . $groupId . ' ' . gettext('not found'), [], 412);
         }
 
         $request = $request->withAttribute('group', $group);
diff --git a/src/ChurchCRM/Slim/Middleware/AuthMiddleware.php b/src/ChurchCRM/Slim/Middleware/AuthMiddleware.php
@@ -14,6 +14,8 @@
 
 class AuthMiddleware implements MiddlewareInterface
 {
+    use BrowserRequestTrait;
+
     public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
     {
         // Construct the full public API path including any subdirectory installation
@@ -86,40 +88,6 @@ private function isPath(ServerRequestInterface $request, string $pathPart): bool
         return in_array($pathPart, $pathAry, true);
     }
 
-    /**
-     * Check if request is from a browser (expects HTML) vs API client (expects JSON)
-     */
-    private function isBrowserRequest(ServerRequestInterface $request): bool
-    {
-        $path = $request->getUri()->getPath();
-
-        // API routes should always return JSON
-        if (str_contains($path, '/api/')) {
-            return false;
-        }
-
-        // Check Accept header - browsers typically send text/html
-        $acceptHeader = $request->getHeaderLine('Accept');
-        if (!empty($acceptHeader)) {
-            // If client explicitly wants JSON, it's an API request
-            if (str_contains($acceptHeader, 'application/json') && !str_contains($acceptHeader, 'text/html')) {
-                return false;
-            }
-            // If client accepts HTML, treat as browser
-            if (str_contains($acceptHeader, 'text/html')) {
-                return true;
-            }
-        }
-
-        // Check X-Requested-With header (AJAX requests)
-        if ($request->getHeaderLine('X-Requested-With') === 'XMLHttpRequest') {
-            return false;
-        }
-
-        // Default to browser for non-API routes
-        return true;
-    }
-
     /**
      * Redirect to the login page
      */
diff --git a/src/ChurchCRM/Slim/Middleware/BrowserRequestTrait.php b/src/ChurchCRM/Slim/Middleware/BrowserRequestTrait.php
@@ -0,0 +1,42 @@
+<?php
+
+namespace ChurchCRM\Slim\Middleware;
+
+use Psr\Http\Message\ServerRequestInterface;
+
+trait BrowserRequestTrait
+{
+    /**
+     * Check if request is from a browser (expects HTML) vs API client (expects JSON).
+     */
+    protected function isBrowserRequest(ServerRequestInterface $request): bool
+    {
+        $path = $request->getUri()->getPath();
+
+        // API routes should always return JSON
+        if (str_contains($path, '/api/')) {
+            return false;
+        }
+
+        // Check Accept header - browsers typically send text/html
+        $acceptHeader = $request->getHeaderLine('Accept');
+        if (!empty($acceptHeader)) {
+            // If client explicitly wants JSON, it's an API request
+            if (str_contains($acceptHeader, 'application/json') && !str_contains($acceptHeader, 'text/html')) {
+                return false;
+            }
+            // If client accepts HTML, treat as browser
+            if (str_contains($acceptHeader, 'text/html')) {
+                return true;
+            }
+        }
+
+        // Check X-Requested-With header (AJAX requests)
+        if ($request->getHeaderLine('X-Requested-With') === 'XMLHttpRequest') {
+            return false;
+        }
+
+        // Default to browser for non-API routes
+        return true;
+    }
+}
diff --git a/src/ChurchCRM/Slim/Middleware/InputSanitizationMiddleware.php b/src/ChurchCRM/Slim/Middleware/InputSanitizationMiddleware.php
@@ -0,0 +1,52 @@
+<?php
+
+namespace ChurchCRM\Slim\Middleware;
+
+use ChurchCRM\Utils\InputUtils;
+use Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Server\MiddlewareInterface;
+use Psr\Http\Server\RequestHandlerInterface;
+
+/**
+ * Sanitizes request body fields before passing to the route handler.
+ *
+ * Fields are sanitized in-place; only fields that are present in the body
+ * are affected. Missing fields are left absent (not set to empty string).
+ *
+ * Supported sanitization types:
+ *  - 'text' → InputUtils::sanitizeText() (trims and strips HTML tags)
+ *  - 'html' → InputUtils::sanitizeHTML() (allows safe HTML, strips scripts)
+ *
+ * Usage:
+ *   ->add(new InputSanitizationMiddleware([
+ *       'title'   => 'text',
+ *       'content' => 'html',
+ *   ]))
+ */
+class InputSanitizationMiddleware implements MiddlewareInterface
+{
+    /**
+     * @param array<string, 'text'|'html'> $fieldMap Map of field name → sanitization type.
+     */
+    public function __construct(private readonly array $fieldMap) {}
+
+    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
+    {
+        $body = $request->getParsedBody();
+
+        if (is_array($body)) {
+            foreach ($this->fieldMap as $field => $type) {
+                if (isset($body[$field])) {
+                    $body[$field] = match ($type) {
+                        'html'  => InputUtils::sanitizeHTML($body[$field]),
+                        default => InputUtils::sanitizeText($body[$field]),
+                    };
+                }
+            }
+            $request = $request->withParsedBody($body);
+        }
+
+        return $handler->handle($request);
+    }
+}
diff --git a/src/ChurchCRM/Slim/Middleware/Request/Auth/BaseAuthRoleMiddleware.php b/src/ChurchCRM/Slim/Middleware/Request/Auth/BaseAuthRoleMiddleware.php
@@ -5,6 +5,7 @@
 use ChurchCRM\Authentication\AuthenticationManager;
 use ChurchCRM\dto\SystemURLs;
 use ChurchCRM\model\ChurchCRM\User;
+use ChurchCRM\Slim\Middleware\BrowserRequestTrait;
 use ChurchCRM\Utils\LoggerUtils;
 use Laminas\Diactoros\Response;
 use Psr\Http\Message\ServerRequestInterface as Request;
@@ -16,6 +17,8 @@
 
 abstract class BaseAuthRoleMiddleware implements MiddlewareInterface
 {
+    use BrowserRequestTrait;
+
     protected User $user;
 
     public function process(Request $request, RequestHandlerInterface $handler): ResponseInterface
@@ -64,40 +67,6 @@ public function process(Request $request, RequestHandlerInterface $handler): Res
         return $handler->handle($request);
     }
 
-    /**
-     * Check if request is from a browser (expects HTML) vs API client (expects JSON)
-     */
-    protected function isBrowserRequest(Request $request): bool
-    {
-        $path = $request->getUri()->getPath();
-        
-        // API routes should always return JSON
-        if (str_contains($path, '/api/')) {
-            return false;
-        }
-        
-        // Check Accept header - browsers typically send text/html
-        $acceptHeader = $request->getHeaderLine('Accept');
-        if (!empty($acceptHeader)) {
-            // If client explicitly wants JSON, it's an API request
-            if (str_contains($acceptHeader, 'application/json') && !str_contains($acceptHeader, 'text/html')) {
-                return false;
-            }
-            // If client accepts HTML, treat as browser
-            if (str_contains($acceptHeader, 'text/html')) {
-                return true;
-            }
-        }
-        
-        // Check X-Requested-With header (AJAX requests)
-        if ($request->getHeaderLine('X-Requested-With') === 'XMLHttpRequest') {
-            return false;
-        }
-        
-        // Default to browser for non-API routes
-        return true;
-    }
-
     /**
      * Redirect to the access-denied page
      */
diff --git a/src/ChurchCRM/Slim/Middleware/RequestParameterValidationMiddleware.php b/src/ChurchCRM/Slim/Middleware/RequestParameterValidationMiddleware.php
diff --git a/src/api/routes/finance/finance-deposits.php b/src/api/routes/finance/finance-deposits.php
diff --git a/src/api/routes/people/people-groups.php b/src/api/routes/people/people-groups.php

Original file line number	Diff line number	Diff line change
`@@ -16,14 +16,14 @@ class GroupMiddleware implements MiddlewareInterface`
`16`	`16`	`public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface`
`17`	`17`	`{`
`18`	`18`	`$response = new Response();`
`19`		`- $groupId = SlimUtils::getRouteArgument($request, 'groupId');`
	`19`	`+ $groupId = SlimUtils::getRouteArgument($request, 'groupID');`
`20`	`20`	`if (empty(trim($groupId))) {`
`21`		`- return $response->withStatus(412, gettext('Missing') . ' GroupId');`
	`21`	`+ return SlimUtils::renderErrorJSON($response, gettext('Missing') . ' GroupId', [], 412);`
`22`	`22`	`}`
`23`	`23`
`24`	`24`	`$group = GroupQuery::create()->findPk($groupId);`
`25`	`25`	`if (empty($group)) {`
`26`		`- return $response->withStatus(412, 'GroupId: ' . $groupId . ' ' . gettext('not found'));`
	`26`	`+ return SlimUtils::renderErrorJSON($response, 'GroupId: ' . $groupId . ' ' . gettext('not found'), [], 412);`
`27`	`27`	`}`
`28`	`28`
`29`	`29`	`$request = $request->withAttribute('group', $group);`