Admin routes in English (non-i18n), with 301 legacy redirects (#145)#161
Open
fabiodalez-dev wants to merge 7 commits into
Open
Admin routes in English (non-i18n), with 301 legacy redirects (#145)#161fabiodalez-dev wants to merge 7 commits into
fabiodalez-dev wants to merge 7 commits into
Conversation
…ts (#145) Rename every Italian /admin/* route segment to English in the route definitions (libri->books, prestiti->loans [merged into the single /admin/loans namespace], utenti->users, collane->series, autori->authors, generi->genres, editori->publishers, prenotazioni->reservations, collocazione->placement, recensioni->reviews, statistiche->statistics; action segments crea->create, modifica->edit, etc.). Add a 301 legacy-redirect block: each old Italian primary path (and its action sub-segments, translated per path-segment) permanently redirects to the English equivalent, so bookmarks and already-sent email links keep working. Admin routes are English literals, never routed through route_path()/RouteTranslator (that system is user-facing only). Neutralize the one latent hook: plugins.php now uses url('/admin/plugins') directly and the 'plugins' entry is dropped from RouteTranslator's fallback map.
…145) Update every url('/admin/...') link, redirect Location header, inline JS fetch and email URL across controllers, views and NotificationService to the renamed English admin paths. Sidebar links and keyboard shortcuts in layout.php included.
z39-server, archives and frbr-lrm plugins: route definitions, view links and JS
fetches updated to the English admin paths. frbr-lrm's attach-opera route moves
from the old italian books path to /admin/books/{id} (would otherwise have been
shadowed by the legacy redirect wildcard).
Update goto/fetch paths and URL-fragment assertions across the spec suite, including escaped-slash regex matchers and bare action fragments in waitForFunction/url checks (modifica->edit, crea->create) that the path-prefix rename did not reach.
Adversarial review of the admin-route rename surfaced defects the residual-grep
missed (corrupted English, not leftover Italian) that the partial E2E did not
exercise:
- P1 detailso: '/dettagli' fired before '/dettaglio' in some files, mangling
series detail links to /admin/series/detailso (404). Fixed to /detail across
CollaneController, collane views and the affected specs.
- P1 createte: 'crea' matched inside the already-translated 'create', producing
/createte (404) for events-create and languages-create, plus a fatal
require pointing at create.php typo'd to createte.php. Fixed repo-wide.
- P1 /admin/reviews fatal: the include path was renamed recensioni->reviews but
the view directory was not — renamed app/Views/admin/recensioni -> reviews.
- P2 legacy redirect: add 'crea-opera' => 'create-opera' to the action map so an
old /admin/collane/crea-opera bookmark 301s to the real route, not a 404.
- P2 route_path('plugins'): the fallback entry was dropped but discogs' settings
view still used it (would resolve to /plugins). Point it at url('/admin/plugins').
- P2 hardening: rawurlencode each redirected path segment (defense in depth).
- P3: English-ify the stale Italian section-header comments in web.php.
All five previously-broken admin pages now return 200; PHPStan clean.
|
Warning Review limit reached
More reviews will be available in 14 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRinomina tutte le rotte admin da italiano a inglese (libri→books, autori→authors, ecc.), aggiorna redirect, link, notifiche e test. Introduce in web.php un redirect legacy che mappa automaticamente i vecchi percorsi IT ai nuovi EN preservando query string. ChangesAdmin routes: migrazione IT→EN
Sequence Diagram(s)sequenceDiagram
participant UI
participant AdminRouter
participant LoansController
participant DB
UI->>AdminRouter: POST /admin/prestiti/rinnova/:id (legacy)
AdminRouter-->>UI: 301 /admin/loans/renew/:id
UI->>AdminRouter: POST /admin/loans/renew/:id
AdminRouter->>LoansController: renew(id)
LoansController->>DB: validate+update
DB-->>LoansController: ok/fail
LoansController-->>UI: 302 /admin/loans?renewed=1 or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/Views/generi/dettaglio_genere.php (1)
41-45: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSostituisci
HtmlHelper::e()in questa view.Questa vista mescola l’escaping consentito con
HtmlHelper::e(), che qui è esplicitamente vietato. Uniforma questi output ahtmlspecialchars(..., ENT_QUOTES, 'UTF-8')per non divergere dalla policy di escaping delle view.As per coding guidelines:
Mai usare HtmlHelper::e() nelle view — usare htmlspecialchars(..., ENT_QUOTES, 'UTF-8')Also applies to: 73-73, 108-108, 200-208
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/generi/dettaglio_genere.php` around lines 41 - 45, The view is using HtmlHelper::e(...) which is forbidden by policy; replace all occurrences of HtmlHelper::e(...) (e.g. HtmlHelper::e($genereName) and the other instances at the noted spots) with htmlspecialchars($variable, ENT_QUOTES, 'UTF-8') so escaping is consistent across the view; update each instance (including where parent name and the other occurrences) to use htmlspecialchars(..., ENT_QUOTES, 'UTF-8') and ensure no other HtmlHelper::e calls remain in this template.tests/full-test.spec.js (2)
1177-1188:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
waitForURL(/admin\/authors/)non aspetta davvero il redirect.La regex matcha già
/admin/authors/createe/admin/authors/edit/{id}, quindi l’attesa può chiudersi prima della submit effettiva. Le query DB subito dopo rischiano di correre prima del commit e rendere la suite flaky. Aspetta la list page oppure escludi esplicitamente/createe/edit.Also applies to: 1195-1206, 1213-1224, 1284-1302
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/full-test.spec.js` around lines 1177 - 1188, The waitForURL(/admin\/authors/) used after submitting the author form can match the /create or /edit routes and return too early; change the wait to target the actual list page (e.g. waitForURL(`${BASE}/admin/authors`) or a stricter regex that excludes /create and /edit such as /\/admin\/authors\/?(?!create|edit)/) or replace it with a stable DOM wait (e.g. waitForSelector for the authors list/table) so the test only proceeds when the list page is fully loaded; update the same pattern where used (the other occurrences around the submit flows) so all post-submit waits use the stricter URL or DOM-based wait.
1351-1362:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStesso race anche sui flussi publisher.
waitForURL(/admin\/publishers/)matcha già/admin/publishers/createe/admin/publishers/edit/{id}. In pratica il test può proseguire senza aspettare il redirect post-submit e leggere il DB troppo presto.Also applies to: 1369-1380, 1433-1443
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/full-test.spec.js` around lines 1351 - 1362, The test is racing after publisher submit because waitForURL(/admin\/publishers/) also matches the create/edit paths so the test can continue before the post-submit redirect; update the submit-wait logic in the block using page.fill / page.locator('button[type="submit"]') / page.waitForURL to wait for the exact list page (e.g. a URL match that ends with /admin/publishers or a specific selector present on the list) or use page.waitForNavigation tied to the submit click; apply the same change to the other occurrences around lines where similar blocks appear (the blocks starting at the noted file ranges) so the test reliably waits for the redirect to the publishers list before proceeding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Controllers/EditorsController.php`:
- Line 96: Replace hardcoded Location header paths with the URL helper or route
translator so redirects respect a non-root base path: change instances of return
$response->withHeader('Location', '/admin/publishers')->withStatus(302); (and
the other two similar returns) to use url('/admin/publishers') or, preferably
per guidelines, route_path('...') / RouteTranslator::route('...') so the
Location header becomes url(...) or route_path(...) instead of a hardcoded
string.
- Around line 140-142: The code reflects the raw Referer into the redirect
Location (variables $referer and $target using str_contains()) which allows
open-redirects; before using $referer, validate it: parse the URL from
$request->getHeaderLine('Referer'), ensure it is same-origin (host and scheme
match your app's expected host/scheme) or its path is an internal absolute path
beginning with '/admin/publishers', and only then set $target to that safe path;
otherwise always fall back to the internal '/admin/publishers'. Update the logic
around $referer, $target, str_contains() and the
response->withHeader('Location', ...) call to enforce this validation.
In `@app/Controllers/ReservationsAdminController.php`:
- Around line 170-174: Nelle due istruzioni di redirect in
ReservationsAdminController (le due occorrenze che usano
$response->withHeader('Location', ...)->withStatus(302)) stai passando percorsi
hardcoded; sostituisci quei percorsi con il wrapper url() (es. usa
url('/admin/reservations?updated=1') e url('/admin/reservations/edit/' . $id .
'?error=update_failed')) in modo che i redirect rispettino il prefisso
dell'applicazione quando è in una sottodirectory.
- Line 227: Il redirect usa un path hardcoded che non funziona in installazioni
in sottodirectory; sostituisci la stringa
'/admin/reservations/create?error=missing_data' con la chiamata al wrapper
url(), cioè usa url('/admin/reservations/create?error=missing_data')
nell'espressione che ritorna $response->withHeader('Location',
...)->withStatus(302) (nella classe ReservationsAdminController, dove compare la
return con withHeader('Location', ...)).
- Around line 299-303: In ReservationsAdminController locate the redirects that
call $response->withHeader('Location', '...') (the success and error branches
that currently use '/admin/reservations?created=1' and
'/admin/reservations/create?error=save_failed') and replace the hardcoded path
strings with the application's url() helper so the base path is preserved; i.e.,
pass the url(...) result into withHeader('Location', ...) for both the success
and failure redirects and keep the withStatus(302) unchanged.
In `@app/Controllers/SearchController.php`:
- Line 295: In SearchController replace the hardcoded 'url' construction for
admin resources (currently using url('/admin/books/' . (int)$row['id']) and
similar for authors/publishers) with the centralized route helper (either
route_path('key') or RouteTranslator::route('key')) and pass the row id as a
route parameter; locate the 'url' entries that build admin links
(books/authors/publishers) and swap them to call the appropriate named route
(e.g., admin.books.show) with the id from $row['id'], and make the same change
for the other occurrences referenced in this diff.
In `@app/Controllers/UsersController.php`:
- Around line 92-109: Replace hardcoded root-relative Location headers in
UsersController (the return statements using $response->withHeader('Location',
'/admin/users/...')->withStatus(302)) with base-aware route/url helpers: build
the target with the app routing helper (e.g., route_path('admin.users.create')
or RouteTranslator::route('admin.users.create') or url('admin/users/create'))
and append the existing query string (e.g., ?error=...) before calling
$response->withHeader('Location', $target)->withStatus(302); update every
instance in this file that sets Location via '/admin/users/...' (including the
blocks shown and the other ranges noted) so redirects respect path prefixes.
In `@app/Routes/web.php`:
- Around line 3139-3165: The route currently maps both 'GET' and 'POST' but
always responds with a 301, which can force legacy POSTs to become GETs and drop
body/CSRF; change the response status depending on the incoming request method:
inspect $request->getMethod() inside the mapped closure and use 301 for GET but
308 for non-GET (POST) so the client preserves method/body. Update the closure
that builds $location and returns $response->withHeader('Location',
$location)->withStatus(301) to determine $status = ($request->getMethod() ===
'GET') ? 301 : 308 and call withStatus($status) instead.
In `@app/Views/libri/modifica_libro.php`:
- Line 47: Sostituisci l'uso di HtmlHelper::e(...) nella view con
htmlspecialchars(..., ENT_QUOTES, 'UTF-8'); in particolare modifica
l'espressione HtmlHelper::e($book['titolo'] ?? '') usata nel link (vicino a
url('/admin/books/' . (int)($book['id'] ?? 0))) per usare
htmlspecialchars($book['titolo'] ?? '', ENT_QUOTES, 'UTF-8') così da rispettare
la regola "Mai usare HtmlHelper::e() nelle view".
In `@tests/full-test.spec.js`:
- Around line 1521-1528: The tests call the Italian route "/admin/books/importa"
causing 404s after migrating admin routes to English; update all page.goto()
usages (e.g., the call inside the "10.2 Upload CSV file" test and the other
occurrence around the noted lines) to the English route "/admin/books/import" so
the tests navigate to the correct import page.
In `@tests/genre-bugs.spec.js`:
- Line 157: The tests still expect legacy Italian paths (e.g. "generi/libri")
after navigation to `${BASE}/admin/genres/create`, causing waits and URL matches
to timeout; update all URL regexes and waits in this scenario to the new English
routes ("genres" and "books") so they match redirects and allow IDs to be
extracted. Search for usages around page.goto(`${BASE}/admin/genres/create`) and
the related waits (e.g. page.waitForResponse, page.waitForURL, any regex matches
or expect checks referencing "generi" or "libri") and replace those patterns
with the corresponding "genres" / "books" regexes; do the same for the other
reported occurrences (lines near 165, 173, 181, 217, 244, 256, 263, 294) so all
assertions and post-submit redirects are consistent.
In `@tests/genre-merge-rearrange.spec.js`:
- Line 34: Tests navigate to /admin/genres/... but assertions still look for the
legacy "generi" path, causing intermittent failures; update all assertions that
use waitForURL and match to expect "genres" instead of "generi" (e.g., near the
page.goto(`${BASE}/admin/genres/create`) call and at the other listed
locations), ensuring every occurrence of waitForURL(...'generi'...) and
.match(/generi/) is replaced with the corresponding "genres" URL or regex so the
navigation and assertions are consistent.
In `@tests/multi-publisher.spec.js`:
- Around line 248-253: The test uses page.waitForURL(/admin\/publishers/) which
can match the current /admin/publishers/create URL and return prematurely;
update the wait so it only resolves when the list page is reached (e.g.,
restrict the pattern to the list path or exclude /create) inside the block that
handles the confirmation click (locator c and the Promise.all that waits for
waitForURL and c.click()). Locate the Promise.all([page.waitForURL(...),
c.click()]) and replace the permissive regex with a pattern that only matches
the list page (or explicitly rejects /create) so the wait completes after the
real redirect and before the SELECT/assert runs.
---
Outside diff comments:
In `@app/Views/generi/dettaglio_genere.php`:
- Around line 41-45: The view is using HtmlHelper::e(...) which is forbidden by
policy; replace all occurrences of HtmlHelper::e(...) (e.g.
HtmlHelper::e($genereName) and the other instances at the noted spots) with
htmlspecialchars($variable, ENT_QUOTES, 'UTF-8') so escaping is consistent
across the view; update each instance (including where parent name and the other
occurrences) to use htmlspecialchars(..., ENT_QUOTES, 'UTF-8') and ensure no
other HtmlHelper::e calls remain in this template.
In `@tests/full-test.spec.js`:
- Around line 1177-1188: The waitForURL(/admin\/authors/) used after submitting
the author form can match the /create or /edit routes and return too early;
change the wait to target the actual list page (e.g.
waitForURL(`${BASE}/admin/authors`) or a stricter regex that excludes /create
and /edit such as /\/admin\/authors\/?(?!create|edit)/) or replace it with a
stable DOM wait (e.g. waitForSelector for the authors list/table) so the test
only proceeds when the list page is fully loaded; update the same pattern where
used (the other occurrences around the submit flows) so all post-submit waits
use the stricter URL or DOM-based wait.
- Around line 1351-1362: The test is racing after publisher submit because
waitForURL(/admin\/publishers/) also matches the create/edit paths so the test
can continue before the post-submit redirect; update the submit-wait logic in
the block using page.fill / page.locator('button[type="submit"]') /
page.waitForURL to wait for the exact list page (e.g. a URL match that ends with
/admin/publishers or a specific selector present on the list) or use
page.waitForNavigation tied to the submit click; apply the same change to the
other occurrences around lines where similar blocks appear (the blocks starting
at the noted file ranges) so the test reliably waits for the redirect to the
publishers list before proceeding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9fc261c6-c751-41ee-9cf3-e40c308e8734
📒 Files selected for processing (108)
app/Controllers/Admin/RecensioniAdminController.phpapp/Controllers/AutoriController.phpapp/Controllers/CollaneController.phpapp/Controllers/CollocazioneController.phpapp/Controllers/CopyController.phpapp/Controllers/CsvImportController.phpapp/Controllers/EditorsController.phpapp/Controllers/GeneriController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriController.phpapp/Controllers/PrestitiApiController.phpapp/Controllers/PrestitiController.phpapp/Controllers/ReservationsAdminController.phpapp/Controllers/SearchController.phpapp/Controllers/UsersController.phpapp/Routes/web.phpapp/Services/BulkEnrichmentService.phpapp/Support/NotificationService.phpapp/Support/RouteTranslator.phpapp/Views/admin/bulk-enrich.phpapp/Views/admin/csv_import.phpapp/Views/admin/pending_loans.phpapp/Views/admin/plugins.phpapp/Views/admin/reviews/index.phpapp/Views/admin/stats.phpapp/Views/autori/crea_autore.phpapp/Views/autori/index.phpapp/Views/autori/modifica_autore.phpapp/Views/autori/scheda_autore.phpapp/Views/collane/dettaglio.phpapp/Views/collane/index.phpapp/Views/collocazione/index.phpapp/Views/dashboard/index.phpapp/Views/editori/crea_editore.phpapp/Views/editori/index.phpapp/Views/editori/modifica_editore.phpapp/Views/editori/scheda_editore.phpapp/Views/generi/crea_genere.phpapp/Views/generi/dettaglio_genere.phpapp/Views/generi/index.phpapp/Views/layout.phpapp/Views/libri/crea_libro.phpapp/Views/libri/import_librarything.phpapp/Views/libri/index.phpapp/Views/libri/modifica_libro.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpapp/Views/plugins/librarything_admin.phpapp/Views/prenotazioni/crea_prenotazione.phpapp/Views/prenotazioni/index.phpapp/Views/prenotazioni/modifica_prenotazione.phpapp/Views/prestiti/crea_prestito.phpapp/Views/prestiti/dettagli_prestito.phpapp/Views/prestiti/index.phpapp/Views/prestiti/modifica_prestito.phpapp/Views/prestiti/restituito_prestito.phpapp/Views/utenti/crea_utente.phpapp/Views/utenti/dettagli_utente.phpapp/Views/utenti/index.phpapp/Views/utenti/modifica_utente.phpstorage/plugins/archives/ArchivesPlugin.phpstorage/plugins/archives/views/authorities/show.phpstorage/plugins/archives/views/search.phpstorage/plugins/discogs/views/settings.phpstorage/plugins/frbr-lrm/FrbrLrmPlugin.phpstorage/plugins/frbr-lrm/views/admin/opere/show.phpstorage/plugins/z39-server/Z39ServerPlugin.phpstorage/plugins/z39-server/views/reicat/author-fields.phpstorage/plugins/z39-server/views/reicat/book-fields.phptests/admin-features.spec.jstests/bnf-sru-features.spec.jstests/book-form-comprehensive.spec.jstests/book-form-regression-guards.spec.jstests/bulk-enrich.spec.jstests/collane-management.spec.jstests/dewey-cascade-404.spec.jstests/discogs-advanced.spec.jstests/discogs-catno-documents.spec.jstests/discogs-import.spec.jstests/discogs-plugin.spec.jstests/email-notifications.spec.jstests/extra-features.spec.jstests/full-test.spec.jstests/genre-bugs.spec.jstests/genre-merge-rearrange.spec.jstests/goodlib-custom-domains.spec.jstests/import-csv-e2e.spec.jstests/import-librarything-e2e.spec.jstests/issue-153-scaffale-multichar.spec.jstests/issue-74-author-autocomplete.spec.jstests/issue-74-author-selection.spec.jstests/issue-75-issn-series-volumes.spec.jstests/issue-81-audio-player.spec.jstests/issue-83-90-csv-genre.spec.jstests/multi-publisher.spec.jstests/multisource-scraping.spec.jstests/persistent-seed-crud.spec.jstests/pr100-media-types.spec.jstests/pr100-review-fixes.spec.jstests/reicat-sbn.spec.jstests/scraping-form-25.spec.jstests/security-hardening.spec.jstests/seed-catalog.spec.jstests/series-collane-crud.spec.jstests/series-cycles.spec.jstests/smoke-install.spec.jstests/sweetalert2-comprehensive.spec.jstests/unified-release-regression.spec.js
💤 Files with no reviewable changes (1)
- app/Support/RouteTranslator.php
| 'sito_web' => $sitoWeb, | ||
| ]); | ||
| return $response->withHeader('Location', '/admin/editori')->withStatus(302); | ||
| return $response->withHeader('Location', '/admin/publishers')->withStatus(302); |
There was a problem hiding this comment.
Usa l’helper URL nei redirect admin.
Questi Location hardcoded rompono i redirect quando l’app gira sotto un base path diverso dalla root (/pinakes, reverse proxy, sottocartella). Qui il resto del PR usa già url('/admin/...'), quindi questi tre punti restano incoerenti e possono mandare l’utente a un percorso sbagliato.
Diff proposta
- return $response->withHeader('Location', '/admin/publishers')->withStatus(302);
+ return $response->withHeader('Location', url('/admin/publishers'))->withStatus(302);- return $response->withHeader('Location', '/admin/publishers')->withStatus(302);
+ return $response->withHeader('Location', url('/admin/publishers'))->withStatus(302);- return $response->withHeader('Location', '/admin/publishers')->withStatus(302);
+ return $response->withHeader('Location', url('/admin/publishers'))->withStatus(302);As per coding guidelines, "Route: mai hardcodare percorsi URL, usare route_path('key') o RouteTranslator::route('key')".
Also applies to: 129-129, 150-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Controllers/EditorsController.php` at line 96, Replace hardcoded Location
header paths with the URL helper or route translator so redirects respect a
non-root base path: change instances of return $response->withHeader('Location',
'/admin/publishers')->withStatus(302); (and the other two similar returns) to
use url('/admin/publishers') or, preferably per guidelines, route_path('...') /
RouteTranslator::route('...') so the Location header becomes url(...) or
route_path(...) instead of a hardcoded string.
Comment on lines
140
to
142
| $referer = $request->getHeaderLine('Referer'); | ||
| $target = str_contains($referer, '/admin/editori') ? $referer : '/admin/editori'; | ||
| $target = str_contains($referer, '/admin/publishers') ? $referer : '/admin/publishers'; | ||
| return $response->withHeader('Location', $target)->withStatus(302); |
There was a problem hiding this comment.
Non riflettere Referer nel Location senza validarlo.
Qui basta un Referer: https://attacker.example/admin/publishers per passare str_contains() e ottenere un 302 verso un dominio esterno. È un open redirect nel ramo di errore della delete.
Diff proposta
$_SESSION['error_message'] = __('Impossibile eliminare l\'editore: sono presenti libri associati.');
$referer = $request->getHeaderLine('Referer');
- $target = str_contains($referer, '/admin/publishers') ? $referer : '/admin/publishers';
+ $target = url('/admin/publishers');
+ $refererHost = (string) parse_url($referer, PHP_URL_HOST);
+ $refererPath = (string) parse_url($referer, PHP_URL_PATH);
+ $currentHost = $request->getUri()->getHost();
+ if ($refererPath === '/admin/publishers' && ($refererHost === '' || $refererHost === $currentHost)) {
+ $target = $referer;
+ }
return $response->withHeader('Location', $target)->withStatus(302);As per coding guidelines, "Input utente: validare e sanitizzare PRIMA dell'uso".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Controllers/EditorsController.php` around lines 140 - 142, The code
reflects the raw Referer into the redirect Location (variables $referer and
$target using str_contains()) which allows open-redirects; before using
$referer, validate it: parse the URL from $request->getHeaderLine('Referer'),
ensure it is same-origin (host and scheme match your app's expected host/scheme)
or its path is an internal absolute path beginning with '/admin/publishers', and
only then set $target to that safe path; otherwise always fall back to the
internal '/admin/publishers'. Update the logic around $referer, $target,
str_contains() and the response->withHeader('Location', ...) call to enforce
this validation.
Comment on lines
+170
to
174
| return $response->withHeader('Location', '/admin/reservations?updated=1')->withStatus(302); | ||
| } else { | ||
| $stmt->close(); | ||
| return $response->withHeader('Location', '/admin/prenotazioni/modifica/' . $id . '?error=update_failed')->withStatus(302); | ||
| return $response->withHeader('Location', '/admin/reservations/edit/' . $id . '?error=update_failed')->withStatus(302); | ||
| } |
There was a problem hiding this comment.
Mancanza del wrapper url() nei redirect.
Le righe 170 e 173 non utilizzano la funzione helper url() per i redirect, a differenza del pattern corretto applicato in PrestitiController. Senza url(), se l'applicazione è installata in una sottodirectory (es. /pinakes/), questi redirect produrranno errori 404.
🔧 Fix suggerito
- return $response->withHeader('Location', '/admin/reservations?updated=1')->withStatus(302);
+ return $response->withHeader('Location', url('/admin/reservations?updated=1'))->withStatus(302);
} else {
$stmt->close();
- return $response->withHeader('Location', '/admin/reservations/edit/' . $id . '?error=update_failed')->withStatus(302);
+ return $response->withHeader('Location', url('/admin/reservations/edit/' . $id . '?error=update_failed'))->withStatus(302);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return $response->withHeader('Location', '/admin/reservations?updated=1')->withStatus(302); | |
| } else { | |
| $stmt->close(); | |
| return $response->withHeader('Location', '/admin/prenotazioni/modifica/' . $id . '?error=update_failed')->withStatus(302); | |
| return $response->withHeader('Location', '/admin/reservations/edit/' . $id . '?error=update_failed')->withStatus(302); | |
| } | |
| return $response->withHeader('Location', url('/admin/reservations?updated=1'))->withStatus(302); | |
| } else { | |
| $stmt->close(); | |
| return $response->withHeader('Location', url('/admin/reservations/edit/' . $id . '?error=update_failed'))->withStatus(302); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 171-174: The method update uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Controllers/ReservationsAdminController.php` around lines 170 - 174,
Nelle due istruzioni di redirect in ReservationsAdminController (le due
occorrenze che usano $response->withHeader('Location', ...)->withStatus(302))
stai passando percorsi hardcoded; sostituisci quei percorsi con il wrapper url()
(es. usa url('/admin/reservations?updated=1') e url('/admin/reservations/edit/'
. $id . '?error=update_failed')) in modo che i redirect rispettino il prefisso
dell'applicazione quando è in una sottodirectory.
| // Validation | ||
| if ($libroId <= 0 || $utenteId <= 0) { | ||
| return $response->withHeader('Location', '/admin/prenotazioni/crea?error=missing_data')->withStatus(302); | ||
| return $response->withHeader('Location', '/admin/reservations/create?error=missing_data')->withStatus(302); |
There was a problem hiding this comment.
Mancanza del wrapper url() nel redirect.
Stessa problematica: senza url(), il redirect non funzionerà con installazioni in sottodirectory.
🔧 Fix suggerito
- return $response->withHeader('Location', '/admin/reservations/create?error=missing_data')->withStatus(302);
+ return $response->withHeader('Location', url('/admin/reservations/create?error=missing_data'))->withStatus(302);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Controllers/ReservationsAdminController.php` at line 227, Il redirect usa
un path hardcoded che non funziona in installazioni in sottodirectory;
sostituisci la stringa '/admin/reservations/create?error=missing_data' con la
chiamata al wrapper url(), cioè usa
url('/admin/reservations/create?error=missing_data') nell'espressione che
ritorna $response->withHeader('Location', ...)->withStatus(302) (nella classe
ReservationsAdminController, dove compare la return con withHeader('Location',
...)).
Comment on lines
+299
to
303
| return $response->withHeader('Location', '/admin/reservations?created=1')->withStatus(302); | ||
| } else { | ||
| $stmt->close(); | ||
| return $response->withHeader('Location', '/admin/prenotazioni/crea?error=save_failed')->withStatus(302); | ||
| return $response->withHeader('Location', '/admin/reservations/create?error=save_failed')->withStatus(302); | ||
| } |
There was a problem hiding this comment.
Mancanza del wrapper url() nei redirect.
Righe 299 e 302: stesso problema di base path mancante.
🔧 Fix suggerito
- return $response->withHeader('Location', '/admin/reservations?created=1')->withStatus(302);
+ return $response->withHeader('Location', url('/admin/reservations?created=1'))->withStatus(302);
} else {
$stmt->close();
- return $response->withHeader('Location', '/admin/reservations/create?error=save_failed')->withStatus(302);
+ return $response->withHeader('Location', url('/admin/reservations/create?error=save_failed'))->withStatus(302);🧰 Tools
🪛 PHPMD (2.15.0)
[error] 300-303: The method store uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Controllers/ReservationsAdminController.php` around lines 299 - 303, In
ReservationsAdminController locate the redirects that call
$response->withHeader('Location', '...') (the success and error branches that
currently use '/admin/reservations?created=1' and
'/admin/reservations/create?error=save_failed') and replace the hardcoded path
strings with the application's url() helper so the base path is preserved; i.e.,
pass the url(...) result into withHeader('Location', ...) for both the success
and failure redirects and keep the withStatus(302) unchanged.
| </h1> | ||
| <p class="text-gray-600 text-base mb-4"> | ||
| <?= __("Aggiorna i dettagli del libro:") ?> <a href="<?= htmlspecialchars(url('/admin/libri/' . (int)($book['id'] ?? 0)), ENT_QUOTES, 'UTF-8') ?>" class="text-blue-600 hover:text-blue-800 hover:underline font-semibold transition-colors"><strong><?php echo HtmlHelper::e($book['titolo'] ?? ''); ?></strong></a> | ||
| <?= __("Aggiorna i dettagli del libro:") ?> <a href="<?= htmlspecialchars(url('/admin/books/' . (int)($book['id'] ?? 0)), ENT_QUOTES, 'UTF-8') ?>" class="text-blue-600 hover:text-blue-800 hover:underline font-semibold transition-colors"><strong><?php echo HtmlHelper::e($book['titolo'] ?? ''); ?></strong></a> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Sostituisci HtmlHelper::e() in questa view.
A Line 47 il titolo nel link usa ancora HtmlHelper::e(), ma nelle view il progetto richiede htmlspecialchars(..., ENT_QUOTES, 'UTF-8').
🔧 Diff proposta
- <?= __("Aggiorna i dettagli del libro:") ?> <a href="<?= htmlspecialchars(url('/admin/books/' . (int)($book['id'] ?? 0)), ENT_QUOTES, 'UTF-8') ?>" class="text-blue-600 hover:text-blue-800 hover:underline font-semibold transition-colors"><strong><?php echo HtmlHelper::e($book['titolo'] ?? ''); ?></strong></a>
+ <?= __("Aggiorna i dettagli del libro:") ?> <a href="<?= htmlspecialchars(url('/admin/books/' . (int)($book['id'] ?? 0)), ENT_QUOTES, 'UTF-8') ?>" class="text-blue-600 hover:text-blue-800 hover:underline font-semibold transition-colors"><strong><?= htmlspecialchars((string)($book['titolo'] ?? ''), ENT_QUOTES, 'UTF-8') ?></strong></a>As per coding guidelines "Mai usare HtmlHelper::e() nelle view — usare htmlspecialchars(..., ENT_QUOTES, 'UTF-8')".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <?= __("Aggiorna i dettagli del libro:") ?> <a href="<?= htmlspecialchars(url('/admin/books/' . (int)($book['id'] ?? 0)), ENT_QUOTES, 'UTF-8') ?>" class="text-blue-600 hover:text-blue-800 hover:underline font-semibold transition-colors"><strong><?php echo HtmlHelper::e($book['titolo'] ?? ''); ?></strong></a> | |
| <?= __("Aggiorna i dettagli del libro:") ?> <a href="<?= htmlspecialchars(url('/admin/books/' . (int)($book['id'] ?? 0)), ENT_QUOTES, 'UTF-8') ?>" class="text-blue-600 hover:text-blue-800 hover:underline font-semibold transition-colors"><strong><?= htmlspecialchars((string)($book['titolo'] ?? ''), ENT_QUOTES, 'UTF-8') ?></strong></a> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Views/libri/modifica_libro.php` at line 47, Sostituisci l'uso di
HtmlHelper::e(...) nella view con htmlspecialchars(..., ENT_QUOTES, 'UTF-8'); in
particolare modifica l'espressione HtmlHelper::e($book['titolo'] ?? '') usata
nel link (vicino a url('/admin/books/' . (int)($book['id'] ?? 0))) per usare
htmlspecialchars($book['titolo'] ?? '', ENT_QUOTES, 'UTF-8') così da rispettare
la regola "Mai usare HtmlHelper::e() nelle view".
Comment on lines
+1521
to
1528
| await page.goto(`${BASE}/admin/books/importa`); | ||
| await page.waitForLoadState('domcontentloaded'); | ||
| await expect(page.locator('body')).not.toBeEmpty(); | ||
| }); | ||
|
|
||
| test('10.2 Upload CSV file', async () => { | ||
| await page.goto(`${BASE}/admin/libri/importa`); | ||
| await page.goto(`${BASE}/admin/books/importa`); | ||
| await page.waitForLoadState('domcontentloaded'); |
There was a problem hiding this comment.
La route di import è rimasta in italiano.
/admin/books/importa contraddice la migrazione “admin routes in English”. Se il router è stato rinominato coerentemente, questa fase andrà in 404 mentre il resto della suite usa già slug inglesi. Allinea i goto() alla route inglese dell’import.
Also applies to: 1556-1557
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/full-test.spec.js` around lines 1521 - 1528, The tests call the Italian
route "/admin/books/importa" causing 404s after migrating admin routes to
English; update all page.goto() usages (e.g., the call inside the "10.2 Upload
CSV file" test and the other occurrence around the noted lines) to the English
route "/admin/books/import" so the tests navigate to the correct import page.
Comment on lines
+248
to
+253
| await page.goto(`${BASE}/admin/publishers/create`); | ||
| await page.fill('input[name="nome"]', existing); | ||
| await page.click('button[type="submit"]'); | ||
| const c = page.locator('.swal2-confirm').first(); | ||
| if (await c.isVisible({ timeout: 4000 }).catch(() => false)) { | ||
| await Promise.all([page.waitForURL(/admin\/editori/, { timeout: 10000 }), c.click()]).catch(() => {}); | ||
| await Promise.all([page.waitForURL(/admin\/publishers/, { timeout: 10000 }), c.click()]).catch(() => {}); |
There was a problem hiding this comment.
Anche qui waitForURL(/admin\/publishers/) può completarsi subito.
Durante il seed sei già su /admin/publishers/create, quindi la wait si soddisfa prima del redirect reale e la SELECT successiva può leggere prima che l’insert sia visibile. Restringi il pattern alla list page o escludi /create.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/multi-publisher.spec.js` around lines 248 - 253, The test uses
page.waitForURL(/admin\/publishers/) which can match the current
/admin/publishers/create URL and return prematurely; update the wait so it only
resolves when the list page is reached (e.g., restrict the pattern to the list
path or exclude /create) inside the block that handles the confirmation click
(locator c and the Promise.all that waits for waitForURL and c.click()). Locate
the Promise.all([page.waitForURL(...), c.click()]) and replace the permissive
regex with a pattern that only matches the list page (or explicitly rejects
/create) so the wait completes after the real redirect and before the
SELECT/assert runs.
… rename (#145) The path-prefix rename only touched /admin/-prefixed strings, so bare route fragments in test selectors/assertions survived and broke against the renamed routes (the app views were already correct): - full-test/extra-features Phase 13: form[action*="scaffali"] / [action*="mensole"] → shelving-units / shelves. The old selectors matched nothing → click timed out (2m) → CI E2E failure. (DB table names scaffali/mensole in SQL left untouched.) - genre-merge-rearrange / genre-bugs: waitForURL(/generi\/\d+/) and url().match(/generi\//) admin-genre URL patterns → genres. The /api/generi endpoints (out of the admin rename scope) are deliberately left as-is.
…ments (#145) - genre-merge-rearrange "merge two genres": the merge form confirms via SweetAlert (window.SwalApp.confirm), not a native confirm() dialog, so the old page.on('dialog') handler never fired and the form never submitted → 2m timeout. Click the .swal2-confirm modal button instead. Now 4/4 green. - form[action*="/elimina"] cleanup selectors → /delete (genre delete route), and admin book-detail URL matchers /libri\/\d+/ → /books\/\d+/. The /api/generi endpoints (out of the admin-rename scope) are intentionally left as-is. Note: genre-bugs' "genre autocomplete in admin book list" is a pre-existing timing-flaky test (identical on main, uses correct routes) and is not part of the CI E2E set.
fabiodalez-dev
added a commit
that referenced
this pull request
Jun 4, 2026
…nu hook, CodeRabbit fixes Hook system: - ScrapeController now emits scrape.data.modify on the final payload in both the plugin-result and built-in fallback branches (before scrape.response), so plugin enrichment (e.g. discogs cover fill) no longer depends on the scraping-pro plugin being the active scraper. enrichWithDiscogsData is idempotent, so a double-fire when scraping-pro is also active is safe. - z39-server: remove the dead admin.menu.items registration + addAdminMenuItem method. The core only emits admin.menu.render (an echo action, see archives); admin.menu.items was never fired, so the sidebar entry never rendered. z39 settings stay reachable via the standard plugin manager like every other plugin. Loan/reservation correctness (review findings): - ReservationManager::processBookAvailability now bails when the FOR UPDATE on libri returns no row (soft-deleted/absent book), instead of converting a reservation for a title removed from the catalogue. - updateQueuePositions decrements only positions strictly after the promoted reservation; the converted reservation is the lowest date-eligible one, not necessarily queue_position=1, so the old >1 decrement could collide positions. - NotificationService::notifyWishlistBookAvailability re-checks copie_disponibili>0 so a copy re-absorbed before the call no longer triggers false-positive emails. - MaintenanceService: instantiate ReservationReassignmentService inside each loop so a rolled-back iteration cannot leak buffered notifications into the next iteration's flush. - cron/full-maintenance: include expired_waitlist_reservations in the action total. - Updater: log the ignorable-SQL DEBUG line only when the error is actually ignorable. Base-path: - Wrap admin Location redirects in url() across the admin controllers (admin routes stay English literals per #145/#161; url() only fixes the base path for sub-folder installs). Sweep covers reservations, autori, editori, generi, eventi, csv-import, plugin, settings, theme, users. Tests: - tests/scrape-hooks-z39.unit.php: 15 assertions covering the scrape.data.modify emission (both branches, ordering, live isbn.validate, discogs idempotency) and the z39 dead-hook removal. - loan-reservation-complete F.21b: assert the queue HEAD is the promoted user. Validated: PHPStan level 5 clean, 15/15 unit, loan-reservation-complete 26/26, loan-overlap 35/35.
fabiodalez-dev
added a commit
that referenced
this pull request
Jun 4, 2026
…e, idempotent seeds) Code: - ScrapeController: fire scrape.data.modify BEFORE the normalizers (normalizeScrapedData / ensureTipoMedia / normalizeIsbnFields) in both the plugin-result and fallback branches, so a field a plugin sets in that hook (tipo_media, format, ISBN) is normalized instead of bypassing it. - UserActionsController::loan: lock the utenti row (SELECT ... FOR UPDATE) before the max_active_loans_per_user COUNT+INSERT, serializing concurrent same-user requests on different books that could otherwise both pass the limit check. - cron/full-maintenance: catch \Throwable (not \Exception) in the top-level handler so engine errors hit the error/cleanup path. - data_it_IT.sql: make the three loan-setting seeds idempotent with ON DUPLICATE KEY UPDATE, matching data_de_DE.sql / data_fr_FR.sql. i18n: - crea_prenotazione help text now interpolates the configured loan duration via sprintf(%d) instead of a hardcoded '+30 giorni'; key updated in all 4 locales. Docs: - libri.MD: strip trailing whitespace (git diff --check clean). - api.MD: language-tag + blank-line the Loan States fenced block (MD031/MD040). - frontend/README.md: fix case-sensitive links api.md->api.MD, installation.md->installation.MD. - utenti.md: blank line after heading (MD022). Skipped (with reason): redirect route_path suggestions contradict the admin-routes-are-English-literals decision (#145/#161) — url() already fixes the base path; CopyRepository pendente concern is a false positive (the copia_id join excludes bare pendings; cross-layer agreement covered by loan-overlap 35/35). Validated: PHPStan L5 clean, unit 15/15, discogs-import 6/6, loan-reservation-complete 26/26.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #145. Makes the admin area fully English: every Italian
/admin/*route segment is renamed to an English literal, with 301 redirects from the old paths so nothing breaks.Decision
Admin routes are English literals, intentionally NOT i18n — they never go through
route_path()/RouteTranslator(that system is user-facing only). The route-translation system already excludes admin; the one latent hook (RouteTranslator'spluginsfallback +route_path('plugins')callers) is neutralized tourl('/admin/plugins').What changed
web.php): libri→books, prestiti→loans (merged into the single/admin/loansnamespace alongside the approval workflow), utenti→users, collane→series, autori→authors, generi→genres, editori→publishers, prenotazioni→reservations, collocazione→placement, recensioni→reviews, statistiche→statistics; action segments crea→create, modifica→edit, dettagli→details, dettaglio→detail, restituito→returned, rinnova→renew, unisci→merge, rinomina→rename, mensole→shelves, scaffali→shelving-units, copie→copies, volumi→volumes.admin_notifications.linkrows hold old IT paths and ride these redirects — a future removal needs a data migration.Review
Ran an adversarial multi-lens review on the diff. It caught three rename-corruption P1s a plain residual-grep missed (corrupted English, not leftover Italian) and two P2s — all fixed in the final commit:
detailso(dettagli-before-dettaglio ordering) → series detail 404createte(crea matching inside create) → events/languages create 404 + a fatal require/admin/reviewsfatal — view dir not renamed (recensioni→reviews)crea-operamissing from the redirect action map;route_path('plugins')fallback regressionVerification
/admin/libri/modifica/5→/admin/books/edit/5)./admin/prefix constrains it).Merge order
Touches
web.php/controllers/tests heavily — please merge after #157 (loans) and #160 (private mode); conflicts resolved at merge time.Summary by CodeRabbit