Skip to content

Secure opaque-origin viewer, SCORM/xAPI bridge and opaque editor preview - #80

Draft
erseco wants to merge 102 commits into
mainfrom
feature/secure-iframe-scorm-bridge
Draft

Secure opaque-origin viewer, SCORM/xAPI bridge and opaque editor preview#80
erseco wants to merge 102 commits into
mainfrom
feature/secure-iframe-scorm-bridge

Conversation

@erseco

@erseco erseco commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Published viewer

An .elpx renders in a sandboxed, opaque-origin iframe served through the
Moodle file API under a response-level sandbox CSP, so author HTML and JavaScript
cannot reach the Moodle page, its cookies or the session. The package only ever
receives a read-only file token — never the sesskey.

Grading still works because the calls are relayed, not granted: SCORM 1.2/2004
and xAPI are validated on the trusted parent by window identity
(event.source === iframe.contentWindow), a closed action list and a per-view
nonce, then POSTed by the parent itself. An opaque origin has no usable
event.origin, which is exactly why window identity is the anchor.

External video and PDFs would blank out under that sandbox, so the shared eXe
shim demotes each provider embed inside the package and a relay on the activity
page overlays the real player in its place — no separate subdomain needed.

Secure mode is never silently downgraded: where it cannot render (a php-wasm
Playground service worker cannot serve an opaque subframe) the page shows a
"blocked by security configuration" notice instead of falling back.
EXELEARNING_UNSAFE_LEGACY_IFRAME is a development-only escape hatch for that
environment — not an administration setting, and never for the editor preview.

Editor preview

Filtered by default. Enabling active content POSTs the whole project as one ZIP
to an authenticated endpoint and serves it from an authless capability URL under
the same sandbox CSP, with a 30-minute idle TTL:

POST/DELETE  editor/preview_session.php?cmid&sesskey   require_login + sesskey + capability
GET          preview.php/{previewId}/{path}            authless, cookieless

Archives are vetted before a byte is written: entry count, declared uncompressed
size, path traversal and symlinks. Limits default to 1 GB / 10 000 entries and
cannot be switched off — a non-positive value falls back to the default.

The plugin previously emitted a preview contract the editor no longer reads,
which left the opaque preview unreachable — it failed closed and silently stayed
filtered. Aligning it with the contract the editor does read accounts for the
~3 800 lines this branch removes.

Notes for review

  • serving keeps the pieces that must not drift from eXe core — the CSP is
    asserted byte-identical — and serve() confirms the resolved realpath sits
    under the snapshot root.
  • The dev-only hatch and the preview CSP are deliberately independent: the
    preview hardcodes its sandbox tokens so the published-content hatch can never
    widen it.

Moodle Playground Preview

The changes in this pull request can be previewed and tested using a Moodle Playground instance.

Preview in Moodle Playground

ℹ️ The eXeLearning editor is fetched from the shared release and unpacked into the plugin when the playground boots, so the first load may take a few extra seconds. ELPX upload, viewer and preview work normally.

@codecov

codecov Bot commented Jun 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.61290% with 39 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
classes/local/ui/player_iframe.php 76.47% 12 Missing ⚠️
js/exe_embed_shim.js 83.60% 10 Missing ⚠️
js/exe_embed_relay.js 94.15% 9 Missing ⚠️
classes/local/package_manager.php 75.00% 4 Missing ⚠️
lib.php 0.00% 3 Missing ⚠️
js/scorm_bridge_relay.js 98.55% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

erseco and others added 14 commits June 13, 2026 09:26
…load fails

On a host whose service worker cannot serve an opaque-origin iframe (e.g. the
PHP-WASM Moodle Playground), the token URL falls through to a 404 and the in-iframe
shim never announces 'ready'. The relay watchdog already reveals the
"blocked by security configuration" notice instead of degrading to same-origin, but
it waited a flat 8s, during which the iframe sat blank with no notice -- so secure
mode "looked like it worked" for several seconds.

