Enhancement/options cutom handling#2447
Enhancement/options cutom handling#2447Greisby wants to merge 10 commits intodrogonframework:masterfrom
Conversation
|
This is an initial draft; please let me know if you’d prefer different policy choices or if you’d rather avoid adding the extra std::string_view utilities. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive API for building consistent OPTIONS/CORS preflight responses in application code, allowing developers to handle CORS manually when opting out of Drogon's internal handling.
Changes:
- Adds convenience methods
isCorsRequest()andisCorsPreflightRequest()to HttpRequest for detecting CORS requests - Introduces
newOptionsResponse()helper to generate generic OPTIONS or CORS preflight responses with validation - Adds
addCorsHeaders()helper for adding CORS headers to non-preflight CORS responses - Implements lightweight string_view utilities (ci_equals, trim, split/join) for efficient header parsing
- Includes comprehensive unit tests covering various CORS scenarios and edge cases
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| trantor | Updates trantor submodule reference |
| lib/tests/unittests/HttpHeaderTest.cc | Adds unit tests for OPTIONS response generation, CORS preflight validation, and CORS header management |
| lib/src/HttpResponseImpl.cc | Implements newOptionsResponse() and addCorsHeaders() methods with CORS validation logic |
| lib/inc/drogon/utils/Utilities.h | Adds string_view utility functions for case-insensitive comparison, trimming, and splitting/joining |
| lib/inc/drogon/HttpResponse.h | Declares newOptionsResponse() and addCorsHeaders() public API methods with detailed documentation |
| lib/inc/drogon/HttpRequest.h | Adds isCorsRequest() and isCorsPreflightRequest() convenience methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @an-tao, I added a short note in the PR description about the policy choices and the helper utilities. I’d appreciate your take when you have a moment — I’d like to align on that before switching the PR out of Draft. |
|
This PR was merged into the master branch a few days ago. |
|
You are mixing with #2444 I think. |
It LGTM. |
Huh?? |
Should I perhaps rebase it? |
Please rebase to the latest commit on the master branch |
|
Hello! Sorry I was at FOSDEM and completely busy. I am back but needs a day to recover from timezone. Please ping me if I forgot about this |
This is a follow up to the PR #2444: now that Drogon can opt out of its internal OPTIONS/CORS short-circuit and let applications handle preflight in route handlers, this PR provides small, self-contained helpers API to build consistent OPTIONS / CORS preflight responses inside application code.
Summary
Concretely, it introduces:
Allow, orAccess-Control-Allow-*headers, with optional policy validation.No default behavior changes: Drogon’s existing internal OPTIONS handling remains untouched unless the application opts out (as introduced in #2444).
Motivation
When an application opts out of the internal preflight shortcut (per #2444), it typically still needs to generate responses that are:
This PR provides that utility without altering the HttpServer default CORS behavior or policy surface.
Policy choice: explicit errors vs silent success
For preflight validation (origin / requested method / requested headers), this implementation returns explicit errors (400/403/405) when the CORS preflight request is malformed or not allowed.
This is a deliberate policy choice:
The Fetch Standard (CORS protocol) does not mandate any particular error status for a denied preflight.
In practice, browsers decide on their side whether the preflight is acceptable based on the presence/values of Access-Control-Allow-* headers. A 204 without the expected allow headers is effectively a “deny” from the browser’s point of view as well.
I chose explicit 403/405 responses, with an
X-Cors-Errordiagnostic header, because it is more logical and much easier to debug for web developers (especially when testing from tools like curl, Postman, or browser devtools).If maintainers would prefer a “silent deny” approach instead (always 204 and omit/limit allow headers), I can adapt the helper accordingly — both approaches are valid, as browsers enforce the decision client-side.
Why the added string_view utilities
The helper needs a few small string operations (case-insensitive compare, trimming, splitting/joining header lists). I implemented them using std::string_view because:
API / Behavior
HttpRequest::isCorsRequest()
Returns true when the request contains an
Originheader (i.e. a CORS request), regardless of the HTTP method.HttpRequest::isCorsPreflightRequest()
Returns true when the request is an OPTIONS request and contains the preflight-defining headers:
OriginAccess-Control-Request-Method(Used as a cheap, explicit gate when implementing custom handlers.)
HttpResponse::newOptionsResponse(...)
Allow:... (method list comes from the request attributes obtained from Drogon’s route handler.Originback inAccess-Control-Allow-Origin(with optional originValidator and allowNullOrigin policy).Access-Control-Allow-Methodsusing the allowed method list (and validatesAccess-Control-Request-Method, returning 405 +Allowif not permitted).Access-Control-Request-Headersas follows:Access-Control-Allow-Credentials: trueto the responseAccess-Control-Request-Private-Network: true, adds the headerAccess-Control-Allow-Private-Network: trueto the responseAccess-Control-Max-Age: <n>on successOn validation failures, the helper returns explicit HTTP status codes and adds a diagnostic X-Cors-Error header to ease debugging.
HttpResponse::addCorsHeaders(…)
Helper for non-preflight CORS requests: reflects the request Origin (unless Access-Control-Allow-Origin is already set), ensures
Vary: Origin, optionally managesAccess-Control-Allow-Credentials, and merges exposed headers intoAccess-Control-Expose-Headers.Unit Tests
Added unit tests to cover:
Allow:).