Finish Phase 1: in-tree EPUB reader, retire the frame-era result views, XSS phase 8 - #270
Merged
Conversation
EPUB import is a core feature, so it should not pull an external Composer
package to work. LWT now ships its own reader under
src/Modules/Book/Infrastructure/Epub/, covering exactly what the importer
consumed:
- EpubArchive read-only ZIP accessor, case-insensitive entry lookup and
path normalisation for hrefs like "text/../images/x.jpg"
- EpubReader OCF container -> OPF package -> manifest + spine, plus both
tables of contents (EPUB 2 NCX and EPUB 3 nav document)
- EpubBook / EpubDocument / EpubChapter the models the service reads
php-ebook also handled MOBI, CBZ and PDF, none of which LWT imports, so most
of it was unused. Removing it dropped four transitive dependencies as well
(kiwilan/php-archive, kiwilan/php-xml-reader, smalot/pdfparser,
spatie/temporary-directory), taking composer.lock from 20 packages to 15.
Parsing needs only ext-zip and ext-dom, both already required.
XML is parsed with LIBXML_NONET and without LIBXML_NOENT, so entity
substitution stays off and a hostile EPUB cannot mount an XXE attack. A test
covers this and is non-vacuous: the same payload does expand under NOENT.
EpubParserService keeps its public contract; only the types it names changed
(isNavigationFile now takes an EpubDocument). resolveFormat() is gone because
the reader detects EPUB from archive contents rather than a filename
extension, which is what #232 worked around. The $originalName parameter
stays and now sharpens error messages.
New: 14 EpubReaderTest cases building real EPUBs on disk, covering NCX and
nav TOCs, spine order, fragment hrefs, duplicate nav points, a missing TOC,
a malformed container, a non-ZIP file and the XXE payload.
This does not on its own remove the need for `composer install` from a source
checkout: phpmailer and the two oauth2 libraries are still required.
Refs #263
…, #262) /word/edit-multi was the last multi-word path that went through server-rendered HTML. It rendered a form, posted back to itself, and replied with edit_multi_update_result.php — a 45-line PHP view whose entire output was a JSON payload wrapped in a <script type="application/json"> tag, which word_result_init.ts read and applied to the opener's DOM. That whole loop is unreachable. The modern reader opens the Alpine multi-word modal instead, which calls POST /api/v1/terms/multi and PUT /api/v1/terms/multi/{id} via TermsApi. Nothing links to /word/edit-multi: the only references left were the route registration, the controller, the two forms whose action pointed back at it, and the tests asserting the route existed. The entry point disappeared when the frame reader was retired. Removed: - MultiWordController + its DI registration (editMulti was its only entry) - form_edit_multi_new.php, form_edit_multi_existing.php, edit_multi_update_result.php - the /word/edit-multi route - initEditMultiUpdateResult + EditMultiUpdateResultConfig in word_result_init.ts, and updateMultiWordInDOM, orphaned with it - WordContextService::exportTermAsJson, whose only caller was the controller Verified: /word/edit-multi now 404s while /words and /api/v1 stay 200; the rebuilt bundle no longer carries the handler; Cypress 09-reading (which covers the multi-word modal) passes 15/15, full suite 215/215; PHPUnit 9186 green; psalm and phpcs clean. This is one slice of #266 — eight *_result.php views remain, and they will follow the same pattern: check reachability first, convert only what is live. Refs #266, #262
Continues the *_result.php sweep. Both routes rendered a server-built page
whose only real payload was a JSON blob that word_result_init.ts applied to
`window.parent.document` — the reading frame. No frameset exists any more:
`frames-r` and `frame-l` appear nowhere in src/**/*.php, so for a same-tab
navigation `window.parent` is the result page itself and every
`querySelectorAll('.wordNNN')` update found nothing.
/vocabulary/term-hover -> superseded by POST /api/v1/terms/quick, which
word_popover.ts, word_modal.ts and text_keyboard.ts
already call
/word/set-all-status -> superseded by TextPositionApiHandler, which calls
the same WordDiscoveryService::markAllWordsWithStatus
Neither had an inbound link: the only references were their own controller
docblocks, the route registrations, and a RoutesTest entry.
Removed: both routes, hover_save_result.php, all_wellknown_result.php,
TermDisplayController::hoverCreate + createFromHover, the CreateTermFromHover
use case (orphaned with them) and its DI registration,
TermStatusController::markAllWords, the initHoverSaveResult /
initAllWellKnownResult handlers, and the matching tests.
markAllWordsWithStatus itself stays — TextPositionApiHandler is a live caller.
markWordWellKnownInDOM / markWordIgnoredInDOM stay too, used by text_keyboard
and simple_interactions.
Verified: both routes now 404 while /words, /word/edit-term, /word/upload and
/api/v1 stay 200; PHPUnit 9164 green, Vitest 4413 green, psalm and phpcs clean,
Cypress 215/215.
Refs #266, #262
Completes the *_result.php sweep. Four views still emitted a
<script type="application/json"> blob that word_result_init.ts applied to
window.parent.document — the reading frame.
That frame is gone. `frames-r` and `frame-l` are emitted nowhere in
src/**/*.php, so on a same-tab navigation window.parent IS the result page and
every querySelectorAll('.wordNNN') found nothing. `#learnstatus`, the target of
updateLearnStatus, is likewise emitted nowhere. closeParentPopup and
cleanupRightFrames both guard on window.parent !== window, so neither fired.
The visible confirmations stay. What goes is the dead payload plus the
server-side work that existed only to build it:
- edit_term_result: two QueryBuilder lookups (WoLgID, LgRegexpWordCharacters)
and the {…} sentence masking, all feeding an unused $sent1
- edit_result / edit_term_result / bulk_save_result: a getTodoWordsContent()
call each, plus tag-list and hex computation
- handleBulkSave: $tid and $cleanUp are no longer needed, so the signature and
its psalm-suppress drop to just $terms
save_result.php is deleted outright. Once the blob went, its only output was
the message createWord already echoes a few lines earlier, so single-word saves
had been rendering it twice — this fixes that.
bulk_save_result showed a spinner labelled "Updating Texts" that the JS removed
on load, i.e. an empty page. It now reports what happened, via a new
vocabulary.result.bulk_saved key ("Saved {count} terms."); the superseded
result.updating_texts is dropped from all nine locales.
upload_result.php is deliberately left alone. Its config blob feeds a real
Alpine component that fetches /api/v1/terms/imported — the CSP-safe pattern
CLAUDE.md prescribes, not frame plumbing.
Also removed: word_result_init.ts and its test, the orphaned updateLearnStatus,
updateNewWordInDOM, updateBulkWordInDOM, updateTestWordInDOM and
completeWordOperation helpers, and show.php's data-lwt-cleanup-frames marker.
updateExistingWordInDOM, updateWordStatusInDOM and the markWord*InDOM pair stay
— text_keyboard and simple_interactions call them after an API write.
Verified: no result-config blob remains in src/ or in the built bundle;
PHPUnit 9164 green, Vitest 4389 green, psalm and phpcs clean, Cypress 215/215.
Refs #266, #262
frame_management.ts had no importer anywhere in src/. Its five exports were all frame-era: showRightFramesPanel/hideRightFrames/cleanupRightFrames target #frames-r and #frame-l, neither of which is emitted by any PHP view, and initHideRightFramesHandler bound a data-action attribute nothing uses. successSound/failureSound were dead here too. The live sound playback is review_store.playSound, which had its own inline copy; shared/utils/ audio_feedback.ts held a third. Consolidated onto audio_feedback, porting review_store's currentTime rewind so rapid consecutive answers stay audible. closeParentPopup and the lwt-close-popup listener in word_popup.ts go with it: frame_management was the only caller, and the guard window.parent !== window is never true without a frameset. Five test files vi.mock'd frame_management for a loadModalFrame export it never had; those mocks are removed.
/terms/for-edit already returned lemma, notes, tags, allTags, similarTerms and the language's translateUri; term_edit_modal rendered none of them and hardcoded its English labels. It now renders every field from the payload and takes its strings from the i18n catalogue, so it is a real replacement for the server-rendered form rather than a reduced one. Two backend gaps closed: - getTermForEdit required a text context even when given a word ID, so any caller without one got "Text not found". An existing term carries its own language, so WoLgID is now the source when wid is supplied. That is what lets the review screen open the editor. - updateTermFull could not write WoText. The legacy form only ever allowed recasing (it asserts mb_strtolower($text) === $textlc), so the API takes the same constraint: WoTextLC is what textitems2 links on, and a genuine rename would orphan every occurrence. The modal mirrors the check client side to avoid a pointless round trip. Review's two /word/edit-term navigations now open the modal in place, and the table refreshes on lwt-term-saved. The route survives as a bookmarkable fallback but no UI links to it any more. Also removed, found while tracing callers: - ReviewController::tableReview and its three views. The method is neither routed nor called — the Alpine tableReview component replaced it — so it was ~180 lines of PHP-emitted table HTML nothing could reach. - word_modal's getEditUrl, superseded by its own in-modal edit form.
import_result.php was the last server-rendered result page: /book/import took a multipart POST, imported the file, and echoed an outcome page. There was no POST endpoint for books, so converting it meant adding one. POST /api/v1/books takes the upload and returns the outcome as JSON. Both callers now post there and render the result in place: - /book/import gets an epubImportForm component that swaps the form for the outcome panel without a page load. - /texts/new intercepts submit only when the picked file is an EPUB; every other source keeps its native POST to /texts/new. BookController::import is left serving the form alone, and processImport / showImportResult are gone. Two defects found by driving the real browser rather than curl: - TextTags arrives as an array once Tagify has replaced the input, and the (string) cast raised a warning the exception handler turned into a 500. curl never sends that shape, so only the E2E run reproduced it. The cast predates this change — it moved out of processImport unaltered — and is now a parser that takes either shape. - apiPostMultipart rendered an error envelope's `error: true` as the literal string "true"; only a string field is usable as a message. cypress/e2e/11-book-import.cy.ts covers the flow end to end, including a real import. It needed a new cy.waitForAlpine(): x-data markup is server rendered, so asserting on it passes even when Alpine never ran, and acting before hydration misses the component's listeners.
) edit_result and edit_term_result -------------------------------- These confirmed a form POST on a page rendered with renderPageStartNobody — no navbar, no links, a dead end left over from the frameset era. The forms behind them (form_edit_new, form_edit_existing, form_edit_term) duplicated an editor the modal already implements in full. term_edit_modal now separates loading and wiring from presentation, so the same editor serves both hosts: loadTermEditor renders it, wireTermEditor attaches handlers, and the host decides what "done" means — the modal closes, the page navigates. /word/edit, /word/edit-term and /words/{id}/edit render only identifiers and let termEditPage mount the editor from GET /api/v1/terms/for-edit. Their POST routes are gone. bulk_save_result ---------------- Saving a batch posted the whole form back, which saved, echoed a count, and rendered the next batch in one response. POST /api/v1/terms/bulk now does the write and the next batch is a plain GET, so no HTML comes back from a write. The offset arithmetic (saved terms leave the unknown-word set) moved to the client unchanged. upload_result ------------- I called this one a non-violation last time. It was not: x-data was an inline object literal *and* a call with arguments, and the page picker ran goToPage(parseInt(...)) — a JS global, undefined in CSP eval scope. Both are listed in CLAUDE.md as things that break. The component already read a JSON blob in init(); the view simply never emitted one. It does now. Also removed: word_form_auto.ts, whose only config host was form_edit_new.php. /word/edit?wid=0 previously reached a RuntimeException and rendered an error page; an early return would have made it a blank one, so it throws instead.
Four DOM sinks assembled HTML by interpolating untrusted values into a markup string, so a quote in the data escaped its attribute: - bulk_translate.ts put Google Translate output into value="…" - expression_interactable.ts serialised a term's attributes into ' k="v"' pairs that user_interactions.ts parsed back via innerHTML, carrying WoTranslation / WoRomanization — both import-plantable - language_list.ts wrote user-supplied language names into a data attribute and a text node when rebuilding the default-language button - language_form.ts rendered LibreTranslate errors, which can quote the configured translator URL, as markup All four now go through createElement/setAttribute/textContent or escapeHtml. newExpressionInteractable takes the attribute map it was always being handed instead of a markup string, which is less code as well as safer; its label is textContent, so ExpressionService sends a literal non-breaking space rather than the entity. Also adds JSON_HEX_TAG | JSON_HEX_AMP to five json_encode calls feeding <script type="application/json"> blocks. These were NOT exploitable: PHP escapes / as \/ by default, so a payload emerges as <\/script>, which the tokenizer does not accept as a closing tag. The flag removes the dependency on that default, which Home/Views/helpers.php already opts out of with JSON_UNESCAPED_SLASHES. A changelog claim that json_encode leaves / unescaped is corrected in the same entry. Since this class has now been swept three times, JsonScriptBlockEscapingTest walks every json_encode call in src/ and fails when one whose output lands in a <script> omits the flag. It found the MediaService site that reading had missed. Also fixes TermEditControllerTest, which still asserted the pre-a6379b39 "return early" behaviour and had been erroring on main since that commit. Roadmap: Phase 1 is complete — its last open bullet listed nine *_result.php views that no longer exist.
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.
Closes #266. Closes #263.
Nine commits that finish Phase 1 of the roadmap (frontend de-coupling), plus one security pass. Net −2583 lines.
1. EPUB reader, in-tree (#263)
kiwilan/php-ebookis replaced bysrc/Modules/Book/Infrastructure/Epub/, which reads the OCF container, the OPF package document, and both tables of contents (EPUB 2 NCX and EPUB 3 nav) — everything the importer consumed. Four transitive dependencies go with it (composer.lock: 20 packages → 15). Parsing needs onlyext-zipandext-dom, both already required, with entity substitution off and network access disabled so a hostile EPUB cannot mount an XXE.2. The frame era is over (#266, #262)
LWT hasn't had a frameset for some time, but a lot of code still assumed one. Result views emitted a JSON blob that got applied to
window.parent.document— for a same-tab navigation that is the result page, so every update found nothing and silently did nothing.Removed across four commits:
word_result_init.tsand its orphaned DOM helpers,frame_management.ts(zero importers), the unreachable multi-word edit page, two superseded term routes, andMultiWordController.save_result.phphad been printing its message twice.Nine
*_result.phpviews are now gone. The one file still named that —Vocabulary/Views/upload_result.php— stays on purpose: it's a live Alpine table backed byGET /api/v1/terms/imported, not a server-rendered fragment. It did need a CSP fix (inlinex-dataobject literal taking arguments, and aparseIntcall in a binding).3. One term editor instead of four
/word/edit,/word/edit-termand/words/{id}/editeach rendered their own PHP form that posted back to a confirmation page with no navbar and no links. They now emit identifiers only and mount the same API-driven editor the reading view opens in a modal —loadTermEditorrenders,wireTermEditorbinds, and the host decides what "done" means (the modal closes, the page navigates). Bulk save moves toPOST /api/v1/terms/bulk; EPUB import moves toPOST /api/v1/books.4. Security — phase 8
Four DOM sinks built HTML by interpolating untrusted values into a markup string, so a quote in the data escaped its attribute:
bulk_translate.ts—value="…"expression_interactable.ts→user_interactions.tsWoTranslation/WoRomanization, plantable via CSV or ePub importlanguage_list.tslanguage_form.tsAll four now use
createElement/setAttribute/textContentorescapeHtml.newExpressionInteractabletakes the attribute map it was always being handed rather than a markup string — safer and less code.Five
json_encodecalls feeding<script type="application/json">also gainedJSON_HEX_TAG | JSON_HEX_AMP. To be clear, these were not exploitable: PHP escapes/as\/by default, so a payload emerges as<\/script>, which the tokenizer won't accept as a closing tag. The flag removes the dependency on that default — whichHome/Views/helpers.phpalready opts out of viaJSON_UNESCAPED_SLASHES. An older changelog entry asserting thatjson_encodeleaves/unescaped is corrected here.Since this class has been swept three times now,
JsonScriptBlockEscapingTestwalks everyjson_encodecall insrc/and fails when one whose output lands in a<script>omits the flag. It found a site inMediaServicethat reading had missed.Each regression test was checked against the pre-fix code to confirm it actually fails there.
Verification
PHPUnit 9167 · Vitest 4355 · Cypress 228/228 · Psalm clean · PHPCS clean at
--warning-severity=1· ESLint and typecheck clean.Two E2E specs are new:
11-book-import.cy.tsand12-term-editor.cy.ts. Acy.waitForAlpine()helper was added after an earlier assertion turned out to be vacuous — asserting on a server-renderedx-dataattribute proves nothing about whether Alpine ever mounted.Notes
TermEditControllerTesthad been erroring since a6379b3 (it still asserted the pre-change "return early" behaviour); fixed here.Endpoints.phpallows onlyGET/POSTonbooks, soDELETE /api/v1/books/{id}is registry-blocked with a 405 despiteBookApiHandler::routeDeleteexisting.StarterVocabController.php:189.