The watchdog now reacts to the iframe element's 'load' event (which fires even when
the navigation ends in an error page such as that 404) and grants only a short grace
(~2.5s) for the handshake; if 'load' never fires it still falls back to the 8s cap.
The load listener attaches immediately if the iframe is present, otherwise on
DOMContentLoaded (the relay is injected inline before the iframe element).

Verified empirically in the Playground via Chrome DevTools: the iframe is built in
secure mode (opaque, sandbox without allow-same-origin), tokenpluginfile returns 404,
and the notice is shown with the iframe hidden -- the token does not help there
because the blocker is the service worker, not the cookie (DEC-0060).

Vitest 52/52 (two new tests cover the load-driven fast path).
The Playground's PHP-WASM service worker cannot serve an opaque-origin iframe, so
secure mode there only ever shows the "blocked by security configuration" notice. Set
iframemode=legacy in the blueprint setConfigs so the preview actually renders the
package and is useful. This is a Playground-only override applied at boot; real Moodle
keeps the secure default.
…e the iframe

Defense in depth for secure mode: the package is served via tokenpluginfile with an
executable CSP, so opening the token URL top-level (e.g. in a new tab) would run author
JS as Moodle's origin. Add a `sandbox allow-scripts allow-popups allow-forms` CSP
directive (secure mode + HTML only, via content_headers) so the document keeps an opaque
origin however it is loaded. Tokens mirror the secure iframe sandbox; the SCORM
postMessage bridge is unaffected because the iframe is already opaque. Unit test asserts
the directive is present in the secure CSP.

Raised while reviewing the sibling omeka-s-exelearning secure-iframe work; applied here
for consistency across the eXeLearning embedders.
In secure mode the package runs in an opaque-origin sandbox, which leaves
YouTube/Vimeo players and PDFs blank (the sandbox flag propagates to nested
iframes; Chrome also blocks its PDF viewer without allow-same-origin). Promote
those embeds to the trusted parent: a shim baked into the package replaces
whitelisted-video / .pdf iframes with placeholders and reports their geometry
via postMessage; an inline relay on the activity page validates + rebuilds the
URL and overlays the real player inline over each placeholder.

- js/exe_embed_shim.js: in-iframe, self-activates only in the opaque origin
  (dormant in legacy); dual-export for Vitest.
- js/exe_embed_relay.js: parent-side validate (host whitelist + canonical URL
  rebuild + same-origin package-file invariant for PDFs) and inline overlay.
- package_manager + scorm_injector: bake the shim + whitelist into every page
  head, alongside (and independent of) the SCORM bridge.
- player_iframe::embed_whitelist(); relay inlined in view.php (secure only).

PDFs: local package PDFs always render; any https .pdf renders; same-origin
PDFs must belong to this package (served as application/pdf, never executable).

Tests: Vitest validator/promote (exe_embed.test.js) + SCORM coexistence guard
(embed_scorm_coexistence.test.js); scorm_injector_test asserts the shim is baked
without dropping the SCORM bridge. Documented in DEC-0061. Fixtures under
research/fixtures/elpx/.
mod_exelearning serves package assets with relative URLs (unlike the wp/omeka
proxies, which rewrite to absolute), so a locally-packaged PDF was reported to the
parent relay as a relative src. The relay resolves URLs against the host page, not
the content, so it rejected the relative path and the local PDF did not render.

The shim runs inside the content, so it now resolves each src against the content
location and reports the ABSOLUTE URL. Verified live (secure mode): the embeds demo
renders 4 players inline (YouTube, Vimeo, remote PDF, local package PDF). Adds a
Vitest regression test (a relative src is reported absolute). Updates DEC-0061 with
the live results, including the SCORM scoring validation (track.php saved a full
attempt; gradebook + attempts report reflect it) confirming the embed shim does not
break scoring.
Comment thread js/exe_embed_shim.js Fixed
- Playwright/Firefox e2e (playwright-embed.config.cjs + tests/e2e/): loads the real
  shim + relay against an opaque-origin sandboxed harness and asserts a whitelisted
  YouTube embed and a RELATIVE local PDF are promoted to inline parent players while a
  non-whitelisted iframe is not. Proves the promote-to-parent mechanism works in
  Firefox, not just Chromium. Run with `npm run test:e2e:embed`.
