Skip to content

Conversation

@JohnRDOrazio
Copy link
Member

@JohnRDOrazio JohnRDOrazio commented Dec 3, 2025

Summary

Additional updates to support Phase 2.5 cookie-only authentication (follow-up to #424):

  • CORS origin validation - Validate origins before reflecting them in error responses (security improvement)
  • CORS config centralization - Move CORS allowed origins to environment variable, remove file-based config
  • OpenAPI documentation updates - Add CookieAuth security scheme and update auth endpoint documentation
  • RegionalDataHandler CORS - Enable allowCredentials for cookie-based authentication on protected routes

Changes

src/Http/Middleware/ErrorHandlingMiddleware.php

  • Add allowedOrigins array and optional originValidator callable via constructor
  • Add isAllowedOrigin() method to validate origins before reflecting
  • Only reflect validated origins with Access-Control-Allow-Credentials for auth endpoints
  • Reject unknown origins by omitting CORS headers (browser blocks request)

src/Router.php

  • Parse CORS_ALLOWED_ORIGINS once at route start
  • Use setAllowedOrigins($allowedOrigins) for handlers instead of file-based config
  • Remove duplicate parsing in middleware setup

src/Handlers/AbstractHandler.php

  • Remove setAllowedOriginsFromFile() method (no longer needed)

public/index.php

  • Add CORS_ALLOWED_ORIGINS validation to DotEnv loader

.env.example

  • Add CORS_ALLOWED_ORIGINS environment variable documentation

CLAUDE.md

  • Document CORS_ALLOWED_ORIGINS configuration

jsondata/schemas/openapi.json

  • Add CookieAuth security scheme for HttpOnly cookie authentication
  • Update BearerAuth description to note it's for server-to-server clients
  • Update /auth/login description to mention HttpOnly cookies are set
  • Update LoginResponse schema to document cookie behavior

src/Handlers/RegionalDataHandler.php

  • Enable allowCredentials for CORS with cookies

Security Notes

  • The originValidator callable allows future extensibility for per-application origin restrictions (e.g., when apps register their own allowed origins via API keys)
  • Origins are validated before being reflected in CORS headers, preventing unauthorized origins from receiving credentialed responses

Test plan

  • PHPStan analysis passes (level 10)
  • PHP parallel-lint passes
  • All PHPUnit tests pass
  • Manual testing of cookie-based authentication flow

Related

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features

    • Added HttpOnly cookie-based authentication for browser clients, complementing existing Bearer token support
    • Token expiration time now included in authentication responses
  • Security Improvements

    • Enhanced CORS configuration with environment-based allowed origins
    • Stricter origin validation for authenticated endpoints and cross-origin requests

✏️ Tip: You can customize this high-level summary in your review settings.

JohnRDOrazio and others added 2 commits December 2, 2025 02:38
When error responses are generated for /auth/* endpoints, return the
specific Origin header value instead of wildcard (*). This is required
for CORS when the client uses credentials: 'include'.

Also includes Access-Control-Allow-Credentials: true header for auth
endpoint error responses.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Add CookieAuth security scheme for HttpOnly cookie-based authentication
- Update BearerAuth description to note it's for server-to-server clients
- Update /auth/login description to mention HttpOnly cookies are set
- Update LoginResponse schema to document cookie behavior
- Add allowCredentials to RegionalDataHandler for CORS with cookies

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 3, 2025

Warning

Rate limit exceeded

@JohnRDOrazio has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 38 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 0f16d4a and 11129b7.

📒 Files selected for processing (3)
  • src/Handlers/AbstractHandler.php (2 hunks)
  • src/Router.php (5 hunks)
  • src/Utilities.php (1 hunks)

Walkthrough

Updates OpenAPI to document cookie-based JWT delivery alongside Bearer tokens, adds CookieAuth and expires_in in LoginResponse, enables CORS credentials in handlers, centralizes allowed-origins handling via CORS_ALLOWED_ORIGINS, and makes error middleware reflect origin-specific CORS (with credentials) for auth endpoints.

Changes

Cohort / File(s) Summary
OpenAPI authentication schema
jsondata/schemas/openapi.json
Add CookieAuth security scheme; update BearerAuth description; expand /auth/login docs and 200 response; add expires_in to components.schemas.LoginResponse; update token property descriptions to reference HttpOnly cookies litcal_access_token and litcal_refresh_token.
Error handling & CORS middleware
src/Http/Middleware/ErrorHandlingMiddleware.php
Constructor now accepts allowedOrigins and optional originValidator; adds isAllowedOrigin(); error responses reflect Origin with Access-Control-Allow-Credentials: true for auth paths when allowed, else use wildcard; ensures Content-Type: application/problem+json.
Router / wiring of allowed origins
src/Router.php
Read CORS_ALLOWED_ORIGINS into centralized allowedOrigins array; pass allowedOrigins into handlers and ErrorHandlingMiddleware; remove file-based origin reads.
Handlers — regional and content handlers
src/Handlers/RegionalDataHandler.php, src/.../MissalsHandler.php, src/.../DecreesHandler.php, src/Handlers/AbstractHandler.php
RegionalDataHandler enables CORS credentials; handlers now accept setAllowedOrigins(array) instead of file-based setters; AbstractHandler::setAllowedOriginsFromFile() removed; isAllowedOrigin() uses strict in_array.
Env, docs & entrypoint
.env.example, CLAUDE.md, public/index.php
Add CORS_ALLOWED_ORIGINS to env example and docs; public/index.php validates non-empty CORS_ALLOWED_ORIGINS if present; docs describe config and security notes.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Router
    participant Handler
    participant ErrorMiddleware as ErrorHandlingMiddleware
    participant ResponseFactory as ResponseFactoryInterface

    Client->>Router: HTTP request (may include Origin)
    Router->>Handler: dispatch request (passes allowedOrigins)
    alt Handler returns error or throws
        Handler-->>Router: error
        Router->>ErrorMiddleware: handle error (request, error)
        ErrorMiddleware->>ErrorMiddleware: check if auth path & has Origin
        alt Origin present && isAllowedOrigin
            ErrorMiddleware->>ResponseFactory: create response
            ResponseFactory-->>ErrorMiddleware: response with Access-Control-Allow-Origin: <Origin>, Access-Control-Allow-Credentials: true
        else
            ErrorMiddleware->>ResponseFactory: create response with Access-Control-Allow-Origin: *
        end
        ErrorMiddleware->>Client: return error response (with appropriate CORS headers)
    else
        Handler->>Client: normal response
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Review points:
    • ErrorHandlingMiddleware::isAllowedOrigin and originValidator edge cases.
    • Router parsing of CORS_ALLOWED_ORIGINS and propagation to handlers.
    • Ensure OpenAPI cookie names/descriptions match auth endpoint behavior.
    • Confirm all callers of removed setAllowedOriginsFromFile() updated.

Possibly related PRs

Suggested labels

enhancement

Poem

🐰 I hopped a patch to set the jar,
HttpOnly crumbs kept safe and far,
Origins checked before they hop,
Cookies served — credentials on top,
A tiny rabbity cheer for auth's new star! 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: CORS updates (origin validation, environment variable configuration) and OpenAPI updates (CookieAuth scheme, LoginResponse documentation) for cookie-based authentication in Phase 2.5.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1478e0b and ab3ff13.

📒 Files selected for processing (3)
  • jsondata/schemas/openapi.json (4 hunks)
  • src/Handlers/RegionalDataHandler.php (1 hunks)
  • src/Http/Middleware/ErrorHandlingMiddleware.php (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.php: Implement PSR-7 request/response handling for all HTTP operations
Use LoggerFactory::create() to instantiate PSR-3 compliant Monolog loggers
Implement JWT authentication via JwtService for protected endpoints (PUT, PATCH, DELETE /data/{category}/{calendar})
Always use timezone Europe/Vatican for all date and time calculations
Enforce PSR-12 coding standards via PHP_CodeSniffer across all PHP files
Enforce PHPStan level 10 static analysis for all PHP files
Use PSR-4 autoload for LiturgicalCalendar\Api namespace
Return HTTP 401 UnauthorizedException or 403 ForbiddenException from Http/Exception namespace for authentication failures
Require PHP >= 8.4 with support for modern syntax like array_find
Use intl, zip, calendar, yaml, gettext, curl, json, xml PHP extensions

Files:

  • src/Handlers/RegionalDataHandler.php
  • src/Http/Middleware/ErrorHandlingMiddleware.php
src/Handlers/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

src/Handlers/*.php: Extend AbstractHandler and implement PSR RequestHandlerInterface for all handler classes
Use Negotiator::pickLanguage() for Accept-Language header processing instead of PHP's Locale::acceptFromHttp()
Use Negotiator::negotiateResponseContentType() to respect return_type query parameter and Accept header for content negotiation
Set allowed methods, accept headers, and content types in handler constructor
Use LitLocale::isValid() to validate supported locales before processing language-specific requests
Return PSR-7 ResponseInterface from all handler handle() methods
Create handler classes extending AbstractHandler in src/Handlers/ directory with one route per handler
Use JWT authentication via Authorization Bearer token header for protected endpoints

Files:

  • src/Handlers/RegionalDataHandler.php
src/Http/Middleware/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

Implement middleware classes extending PSR-15 MiddlewareInterface in src/Http/Middleware/ directory

Files:

  • src/Http/Middleware/ErrorHandlingMiddleware.php
jsondata/schemas/*.json

📄 CodeRabbit inference engine (CLAUDE.md)

Define JSON Schema schemas in jsondata/schemas/ directory with separate files for each data type (DiocesanCalendar.json, NationalCalendar.json, etc.)

Files:

  • jsondata/schemas/openapi.json
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Handlers/*.php : Use JWT authentication via Authorization Bearer token header for protected endpoints
Learnt from: JohnRDOrazio
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 349
File: .github/workflows/phpunit.yml:84-101
Timestamp: 2025-09-06T17:19:28.704Z
Learning: JohnRDOrazio prefers to fail early within the same GitHub Actions step rather than using $GITHUB_OUTPUT to communicate between steps, when fixing issues where a workflow might pass without actually running the intended operations.
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Handlers/*.php : Use JWT authentication via Authorization Bearer token header for protected endpoints

Applied to files:

  • src/Handlers/RegionalDataHandler.php
  • src/Http/Middleware/ErrorHandlingMiddleware.php
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Handlers/*.php : Set allowed methods, accept headers, and content types in handler constructor

Applied to files:

  • src/Handlers/RegionalDataHandler.php
🔇 Additional comments (5)
src/Handlers/RegionalDataHandler.php (1)

69-70: CORS credentials flag looks correct; confirm origin behavior with credentials

Setting $this->allowCredentials = true; in the constructor is the right move to support cookie-based auth on these endpoints, and the placement after parent::__construct($requestPathParams) is appropriate so you override the default for this handler only.

Please just double‑check that the CORS helpers (handlePreflightRequest / setAccessControlAllowOriginHeader) never emit Access-Control-Allow-Origin: * when $this->allowCredentials === true, since browsers will ignore Access-Control-Allow-Credentials: true in that case. As long as they echo the request Origin (as done for /auth/*), this change should behave as intended.

jsondata/schemas/openapi.json (4)

896-896: Endpoint descriptions properly document cookie and body token delivery.

The /auth/login descriptions at lines 896 and 909 clearly communicate the dual-delivery mechanism (response body + HttpOnly cookies) and provide appropriate guidance for browser vs. server clients. The wording is slightly verbose but appropriate for OpenAPI documentation context.

Also applies to: 909-909


4308-4337: LoginResponse schema properly documents token delivery and expiry.

The schema updates comprehensively document the dual-delivery pattern, include XSS-protection guidance for browser clients, clearly map response fields to their HttpOnly cookie equivalents, and properly expose token expiry. The expires_in field is correctly marked required and documented as "Access token expiry time in seconds".


5172-5185: Security schemes properly document both authentication methods with clear guidance.

The BearerAuth scheme description (line 5177) is updated to clarify its server-to-server/non-browser focus and acknowledges the cookie-based alternative. The new CookieAuth scheme (lines 5179-5184) is correctly defined as an apiKey type in cookie with the explicit cookie name litcal_access_token, and the description helpfully includes XSS-protection rationale and the important credentials: 'include' guidance for CORS cross-origin requests.

Verify that the new CookieAuth security scheme is applied (referenced in the security arrays) to all protected endpoints that should accept cookie-based authentication. Please confirm the protected endpoints at /auth/logout, /auth/me, and the /data/* mutation endpoints reference the appropriate security schemes (both BearerAuth and/or CookieAuth where applicable).


1-5187: Overall OpenAPI specification updates align well with Phase 2.5 cookie authentication implementation.

The changes comprehensively document the cookie-based JWT authentication mechanism across endpoint descriptions, schema documentation, and security scheme definitions. Cookie names are consistently referenced, token expiry is properly exposed, and security scheme guidance is clear for both browser and server clients. The documentation should effectively guide client developers in implementing cookie-based authentication flows.

JohnRDOrazio and others added 2 commits December 3, 2025 03:31
- Update ErrorHandlingMiddleware to accept allowedOrigins array and optional
  originValidator callable via constructor
- Add isAllowedOrigin() method to validate origins before reflecting in CORS headers
- Only reflect validated origins with Access-Control-Allow-Credentials for auth endpoints
- Reject unknown origins by omitting CORS headers (browser blocks request)
- Add CORS_ALLOWED_ORIGINS environment variable (comma-separated list or '*')
- Update Router.php to parse and pass allowed origins from environment
- Document CORS configuration in CLAUDE.md

This prevents unauthorized origins from receiving credentialed responses.
The originValidator callable allows future extensibility for per-application
origin restrictions (e.g., when apps register their own allowed origins).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
…fig in env

- Remove setAllowedOriginsFromFile method from AbstractHandler
- Update Router.php to parse CORS_ALLOWED_ORIGINS once at route start
- Use setAllowedOrigins($allowedOrigins) for handlers instead of file-based config
- Add CORS_ALLOWED_ORIGINS validation to DotEnv loader in index.php
- Eliminates need for separate allowedOrigins.txt file

This simplifies CORS configuration by using a single environment variable
(CORS_ALLOWED_ORIGINS) instead of a separate text file. The value is parsed
once at the beginning of request routing and reused for both handler-level
CORS and error response CORS.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/Router.php (1)

334-364: Add setAllowedOrigins() call for RegionalDataHandler protected operations.

RegionalDataHandler handles PUT/PATCH/DELETE operations (lines 341–350) but never receives setAllowedOrigins($allowedOrigins), whereas MissalsHandler and DecreesHandler explicitly restrict origins for their protected write operations. Add the same conditional origin restriction for RegionalDataHandler's PUT/PATCH/DELETE methods:

if (
    in_array($this->request->getMethod(), [ RequestMethod::PUT->value, RequestMethod::PATCH->value, RequestMethod::DELETE->value ], true)
    && false === Router::isLocalhost()
) {
    $regionalDataHandler->setAllowedOrigins($allowedOrigins);
}

Currently, RegionalDataHandler defaults to allowedOrigins = ['*'] with allowCredentials = true, allowing credentials from any origin—inconsistent with the restricted origin policy applied to other handlers' protected endpoints.

🧹 Nitpick comments (1)
src/Http/Middleware/ErrorHandlingMiddleware.php (1)

97-115: Minor inconsistency with AbstractHandler::isAllowedOrigin() method.

This implementation differs slightly from AbstractHandler::isAllowedOrigin() (lines 340-343 in relevant snippets) in two ways:

  1. This version uses strict comparison (in_array(..., true)), while AbstractHandler uses loose comparison
  2. This version includes empty string check and wildcard handling

Consider aligning both implementations for consistency, perhaps by extracting a shared utility. However, this is not blocking since the stricter checks here are safer.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ab3ff13 and 68e130d.

📒 Files selected for processing (6)
  • .env.example (1 hunks)
  • CLAUDE.md (1 hunks)
  • public/index.php (1 hunks)
  • src/Handlers/AbstractHandler.php (0 hunks)
  • src/Http/Middleware/ErrorHandlingMiddleware.php (3 hunks)
  • src/Router.php (4 hunks)
💤 Files with no reviewable changes (1)
  • src/Handlers/AbstractHandler.php
✅ Files skipped from review due to trivial changes (1)
  • CLAUDE.md
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.php: Implement PSR-7 request/response handling for all HTTP operations
Use LoggerFactory::create() to instantiate PSR-3 compliant Monolog loggers
Implement JWT authentication via JwtService for protected endpoints (PUT, PATCH, DELETE /data/{category}/{calendar})
Always use timezone Europe/Vatican for all date and time calculations
Enforce PSR-12 coding standards via PHP_CodeSniffer across all PHP files
Enforce PHPStan level 10 static analysis for all PHP files
Use PSR-4 autoload for LiturgicalCalendar\Api namespace
Return HTTP 401 UnauthorizedException or 403 ForbiddenException from Http/Exception namespace for authentication failures
Require PHP >= 8.4 with support for modern syntax like array_find
Use intl, zip, calendar, yaml, gettext, curl, json, xml PHP extensions

Files:

  • src/Http/Middleware/ErrorHandlingMiddleware.php
  • src/Router.php
src/Http/Middleware/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

Implement middleware classes extending PSR-15 MiddlewareInterface in src/Http/Middleware/ directory

Files:

  • src/Http/Middleware/ErrorHandlingMiddleware.php
public/index.php

📄 CodeRabbit inference engine (CLAUDE.md)

public/index.php: Instantiate Router and call route() in public/index.php entry point after loading environment with Dotenv
Locate project root via composer.json in public/index.php and configure error handling and logging

Files:

  • public/index.php
src/Router.php

📄 CodeRabbit inference engine (CLAUDE.md)

src/Router.php: Add route case in Router::route() switch statement when implementing a new handler
Delegate routing to appropriate Handler classes in Router based on URL path
Run PSR-15 middleware pipeline (ErrorHandling, Logging) in Router for all requests

Files:

  • src/Router.php
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Handlers/*.php : Use JWT authentication via Authorization Bearer token header for protected endpoints
Learnt from: JohnRDOrazio
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 349
File: .github/workflows/phpunit.yml:84-101
Timestamp: 2025-09-06T17:19:28.704Z
Learning: JohnRDOrazio prefers to fail early within the same GitHub Actions step rather than using $GITHUB_OUTPUT to communicate between steps, when fixing issues where a workflow might pass without actually running the intended operations.
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Handlers/*.php : Use JWT authentication via Authorization Bearer token header for protected endpoints

Applied to files:

  • src/Http/Middleware/ErrorHandlingMiddleware.php
  • src/Router.php
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Handlers/*.php : Set allowed methods, accept headers, and content types in handler constructor

Applied to files:

  • src/Router.php
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Router.php : Add route case in Router::route() switch statement when implementing a new handler

Applied to files:

  • src/Router.php
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Router.php : Delegate routing to appropriate Handler classes in Router based on URL path

Applied to files:

  • src/Router.php
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Router.php : Run PSR-15 middleware pipeline (ErrorHandling, Logging) in Router for all requests

Applied to files:

  • src/Router.php
📚 Learning: 2025-09-16T17:02:12.414Z
Learnt from: JohnRDOrazio
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 366
File: src/Router.php:386-389
Timestamp: 2025-09-16T17:02:12.414Z
Learning: In the LiturgicalCalendarAPI Router middleware pipeline, ErrorHandlingMiddleware is intentionally placed as the outermost middleware to catch any errors from inner middleware including LoggingMiddleware. ErrorHandlingMiddleware handles error logging while LoggingMiddleware handles request/response logging, creating separation of concerns between operational and error logging.

Applied to files:

  • src/Router.php
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Http/Middleware/*.php : Implement middleware classes extending PSR-15 MiddlewareInterface in src/Http/Middleware/ directory

Applied to files:

  • src/Router.php
🧬 Code graph analysis (2)
src/Http/Middleware/ErrorHandlingMiddleware.php (2)
src/Handlers/AbstractHandler.php (2)
  • __construct (58-68)
  • isAllowedOrigin (341-344)
src/Handlers/RegionalDataHandler.php (1)
  • __construct (66-78)
src/Router.php (2)
src/Handlers/AbstractHandler.php (1)
  • setAllowedOrigins (75-80)
src/Http/Middleware/ErrorHandlingMiddleware.php (1)
  • ErrorHandlingMiddleware (14-321)
🔇 Additional comments (6)
src/Http/Middleware/ErrorHandlingMiddleware.php (1)

176-198: CORS handling for error responses looks correct.

The implementation properly addresses the security concern from the previous review:

  • Auth endpoints validate origin before reflecting it with Access-Control-Allow-Credentials: true
  • Unauthorized origins receive no CORS headers, causing browsers to block the response
  • Non-auth endpoints correctly use wildcard * for public access
public/index.php (1)

70-70: LGTM!

The validation follows the established pattern and prevents misconfiguration where CORS_ALLOWED_ORIGINS is set but empty.

.env.example (1)

56-62: Good documentation for the new CORS configuration.

The example clearly documents the format, provides a realistic example with multiple origins, and appropriately warns against using * in production with cookie-based authentication.

src/Router.php (3)

94-101: LGTM - CORS origin parsing is well-implemented.

The parsing logic correctly:

  • Defaults to '*' when the environment variable is unset
  • Handles the wildcard case by producing ['*'] array
  • Trims whitespace from each origin in a comma-separated list
  • Filters out empty strings that could result from trailing commas

202-208: Correctly applies origin restrictions to protected routes.

Origin restrictions are appropriately applied only to write operations (PUT/PATCH/DELETE) on non-localhost deployments, maintaining the existing security pattern while allowing flexible development.


398-401: Middleware pipeline correctly configured with CORS origins.

The ErrorHandlingMiddleware now receives the parsed allowed origins, ensuring error responses respect the same CORS policy as successful responses.

JohnRDOrazio and others added 2 commits December 3, 2025 03:51
…erations

Apply same origin restriction to RegionalDataHandler PUT/PATCH/DELETE
operations that MissalsHandler and DecreesHandler already have. This
ensures consistent CORS policy for all protected endpoints.

Previously, RegionalDataHandler defaulted to allowedOrigins=['*'] with
allowCredentials=true, allowing credentials from any origin.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
…igin

Align AbstractHandler::isAllowedOrigin() with ErrorHandlingMiddleware
by using strict comparison (in_array third parameter = true).

This ensures consistent and safer origin matching across both
implementations. Added docstring note explaining that wildcard handling
is done separately in setAccessControlAllowOriginHeader().

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/Handlers/AbstractHandler.php (1)

338-339: Stricter origin check and clarified wildcard semantics look good

The docblock note plus the updated isAllowedOrigin() implementation ($origin !== '' && in_array(..., true)) make the CORS origin check safer and more predictable, with wildcard handled exclusively in setAccessControlAllowOriginHeader(). This keeps credentialed CORS locked to explicit origins and avoids accidental acceptance of empty origins.

If you want full consistency, you could also switch isAllowedReferer() to use strict in_array(..., true), but that’s purely optional here.

Also applies to: 345-345

src/Router.php (1)

94-101: Centralized CORS origin handling and wiring into handlers/middleware is sound

Parsing CORS_ALLOWED_ORIGINS once in Router::route(), passing the resulting $allowedOrigins into MissalsHandler, DecreesHandler, RegionalDataHandler (for non-localhost PUT/PATCH/DELETE), and into ErrorHandlingMiddleware cleanly centralizes CORS configuration and keeps auth-related CORS behavior consistent and fail-closed.

A couple of minor considerations (non-blocking):

  • If CORS_ALLOWED_ORIGINS is set but empty/whitespace, $allowedOrigins becomes an empty array, effectively disabling all cross-origin access for the guarded routes. That’s secure but might be worth treating as an explicit configuration error or normalizing to ['*'] according to how you validate this env var elsewhere (e.g., in public/index.php).
  • If you later add more entry points (CLI servers, workers) that also need this value, it might be worth extracting the parse/validate logic into a small shared helper to avoid configuration drift.

Functionally and from a security perspective this change looks correct and aligned with the Phase 2.5 cookie-auth goals.

Also applies to: 206-207, 240-241, 363-368, 405-405

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 68e130d and 0f16d4a.

📒 Files selected for processing (2)
  • src/Handlers/AbstractHandler.php (1 hunks)
  • src/Router.php (5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.php: Implement PSR-7 request/response handling for all HTTP operations
Use LoggerFactory::create() to instantiate PSR-3 compliant Monolog loggers
Implement JWT authentication via JwtService for protected endpoints (PUT, PATCH, DELETE /data/{category}/{calendar})
Always use timezone Europe/Vatican for all date and time calculations
Enforce PSR-12 coding standards via PHP_CodeSniffer across all PHP files
Enforce PHPStan level 10 static analysis for all PHP files
Use PSR-4 autoload for LiturgicalCalendar\Api namespace
Return HTTP 401 UnauthorizedException or 403 ForbiddenException from Http/Exception namespace for authentication failures
Require PHP >= 8.4 with support for modern syntax like array_find
Use intl, zip, calendar, yaml, gettext, curl, json, xml PHP extensions

Files:

  • src/Handlers/AbstractHandler.php
  • src/Router.php
src/Handlers/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

src/Handlers/*.php: Extend AbstractHandler and implement PSR RequestHandlerInterface for all handler classes
Use Negotiator::pickLanguage() for Accept-Language header processing instead of PHP's Locale::acceptFromHttp()
Use Negotiator::negotiateResponseContentType() to respect return_type query parameter and Accept header for content negotiation
Set allowed methods, accept headers, and content types in handler constructor
Use LitLocale::isValid() to validate supported locales before processing language-specific requests
Return PSR-7 ResponseInterface from all handler handle() methods
Create handler classes extending AbstractHandler in src/Handlers/ directory with one route per handler
Use JWT authentication via Authorization Bearer token header for protected endpoints

Files:

  • src/Handlers/AbstractHandler.php
src/Router.php

📄 CodeRabbit inference engine (CLAUDE.md)

src/Router.php: Add route case in Router::route() switch statement when implementing a new handler
Delegate routing to appropriate Handler classes in Router based on URL path
Run PSR-15 middleware pipeline (ErrorHandling, Logging) in Router for all requests

Files:

  • src/Router.php
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Handlers/*.php : Use JWT authentication via Authorization Bearer token header for protected endpoints
Learnt from: JohnRDOrazio
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 349
File: .github/workflows/phpunit.yml:84-101
Timestamp: 2025-09-06T17:19:28.704Z
Learning: JohnRDOrazio prefers to fail early within the same GitHub Actions step rather than using $GITHUB_OUTPUT to communicate between steps, when fixing issues where a workflow might pass without actually running the intended operations.
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Handlers/*.php : Set allowed methods, accept headers, and content types in handler constructor

Applied to files:

  • src/Handlers/AbstractHandler.php
  • src/Router.php
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Router.php : Add route case in Router::route() switch statement when implementing a new handler

Applied to files:

  • src/Router.php
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Router.php : Delegate routing to appropriate Handler classes in Router based on URL path

Applied to files:

  • src/Router.php
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Router.php : Run PSR-15 middleware pipeline (ErrorHandling, Logging) in Router for all requests

Applied to files:

  • src/Router.php
📚 Learning: 2025-09-16T17:02:12.414Z
Learnt from: JohnRDOrazio
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 366
File: src/Router.php:386-389
Timestamp: 2025-09-16T17:02:12.414Z
Learning: In the LiturgicalCalendarAPI Router middleware pipeline, ErrorHandlingMiddleware is intentionally placed as the outermost middleware to catch any errors from inner middleware including LoggingMiddleware. ErrorHandlingMiddleware handles error logging while LoggingMiddleware handles request/response logging, creating separation of concerns between operational and error logging.

Applied to files:

  • src/Router.php
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Http/Middleware/*.php : Implement middleware classes extending PSR-15 MiddlewareInterface in src/Http/Middleware/ directory

Applied to files:

  • src/Router.php
📚 Learning: 2025-11-27T05:16:34.376Z
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T05:16:34.376Z
Learning: Applies to src/Handlers/*.php : Use JWT authentication via Authorization Bearer token header for protected endpoints

Applied to files:

  • src/Router.php
🧬 Code graph analysis (2)
src/Handlers/AbstractHandler.php (1)
src/Http/Middleware/ErrorHandlingMiddleware.php (1)
  • isAllowedOrigin (97-115)
src/Router.php (2)
src/Handlers/AbstractHandler.php (1)
  • setAllowedOrigins (75-80)
src/Http/Middleware/ErrorHandlingMiddleware.php (1)
  • ErrorHandlingMiddleware (14-321)

JohnRDOrazio and others added 2 commits December 3, 2025 04:02
…ency

Align isAllowedReferer() with isAllowedOrigin() by:
- Using strict comparison (in_array third parameter = true)
- Adding null coalescing for HTTP_REFERER to handle missing header
- Adding empty string check to reject empty referers

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Add Utilities::parseCorsAllowedOrigins() method that:
- Provides consistent parsing across all entry points
- Handles null, wildcard ('*'), and comma-separated lists
- Gracefully handles empty/whitespace-only values (defaults to ['*'])
- Optionally throws exception on empty result for strict validation

Update Router.php to use the new utility method.

This enables reuse in CLI servers, workers, or other entry points
without configuration drift.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@JohnRDOrazio JohnRDOrazio merged commit ddc65be into development Dec 3, 2025
2 checks passed
@JohnRDOrazio JohnRDOrazio deleted the feature/phase-2.5-docs-update branch December 3, 2025 03:08
@coderabbitai coderabbitai bot mentioned this pull request Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants