feat(settings): global metadata field×priority matrix (bookshelf-4y3y.2) #445
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-4y3y.2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
MergeMetadataByFieldPriorityfrom series-only to all 14 content fields + cover using type-safemergeStringField,mergeSliceField,mergeCategoriesField,mergePointerFieldhelpersGET/PUT /settings/metadata-field-prioritywith per-field enabled toggle/settings/field-prioritysidebar link with new URL (redirect kept for the legacy/settings/field-priority-matrixvariant)app_settingsJSON only (no schema migration)field_priority_matrix_controller.jsStimulus controller +settings_metadata_field_priority.htmltemplateTest plan
make buildpassesmake testpasses (all packages, includinginternal/books,internal/settings)golangci-lint run ./internal/settings/... ./internal/books/...— 0 issuesGET /settings/metadata-field-priorityreturns 200 HTML with matrix tablePUT /settings/metadata-field-priorityreturns 204 and persists/settings/field-prioritysidebar link redirects to new matrix pageCloses bead bookshelf-4y3y.2 on merge.
d1269058b1171a287d60Code Review (bot)
PR #445 — bookshelf-4y3y.2: unified global Field×Priority metadata matrix
Phase 0: DEMO Verification
No DEMO block was required for this bead type (settings UI + merge extension). CI is green per the review dispatch.
Findings
[MAJOR] internal/books/merge.go:129-145 —
mergeSliceField/mergeCategoriesFielduseout.*(primary) as fallback;PersistMetadataonly checks*Locked, notEnabledFieldsWhen
AuthorsorCategoriesis disabled (not locked) in the matrix,mergeSliceFieldreturnsout.Authors/out.Categories— the first-winner provider's values (i.e.primary.Authors). These values then flow intoPersistMetadata, which only gates the DB write onexisting.AuthorsLocked/existing.CategoriesLocked(metadata_store.go:530-535). If the field is disabled but not locked,PersistMetadatacallssyncAuthors/syncCategorieswithprimary.Authorsand overwrites the existing DB rows — the opposite of what "disabled" should mean.Scalar fields (
Title,Publisher, etc.) correctly returnexistingValwhen disabled, so their DB values are preserved. Slice fields returnprimary.*which gets written. The tests at merge_test.go:500+ assert "preserves primary authors unchanged" which describes the wrong semantics — "disabled" should preserve the existing DB authors, not the primary provider's authors.Fix options: (a) thread
existing.Authors []stringthroughMetadataRowsomergeSliceFieldcan return the true DB value when disabled/locked, or (b) add aEnabledFields.Authors / .Categoriesguard inPersistMetadataalongside the existingAuthorsLockedguard, or (c) change the fallback inmergeSliceFieldtonil/ empty slice when disabled, and guardsyncAuthorsonlen(m.Authors) > 0inPersistMetadata.[MAJOR] internal/settings/routes.go:15-17 / internal/settings/matrix_handler.go (whole file) — Bead spec says "REPLACE the current rating-only
/settings/field-prioritypage (old URL redirected)" but old URL is not redirected; redirect is registered for a never-existed URLOn
main,/settings/field-prioritywas the only settings page (rating-only). The bead spec says this PR replaces it with the new matrix page and redirects the old URL. What actually landed:/settings/field-prioritystill exists (unchangedListFieldPriorityHandler), no longer linked in the sidebar nav but fully accessible — the old rating-only page coexists with the new matrix.GET /settings/field-priority-matrix → /settings/metadata-field-priority. There was no/settings/field-priority-matrixURL onmain, so this redirect has no legacy traffic to serve.HardcoverRating,GoogleBooksRating,OpenLibraryRating) are explicitly excluded fromcontentProvidersand fromfieldDefs, so they do not appear in the new matrix as the spec requires ("rating becomes one row").Result: two separate pages for provider priority settings now coexist, with no migration path between them. Users who bookmarked
/settings/field-prioritysee the old page without indication that a replacement exists. Rating configuration is split across two pages.Fix: either (a) add
http.RedirectforGET /settings/field-priority→/settings/metadata-field-priority, remove or repurposeListFieldPriorityHandler, and add rating fields tofieldDefs+contentProviders; or (b) leave the two pages coexisting and update the bead description to reflect the intentional difference in scope.[MINOR] internal/settings/matrix_handler.go:381-386 —
applyMatrixUpdatedoes not validateReplaceModevalueAny string is accepted as
ReplaceModein a PUT request. The merge engine treats anything other than the literal string"REPLACE"asREPLACE_MISSING(merge.go:76:if replaceMode != "REPLACE"), so a client submitting"REPLACE_ALL"silently getsREPLACE_MISSINGsemantics. Not a security issue (no privilege escalation) but a confusing silent fallback. Fix: validate thatReplaceModeis one of the two known values before persisting.[MINOR] internal/settings/matrix_handler.go:33-42 —
FieldRowhas nojson:struct tags; serializes with capitalized field names ("Key","P1","Label", etc.)All other JSON-serialized types in this codebase use lowercase
json:"…"tags.FieldRowexposes"Key","P1","P2","P3","P4","Label","Enabled"with Go-default PascalCase keys. The e2e test (e2e/api/settings_metadata_field_priority_test.go:38) decodes withjson:"Key"to match. This works but deviates from API naming conventions. Fix: addjson:"key",json:"p1", etc. tags; update the corresponding test decode struct and the e2e test decode struct.Phase 2 Highlights (passing checks)
mergeStringField/mergePointerFieldcorrectly implement the 5-step spec (disabled → existing, locked → existing, walk chain, apply replace-mode, fallback to existing). REPLACE_MISSING preserve-on-non-empty is correct (line 76:replaceMode != "REPLACE" && existingVal != "").mergeCategoriesUnioncorrectly deduplicates in encounter order with aseenmap.settings.FieldOptions(the persisted shape) andbooks.RefreshFieldOptions(the in-process shape) are expanded to the same 14+cover set.SettingsOptsToRefreshOptsmaps every field inwire.go:607-649. The nil-FieldProvider → empty-FieldProvider default is correct (no chain → preserves existing).settings.defaultQuickBookMatchandbooks.DefaultMetadataRefreshOptionsnow usehardcover P1 / open-library P2 / google-books P3for all fields. Fallback and persisted default match.fieldDefsorder; cover is last. e2e and unit tests verify this.applyMatrixUpdate→Save(UpsertAppSetting) →Loadround-trip is tested in e2e/api via GET-after-PUT assertion.title=""+ asserting it fills to"Enriched By Workflow"correctly proves the REPLACE_MISSING fill-empty path. The regression guard (workflow no-op times out with"") is valid.Describeblocks (locked + unlocked-empty) cleanly separate the two behaviors. The unlocked test now seedstitle=""so REPLACE_MISSING fills it — the assertionContainSubstring("Refetched Title")is non-vacuous.field_priority_matrix_controller.js, registered as"field-priority-matrix", classwindow.FieldPriorityMatrixController extends window.StimulusController— all conventions correct.saveUrlValueis a Stimulus value; save button usesdata-action="click->field-priority-matrix#save".app_settingsviaUpsertAppSetting. Grimmory-schema guard is not affected.newMysqlBackendextraction: Straightforward testability refactor;defaultNewMysqlBackendpreserves identical production behavior.sortKeyIndexrefactor (store.go): Switch → map lookup; behavior identical.REVIEW VERDICT: 0 blocker, 2 major, 2 minor
Security Review (bot)
PR #445 — Global metadata Field×Priority matrix settings page (bead bookshelf-4y3y.2)
Findings
[MAJOR] internal/settings/routes.go:11-18 — New
/settings/metadata-field-priorityroutes lackAdminRequiredguard; any authenticated non-admin user can read and overwrite global metadata settingsRegisterRoutesregisters bothGET /settings/metadata-field-priorityandPUT /settings/metadata-field-priorityviaeh.Wrap(...)only — noAdminRequiredmiddleware is applied. Compare withPOST /admin/covers/refresh-alland all/admin/users/*routes, which wrap handlers withadminRequired(eh.Wrap(h)). The entire settings package (Wire,RegisterRoutes) never receivesd.AdminRequiredfromappwire.Deps— it is available inDepsbut simply not passed or used.A regular (non-admin) authenticated user can therefore:
GET /settings/metadata-field-priority— read global priority matrixPUT /settings/metadata-field-priority— overwrite global metadata merge settings, including disabling all fields or redirecting all metadata to a provider of their choiceThe pre-existing
/settings/field-priorityand/settings/metadata-providersroutes have the same gap (pre-existing, not introduced by this PR), but this PR adds a new state-mutating PUT that makes the impact worse.Fix: thread
adminRequired func(http.Handler) http.HandlerintoRegisterRoutesand apply it to all/settings/*handlers (or at minimum the new matrix routes and the existing PUT), matching the pattern ininternal/cover/routes.goandinternal/users/admin_handler.go.[MINOR] static/js/controllers/field_priority_matrix_controller.js:30-36 — CSRF token omitted from PUT fetch call
The
field_priority_matrix_controller.jssave()method issuesfetch(url, { method: "PUT", headers: { "Content-Type": "application/json" }, ... })without including theX-CSRF-Tokenheader. Every other state-mutating Stimulus controller in this codebase (provider_settings_controller.js:47-54,provider_priority_controller.js:29-36) reads the CSRF token fromdocument.querySelector('meta[name="csrf-token"]')and passes it as"X-CSRF-Token": csrf. The middleware's double-submit CSRF check (internal/middleware/csrf.go:100-106) will reject this PUT with 403 on any real browser session that has the CSRF cookie set.Note: this is a correctness/functional bug (the save button silently fails), but it is also technically a CSRF gap for any non-browser API client that can reach the endpoint without the cookie (which is blocked by the
AdminRequiredgap above anyway). Fix: addconst csrfMeta = document.querySelector('meta[name="csrf-token"]'); const csrf = csrfMeta ? csrfMeta.getAttribute("content") : "";and include"X-CSRF-Token": csrfin the fetch headers, matchingprovider_settings_controller.js.Confirmed clean
SSRF (cover row):
internal/cover/download.goapplies a comprehensive defense-in-depth: scheme allowlist (http/httpsonly,ErrBadScheme), customsafeDialContextthat resolves the hostname and rejects all private/loopback/link-local/reserved CIDRs (10/8, 172.16/12, 192.168/16, 169.254/16, fc00::/7, fe80::/10, 127/8, 0/8, ::1, :: —ErrPrivateAddress), redirect refusal (CheckRedirect: ErrUseLastResponse), body size cap, Content-Type allowlist, and magic-byte validation. The resolvedCoverURLfromMergeMetadataByFieldPriorityflows throughpersist→metadata_store.go→ DB → cover generation worker, which callsDownloadCoverProduction(the safe variant). This SSRF guard was already in place and is applied to this new code path.app_settings write scope:
SaveQuickBookMatchhard-codes the key"QUICK_BOOK_MATCH"— the PUT handler cannot write to any otherapp_settingskey regardless of request content.Injection / template rendering: All field labels and provider names rendered in
settings_metadata_field_priority.htmlcome from the server-sidefieldDefsandcontentProvidersslices (compile-time constants), not from user input or DB values.html/templateauto-escaping applies. Notemplate.HTMLorsafeHTMLcasts are present in the new code. Provider IDs stored inFieldProvider(P1–P4) are string-stored settings values used only as map keys intoallResults map[string]metadata.Metadata— no SQL interpolation, no shell exec, no template rendering of raw user-supplied provider strings.Provider/field key validation:
applyMatrixUpdatepasses unknownfe.Keyvalues throughsetFieldProvider/setFieldEnabledswitch statements that silently no-op on unrecognized keys. UnknownP1–P4provider IDs that don't match anyallResultskey are harmlessly skipped inmergeStringField/mergeSliceField. No injection risk; the worst outcome is a misconfigured priority chain that yields empty results.AuthN (authentication): All
/settings/*routes sit behind the globalusers.AuthMiddleware(seeapp.go:404), so unauthenticated requests are rejected before reaching any handler. The gap is AuthZ (admin role check), not AuthN.REVIEW VERDICT: 0 blocker, 1 major, 1 minor
171a287d60cf4bb72decScreenshot available locally at /tmp/field_priority_matrix.png — see PR description for details.
Matrix UI polish + Set All row — screenshot
What's visible:
settings-section--matrixgives the table room to breatheChanges:
static/css/main.css— addedsettings-section--matrix+ allfpm-*table stylestemplates/pages/settings_metadata_field_priority.html— applied new CSS, Set All row, renamed headersstatic/js/controllers/field_priority_matrix_controller.js— addedsetAll()action, fixed controller to usewindow.Stimulus.Controller(was wrongly usingwindow.StimulusController)static/js/test/field_priority_matrix_controller.test.js— 9 JS unit tests (one assertion perit)Security RE-Review (bot)
Prior finding 1: AdminRequired on GET + PUT /settings/metadata-field-priority
FIXED. Both routes are wrapped in
adminRequired(...)ininternal/settings/routes.go. The middleware is injected fromd.AdminRequired(declared ininternal/appwire/appwire.go) and passed throughsettings.Wire → RegisterRoutes. E2E tests confirm 401 on unauthenticated GET and PUT. No path bypasses the guard.One observation:
GET /settings/field-priority(the 301 redirect to the new canonical URL) is registered viamux.HandleFuncwithoutadminRequired. This is acceptable — a 301 with no body reveals nothing; the destination/settings/metadata-field-priorityis admin-guarded and will enforce auth after the redirect. No information is leaked.Prior finding 2: X-CSRF-Token missing from matrix PUT fetch
FIXED.
field_priority_matrix_controller.jsreadsdocument.querySelector('meta[name="csrf-token"]')and sends"X-CSRF-Token": csrfin the fetch headers, matching the pattern used by other controllers.Re-confirmed clean surface areas
DOM XSS (template rendering): The matrix template (
templates/pages/settings_metadata_field_priority.html) uses onlyhtml/template{{...}}interpolations. NoinnerHTML,.html(),template.HTMLcasts, ortemplate.JScasts appear. Field/provider labels (FieldRow.Label,providerOption.Name) are compile-time string literals fromfieldDefsandcontentProviders— they never originate from user input. Auto-escaping is complete.SSRF on cover_url path: The existing
cover/download.goSSRF guard (scheme allowlist + private-range IP block) is unchanged by this PR. The matrixcoverfield stores a provider priority string ("hardcover","google-books","open-library") — not a URL — so the matrix write path does not introduce a new cover-URL injection surface.app_settings write scope:
SaveQuickBookMatchuses the fixed keyQUICK_BOOK_MATCH. The handler never accepts an arbitrary key from the request body; only the structuredMatrixUpdateRequestfields reachUpsertAppSetting.Field/provider key injection via arbitrary keys:
setFieldProviderandsetFieldEnabledare exhaustiveswitchstatements over the 16 known field keys. Unknown keys inreq.Fields[*].Keyfall through to thedefaultno-op — they are silently discarded and cannot mutate unintended settings fields. Provider ID values (P1–P4) are stored as opaque strings in JSON; at resolution time they are compared against provider map keys inMergeMetadataByFieldPriority— an unknown value simply never matches, so arbitrary provider strings are harmless data noise, not an injection path.Set-All is client-side only:
setAll()in the controller manipulates DOM<select>values in the browser; it does not issue any network request. The single server boundary remains the existingsave()PUT, which is admin-guarded and CSRF-protected.REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Code RE-Review (bot)
PR #445 — bookshelf-4y3y.2 re-review (post-fix + new UI work)
CI: green (
a84aad7f). Reviewing the full diff againstgit merge-base origin/main origin/bd-bookshelf-4y3y.2.Prior Major #1 — Disabled/locked Authors & Categories preserve DB value
FIXED.
internal/books/merge.gonow routes Authors throughmergeSliceFieldand Categories throughmergeCategoriesField. Both functions takefallback []stringsourced fromexisting.Authors/existing.Categories(populated by the newlistAuthors/listCategoriescalls inGetMetadataForRefetch) and return it unchanged when!enabled || locked. Tests atmerge_test.go:524–590(authors disabled→preserve, locked→preserve) and:592–656(categories disabled→preserve, locked→preserve) confirm the correct semantics.Prior Major #2 — Old
/settings/field-priorityURL redirect + rating as matrix rowFIXED.
internal/settings/routes.go:12–16registers a301 MovedPermanentlyredirect fromGET /settings/field-priority→/settings/metadata-field-priority.fieldDefsinmatrix_handler.go:99includes{"rating", "Rating"}as the 16th row. The sidebar link intemplates/layouts/base.html:348was updated to/settings/metadata-field-priority. E2E test ate2e/api/settings_field_priority_test.go:35verifies the redirect.Prior Minor #1 — ReplaceMode allowlist validation
FIXED.
internal/settings/matrix_handler.go:116–122validatesreq.ReplaceModeagainstvalidReplaceModesbefore applying it; unknown values are silently preserved (old value kept). Test atmatrix_handler_test.go:400–426assertsUNKNOWN_MODEleavesReplaceModeunchanged.Prior Minor #2 — FieldRow JSON tags
FIXED.
matrix_handler.go:32–52:Key,Label,Enabled,P1–P4all have lowercasejson:"..."tags.New UI code review
setAll()/clearAll—field_priority_matrix_controller.jsThe controller (
static/js/controllers/field_priority_matrix_controller.js:21–34) correctly:tr[data-field-key]rows fromtbodyTarget, so the set-all header row (which has classfpm-set-all-rowbut nodata-field-key) is excluded."__clear__"→""for each field select.""(back to placeholder) after applying.No DOM bugs found. Column targeting via
selects[col]with an index guard is correct.JS tests —
field_priority_matrix_controller.test.js9 specs covering: set-all sets target column, applies to all rows, leaves adjacent columns untouched, clears via
__clear__, resets dropdown to placeholder; save() reports Saved/Save failed on ok/error; save() collects onlydata-field-keyrows; set-all + save integration. Eachithas oneexpect. Follows project conventions.Server-rendered Set All row
Template at
templates/pages/settings_metadata_field_priority.html:45–88places the set-all row inside<tbody data-field-priority-matrix-target="tbody">. The row usesclass="fpm-set-all-row"with nodata-field-key, soquerySelectorAll("tr[data-field-key]")in bothsetAll()andsave()correctly excludes it.CSS —
static/css/main.cssAll new rules use CSS variables (
var(--surface),var(--bg-input), etc.). No inline styles.white-space: nowrapon.fpm-field-labelprevents label wrapping. Priority columns arewidth: 160px. No!important, no hardcoded colors bypassing the theme.CSP safety
No inline event handlers. All Stimulus wiring via
data-actionattributes.field_priority_matrix_controller.jsis loaded via<script defer src="...?v={{.AssetVersion}}">inbase.html:65. Compliant.New findings
[MINOR]
templates/pages/settings_metadata_field_priority.html:54— redundant "— none —" in set-all selectscontentProviders(first entry:{ID: "", Name: "— none —"}) is ranged into the set-all dropdowns alongside the explicit<option value="">— set all P1 —</option>placeholder and the<option value="__clear__">Clear All</option>. This produces twovalue=""options: the placeholder and "— none —". Functionally harmless (both clear), but "— none —" in a "Set All" context is confusing to users — it looks like a provider option but actually clears the column. Fix: use a separatesetAllProviderslist that omits the{ID: ""}entry (relying on the explicit per-column placeholder instead).[MINOR]
internal/books/metadata_store.go:176–185—listAuthors/listCategorieserrors silently swallowedWhen
listAuthorsorlistCategoriesreturns an error, the slice is silently set tonil, which becomes the fallback for the merge engine. If a field is disabled or locked and the DB call for that field's existing value fails, the merge engine will returnnil(clearing the data) instead of preserving the real DB value. The test atmetadata_store_test.go:248acknowledges this — it asserts "nil Authors and no error". In practice, the failure window is narrow (main query succeeded, secondary query failed) and covered by CI. Acknowledge in a comment or log the error atWarnper the logging standard trigger ("Write a catch/except → log with context").REVIEW VERDICT: 0 blocker, 0 major, 2 minor
a84aad7f86260c05a51e