- Vitest: add coverage for relay makePlayer() (video vs PDF attributes) and shim
  collect() (geometry report). The browser-bootstrap paths (init/report/observer) are
  covered by the e2e.
- README: document external embeds in Secure mode (whitelist + PDF policy + how to
  run the tests).
Comment thread tests/e2e/embed.spec.cjs Fixed
erseco added 3 commits June 14, 2026 06:54
…tch gate

- CodeQL (high): the e2e spec asserted a non-whitelisted host was absent with
  src.includes('example.com') ("incomplete URL substring sanitization"). Rewrite the
  assertions to parse exact hostnames (new URL().hostname) and an anchored regex for the
  canonical YouTube URL, so no URL is checked by substring.
- Codecov patch: cover the relay's createRelay()/onMessage -> overlay player path and
  the instance validate() in Vitest, and mark the browser-only bootstrap of both the
  shim (init/report/observer) and the relay (init/pingAll/scheduleReflow) as v8-ignore
  (they require a framed, opaque-origin window and are exercised by the Firefox e2e, not
  happy-dom). exe_embed_relay.js 95% / exe_embed_shim.js 84% line coverage; 77 unit tests.
…dow first

The vendored pipwerks API.get() started its lookup at win.parent and skipped the
current window. In secure (opaque-origin) mode the SCORM API is provided locally
as window.API by the in-iframe bridge shim (DEC-0059) and the Moodle parent is a
cross-origin/opaque frame, so reaching into parent.API threw SecurityError,
init() never activated the connection, and every LMSSetValue/LMSCommit became a
silent no-op -- no attempt rows were ever written. Legacy (same-origin) mode
masked it because the parent there hosts the API.

Restore the standard pipwerks order: try find(window) first, then fall back to
the parent and opener, with every cross-origin hop wrapped so an opaque ancestor
can never abort the lookup. Legacy keeps working: with no local API, find(window)
walks up to the same-origin parent exactly as before.

Add tests/js/scorm_api_wrapper.test.js (loads the vendored wrapper with a
controllable window) covering current-window-first, opaque-parent safety, the
null fallback, the legacy walk-up, and the full init()/set() save path. Document
the root cause and the DEC-0059 verification gap in DEC-0062.
…eometry

Extend the external-embed allowlist with Dailymotion (www/geo.dailymotion.com,
/embed/video/{id}) and EducaMadrid/Mediateca de Madrid (mediateca.educa.madrid.org,
/video/{id}/fs), each with a per-provider canonical-URL validator in the relay so
only a reconstructed, known-good embed URL is ever rendered. Clamp the relayed
player overlay to the placeholder's rect (defence in depth against geometry-driven
clickjacking; the overlay already clips with overflow:hidden). Mark the shim/relay
as the canonical source for the wp/omeka mirrors. Refresh the demo fixture and the
Vitest + Firefox e2e coverage with the new hosts and their reject cases.
Comment thread js/exe_embed_shim.js Fixed
The in-iframe shim restarts its embed-id counter on every page, so after the
content navigates (e.g. eXe multi-page next/prev), the new page's first embed
reuses id exe-embed-1. The relay reused the existing player for that id and only
repositioned it, never updating its src -- so the previous page's video (e.g.
YouTube) lingered on the next page, stretched to the new box.

