feat(interop): v0.7.5 — French locale (fr_FR), BNF SRU/UNIMARC, all interop plugins (OAI-PMH, NCIP, Z39.50, VIAF, BIBFRAME, ResourceSync)#132
Open
fabiodalez-dev wants to merge 101 commits into
Open
Conversation
…bels (#123, #121) - Add iiif_manifest_url VARCHAR(2000) column to archival_units (idempotent migration) - GET /admin/archives/{id}/manifest.json and /archives/{id}/manifest.json return IIIF Presentation API 3.0 JSON with metadata crosswalk, seeAlso links to DC/OAI-PMH, and a Canvas for locally stored cover images; CORS header for external viewers - iiif_manifest_url field in edit form links to external IIIF server manifests - <link rel="alternate"> for manifest.json added to admin show and public detail pages - form.php: wrap fields in ISAD(G) area section headers (Area 1 Identity Statement, Area 3 Content & Structure, Area 4 Conditions of Access) matching AtoM presentation
…tion endpoint
- iiifManifestAction: real canvas width/height via getimagesize() (fallback 1500×2000)
- iiifManifestAction: partOf link to parent Collection or root collection.json
- iiifManifestAction: structures[] with ancestor breadcrumb Range (IIIF TOC)
- iiifBuildAncestorChain(): walk parent_id chain to build Fondo › Serie › … path
- iiifCollectionAction(): GET /archives/collection.json (all top-level fondi)
and GET /archives/{id}/collection.json (direct children as Manifest or Collection)
— child units with children become sub-Collections, leaf units stay Manifests
- Added two public routes for Collection endpoints (CORS header included)
…authority links - behavior: viewer UX hint (paged/individuals/unordered) derived from specific_material - requiredStatement + provider: institution attribution as IIIF Agent (machine + human) - rights: new rights_statement_url column (VARCHAR 500) exposed as manifest['rights'] - structures: replace flat breadcrumb with nested Range hierarchy (§1.1) — each ancestor wraps the next as an outer Range; innermost Range holds Canvas items - authority links: creator/subject names + VIAF/Wikidata URIs from external_refs added to manifest metadata - form: rights_statement_url field in ISAD(G) Area 4 (Conditions of Access) - new private helpers: iiifBuildNestedStructures(), iiifFetchAuthoritiesWithRefs()
…oints
- ARK identifier (ark_identifier VARCHAR 255): surfaced in IIIF seeAlso via
n2t.net resolver, EAD3 <recordid>, Dublin Core dc:identifier, form Area 1
- Version note (version_note VARCHAR 500): shown in IIIF metadata, form
- EAD3 per-unit: GET /archives/{id}/ead.xml (public + admin) — single-unit
finding aid from existing writeEad3Document; adds <dao> IIIF manifest link
and optional cover image to the bulk exporter as well
- METS per-unit: GET /archives/{id}/mets.xml (public + admin) — wraps DC
inline + EAD3 by reference + IIIF manifest link; suitable for Europeana
submission; covers fileSec with thumbnail if cover image present
- Dublin Core: adds ark as second dc:identifier, rights_statement_url as dc:rights
- IIIF seeAlso: adds EAD3, METS, ARK entries alongside existing DC and OAI-PMH
- headLinks: EAD3 and METS link rel="alternate" in admin and public show views
…erop standards Covers all new fields (ark_identifier, version_note, rights_statement_url), IIIF 3.0 advanced features (behavior, nested Ranges, requiredStatement, provider, rights, authority links), per-unit EAD3 and METS exports, headLinks, and ISAD(G) form labels validation.
…new fields in show.php version_note and iiif_manifest_url are record-level metadata (not material-specific) — moved outside the collapsible <details> accordion so they are always visible. Added ark_identifier, rights_statement_url, version_note display to admin show view. Fixed E2E test 8 to open the accordion before selecting specific_material. All 25 interop E2E tests now pass.
… seed for new fields - tests/seeds/librarything-aldebaran.tsv: 18 Aldebaran volumes (4 cycles) to test LibraryThing series import with collana/numero_serie parsing - tests/seeds/books-general.csv: 15 classic books for standard CSV import - tests/seeds/books-seed.sql: idempotent SQL seed (INSERT IGNORE) for fast setup; covers all 33 books, authors, publishers, series, and libri_collane links - tests/seeds/archives-feature-20.sql (v2): adds ark_identifier, rights_statement_url, version_note to temp table and INSERT/ON DUPLICATE KEY - .gitignore: allow *.csv and *.tsv in tests/seeds/
P1 — fresh-install schema integrity:
- Add 4 missing columns to ddlArchivalUnits() DDL (iiif_manifest_url,
rights_statement_url, ark_identifier, version_note) so a fresh CREATE TABLE
includes all interoperability-standards columns without needing ALTER
- Move migrateImageColumns() call to AFTER the CREATE TABLE foreach loop so
the table exists before the additive ALTER migrations attempt to run
P2 — language-code parsing:
- iiifManifestAction(): use preg_split('/[,;\s]+/', ...) instead of
explode(',', ...) so seed values like 'ita;eng' are correctly split
- Dublin Core output: replace single compound writeElementNs with a loop that
emits one dc:language element per code (RFC 4646 / MARC 21 compliant)
…-up) The seed archives-feature-20.sql inserts iiif_manifest_url, rights_statement_url, ark_identifier, and version_note, but the migration that archives-feature-documents.spec.js applies on a clean DB stopped at document_filename. On a fresh install with the migration- only path (no ensureSchema() call), the seed would fail with 'Unknown column'. Add four idempotent ALTER TABLE blocks following the same pattern as all other additive columns in the migration.
cleanupTag (spec) — FK-safe deletion order: delete archival_unit_authority rows first via JOIN, then archival_units children (parent_id IS NOT NULL) then roots; avoids MySQL 1093 from self-referential NOT EXISTS subquery. show.php — validate rights_statement_url scheme before rendering as <a href>: only http/https URLs get a clickable link; anything else (incl. javascript:) renders as plain escaped text, preventing href injection. ArchivesPlugin — IIIF 3.0 §3.3 compliance: manifest items must have cardinality ≥ 1; when no local cover image is present, add a minimal placeholder Canvas so clients receive a valid (if unpainted) manifest. ArchivesPlugin — EAD3 schema fix: move <daoset> block inside <did> (before its closing tag) where EAD3 requires it; the block was previously emitted after </did>, violating the EAD3 DTD. form.php — add rel="noopener noreferrer" to external rightsstatements.org link that uses target="_blank" (prevents tabnabbing/opener leakage).
ScrapeController::byIsbn() now calls session_write_close() before
hitting Discogs/OpenLibrary. PHP file-based sessions hold an exclusive
lock for the entire request duration, causing subsequent page.goto()
calls in Playwright (or any same-session request) to block until the
slow external fetch completes — typically 10–30 s, long enough to
cascade-fail all subsequent serial tests.
Also hardens seed-catalog.spec.js: replaces waitForTimeout(8000) with
waitForLoadState('networkidle'), adds trySave() helper that logs a
warning instead of throwing on save failure (seeder resilience), and
clears isbn13/isbn10 before fallback fill to avoid unique-constraint
violations.
ci-quality.yml (new): - PHP syntax check with xargs -P4 on app/, config/, public/, installer/, server/ - PHPStan level 5 (mirrors pre-commit hook in CI for PRs from forks or CI-only runs) - Composer security audit against Packagist CVE database - Triggers on any *.php / composer.json / phpstan.neon change codeql.yml (new): - CodeQL security-and-quality queries for PHP and JavaScript - Detects SQL injection, XSS, path traversal, CSRF, open redirect - Runs on push/PR to main + weekly schedule test-migrations.yml (updated): - Added trigger on version.json changes - Job verify-versions: checks every migrate_*.sql has version ≤ version.json; catches the silent-skip bug (updater guards with version_compare <=) - Job test-full-chain: applies schema.sql + all migrations in sort -V order, then asserts 5 expected tables and 6 expected columns from 0.5.x/0.5.9.x migrations are present - Existing job test-migrations (0.4.0 upgrade path) preserved unchanged
…ckout@v6 PHPStan installed via setup-php tools: doesn't auto-discover phpstan.neon when run as a global binary in some 2.x versions, causing "At least one path must be specified to analyse". Passing --configuration makes it explicit and version-agnostic. Also upgrades actions/checkout v4→v6 and actions/cache v4→v5 across all three workflows to suppress the Node.js 20 deprecation warning (Node 24 becomes default June 2026).
Adds E2E_DB_HOST and E2E_DB_PORT env vars to mysqlArgs() in archives-seed-50, archives-interop-standards and full-test specs. Prioritises TCP (host+port) over socket when DB_HOST is set, enabling remote DB access via SSH wrapper for production smoke-tests.
Both files were gitignored, so GitHub Actions could not find the PHPStan config file during CI runs (exit 1: "does not exist"). Remove them from .gitignore so the runner picks them up on checkout.
v3 (toolset 2.25.3) does not recognize 'php' as a language, causing the Analyze (php) job to fail. v4 restores PHP support and removes the Node.js 20 deprecation warnings.
The /server/ directory is gitignored and absent in CI, causing PHPStan to abort with "Path does not exist". No analyzed code in app/ or plugins references classes from server/src, so the entry is safe to remove.
PHP 8.2 constants like CURLOPT_REDIR_PROTOCOLS_STR were reported as 'not found' because phpVersion was set to 80100. The CI and production server both run PHP 8.2; aligning the phpstan target version prevents false positives on 8.2-only constants.
With phpVersion bumped to 80200, PHPStan resolves CURLOPT_REDIR_PROTOCOLS_STR and collapses the old array-shape offset errors. Regenerated baseline now captures only the two remaining legitimate suppressions.
PHPStan 2.1.54 (installed in CI via tools:phpstan) reports: - CURLOPT_REDIR_PROTOCOLS_STR not found: constant is PHP 8.2 but absent from PHPStan 2.1.54 stubs; guarded by defined() at runtime - isset.offset for traduttore/illustratore: stricter array-shape narrowing in 2.1.54 vs 2.1.40 (reportUnmatchedIgnoredErrors=false keeps these baseline entries harmless on older local versions)
thepixeldeveloper/sitemap is abandoned (no replacement available) — the audit exits 2 by default. --abandoned=report logs it without failing the pipeline.
CodeQL CLI 2.25.x does not recognize 'php' as a language, causing the Analyze (php) job to fail. PHP security is already covered by PHPStan level 5. Retain only javascript-typescript CodeQL analysis.
- tests/archives-interop-standards.spec.js: add escapeLike() helper and use
ESCAPE '\\' in cleanupTag() LIKE queries to prevent SQL wildcard expansion
on underscores in TAG (critical fix); remove -p${DB_PASS} from argv and
pass password via MYSQL_PWD env var instead; fix test 12 n2t.net check
to use startsWith() instead of includes(); refactor test 25 to use
Playwright locators instead of page.content(); add Number.isInteger()
guard in afterAll cleanup
- tests/archives-seed-50.spec.js: same MYSQL_PWD argv fix
- tests/full-test.spec.js: same MYSQL_PWD argv fix
- .github/workflows/test-migrations.yml: add workflow file to PR paths;
replace || true with targeted idempotency handler that propagates
unexpected errors; add rights_statement_url to column count (6→7)
- installer/database/migrations/migrate_0.5.9.7.sql: add UNIQUE KEY on
archival_units.ark_identifier for upgrade paths
- storage/plugins/archives/ArchivesPlugin.php: add URL/length validation
for iiif_manifest_url, rights_statement_url, ark_identifier, version_note
in validateArchivalUnit(); add app-level ARK uniqueness check using
excludeId to allow in-place updates; add UNIQUE KEY uq_ark_identifier to
ensureSchema() CREATE TABLE; fix O(n) per-child queries in
iiifCollectionAction() by computing has_children via EXISTS() subquery in
the initial SELECT
- storage/plugins/archives/views/form.php: wrap placeholder in $e(); wrap
parent_id help text in __()/$e() for i18n consistency
- app/Controllers/ScrapeController.php: add comment after session_write_close()
warning plugin authors not to write to $_SESSION in Hook callbacks
- app/Support/MaintenanceService.php: add expired_pickups? to runIfNeeded()
return type shape to fix PHPStan nullCoalesce.offset warning properly
- phpstan-baseline.neon: remove expired_pickups suppression (now fixed in
the type annotation)
- tests/seeds/archives-feature-20.sql: version_note TEXT → VARCHAR(500) to
match archival_units column type and prevent silent truncation
- migrate_0.5.9.7.sql: add deduplication pre-check before UNIQUE KEY (NULL-out duplicate ARKs keeping earliest row, guarded by idx_exists) - phpstan-baseline.neon: remove 3 stale suppressions (traduttore, illustratore, CURLOPT_REDIR_PROTOCOLS_STR); keep booleanNot.alwaysTrue which is a real PHPStan finding in QueryCache while-loop - ArchivesPlugin validateArchivalUnit(): remove AND deleted_at IS NULL from ARK uniqueness check to align with DB UNIQUE KEY semantics - ArchivesPlugin iiifCollectionAction(): restrict root collection to level='fonds' only (parent_id IS NULL was too broad) - tests: (body['seeAlso'] || []).find(...) guard against absent seeAlso; 'Content & Structure' selector instead of ambiguous 'Content'
The migration renames the typo column classificazione_dowey→classificazione_dewey. On fresh schema installs the correct column already exists, so CHANGE COLUMN fails with 'Unknown column' — not in the CI tolerated-error list. Wrap with INFORMATION_SCHEMA guard (same pattern as migrate_0.5.9.7.sql).
…0.5.9.7 upgrades migrate_0.5.9.sql has version 0.5.9 which is older than the released 0.5.9.6, so it never runs when upgrading from 0.5.9.x. This caused Test B of the reinstall test to fail with 'archival_units doesn't exist'. The fix merges the full archives DDL (CREATE TABLE IF NOT EXISTS + idempotent column additions + plugin row) into migrate_0.5.9.7.sql so that any upgrade path from before 0.5.9.7 creates all archives tables in a single idempotent pass. Also move the book-type badge above the <h1> on the frontend book-detail page and set an absolute font-size (0.7rem) so it renders small regardless of heading size.
…0.5.9.7 upgrades migrate_0.5.9.sql has version 0.5.9 which is older than released 0.5.9.6, so it never runs when upgrading from 0.5.9.x. Confirmed by reinstall-test.sh Test B: upgrade from v0.5.9.6 failed with 'archival_units doesn't exist'. Merged the full archives DDL into this file: CREATE TABLE IF NOT EXISTS for all 4 tables, idempotent column additions via INFORMATION_SCHEMA guards, plugin row upsert, and the original dedup+UNIQUE KEY — so any pre-0.5.9.7 upgrade path gets all archives tables in a single idempotent pass.
PHPStan baseline: - Restore 3 suppressed entries removed erroneously: CURLOPT_REDIR_PROTOCOLS_STR (PHP 8.1 compatibility), traduttore/illustratore isset.offset in BookRepository migrate_0.5.9.7.sql: - Fix plugin version 1.0.0 → 1.1.0 to match storage/plugins/archives/plugin.json - Use full extended ENUM for specific_material ADD COLUMN (avoids redundant MODIFY) - Emit dedup count as migration_notice before nulling duplicate ARKs ArchivesPlugin.php: - dc.xml headLinks/seeAlso: application/rdf+xml → application/xml (OAI-DC is not RDF/XML; wrong MIME breaks parsers) - validateArchivalUnit: normalize resolver URLs (https://n2t.net/ark:/...) to canonical ark:/... form and reject non-ARK strings with a clear error message - migrateImageColumns: add idempotent guard for uq_ark_identifier UNIQUE KEY so in-place upgrades get the same DB-level uniqueness as fresh installs tests/archives-interop-standards.spec.js: - Test 20 EAD3 bulk export: remove manual Cookie forwarding (page.request already shares the browser context's storage)
…nded enum + fixes)
… dev schema detected Pre-release dev builds used source_code/source_id column names. The current migration expects source/authority_id. CREATE TABLE IF NOT EXISTS skipped the DDL on installations that had the dev table, causing ADD KEY idx_authority (source, authority_id) to fail with 'Key column source does not exist'. Add a guard that detects the old schema via source_code column presence and drops the empty dev table before recreating with the correct schema.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const { execFileSync } = require('child_process'); | ||
|
|
||
| const DB_HOST = process.env.E2E_DB_HOST || ''; | ||
| const DB_PORT = process.env.E2E_DB_PORT || ''; |
| return args; | ||
| } | ||
|
|
||
| function dbQuery(sql) { |
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| const BASE = process.env.E2E_BASE_URL || 'http://localhost:8081'; |
|
Important Review skippedToo many files! This PR contains 163 files, which is 13 over the limit of 150. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (163)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
…sue #130) On Windows, RecursiveDirectoryIterator::getPathname() returns backslash paths, breaking str_replace prefix-stripping and mkdir target construction in copyDirectoryRecursive() and copyDirectory(). All paths now normalized to forward slashes at function entry; realpath() returns also normalized. Also remove unused SEED_FILES variable from install-compat-standard.spec.js (CodeQL js/unused-local-variable #20) and harden test selectors.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…ition archives-upload-assets.spec.js ensureUploadFixtures(): - Remove existsSync+writeFileSync TOCTOU pattern (race condition fix) - Replace hardcoded /tmp/ paths with path.join(os.tmpdir(), ...) for cross-platform safety and to satisfy CodeQL js/insecure-tmp rule
__() uses Italian as the native fallback — keys ARE Italian text, so if a key is missing from it_IT.json the function returns the key itself. it_IT.json is intentionally sparse (overrides only). Only en_US ↔ de_DE requires exact key-set parity.
CodeQL js/insecure-temporary-file:
- Add binary fixtures to tests/fixtures/ (jpg/pdf/mp3, csv/tsv) and
allow the directory in .gitignore (mirrors tests/seeds/ pattern)
- archives-upload-assets.spec.js reads from tests/fixtures/ instead of
writing predictable filenames to os.tmpdir()
OAI-PMH:
- Catch remaining \Throwable in record serialisation loop to skip
malformed metadata with a warning log instead of crashing
E2E test stability:
- bibframe-persistent-uri: check @id contains /id/work/{id}, not suffix /work
- book-form-comprehensive: simplified Choices.js check (no XPath)
- bulk-enrich: nullify ISBN before DELETE to avoid UK constraint on re-run
- multisource-scraping: accept 'Z39.50/SRU' as valid source for Nevermind
- smoke-install: make author input fill conditional (visible guard)
Owner
Author
Code reviewBranch: Found 29 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (3)
Details and fix proposalsF002 — In
|
| # | Score | Impact | File | Issue |
|---|---|---|---|---|
| F031 | 48 | security | storage/plugins/oai-pmh-server/OaiPmhServerPlugin.php:2497-2524 |
requireAdminForDownload() accepts HTTP Basic Auth credentials in-band over potentially unencrypted HTTP connections, transmitting admin passwords in cleartext if TLS is not enforced at the infrastructure level. |
| F032 | 58 | correctness | app/Controllers/FrontendController.php:830-890 |
FrontendController catalog search builds LIKE patterns by wrapping $wordBase in '%...' without escaping literal % and _ wildcards, unlike AutoriApiController and CollaneController which use str_replace escaping — users searching for underscore or percent get wrong matches on the main book catalog. |
Phase 4 couldn't confirm decisively. Re-run /adamsreview:review if you suspect this deserves
further investigation with fresh context.
Fix runs
Run fixrun_01KRBB2ZT91936NP8ZB0X076FB — 2026-05-11T11:17:54Z
- Outcomes: 9 fixed and verified
- Commits:
6a93828
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F002 | FG-1 | ✓ fixed and verified | |
| F003 | FG-2 | ✓ fixed and verified | |
| F005 | FG-3 | ✓ fixed and verified | |
| F007 | FG-1 | ✓ fixed and verified | |
| F010 | FG-4 | ✓ fixed and verified | |
| F020 | FG-5 | ✓ fixed and verified | |
| F023 | FG-6 | ✓ fixed and verified | |
| F027 | FG-7 | ✓ fixed and verified | |
| F028 | FG-8 | ✓ fixed and verified |
🤖 Generated with Adam's Claude Code Review Command
Owner
Author
Auto-recommendation acceptance
6 auto-rec finding(s) eligible at threshold ≥ 60. Of those, 6 auto-promoted via batch, 0 promoted with an edited or alternative hint, 0 skipped per-finding. Promoted (batch)
Auto-recommendation acceptance: append-only audit. Each |
Fix groups (committed): - [FG-1] F002, F007 — storage/plugins/archives/ArchivesPlugin.php: verified - [FG-2] F003 — storage/plugins/bibframe-linked-data/BibframeLinkedDataPlugin.php: verified - [FG-3] F005 — storage/plugins/oai-pmh-server/OaiPmhServerPlugin.php: verified - [FG-4] F010 — app/Controllers/FrontendController.php: verified - [FG-5] F020 — storage/plugins/oai-pmh-server/views/book-digital-assets.php: verified - [FG-6] F023 — storage/plugins/archives/views/authorities/show.php: verified - [FG-7] F027 — app/Views/admin/plugins.php: verified - [FG-8] F028 — storage/plugins/archives/views/show.php: verified Post-fix review: 8/8 groups verified complete; 0 group(s) partial; 0 group(s) reverted.
| const TEST_AUDIO = path.join(os.tmpdir(), 'archive-test-audio.wav'); | ||
|
|
||
| function ensureAudioFixture() { | ||
| fs.writeFileSync(TEST_AUDIO, Buffer.from([ |
Owner
Author
Code reviewBranch: Found 47 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (8)
Cross-cutting group G1: F024 + F025 — Both findings are the same soft-delete invariant violation in the same file (NcipServerPlugin.php), targeting two UPDATE libri statements in adjacent transactional flows (createLoanAtomic at line 1212 and closeLoan at line 1323). They must be patched together in a single edit pass to ensure the loan create/close lifecycle remains consistent and both WHERE clauses are updated atomically. Details and fix proposalsF003 — errorResponse() builds child elements with createElement() while all other response methods use createElementNS(NS_SRU,...) — error responses will have children in no namespaceFile: Evidence:
Approach: Add $ns = self::NS_SRU; and replace every createElement() child with createElementNS($ns, ...) in errorResponse(), mirroring the FIX 1 / FIX 1b pattern in sibling methods. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F018 — UNIMARC 461 $t (series title) written to numero_serie (volume number / UNSIGNED INT); series title should go to collana/series fieldFile: Evidence:
Approach: Map UNIMARC 461 $t to collana (series title) and 461 $v to numero_serie (volume number); fallback to 225 $a/$v when 461 absent; truncate to varchar(50) defensively. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F019 — localBookUrl() hardcodes '/libro/{id}' instead of RouteTranslator::route('book'); en_US installs get 404File: Evidence:
Approach: Replace localBookUrl(request, int $bookId) with localBookUrl(request, array $book) and use book_url($book) instead of manual /libro/ concatenation; prepend only $origin (scheme://host[:port]) since book_url() already includes base path. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F020 — fetchChangedBooks() (no-since) SELECT from libri without deleted_at IS NULL — leaks soft-deleted book IDs to harvesters and violates CLAUDE.md ruleFile: Evidence:
Approach: Two options: (a) Strict CLAUDE.md compliance — add WHERE deleted_at IS NULL to no-since branch, accept no tombstones without ?from; (b) Bounded tombstone mode — add WHERE updated_at >= DATE_SUB(NOW(),INTERVAL 30 DAY) OR deleted_at >= DATE_SUB(NOW(),INTERVAL 30 DAY) to bound tombstone window. Recommend (b) for protocol alignment. Since-branch unchanged. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F024 — UPDATE libri in createLoanAtomic() missing AND deleted_at IS NULL — violates CLAUDE.md absolute soft-delete ruleFile: Evidence:
Approach: Add AND deleted_at IS NULL to both UPDATE libri WHERE clauses (F024 at line 1212 and F025 at line 1323) in a single patch. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F025 — UPDATE libri in closeLoan() missing AND deleted_at IS NULL — violates CLAUDE.md absolute soft-delete ruleFile: Evidence:
Approach: Same combined patch as F024 — add AND deleted_at IS NULL to WHERE clause at line 1323. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F027 — ensureSchema() returns void instead of array{created,failed}; onActivate() cannot check $result['failed'] per Plugin Schema RuleFile: Evidence:
Approach: Change ensureSchema() to return array{created:list,failed:list}; wrap ensureSchemaColumns() and ensureAlternatesTable() calls in try/catch Throwable blocks, accumulating created/failed lists. Update onActivate() and onInstall() to check $result['failed'] and throw RuntimeException. Mirror OaiPmhServerPlugin pattern exactly. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F045 — requireAdmin() checks only tipo_utente === 'admin'; plugin docs state 'admin/staff' can access write endpoints — privilege mismatchFile: Evidence:
Approach: Rename requireAdmin() to requireAdminOrStaff() and broaden role check to in_array(tipo_utente, ['admin','staff'], true). Update 4 call sites. Optionally reuse existing isStaff() helper. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified ℹ Uncertain (2)
Phase 4 couldn't confirm decisively. Re-run Fix runsRun
|
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F003 | FG-1 | ✓ fixed and verified | |
| F018 | FG-2 | ✓ fixed and verified | |
| F019 | FG-3 | ✓ fixed and verified | |
| F020 | FG-4 | ✓ fixed and verified | |
| F023 | FG-5 | ✓ fixed and verified | |
| F024 | FG-6 | ✓ fixed and verified | |
| F025 | FG-6 | ✓ fixed and verified | |
| F027 | FG-7 | ✓ fixed and verified | |
| F028 | FG-8 | ✓ fixed and verified | |
| F029 | FG-9 | ✓ fixed and verified | |
| F045 | FG-6 | ✓ fixed and verified |
🤖 Generated with Adam's Claude Code Review Command
Owner
Author
Auto-recommendation acceptance
4 auto-rec finding(s) eligible at threshold ≥ 60. Of those, 4 auto-promoted via batch, 0 promoted with edited/alternative hint, 0 skipped per-finding. Promoted (batch)
Auto-recommendation acceptance: append-only audit. Promoted findings are now |
Fix groups (committed): - [FG-1] F003 — storage/plugins/z39-server/classes/SRUServer.php: verified - [FG-2] F018 — storage/plugins/z39-server/classes/SruClient.php: verified - [FG-3] F019 — storage/plugins/openurl-resolver/OpenUrlResolverPlugin.php: verified - [FG-4] F020 — storage/plugins/resource-sync/ResourceSyncPlugin.php: verified - [FG-5] F023 — app/Controllers/ScrapeController.php: verified - [FG-6] F024, F025, F045 — storage/plugins/ncip-server/NcipServerPlugin.php: verified - [FG-7] F027 — storage/plugins/viaf-authority/ViafAuthorityPlugin.php: verified - [FG-8] F028 — storage/plugins/archives/ArchivesPlugin.php: verified - [FG-9] F029 — storage/plugins/oai-pmh-server/OaiPmhServerPlugin.php: verified Post-fix review: 9/9 groups verified complete; 0 group(s) partial; 0 group(s) reverted.
Owner
Author
Walkthrough decisions — 2026-05-11T13:15:03ZReviewer: fabiodalezbackup@gmail.com Pre-existing findings
Generated by |
Owner
Author
Code reviewBranch: Found 34 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (6)
Cross-cutting group G1: F007 + F041 — Both findings are manifestations of the same root cause: inconsistent local-timezone date emission inside OaiPmhServerPlugin.php. Fix together as a single 'normalize all catalog date emissions to UTC' pass: convert all five sites to strtotime()+gmdate() pattern (lines 596-601, 610-615 for F007; lines 1148, 1153, 1634 for F041), while explicitly preserving line 2211 (resumption-token expires_at). The UNIMARC sibling at line 1348 already uses gmdate() — use it as the canonical pattern throughout. Details and fix proposalsF007 — earliestDatestamp in oaiIdentify uses DateTime::createFromFormat with no timezone — parses MySQL datetime in PHP's default TZ, then format('Y-m-d\TH:i:s\Z') appends Z literally; emitted datestamp is local time mislabelled as UTC. recordDatestamp() in the same file correctly uses strtotime+gmdate; this is the outlier.File: Evidence:
Approach: Replace DateTime::createFromFormat (no timezone) + escaped \Z suffix with strtotime()+gmdate() pattern, matching the existing correct recordDatestamp() implementation. Apply to both the libri block (lines 596-601) and the archival_units block (610-615). Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRDDAGWPXE3XMZNNRSRW3ATQ): fixed and verified F008 — bookDetail() issues SELECT 1 FROM plugins on every render — N+1 DB round-trip on the hottest page. No caching; runs even for anonymous catalog crawls.File: Latest fix attempt (fixrun_01KRDP7GN6SQCYKJFXDD38TMKD): fixed and verified F012 — writeBookMag calls fetchDigitalAsset per record — the batch path in fetchRecordsPage does NOT pre-fetch digital_assets, so every MAG record on a page triggers an extra prepared-statement query (100+ queries per page at PAGE_SIZE=100).File: Evidence:
Approach: Add a 6th batch-fetch of digital_assets in fetchRecordsPage() alongside the existing 5 batched fetches; stamp _digital_asset on each row; update writeBookMag() to consume _digital_asset via array_key_exists() — mirroring the _authors/_publisher/_genre/_mag_config pattern already in the function. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRDDAGWPXE3XMZNNRSRW3ATQ): fixed and verified F018 — Path-traversal guard uses prefix-only comparison
|
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F007 | FG-1 | ✓ fixed and verified | |
| F012 | FG-1 | ✓ fixed and verified | |
| F031 | FG-2 | ✓ fixed and verified | |
| F034 | FG-3 | ✓ fixed and verified | |
| F041 | FG-1 | ✓ fixed and verified |
Run fixrun_01KRDP7GN6SQCYKJFXDD38TMKD — 2026-05-12T09:11:05Z
- Outcomes: 5 fixed and verified
- Commits:
27d51ad
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F008 | FG-1 | ✓ fixed and verified | |
| F018 | FG-2 | ✓ fixed and verified | |
| F029 | FG-3 | ✓ fixed and verified | |
| F033 | FG-4 | ✓ fixed and verified | |
| F040 | FG-5 | ✓ fixed and verified |
🤖 Generated with Adam's Claude Code Review Command
Owner
Author
Auto-recommendation acceptance
2 auto-rec finding(s) eligible at threshold ≥ 60. Of those, 2 auto-promoted via batch, 0 promoted with an edited or alternative hint, 0 skipped per-finding. Promoted (batch)
Auto-recommendation acceptance: append-only audit. Each |
Fix groups (committed): - [FG-1] F007, F041, F012 — storage/plugins/oai-pmh-server/OaiPmhServerPlugin.php: verified - [FG-2] F031 — app/Views/frontend/layout.php, locale/en_US.json, locale/de_DE.json, locale/fr_FR.json, locale/it_IT.json: verified - [FG-3] F034 — app/Views/admin/plugins.php: verified Post-fix review: 3/3 groups verified complete; 0 group(s) partial; 0 group(s) reverted.
Owner
Author
Walkthrough decisions — 2026-05-12T08:05:17ZReviewer: fabiodalezbackup@gmail.com
Summary: 5 promoted, 0 skipped. Run 🤖 Generated with Adam's Claude Code Review Command |
Reconciled fix (one merge pass after Phase 9.pre overlap): Findings: F008, F018, F029, F033, F040 Files: app/Controllers/FrontendController.php, app/Support/PluginManager.php, app/Support/Updater.php, app/Views/frontend/book-detail.php, app/Views/frontend/layout.php, storage/plugins/oai-pmh-server/OaiPmhServerPlugin.php Overlaps: app/Views/frontend/layout.php <- FG-1, FG-3 Findings addressed: - F008 (PluginManager::isActive cache): replace uncached `SELECT 1 FROM plugins` in FrontendController::bookDetail() and frontend/layout.php with per-process cached PluginManager::isActive() lookup. Eliminates an extra DB round-trip on every catalog page render (including anonymous crawls). - F018 (CWE-22 prefix-without-separator): tighten path-traversal guard in Updater.php — use str_starts_with($parentTarget, $realDest.'/') to prevent '/var/www/dest2' from passing when $realDest='/var/www/dest'. - F029 (WCAG 2.1 SC 1.1.1): add aria-hidden="true" to fa-archive icon in search-dropdown builder (frontend/layout.php JS). - F033 (WCAG 2.1 SC 1.1.1): add aria-hidden="true" to media-type icon in book-detail view (label is announced separately). - F040 (RFC 7235): split requireAdminForDownload() OAI-PMH auth flow — 401 + WWW-Authenticate when no credentials supplied, 403 when credentials present-but-invalid. Post-fix review: 1/1 group verified complete; 0 partial; 0 reverted.
PR #132 polish bundle: - book-detail.php: htmlspecialchars() around ucfirst($author['ruolo']) — was the only raw role output in the author span (F048). - tests/pr132-fix-regressions.spec.js: new E2E regression suite covering F003 (SRU errorResponse namespace), F018 (UNIMARC 461 $t/$v), F019 (OpenURL locale-aware book_url), F020 (ResourceSync 30-day deleted window), F024/F025 (NCIP UPDATE libri AND deleted_at IS NULL), F027 (ViafAuthority ensureSchema return shape), F029 (OAI-PMH ListMetadataFormats includes unimarc), F045 (NCIP admin endpoints reachable by staff), F048 (book-detail XSS escape). - tests/discogs-import.spec.js: mock Discogs scrape endpoint for deterministic E2E so the suite no longer depends on live Discogs API. - tests/series-collane-crud.spec.js: mysql invocation now supports TCP host alongside socket connection and pipes the migration via stdin. - tests/bulk-enrich.spec.js: raise per-test timeout to 240s and surface Gateway-Timeout retries as explicit errors. - locale/fr_FR.json: 849 additional Italian→French translations (admin UI, archives, NCIP, OAI-PMH, plugin metadata) — closes the FR locale gap left by feat/fr-bnf-integration. - storage/plugins/archives/views/index.php: responsive toolbar (hidden on mobile, accessible "more" dropdown), aria-hidden on decorative SVG icons.
Owner
Author
|
@coderabbitai full review Triggering full review on the latest cleanup + fix commits (27d51ad, 9d70038) before running the local adamsreview pipeline. Please cover the 5 findings just resolved (F008 PluginManager::isActive cache, F018 CWE-22 path-traversal guard, F029/F033 WCAG aria-hidden icons, F040 RFC 7235 OAI auth) plus the chore bundle (PR #132 regression tests, FR translations, archives UI responsive). |
|
User feedback: Revisione focalizzata sui 5 fix (F008, F018, F029, F033, F040) e sul bundle chore (regression tests, traduzioni fr_FR, archives UI responsive). Evitare suggerimenti stilistici minori; priorità a bug reali, sicurezza e violazioni delle regole di progetto. ✅ Actions performedFull review triggered. |
Owner
Author
Code reviewBranch: Found 92 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (50)
Details and fix proposalsF003 — session_write_close() at byIsbn() entry breaks every in-process caller. LibriController::fetchCover() (line 2351) and ScrapingService::scrapeBookData() (LibraryThingImportController, CsvImportController, BulkEnrichmentService) invoke byIsbn() in-process. After this PR, subsequent $_SESSION[...]=... writes are silently discarded — flash messages, CSRF rotations, success states evaporate.File: F004 — Public unified search merges books+authors+publishers, appends archive results via hook, then array_slice(15). If core results fill 15 (10 books + 5 authors typical), no archive results ever appear — silently dropped. Admin variant caps core to 15 first then appends hooks before 20 cap. Same hook, two different truncation behaviours; public users never see archives in search-as-you-type.File: F006 — CIDR check only allows IPv4 — IPv6 CIDR ranges in TRUSTED_PROXIES are silently skipped (continue). Operators expecting IPv6 trusted-proxy list to work get no warning and X-Forwarded-Proto will be ignored || isRemoteAddrTrustedProxy silently skips IPv6 CIDR entries via strlen($networkIp) !== 4 (line 250) with no warning logged — admins configuring TRUSTED_PROXIES with IPv6 CIDR (e.g. 2001:db8::/32) get no error, but the proxy is not trusted and X-Forwarded-Proto detection silently breaks on dual-stack deployments.File: F007 — getBaseUrl() trusts X-Forwarded-Proto only when REMOTE_ADDR in TRUSTED_PROXIES (good), but unconditionally trusts HTTP_HOST without checking X-Forwarded-Host against proxy whitelist. Behind a proxy that does NOT rewrite Host, attacker can send crafted Host header poisoning every absolute URL — OAI baseURL, BIBFRAME persistent URI sameAs, FAIR Signposting Link headers, schema.org isPartOf, email links. New OAI/BIBFRAME features amplify blast radius.File: F008 — Reworked while loop exits via $staleLock=true continue re-evaluating the while condition. When $timedOut=true with neither $lockAcquired nor $staleLock, code falls through to call $callback() and self::set() WITHOUT holding the flock — defeating stampede protection at the 8-second timeout boundary. Pre-existing latent invariant the refactor did not address.File: F010 — Path traversal: second copyDirectory() still uses vulnerable prefix-match (strpos !== 0) without trailing '/' guard — F018 fix applied only to copyDirectoryRecursive(). A package containing a path that resolves under '/var/www/destSibling' would pass the check when realDest='/var/www/dest'. || copyDirectory() retains the old vulnerable strpos($parentTarget, $realDest) !== 0 prefix check while parallel copyDirectoryRecursive() at line 2247 was hardened with !== $realDest && !str_starts_with($realDest . '/'); the path-traversal/prefix-collision strengthening is asymmetric across the two parallel directory-copy paths. || copyDirectory() retains the prefix-collision security bug fixed in sibling copyDirectoryRecursive() during the same edit. Recursive variant uses !== $realDest && !str_starts_with($realDest.'/'); copyDirectory still uses original strpos !== 0. Two parallel paths whose security invariants diverged after one was patched.File: F012 — $bookSchema['sameAs'] is now assigned unconditionally — previously only set when $sameAsLinks was non-empty. If the book has no ISBN AND the BIBFRAME plugin is inactive, JSON-LD output will include an empty 'sameAs': [] array.File: F015 — Regex '#^/[A-Za-z0-9/_-.~%]*$#' for archive URL whitelisting rejects query strings (?), fragments (#), and many legitimate URL chars — any archive result URL with a query string or extra punctuation will be silently replaced with '#'File: F018 — cmd_viaf_reconcile_all loads the entire SELECT id, nome FROM autori WHERE viaf_id IS NULL into a PHP array when --limit is unset (defaults to PHP_INT_MAX, SQL LIMIT omitted). For libraries with O(100k+) authors this exhausts PHP memory before any reconciliation work begins; should stream with bounded chunks.File: F020 — Plain INSERT INTO languages (no ON DUPLICATE KEY UPDATE) in data_it_IT.sql and data_en_US.sql vs data_de_DE.sql and data_fr_FR.sql use ON DUPLICATE KEY UPDATE. Re-running fresh install over existing DB (recovery/reinstall-test.sh) fails with duplicate-key error for it_IT and en_US but succeeds for de_DE/fr_FR. Inconsistent idempotency. || languages seeder rows ship stale, wrong total_keys/translated_keys/completion_percentage that are surfaced on admin Languages UI as honest data. Real fr_FR.json has 4993 keys, seeder claims 4080; it_IT.json has 493 keys, seeder claims 2015; en_US.json/de_DE.json have 4653, seeder claims 2015/4009. All four counters lie.File: F021 — migrate_0.7.4.sql registers ncip-server, openurl-resolver, bibframe-linked-data, resource-sync into plugins table via INSERT ON DUPLICATE but OMITS oai-pmh-server and viaf-authority — both added to BundledPlugins::LIST in this PR. On 0.7.3→0.7.4 upgrades these will be missing from plugins until autoRegisterBundledPlugins() picks them up on a later request.File: F022 — migrate_0.7.4.sql backfills fr_FR using INSERT IGNORE — on installs that already have an fr_FR row with is_active=0 (partial 0.7.4-beta install), activation flag is NOT updated. migrate_0.7.5.sql at line 6-15 uses ON DUPLICATE KEY UPDATE for the same INSERT repairing it. Asymmetric idempotency between two upgrade migrations.File: F023 — migrate_0.7.5.sql sets total_keys=4145 with ON DUPLICATE KEY UPDATE while migrate_0.7.4.sql and data_*.sql seeders set 4080. Two different numbers for the same column: upgrades land at 4145; fresh installs at 4080.File: F025 — French route file ships English paths (e.g. /login, /catalog, /register, /events) while routes_it_IT.json uses Italian (/accedi) and routes_de_DE.json uses German equivalents. French users get inconsistent URL voice — French content under English-named URLs breaks the locale-aware routing convention. || French routes file is byte-identical to routes_en_US.json — all paths are English (/login, /catalog, /book, /author, /publisher, /about-us, /privacy-policy) instead of French (/connexion, /catalogue, /livre, /auteur). it_IT and de_DE use translated paths. UX regression: French users get English URLs. || routes_fr_FR.json is a byte-identical clone of routes_en_US.json — keys like login/profile/catalog/book resolve to English URLs (/login, /profile, /catalog, /book) for French installs. Other locales properly translate (/accedi IT, /anmelden DE). French users see English URLs throughout — route translation contract is broken for the new locale introduced by this same PR.File: F026 — violation of CLAUDE.md ABSOLUTE RULE #1: git diff summary shows 'mode change 100755 => 100644 scripts/create-release.sh' — the release script lost its executable bit, so
|
| # | Score | Impact | File | Issue |
|---|---|---|---|---|
| F002 | 45 | security | app/Controllers/FrontendController.php:760-790 |
Book-detail response Link header injects author VIAF URI from autori.viaf_uri DB column. Although filter_var + scheme regex + strpbrk for <>,\r\n are applied, the underlying VIAF URI value is admin-controlled and could carry CRLF or '>'-terminator characters that the strpbrk filter checks for — but inet protections rely on those checks being complete. An admin storing 'https://viaf.org/viaf/123\"; rel=stylesheet; href=evil' could try to break out via a quote character (not in the deny list). |
| F009 | 45 | correctness | app/Support/Updater.php:2244-2254 |
After str_starts_with check using $realDest with forward-slashes, the prefix-collision guard appends '/' — but when $realDest is the filesystem root '/' on Unix, $realDest . '/' becomes '//' and str_starts_with(...,'//') will reject every legitimate path |
| F011 | 45 | correctness | app/Views/admin/plugins.php:1004-1008 |
presetConfig copies `preset.quote_search_terms |
| F016 | 45 | correctness | app/Views/frontend/layout.php:1731-1738 |
searchViewAllLabel computed once via PHP json_encode(__('Vedi tutti i risultati'), JSON_HEX_TAG) at script render time, but the same script body uses __('Archivio'), __('Libri'), __('Autori'), __('Editori'), __(' libri') as if PHP — but they're inside JS function body (after first <script>), meaning they call a JS function named __. JS __() defined for book_form.php (line 1077) but frontend layout has no equivalent JS __ init — anonymous frontend pages may hit ReferenceError when search returns results. |
| F049 | 45 | security | storage/plugins/ncip-server/NcipServerPlugin.php:505-525 |
NCIP LookupUserResponse for a patron looking up themselves emits AgencyUserPrivilegeType containing tipo_utente (admin/staff/utente) of the requester. Exposes the user's privilege level back to a Basic-Auth'd client over the network — minor info disclosure useful for reconnaissance. |
| F050 | 45 | correctness | storage/plugins/ncip-server/NcipServerPlugin.php:1177-1230 |
createLoanAtomic() calls $this->db->begin_transaction()/commit()/rollback() without the @@autocommit detection pattern documented in CLAUDE.md and used in recalculateBookAvailability. If an external orchestrator wraps an NCIP request in an outer transaction, inner begin_transaction implicitly commits the outer one corrupting audit log. |
| F051 | 45 | correctness | storage/plugins/ncip-server/NcipServerPlugin.php:1241 |
createLoanNcip bind_param('iisss', ...) — if origine ENUM doesn't include 'ncip', the INSERT silently fails inserting empty string in strict mode and returns null loan id with no actionable error logged |
| F061 | 45 | correctness | storage/plugins/oai-pmh-server/OaiPmhServerPlugin.php:1101-1102 |
dc:type emits ucfirst($tipo) — for non-ASCII media types with UTF-8 characters this only uppercases the first byte and may corrupt multi-byte sequences; should use mb_convert_case or static type mapping |
| F062 | 45 | correctness | storage/plugins/oai-pmh-server/OaiPmhServerPlugin.php:1156 |
008 field uses str_pad with default ' ' (space) padding for year — if anno_pubblicazione is empty, gets ' ' (4 spaces) acceptable; but if shorter than 4 digits (e.g. '999'), the pad makes it ' 999' which violates MARC21 008 format (single-known-date should be 4 zero-padded digits) |
| F064 | 45 | security | storage/plugins/oai-pmh-server/OaiPmhServerPlugin.php:2630-2680 |
digitalAssetAddAction stores an admin-supplied URL into digital_assets.url after only filter_var(FILTER_VALIDATE_URL) and http/https scheme check. The URL is later emitted in OAI-PMH MAG elements served publicly. No allow-list of hosts; admins could store URLs pointing to internal infra or mark book metadata with attacker-controlled URLs. Admin-only, so low severity, but worth review. |
| F066 | 45 | security | storage/plugins/oai-pmh-server/OaiPmhServerPlugin.php:2710-2745 |
digitalAssetAddAction reads JSON body with json_decode((string)$request->getBody()) but does not verify Content-Type, and validates the CSRF token from JSON body (data['csrf_token']) instead of header. Combined with the OPTIONS/JSON path this can allow a same-site fetch with text/plain to bypass Slim's CSRF middleware (which only validates form-encoded POSTs) — though the explicit Csrf::validate inside the action does enforce token presence. |
| F073 | 45 | security | storage/plugins/openurl-resolver/OpenUrlResolverPlugin.php:133-134 |
injectCoinsScript builds fetch URL by concatenating json_encode(basePath + '/api/coins/book/') + id — basePath from defined('BASE_PATH') ? BASE_PATH : '' but app elsewhere uses HtmlHelper::getBasePath(); a stale BASE_PATH constant will produce wrong fetch path on subfolder installs |
| F074 | 45 | correctness | storage/plugins/openurl-resolver/OpenUrlResolverPlugin.php:327-329 |
preg_replace coerced via '?? '''; loop continues even when value doesn't normalize to 10/13 digits, so 'isbn'/'rft_id' fields with junk fall through, possibly confusing callers expecting validation |
| F075 | 45 | security | storage/plugins/openurl-resolver/OpenUrlResolverPlugin.php:380-395 |
localBookUrl() builds the redirect origin from $request->getUri()->getHost() / getScheme(), which derive from the user-supplied Host header. An attacker controlling the Host header can influence the 302 Location origin used for the local-book branch (open-redirect-by-Host-spoofing if Slim/PSR-7 trusts the Host header in this deployment). |
| F085 | 45 | correctness | storage/plugins/z39-server/classes/SRUServer.php:85 |
NS_DIAG constant changed from 'http://www.loc.gov/zing/srw/diagnostic/' to 'info:srw/diagnostic/1/' — diagnostic uri text content becomes 'info:srw/diagnostic/1/' . $code; existing clients pointing at the old prefix will silently break |
| F087 | 45 | correctness | storage/plugins/z39-server/classes/SRUServer.php:1146-1150 |
writeAccessLog now early-returns when stmt prepare fails — silently. Repeated access-log failures (e.g. table missing during migration) produce no observable signal |
| F089 | 45 | security | storage/plugins/z39-server/classes/SruClient.php:149-156 |
Term wrapping for CQL: when server.quote_search_terms is enabled the user-controlled $term is wrapped in CQL double-quotes without escaping embedded '"' characters. A term containing a double-quote can terminate the literal early and inject CQL operators into the outbound SRU query (e.g. to remote BNF/SUDOC). |
| F090 | 45 | correctness | storage/plugins/z39-server/classes/SruClient.php:150-155 |
After loadXML failure, libxml_use_internal_errors($prevLibxmlErrors) is called inside the if-block, but is also called after — when loadXML fails the restoration line outside is never reached (throws first) |
Phase 4 couldn't confirm decisively. Re-run /adamsreview:review if you suspect this deserves
further investigation with fresh context.
Light lane — ux, policy, architecture
Light-lane findings — including rows labeled auto-fixable — aren't applied by /adamsreview:fix directly. Findings with an auto-recommendation get batch-confirmed at :fix's Phase 7.5 preflight (or :walkthrough Step 4.5); use /adamsreview:promote <finding_id> for a single-finding manual override.
| # | Score | Impact | File | Finding | Disposition |
|---|---|---|---|---|---|
| F005 | 60 | ux | app/Models/Language.php:286-294 |
setDefault() now flips is_active=1 together with is_default=1. Fix description says 'can't default to inactive language' but prior bug was admins setting inactive language as default and getting locked out. Side effect: setting default also silently re-enables, even if admin had just deactivated it. Two state transitions in one query without audit trail. | manual |
| F014 | 60 | ux | app/Views/frontend/catalog.php:306-332 |
'Trovato anche nell'archivio' suggestion box renders archive results below the empty-catalog state but only when $archiveResults is non-empty — it's outside the empty-state container, which means when the search HAS book results AND archive results, the archive panel still renders after the books but ONLY in the empty branch. Test by reading the surrounding control flow: the box is rendered inside the that holds the 'Pulisci filtri' button which itself is inside the empty-results branch. |
manual |
| F017 | 75 | ux | app/Views/user_layout.php:1215 |
Archive search-result avatar icon lacks aria-hidden="true", unlike the parallel block in frontend/layout.php which sets it. Screen readers will announce 'archive' twice (icon + label). | |
| F024 | 75 | ux | locale/en_US.json:1 |
New archive form section labels ('Area di identificazione', 'Contenuto e struttura', 'Condizioni di accesso e uso', 'Identificatore ARK', 'Dichiarazione dei diritti (URL)', 'URL manifest IIIF (server esterno)', 'Nota di versione') translated only in fr_FR.json — missing from en_US.json and de_DE.json. EN/DE users will see Italian section headings. | |
| F035 | 60 | ux | storage/plugins/archives/views/authorities/show.php:39-48 |
'Indietro' (back) button uses inline JS history.back() with referrer-origin check, but the same-origin referrer guard is too strict: when the user opens the authority page in a new tab (common from search results), document.referrer is empty and they get sent to the authorities list rather than the search results they came from. Also data-fallback-url is the static list, not the search query. | manual |
| F037 | 60 | ux | storage/plugins/archives/views/form.php:783-789 |
version_note placeholder logic is inverted: $val('version_note') !== '' ? '' : $e(__('es. ...')) — when there is no current value the placeholder shows, when there IS a value the placeholder is blanked. But since already shows the value, the conditional is unnecessary; the bigger issue is the literal year '2024' in the placeholder text is becoming outdated (PR date is 2026). | manual |
| F038 | 60 | ux | storage/plugins/archives/views/index.php:1013-1016 |
Inline <style> block injected mid-page inside an admin layout (#arc-actions-details[open] .arc-chevron) — violates content-security-policy with inline style, and admin pages elsewhere put rules in CSS asset files. Also breaks visual consistency if site CSP is tightened. | manual |
| F039 | 75 | ux | storage/plugins/archives/views/index.php:1080-1088 |
Filtered result label __("livello") ?>: echoes the raw enum value (fonds, series, file, item) instead of the localized label (Fondo, Serie, Fascicolo, Unita) already prepared in $levelLabel for the badge cells. Inconsistent with the option dropdown above which uses localized labels. | |
| F040 | 60 | ux | storage/plugins/archives/views/public/index.php:1244-1252 |
The 'Azzera filtri' (clear filters) button is rendered as a single 'times' character with only a tooltip title=; on touch devices the tooltip is invisible and the glyph alone fails to meet the 44x44 px touch target / accessible-name requirement. The form's submit button has no aria-label either. | manual |
| F041 | 75 | ux | storage/plugins/archives/views/public/index.php:1260-1271 |
Public archive search results have same pluralization issue: '1 risultati' for single match. Date-range fallback uses ellipsis character for missing bound which is unintuitive — better to show 'dal X' / 'fino al Y' explicitly. | manual |
| F043 | 60 | ux | storage/plugins/archives/views/public/show.php:1385-1400 |
Multi-file download list shows MIME type next to each file but no file size and no upload date. For audio files only the player renders; the user cannot see what they're about to listen to (e.g. duration, format) before pressing play. Filename fallback uses basename() which can leak storage paths if original_filename is null. | manual |
| F046 | 75 | policy | storage/plugins/ncip-server/NcipServerPlugin.php:17-30 |
Docblock 'Supported messages' lists only 5 (LookupItem, LookupUser, CheckOutItem, CheckInItem, RenewItem) but match() at lines 432-439 handles 7 — adds RequestItem and CancelRequestItem; capability XML at line 818 advertises all 7 | manual |
| F054 | 75 | ux | storage/plugins/ncip-server/views/partners.php:152-161 |
Destructive partner-delete uses native confirm('Eliminare questo partner?') with no name, ISIL, or endpoint context — admin can't tell which partner is about to be removed and there is no preview of side effects (transactions referencing this partner). Other destructive flows in this codebase use SweetAlert with detail. | manual |
| F055 | 60 | ux | storage/plugins/ncip-server/views/transactions.php:93-100 |
Status badge color-codes 'ok'/'error'/'pending' but the raw $tx['status'] value is rendered () — not translated, so users see the English/protocol status string in all locales. Should map to localized labels matching the colored badge. | manual |
| F069 | 60 | ux | storage/plugins/oai-pmh-server/views/book-digital-assets.php:85-90 |
Icon-only delete button exposes accessible name only via title="Elimina"; HTML title is not consistently announced by screen readers (NVDA/JAWS often ignore it). Should use aria-label="" like other icon-only buttons fixed in this PR (e.g. plugins.php uninstall, z39 server row delete). | manual |
| F070 | 60 | ux | storage/plugins/oai-pmh-server/views/book-digital-assets.php:110-162 |
Add-digital-copy form has no client-side validation of MD5 hash format or image dimensions before submit; the server-side error returns via setError() but the URL input has required only, while MD5/dimensions only pattern/min — no inline error UI, no per-field error placement. Users get one banner error for whichever field failed first. | manual |
| F071 | 60 | ux | storage/plugins/oai-pmh-server/views/book-digital-assets.php:254-272 |
Digital-asset delete uses native confirm() with generic message and shows no URL/filetype/size of the row being removed; after success only the row is spliced from DOM with no toast/feedback. Errors surface via native alert(), inconsistent with the rest of Pinakes which uses SweetAlert and inline alerts. | |
| F076 | 75 | policy | storage/plugins/openurl-resolver/wrapper.php:10-11 |
Comment asserts getPluginClassName('openurl-resolver') returns 'OpenUrlResolverPlugin' (capital U mid-word) but PluginManager::getPluginClassName uses explode('-')+ucfirst per segment, which yields 'OpenurlResolverPlugin' (no second cap). The wrapper class is declared as 'OpenUrlResolverPlugin' and only PHP's case-insensitive class lookup keeps the lookup working | manual |
| F001 | 60 | architecture | app/Controllers/FrontendController.php:519-524 |
catalogAPI() (AJAX/JSON variant) computes $archiveResults and exposes as 'archives' in JSON response, but no current JS consumer reads response.archives — layout.php search dropdown reads from /api/search/unified which uses SearchController, not /api/catalog. catalogAPI changes ship dead data: server cost of plugin hook invocation on every search request with zero observable effect. | informational |
| F019 | 75 | architecture | installer/classes/Installer.php:1460-1474 |
Fresh install's installPluginsFromZip() hardcodes 5-plugin allow-list (open-library, z39-server, api-book-scraper, digital-library, dewey-editor). This PR expands BundledPlugins::LIST to 16 plugins including bibframe-linked-data, ncip-server, oai-pmh-server, openurl-resolver, resource-sync, viaf-authority, archives, deezer, discogs, goodlib, musicbrainz. Upgrade paths get them via migrate_0.7.4.sql; fresh installs only get them later via autoRegisterBundledPlugins() and they land deactivated. Two divergent paths. | informational |
| F013 | 45 | ux | app/Views/frontend/book-detail.php:1605-1610 |
Media-type badge moved out of into a separate sibling div above the title. Visual hierarchy now reads: badge title authors. But the badge uses font-size: 0.7rem directly in inline style and bg-light text-secondary while the rest of the page uses availability-badge class with stronger contrast. The badge is purely decorative ('Libro', 'CD', etc.) without role/label context — screen reader will read it before the book title with no separator. |
uncertain |
| F036 | 45 | ux | storage/plugins/archives/views/form.php:683-701 |
ARK identifier and IIIF manifest URL inputs accept arbitrary text but help-text below only describes the expected pattern in prose. There is no inline validation (pattern attribute, datalist of common NAANs) — invalid values are only caught server-side, returning user to a fresh form. Should add pattern="^ark:/.+" and/or example datalist for better diagnostic. | uncertain |
| F053 | 45 | ux | storage/plugins/ncip-server/views/partners.php:85-91 |
'Note' partner field has no maxlength visible to user (maxlength="500" is set on the input but no character counter), and the table renders Notes with max-w-xs truncate — long notes are silently cut off with no tooltip or expand affordance, so admins can't see what they typed without editing. | uncertain |
| F056 | 45 | ux | storage/plugins/ncip-server/views/transactions.php:111-131 |
Pagination shows only prev/next buttons and 'Pagina X di Y' text — no page-size selector, no jump-to-page, no per-page count, no link to first/last. For a transaction log this is painful once volume exceeds a few pages. Also no filter by status/partner/date even though those columns are displayed. | uncertain |
🤖 Generated with Adam's Claude Code Review Command
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
2 participants
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.
Summary
migrate_0.7.5.sqlusesON DUPLICATE KEY UPDATEto activate fr_FR on upgrades (wasINSERT IGNORE);Language::setDefault()now auto-activates the language to preventis_default=1, is_active=0inconsistencyauthor_authority_alternatesif old pre-release dev columns (source_code) are detected — fixes upgrade failure on installations that had dev migrations applied manuallybibframe.bookkey toroutes_de_DE.json(parity with it_IT, en_US, fr_FR)ensureSchema()fix), BIBFRAME/RDF, ResourceSync, VIAF authority controlRoot causes fixed in this batch
fr_FR not activating after upgrade
migrate_0.7.5.sqlusedINSERT IGNORE— if the row existed withis_active=0, the row was silently skipped. Switched toON DUPLICATE KEY UPDATE is_active=1.Site staying Italian after setting French as default
I18n::loadFromDatabase()queriesWHERE is_active=1— anis_active=0language is invisible to the entire locale resolution chain even ifis_default=1. Root fix:Language::setDefault()now forcesis_active=1on the target language."Key column 'source' doesn't exist in table" on upgrade
Some installations had
author_authority_alternatescreated by an early dev migration with columnsource_codeinstead ofsource.CREATE TABLE IF NOT EXISTSskipped the table, then the subsequentADD KEY idx_authority (source, authority_id)failed. Fixed by adding a guard that DROPs the table when the oldsource_codecolumn is detected.Test Plan
author_authority_alternates: verify migration completes without errorphpstan analyse --memory-limit=512M(0 errors)full-test.spec.js(97 tests)🤖 Generated with Claude Code