Public CORS decorator (PP-5045) - #3680
Conversation
|
Claude finished @tdilauro's task in 8m 49s —— View job Code review
SummaryThis looks correct and well-tested; both of jonathangreen's suggestions (the |
Greptile SummaryAdds a reusable wildcard CORS decorator for public read-only routes, including preflight handling and error-response back-filling.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/palace/manager/api/routes.py | Introduces the public CORS decorator, stacking guards, shared policy, and error-response back-fill hook. |
| src/palace/manager/api/util/flask.py | Adds the read-only public CORS method set and validates marked routes during registration. |
| tests/manager/api/test_routes.py | Adds functional coverage for public CORS responses, preflights, errors, short circuits, and decorator combinations. |
| tests/manager/api/util/test_flask.py | Verifies route-registration enforcement for marked public CORS views. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Request[Cross-origin request] --> Route{Public CORS route?}
Route -->|No| Normal[Normal response handling]
Route -->|Yes| Method{OPTIONS preflight?}
Method -->|Yes| Preflight[Return GET HEAD OPTIONS CORS policy]
Method -->|No| View[Run decorated view]
View --> Response{Wrapper sees response?}
Response -->|Yes| Headers[Add wildcard origin header]
Response -->|No: error or short-circuit| Hook[After-request hook back-fills header]
Headers --> Client[Browser-readable response]
Hook --> Client
Reviews (11): Last reviewed commit: "CI AI code review feedback" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| assert False == routes.app.url_map.merge_slashes | ||
|
|
||
|
|
||
| class TestAllowsPublicCors: |
There was a problem hiding this comment.
TestAllowsPublicCors organizes these tests around one behavior rather than the module under test, contrary to the repository's module-oriented test-class convention and making related route tests less consistent to locate.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3680 +/- ##
=======================================
Coverage 93.55% 93.56%
=======================================
Files 513 513
Lines 46907 46948 +41
Branches 6405 6414 +9
=======================================
+ Hits 43884 43926 +42
+ Misses 1954 1953 -1
Partials 1069 1069 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
More Claude / greptile beef. And I'm here for it! 😂
|
1dae183 to
2183995
Compare
jonathangreen
left a comment
jonathangreen
left a comment
There was a problem hiding this comment.
This looks great! I added a couple suggestions to consider, but otherwise I think its good to go.
| stacking them raises PalaceValueError at decoration time. The | ||
| advertised methods list only limits what a preflight advertises; it | ||
| does not block other methods on the actual response, so apply this | ||
| decorator only to GET/HEAD routes. |
There was a problem hiding this comment.
It would be nice if we could enforce this, instead of just having it in the comments here. Just to prevent the footgun where someone adds the decorator and someone else expands the route to take additional methods. It might be hard to do though, since we don't know at decoration time what route this applies to. Claude has a suggestion for this, but I don't like what it suggested (I don't think it would even work).
One thing that might work is overriding add_url_rule in PalaceFlask and having it handle raising. Something like this perhaps (in api/util/flask.py):
#: The methods a route may allow while using the public wildcard CORS policy.
#: A wildcard Access-Control-Allow-Origin is only safe on requests that cannot
#: change state.
PUBLIC_CORS_METHODS = frozenset({"GET", "HEAD", "OPTIONS"})
class PalaceFlask(flask.Flask):
def add_url_rule(
self,
rule: str,
endpoint: str | None = None,
view_func: ft.RouteCallable | None = None,
provide_automatic_options: bool | None = None,
**options: Any,
) -> None:
"""Register a URL rule, rejecting unsafe uses of allows_public_cors.
A route's methods are not known until it is registered, so this is the
first point where the GET/HEAD-only requirement can be checked.
"""
if getattr(view_func, "allows_public_cors", False):
methods = (
options.get("methods")
or getattr(view_func, "methods", None)
or ("GET",)
)
unsafe = {method.upper() for method in methods} - PUBLIC_CORS_METHODS
if unsafe:
raise PalaceValueError(
f"Route '{rule}' allows {', '.join(sorted(unsafe))}, so it must "
f"not use allows_public_cors: a wildcard "
f"Access-Control-Allow-Origin would let any web origin read the "
f"response."
)
super().add_url_rule(
rule, endpoint, view_func, provide_automatic_options, **options
) There was a problem hiding this comment.
Great idea! I did not have a good answer for that one, so I was going to let it ride for now.
|
|
||
| _public_cors = cross_origin( | ||
| origins="*", | ||
| methods=["GET", "HEAD", "OPTIONS"], |
There was a problem hiding this comment.
Bonus: If you define PUBLIC_CORS_METHODS, you could use that here as well, so they stay aligned.
| @app.after_request | ||
| def add_public_cors_to_error_responses(response: Response) -> Response: | ||
| """Back-fill the wildcard CORS header on public routes. | ||
|
|
||
| The allows_public_cors decorator cannot add headers to a response it | ||
| never sees. A view that raises gets its response built by the app-level | ||
| error handler, and cross-origin clients need the header on that | ||
| response to read the error body. As a safety net, the hook also covers | ||
| stacks that ignore the placement rule in allows_public_cors, where an | ||
| outer decorator returns a problem detail without calling the view. | ||
|
|
||
| Responses produced before routing resolves an endpoint cannot be | ||
| attributed to a view and are not back-filled: a 405 for a method the | ||
| route does not allow, and a strict_slashes redirect. Register public | ||
| CORS routes with strict_slashes=False (see library_dir_route) so a | ||
| trailing-slash mismatch never becomes an unreadable redirect. | ||
| """ | ||
| if "Access-Control-Allow-Origin" not in response.headers and request.endpoint: | ||
| view = current_app.view_functions.get(request.endpoint) | ||
| if getattr(view, "allows_public_cors", False): | ||
| response.headers["Access-Control-Allow-Origin"] = "*" | ||
| return response |
There was a problem hiding this comment.
Suggestion: Instead of hand rolling this, perhaps we use set_cors_headers and get_cors_options so that the options we use don't drift, and we get all the guards built into these functions.
Add this above:
_PUBLIC_CORS_OPTIONS = frozendict(
origins="*",
methods=PUBLIC_CORS_METHODS,
max_age=3600,
send_wildcard=True,
supports_credentials=False,
)
_public_cors = cross_origin(**_PUBLIC_CORS_OPTIONS)Then in this function:
@app.after_request
def add_public_cors_to_error_responses(response: Response) -> Response:
if "Access-Control-Allow-Origin" in response.headers or not request.endpoint:
return response
view = current_app.view_functions.get(request.endpoint)
if getattr(view, "allows_public_cors", False):
set_cors_headers(response, get_cors_options(current_app, _PUBLIC_CORS_OPTIONS))
return response
## Description Applies the new `allows_public_cors` decorator to the routes that serve public, credential-free data: the library index, authentication document, catalog feeds (groups, feed, navigation), search, crawlable feeds, the MARC download page, works lookups (URN lookup, contributor, series, permalink, recommendations, related books), analytics event tracking (authentication there is optional, and events record with or without a patron), and `version.json`. These routes now send `Access-Control-Allow-Origin: *` without credentials. The authentication document, the MARC page, and `/version.json` previously sent no CORS headers at all. Authenticated routes (loans, holds, borrow, fulfill, revoke, annotations, and the patron profile, device, and token endpoints) keep the existing `allows_patron_web` allowlist behavior. Per the decorator's placement rule, it sits outside `has_library`, so OPTIONS preflights get full CORS headers before library resolution can short-circuit. Updates the `PALACE_PATRON_WEB_HOSTNAMES` documentation to match. > [!NOTE] > This PR is stacked atop #3680. It should not be merged until that one has landed. ## Motivation and Context These routes are readable by anyone without credentials, so restricting browser origins adds no protection. It only breaks web catalog clients whose hosts are not in the configured allowlist, which creates friction for dev instances and new deployments. With open CORS on public routes, a web catalog works against any Circulation Manager without configuration; the allowlist now only governs credentialed endpoints. Patron-specific variants of these routes (root lane redirects, loan state on permalinks, age-based lane filtering) are reachable only through an explicit `Authorization` header. The Palace web client sends that header on a plain, non-credentialed fetch, so the wildcard origin does not affect it; cookie-credentialed requests are not part of the patron API. ## How Has This Been Tested? New parametrized tests request each of the 18 public routes through the route test fixture with a real `Origin` header and assert the wildcard origin with no credentials header. A negative test asserts `/loans` still echoes only configured allowlist origins and never the wildcard, and a library-not-found test pins the decorator's placement outside `has_library`. All existing route and controller tests pass. ## Checklist - [x] I have updated the documentation accordingly. - [x] All new and existing tests passed. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2 participants
Description
Adds an
allows_public_corsroute decorator that sends open CORS headers and handles preflight requests:Access-Control-Allow-Origin: *,GET,HEAD, andOPTIONSmethods.Stacking it with
allows_patron_webin either order, or applying it twice, raisesPalaceValueErrorat decoration time (allows_patron_webgained the reciprocal check). Registering a decorated route that allows write methods also raisesPalaceValueError, enforced inPalaceFlask.add_url_ruleat route registration time. The decorator's docstring additionally requires placing it outside decorators that can answer a request without calling the view, such ashas_library.An
after_requesthook backfills the public CORS headers on responses the decorator never sees, applying the same shared options as the decorator through flask-cors'sget_cors_options/set_cors_headers, such as a raising view converted by the app-level error handler, or an outer decorator returning a problem detail. Without it, cross-origin clients would get an opaque CORS failure instead of a readable error body. Responses produced before routing resolves an endpoint (a 405 for a method the route does not define, or astrict_slashesredirect) cannot be attributed to a view and are not backfilled; the follow-up PR should register public routes withstrict_slashes=False.No routes use it yet.
Motivation and Context
Public catalog data is readable by anyone without credentials, so an origin allowlist adds no protection there. What it does do is break browser-based catalog clients (like CPW) when their hosts are not configured, which creates friction for dev instances and new deployments. This decorator is the first step. A follow-up PR will apply it to the public routes.
[Jira PP-5045]
How Has This Been Tested?
Checklist