Tag each player with the URL it renders (data-exe-embed-src) and, in sync(),
replace the player when a reused id now maps to a different URL instead of
repositioning the stale one. Add a regression test covering a reused id that
navigates from YouTube to Vimeo.
erseco added 6 commits July 11, 2026 13:44
store_assets() called evict_others_for_budget() inside the per-entry loop, and
that unconditionally enumerate()d the whole store (scandir + read_json of every
session's meta) once PER asset — a 5000-file batch meant thousands of full-store
scans in one request (O(entries × sessions)). Take the global-ceiling snapshot at
most ONCE per batch and evict from (and mutate) it in place across entries, never
re-enumerating; per-entry rejection semantics are unchanged. evict_others_for_
budget() (apply_revision's single call) now delegates to the same two helpers,
keeping one source of truth. A test proves enumerate() runs once for a five-asset
batch (instrumentation seam), plus one covering the LRU-eviction path.
…t 416)

parse_range() checked satisfiability (first-byte-pos >= length -> 416) BEFORE the
inverted-spec guard (last < first -> ignore), so bytes=15-2 on a <=15-byte asset
returned 416. The frozen contract (§4) requires structural invalidity to win: an
inverted byte-range-spec is invalid per RFC 9110 and is ignored (served as a full
200), even when the first-byte-pos is also beyond the body. Reordered the checks
and added bytes=15-2-on-a-short-body to test_parse_range.
The bootstrap neutralizes the static editor's preview-sw.js registration to keep
its 404 (on hosts that don't serve the SW script) off the console. It returned a
bare { scope: "" }, but the editor's preview provider treats the registration as
an EventTarget — registration.addEventListener("updatefound", …) — and reads
installing/waiting/active, so the bare object threw "addEventListener is not a
function" and aborted the preview/export iframe. Return a faithful shape instead:
a non-empty scope, null workers (so the provider's activation wait resolves at
once and it claims no clients) and no-op event/lifecycle methods, letting the
provider complete and fall back cleanly. Verified against the bundled editor's
ServiceWorkerPreviewProvider usage in dist/static/app/app.bundle.js.
…ot Location

Re-vendor tests/fixtures/preview-contract/vectors.json verbatim from core
(5675623a): adds the bare-root-302 step and the Range cases garbage / multi /
non-bytes / bytes=5-2 / bytes=15-2 -> 200-ignore and bytes=-0 -> 416. serve() and
parse_range already produce these; the conformance harness gains plumbing for the
two new step shapes (bare-root redirect before session lookup, and {previewId}
substitution in expected header values) without forking the JSON.

The bare-root redirect now emits a RELATIVE Location resolved against the request
URL — "{previewId}/index.html" without a trailing slash, "index.html" with one —
via serving::bare_root_location(), so it is correct under any $CFG->wwwroot
subdirectory and byte-matches the canonical vector (was an absolute wwwroot URL).

Also reword the intro away from the removed `srcdoc` fallback: the embed uses the
HTTP preview or fails closed; static-service-worker is standalone-only.
…comment

test_editor_bootstrap_sw_stub_is_faithful asserted the bare `{ scope: "" }` stub
was absent from editor/index.php, but the code comment that documents WHY the
faithful shape is needed names that literal — so the negative substring match hit
the comment and failed on every matrix cell. Retarget the assertion at the actual
bare RETURN pattern (`Promise.resolve({ scope: "" })`), which the comment does not
contain and a regression would; the positive checks (fakeSwRegistration, the
no-op addEventListener) are unchanged. Test-only; the stub itself is correct.
@erseco erseco changed the title Secure opaque-origin iframe: published-content sandbox + SCORM/xAPI bridge + HTTP editor preview (serving contract v2) Secure opaque-origin viewer, SCORM/xAPI bridge and HTTP editor preview v2 Jul 12, 2026
erseco and others added 14 commits July 20, 2026 14:12
…he URL

Resolved eleven conflicts. The mechanical ones (README, lang catalogs, the
notas index) took main's rewritten bullets and dropped the strings the removed
runtime installer left behind — confirmuninstall*, embeddededitor*,
stillworking were defined in lang/ and referenced nowhere in code. The
cspprofile*, embedmode* and securemodeblocked strings this branch adds are
kept.

settings.php keeps this branch's cspprofile/embedmode settings and takes
main's DEC-0066 editor toggle after them.

view.php and js/scorm_tracker.js needed real integration, not a side. Main
moved the session key out of the endpoint URL into the POST body (SEC-04) via
tracking_endpoint::scorm_config()/xapi_config(); this branch had built both
URLs by hand and branches on secure mode. Both clients now consume the shared
config, and the xAPI listener still swaps allowedOrigin for iframeid under
secure mode, where the package's event.origin is "null".

That merge silently broke secure mode: track.php now authenticates with
require_body_sesskey(), but the parent-side relay carried no sesskey at all —
it used to ride in the URL. js/scorm_bridge_relay.js now takes it from its
config and puts it in the body it POSTs. Its unit test asserted the body
shape but never configured a sesskey, so the omission passed unnoticed; it
now does, and a second test pins the key to the body and out of the URL.

Not resolved here: main and this branch both used DEC-0065, DEC-0066 and
DEC-0067 for different decisions, so six ADR files now share three ids. The
index keeps both sets verbatim rather than guessing a renumbering.
Mirrors the change already in wp-exelearning: exe_media_host.js is the
canonical parent-side media host shared by the plugins, and it grows
closeAll() plus an IntersectionObserver that ties a promoted player's
lifetime to its source iframe. A player opens in a top-layer <dialog> on
the trusted page, so without this it outlives the iframe it belongs to and
floats above whatever replaces it — including the editor overlay, which
editor_modal.js now closes it before opening.
main and this branch each landed decisions numbered DEC-0065, DEC-0066 and
DEC-0067, so the merge left six ADR files sharing three ids and comments that
could mean either decision — view.php said DEC-0065 for xAPI-primary while
settings.php said DEC-0065 for the bundled editor.

main is the trunk and its numbers are already published, so this branch moves:

  DEC-0065 → DEC-0069  xAPI over the secure bridge (window identity)
  DEC-0066 → DEC-0070  teacher-mode via the core ?exe-teacher parameter
  DEC-0067 → DEC-0071  unified external media in an opaque origin

Renumbered per file rather than by a blind search-and-replace, because most
files carry references to only one side: settings.php, lib.php, editor/*, the
editor docs and the bundled-editor tests keep main's numbers untouched.
Six strings this branch added existed only in lang/en: the four cspprofile*
settings and previewsessionbusy / previewsessioncleanup, the latter two
raised by session_store and the cleanup task at runtime. They are now in ca,
es, eu and gl, carrying the leading '~' machine-translation marker that
scripts/check-package.sh strips on release, so a reviewer can tell them from
the human-reviewed strings.

The other direction was broken too: iframemode, iframemode_desc,
iframemode_legacy, iframemode_secure and installstale survived in the four
translations after lang/en dropped them. iframemode is only ever read as a
config key in tests, never through get_string, and installstale belonged to
the removed runtime installer. Deleted, so all five packs now define exactly
the same 341 keys.
The bundle-only refactor on main dropped get_active_dir() from
embedded_editor_source_resolver — with moodledata gone as a source there is
only the bundled directory left — but preview/fixed_resources.php still called
it. Nothing conflicted textually, so the merge produced a tree that fataled the
moment a preview session resolved its fixed resources: two suite errors, both
"Call to undefined method ... get_active_dir()".

get_editor_dir() carries the same contract (validated absolute path, or null
when there is no usable editor), so the call moves there.
The editor stopped speaking protocol v2: the bundle built from the current
branch has no reference to previewHttp at all and looks for previewSnapshot /
managementUrl / servingBaseUrl instead. editor/index.php still hands it the v2
activation block and nothing else, so enabling external scripts in an activity
finds no route it understands and falls back to the filtered preview. The
opaque editor preview is, in effect, unreachable here today.

This is the first piece of the replacement, and it is additive: nothing calls
it yet, so the branch keeps building and testing exactly as before.

snapshot_store keeps a whole project under one capability id, replacing the
layered v2 store (immutable asset keys, incremental revisions, fixed
installation resources) — machinery that existed only to avoid re-uploading
unchanged bytes. Content sits in its own content/ subdirectory so an author
path can never collide with the store's own files, which is what forced the
reserved-name checks in the WordPress port. Writes are staged beside the live
tree and swapped in, so a reader sees the old snapshot or the new one, never a
half-written one.

zip_inspector vets an archive before a byte is written: entry count, declared
uncompressed size, symlinks, and — reusing serving::normalize_content_path —
the same path rule the serving side enforces, so an entry that could not be
requested back can never be stored in the first place.

Still to come: the value object, the two endpoints, the editor config block,
the v2 removal and the tests.
Twelve cases over the two classes added in the previous commit, while nothing
calls them yet: round-trip, whole-tree swap (a file dropped from the new ZIP
disappears), owner and course-module scoping on both replace and delete,
refusing an unknown capability rather than silently minting it, idle expiry and
the sweep, the idle clock being pushed back on serving, and the four archive
rejections — missing index, traversal escape, entry count, total size.

Two of them pin properties that are easy to lose in a refactor: a rejected
upload leaves no .staging- directory behind, and a non-positive configured
limit falls back to the default so the guard cannot be switched off from the
admin page.
amd/build is committed in a Moodle plugin and must match amd/src; the closeAll()
call added to amd/src/editor_modal.js never made it into the compiled module, so
moodle-plugin-ci's Grunt step failed the whole matrix with "File is stale and
needs to be rebuilt". The first run buried that behind an unrelated
"moodle-plugin-ci: command not found" in every step, which is why this took a
rerun to see.

Rebuilt with Moodle's own grunt on Node 22. Only editor_modal changed; the other
four AMD modules recompile byte-identical, so this is the one bundle that was out
of date rather than a toolchain difference.
moodle-plugin-ci phpdoc --max-warnings 0 started failing the whole matrix with
the commit that added snapshot_store and zip_inspector. It is those two files:
the step runs before Grunt, and the previous run reached Grunt, so PHPDoc was
still clean one commit earlier.

The CI log for that step prints only the list of scanned files and no messages,
and it could not be reproduced locally (moodlecheck needs a Moodle bootstrap and
the local checkout's config.php asks for a pdo/sqlite3 driver this PHP lacks).
So this is the one construct in the new files that appears nowhere in code that
was already passing: `@return true|string`, a literal type moodlecheck does not
recognise. Everything else — the array shapes, the unions, `@var` on a const —
is used by serving.php and session_store.php, which pass.

bool|string describes the same contract. If the gate stays red the next suspect
is the array-shape unions, which are new in combination even though shapes
themselves are not.
The real message, finally read by running local_moodlecheck against the plugin
inside the dev container:

  snapshot_store::set_limits_for_testing has incomplete parameters list
  zip_inspector::inspect has incomplete parameters list   (source: functionarguments)

The functionarguments sniff splits a @param line on the first comma, so a type
carrying one — array<string,int>, array{maxfiles:int,maxbytes:int} — swallows the
variable name and the parameter looks undocumented. serving.php gets away with
the same shapes because they sit in @return, which is not parsed that way. The
literal `true` I changed in the previous commit was never the problem; that guess
was wrong, as were the two before it.

@param now takes a plain array and the shape is described in words. Verified
locally against the same checker CI runs: zero findings in the plugin's own
files.
The plugin handed the editor a `previewHttp` block for a protocol the current
build does not read, and never handed it the `previewSnapshot` one it does. The
opaque preview was unreachable in Moodle: enabling external scripts found no
route it understood and fell back to the filtered preview.

editor/index.php now emits previewSnapshot, with an explicit deleteUrlTemplate —
the client's default delete target appends /{previewId} to managementUrl, and the
URL constructor drops the query string when it does, taking cmid and sesskey with
it.

editor/preview_session.php drops the four-operation dispatcher for the two the
contract has: POST one whole ZIP (minting or replacing a capability), DELETE by
id. The auth preamble is unchanged — require_login + sesskey + capability on the
module context, owner- and cmid-scoped.

serving keeps what was worth keeping: the CSP byte-identical to core, path
normalization, MIME resolution, Range/ETag, the capability-URL parsing and the
hardening headers. serve() now takes a directory instead of a session object, and
confirms the resolved realpath sits under the snapshot root, so a symlink that
somehow survived extraction still cannot aim the response outside it. The
layered-protocol half — create-session, asset upload, revision publication — goes,
along with session_store, preview_session and fixed_resources.

Net: 3832 lines out, 313 in. 401 tests pass (the 37 that went were v2's own),
plus three new ones covering serve() over a snapshot: the CSP and no-store on a
document, ETag/304/206 on an asset, and 404 on four different escape attempts.

The contract doc is rewritten to describe what the code does now, and the v2
conformance vectors go with the test that replayed them.
@erseco erseco changed the title Secure opaque-origin viewer, SCORM/xAPI bridge and HTTP editor preview v2 Secure opaque-origin viewer, SCORM/xAPI bridge and opaque editor preview Jul 25, 2026
erseco added 7 commits July 25, 2026 15:34
The task doc block still described file-backed sessions under 'serving contract
v2'. The store it sweeps is the snapshot store, and the opportunistic sweep now
happens on every replace rather than on session access.
snapshot_store::delete() collapses "does not exist" and "not yours" into a
single false, so the DELETE endpoint answered 404 for both. Publishing over
another author's capability answers 403 for the very same condition, which left
the two halves of the management API disagreeing about what owner scoping looks
like.

Adds snapshot_store::owner_of(), which keeps the two cases apart, and reuses it
in delete() so the ownership rule lives in one place.

Matches the same fix in the Omeka S and Nextcloud adapters, where an API
end-to-end test against a real server caught the divergence.
…verdict

Review pass over the snapshot migration.

The 403-vs-404 fix landed one level too low. snapshot_store::authorize() already
produced the verdict for the publish path, so adding owner_of() and reassembling
the decision in the endpoint wrote the ownership rule a third time — and the two
verbs still disagreed: a malformed previewId answered 400 invalidpreviewid on
publish but 404 previewnotfound on delete, using an error code that bypassed the
status table 26 lines below it.

authorize() becomes public, delete_owned() runs through it, owner_of() is gone,
and the status mapping is one function both verbs call. A future change to the
rule — cmid scoping, an admin override, project sharing — now has one place to
land instead of three.
…nd ranges

serve() read the entire file and MD5'd it to build the ETag before deciding
anything. A conditional GET that ends as a 304 with an empty body, and a Range
seek into a large video, both pulled the whole file into memory and hashed it
first — and scrubbing a video issues exactly those requests, over and over.

Nothing is read now until it is known what will be sent: a 304 and a 416 read
nothing, a 206 seeks and reads its slice, and only a full response and a
scriptable document (always sent whole) read everything.

The ETag becomes identity-based. Path, mtime and size alone are not enough:
mtime has one-second granularity, so an author refreshing twice inside the same
second with an edit that keeps a file the same length would produce the same tag
and be handed a 304 for the previous bytes. Reproduced before fixing — mtime and
size are unchanged across such a replace while the content directory's inode is
not, because every publish extracts into a fresh directory and renames it in.
The inode joins the tag; a filesystem that does not report one degrades to the
mtime/size form, which is no worse.
Migrate the embed/media pair to the vendored exe_external_media bundle
(child + host + contract/manifest, verified by a new CI step and by
js/exe_external_media/verify.mjs), serve the child bundle from
package_manager.php, and add a hello/welcome handshake between
exe_embed_shim and exe_embed_relay so the parent frame authenticates
before attachmedia runs. Fix attachmedia in view.php to wait for
DOMContentLoaded instead of racing the embed script. Add PHPUnit
coverage for the new extraction paths in lib_extract_test.php and an
e2e no-host fixture (tests/e2e/embed/parent-nohost.html) to exercise
the handshake when no parent listener is present.
One argument per line in the multi-line get_file() call, and a proper
multi-line docblock for test_host_bundle_carries_no_provider_sdk -- the
checker does not accept a single-line /** */ as a description.
moodle-plugin-ci phpcs runs with --max-warnings 0, so the single
inline-comment warning left in view.php failed the whole matrix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants