Skip to content

Use open CORS on public catalog routes (PP-5045) - #3693

Merged
tdilauro merged 2 commits into
mainfrom
feature/public-cors-routes
Sep 1, 2026
Merged

Use open CORS on public catalog routes (PP-5045)#3693
tdilauro merged 2 commits into
mainfrom
feature/public-cors-routes

Conversation

@tdilauro

@tdilauro tdilauro commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@claude

claude Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Claude finished @tdilauro's task in 6m 33s —— View job


Summary

Solid, well-scoped change: the decorator sits outside has_library everywhere it's applied, so preflights and library-not-found short circuits both get headers, and every newly decorated route is GET/HEAD-only, so PalaceFlask.add_url_rule's guard passes. I verified that allows_patron_web is still on exactly the patron-data routes (/patrons/me*, /loans*, /annotations*, borrow, fulfill, revoke), that the marker attribute survives the outer library_route registrations (so add_public_cors_to_error_responses never back-fills a wildcard onto a patron route), and that PALACE_PATRON_WEB_HOSTNAMES has no consumer other than allows_patron_web — so the README rewording is accurate. One Minor below. Separately, the description's merge-blocker note is stale: #3680 (b9bdc5e) is already in main and in this branch's history, so that note can come out. I did not run the test suite locally (the route tests need the docker/DB tox environment); CI and Codecov report green on 5125daf.

Details

Minor: src/palace/manager/api/routes.py:636-641

The latest commit moved track_analytics_event from allows_patron_web to allows_public_cors, but the PR description still lists analytics among the routes that "keep the existing allows_patron_web allowlist behavior", and the README sentence enumerates only read-only endpoints ("catalog feeds, search, work details, the authentication document, and operational endpoints like /version.json"). This is also the one route under the public policy whose GET mutates state — it writes a CirculationEvent via AnalyticsController.track_event — which is exactly the assumption PUBLIC_CORS_METHODS documents as the reason the GET/HEAD check is a sufficient guard ("A wildcard Access-Control-Allow-Origin is only safe on requests that cannot change state"). The exposure looks acceptable in practice (the event was always triggerable cross-origin without CORS, and the success body is an empty 200), but as written the invariant and the docs no longer match the route table. Worth updating the description and the README enumeration to name the analytics event endpoint — and a short comment on the route recording why a state-changing GET is acceptable here would keep the next reader from treating the mismatch as a bug.

@library_route("/analytics/<identifier_type>/<path:identifier>/<event_type>")
@allows_public_cors
@has_library
@allows_auth
@returns_problem_detail
def track_analytics_event(identifier_type, identifier, event_type):

| Branch: feature/public-cors-routes

@greptile-apps

greptile-apps Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR applies wildcard, non-credentialed CORS handling to public catalog routes and updates the documented scope of the patron-web hostname allowlist.

  • Moves public CORS handling outside library resolution so preflight and library-error responses receive the appropriate headers.
  • Covers catalog feeds, search, work endpoints, analytics, the authentication document, MARC output, and version metadata.
  • Adds route-level tests for wildcard public CORS and continued allowlist behavior on patron endpoints.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains within the reviewed follow-up scope.

No blocking failure remains.

Important Files Changed

Filename Overview
src/palace/manager/api/routes.py Replaces patron-origin CORS with wildcard non-credentialed CORS on public routes and positions it before library resolution.
tests/manager/api/test_routes.py Adds coverage for wildcard public-route responses, library-resolution errors, and retained patron-route allowlisting.
tests/fixtures/api_routes.py Extends the direct route-request fixture to accept request headers needed by CORS tests.
README.md Clarifies that the hostname allowlist governs patron-data endpoints rather than public catalog endpoints.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    Browser[Browser client] --> Route{Requested route}
    Route -->|Public catalog route| PublicCORS[Wildcard origin without credential support]
    Route -->|Patron-management route| PatronCORS[Configured patron-web origin allowlist]
    PublicCORS --> Library[Resolve library when required]
    PatronCORS --> Auth[Authenticate patron]
    Library --> PublicResponse[Public response]
    Auth --> PatronResponse[Patron-specific response]
Loading

Reviews (3): Last reviewed commit: "CI AI code review feedback" | Re-trigger Greptile

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.56%. Comparing base (b9bdc5e) to head (e18169b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3693   +/-   ##
=======================================
  Coverage   93.56%   93.56%           
=======================================
  Files         513      513           
  Lines       46948    46951    +3     
  Branches     6414     6414           
=======================================
+ Hits        43926    43931    +5     
+ Misses       1953     1952    -1     
+ Partials     1069     1068    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jonathangreen jonathangreen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me!

Base automatically changed from feature/public-cors-decorator to main September 1, 2026 17:02
@tdilauro
tdilauro force-pushed the feature/public-cors-routes branch from 5125daf to e18169b Compare September 1, 2026 17:02
@tdilauro
tdilauro merged commit d2ead5c into main Sep 1, 2026
25 checks passed
@tdilauro
tdilauro deleted the feature/public-cors-routes branch September 1, 2026 17:24
@jonathangreen jonathangreen added the feature New feature label Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants