feat(web-app-html-editor): add an HTML editor with live preview#13895
feat(web-app-html-editor): add an HTML editor with live preview#13895dj4oC wants to merge 10 commits into
Conversation
Document how an oCIS editor app actually works before writing code: apps register by file extension (not MIME type), and AppWrapper already provides the WebDAV load/save, dirty tracking, Ctrl+S, unsaved-changes guard and error toasts. Record the editor-library, preview-sandboxing, layout and registration decisions, each citing the Phase 1 file it relies on. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: David Walter <david.walter@kiteworks.com>
Mirror the text-editor package layout (package.json, vitest config, l10n) and add CodeMirror 6 (already resolved in the lockfile via web-pkg) as direct deps. Enable the app by adding "html-editor" to the default and sample-ocis configs; the build auto-discovers it by its web-app-* directory name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: David Walter <david.walter@kiteworks.com>
…ew modes Register for the html, htm and xhtml extensions and route through AppWrapper, so WebDAV load/save, dirty state, Ctrl+S and the unsaved-changes guard are inherited. App.vue is a thin shell that binds currentContent and emits update:currentContent; it composes a CodeMirror 6 source editor (HTML mode, themed via ODS tokens), a sandboxed srcdoc iframe preview (no allow-same-origin), and a view-mode toolbar (Editor | Split | Preview) laid out with CSS grid. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: David Walter <david.walter@kiteworks.com>
…olbar Cover the app wiring (re-emits edits, switches view mode, debounces the preview), the CodeMirror pane (renders, emits on change, applies external content), the preview pane (srcdoc, sandbox without allow-same-origin, no referrer) and the toolbar (active mode, emits changeMode). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: David Walter <david.walter@kiteworks.com>
…view An adversarial security review (SECURITY-REVIEW.md) confirmed the design is sound against the primary threat (a victim opening an attacker-controlled HTML file): the opaque-origin iframe plus bearer-token-in-JS auth prevent token theft, parent-origin XSS and acting as the victim. This tightens the residual low/info findings: - Reduce the iframe sandbox to "allow-scripts" only. allow-forms and allow-popups added no value for a preview but enabled zero-click phishing / external form-POST beaconing from the opaque-origin frame; dropping them removes the vector. - Inject a strict, self-contained CSP into the preview srcdoc (default-src 'none'; form-action 'none'; base-uri 'none'; inline script/style and data:/blob: only) so the preview is network-isolated and not reliant on the deployment proxy CSP. - Pause the live preview for large files until the user opts in, so a huge or hostile document cannot freeze the tab on open. - Pin the full sandbox contract (exact token set + srcdoc-not-src) in tests so a future change cannot silently loosen the one control everything depends on. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: David Walter <david.walter@kiteworks.com>
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
#13895 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: David Walter <david.walter@kiteworks.com>
SonarCloud flagged the appInfo.extensions map and the app-switcher menu-item block in index.ts as duplicated against web-app-text-editor (new-code duplication over the 3% gate). Assign the already-typed fileExtensions directly instead of re-mapping them, and drop the app-switcher launcher entry: it was copied boilerplate and is not needed for v1. Users can still create a new HTML file from the Files "New" menu via the extension's newFileMenu. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: David Walter <david.walter@kiteworks.com>
… test The shared unit-test run (vitest projects) went red on web-pkg's @vitest/web-worker tests with timeouts only after this package was added, and the symptom (worker callbacks never firing) matches fake timers being active during real-timer async. Rewrite the debounce assertion to use real timers and a short wait so this test project cannot influence timer state in the shared run. No change to behavior under test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: David Walter <david.walter@kiteworks.com>
LukasHirt
left a comment
There was a problem hiding this comment.
Is this the correct location or should it live rather in the web-extensions repo?
If we move this to web-extensions, we need to move md-editor as well. Same stuff is reused in here |
Adding the html-editor package regenerated the lockfile, which let vitest
('^4.1.5') float to 4.1.6 while @vitest/web-worker and @vitest/coverage-v8
stay pinned at 4.1.5. That resolved a second vitest@4.1.5 copy into web-pkg,
so @vitest/web-worker and the test runner loaded different vitest module
instances and the webWorker specs (exportAsPdfWorker, pasteWorker,
restoreWorker) hung until the 5s timeout.
Pin vitest to 4.1.6 via pnpm overrides so the whole workspace resolves a
single vitest, matching master's (green) topology.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: David Walter <david.walter@kiteworks.com>
| if (!html) { | ||
| return META | ||
| } | ||
| const headOpen = /<head\b[^>]*>/i |
There was a problem hiding this comment.
[Security — High] The regex /<head\b[^>]*>/i matches the first occurrence of <head anywhere in the document, including inside HTML comments. A document like:
<!-- <head> -->
<html><head><title>X</title></head>...causes .replace() to inject the CSP <meta> into the comment (which the browser ignores), while the real <head> element receives no CSP injection. The preview iframe then renders the hostile document with default-src 'none' and form-action 'none' unenforced — defeating the defense-in-depth layer.
The same applies to the /<html\b[^>]*>/i branch at line 41.
Fix: strip HTML comments before testing, or use a parser instead of regex to locate the insertion point.
| watch( | ||
| () => currentContent, | ||
| (value) => { | ||
| if (isPreviewTooLarge(value) && !renderLargeAnyway.value) { |
There was a problem hiding this comment.
[Security — High] renderLargeAnyway is set to true in showPreviewAnyway() but is never reset. After a user opts in once for a large file, any subsequent change to currentContent — including a conflict-reload that replaces the file with an attacker-controlled version — immediately calls schedulePreview() without going through the pause gate, because this watch condition evaluates to false.
A realistic path: user clicks 'Show preview anyway' on a 600 KB file → attacker updates the file via a concurrent write → AppWrapper detects a 412 conflict and reloads → the watch fires with the new (still large, now hostile) content → renderLargeAnyway.value === true → preview renders automatically, no prompt.
Fix: reset renderLargeAnyway.value = false at the top of the watch handler (before the early-return check) so each new content version starts paused.
| () => currentContent, | ||
| (value) => { | ||
| if (isPreviewTooLarge(value) && !renderLargeAnyway.value) { | ||
| previewContent.value = '' |
There was a problem hiding this comment.
[Bug — Medium] The early-return path for large content sets previewContent.value = '' but does not cancel the pending previewTimer. If a timer was queued from a previous small-content update, it fires up to 250 ms later and writes stale HTML back into previewContent, restoring an old preview snapshot even though the pause gate is active.
Sequence:
- User types;
schedulePreview()queues a 250 ms timer with snapshot S. - User pastes a large block; watch fires, sets
previewContent = '', returns early — timer is still live. - Timer fires →
previewContent = wrapWithPreviewCsp(S)(stale snapshot re-appears). - User clicks 'Show preview anyway' → sees stale content for up to 250 ms instead of the actual current document.
Fix: call clearTimeout(previewTimer) (and set previewTimer = undefined) before returning early.
|
|
||
| const showPreviewAnyway = () => { | ||
| renderLargeAnyway.value = true | ||
| schedulePreview(currentContent) |
There was a problem hiding this comment.
[Bug — Medium] showPreviewAnyway() sets renderLargeAnyway.value = true synchronously, which immediately flips the v-if/v-else in the template so <html-preview-pane> mounts. At that moment previewContent.value is still '' (it was cleared by the watch's early-return path), so the iframe receives srcdoc="" — an empty, CSP-less page — and the user sees a blank preview for up to 250 ms until the schedulePreview timer fires.
Fix: call schedulePreview synchronously and await nextTick() before flipping renderLargeAnyway, or set previewContent to the wrapped value synchronously inside this function (skipping the debounce for this one explicit opt-in action).
| // reaches the (sandboxed, opaque-origin) preview. Both panes stay mounted across | ||
| // view-mode switches (CSS grid collapses the hidden column) so the editor keeps | ||
| // its cursor/undo. | ||
| const previewContent = ref(previewPaused.value ? '' : wrapWithPreviewCsp(currentContent ?? '')) |
There was a problem hiding this comment.
[Bug — Minor] The initial previewContent is computed synchronously at setup time, bypassing the watcher and the 250 ms debounce. Subsequent external content changes (e.g. a conflict-reload updating the currentContent prop) go through schedulePreview and arrive 250 ms later. This creates a structural inconsistency: the initial render is immediate, but every prop update after that is delayed.
In practice this means a user who saves the file from another tab and triggers a conflict-reload will see the editor pane update instantly while the preview pane lags by 250 ms — a momentary split-brain state that could look like a failed save.
Consider routing the initial render through schedulePreview as well (or using schedulePreview with a 0 ms delay for the initial case) so the two paths are identical.
| } | ||
| ) | ||
|
|
||
| watch( |
There was a problem hiding this comment.
[Bug — Minor] The watch source is () => isDark(), but isDark() is a plain function that reads themeStore?.currentTheme via optional chaining. When themeStore is undefined (as it is under vi.mock('@ownclouders/web-pkg')), the getter has no reactive dependencies to track, so the watcher never fires in tests. Any regression that breaks dark-mode theme switching in the editor passes the full test suite silently — the watch path is structurally untested.
In production the watcher works because themeStore is always defined, but the tested and production code paths diverge structurally. This is related to the test-accommodation issue at line 39.
| watch( | ||
| () => currentContent, | ||
| (value) => { | ||
| if (isPreviewTooLarge(value) && !renderLargeAnyway.value) { |
There was a problem hiding this comment.
[Cleanup] The condition isPreviewTooLarge(value) && !renderLargeAnyway.value here duplicates the previewPaused computed defined at line 73. If the pause logic ever gains a third flag, the watch body must be updated separately and the two can silently diverge.
Consider replacing with previewPaused.value to keep a single source of truth for what 'paused' means.
| // The theme store is optional here: in unit tests `@ownclouders/web-pkg` is mocked | ||
| // and `useThemeStore()` returns undefined, so we read it defensively. | ||
| const themeStore = useThemeStore() | ||
| const isDark = () => Boolean(unref(themeStore?.currentTheme)?.isDark) |
There was a problem hiding this comment.
[Cleanup] The themeStore?.currentTheme optional-chaining guard was added to survive vi.mock('@ownclouders/web-pkg') returning undefined in tests — a test-accommodation that leaks into production code. Every other consumer of useThemeStore in the monorepo (e.g. web-pkg/src/components/TextEditor/TextEditor.vue, web-app-epub-reader/src/App.vue) destructures currentTheme directly without optional chaining, because the real store always initialises a theme.
The correct fix is in the test: use a partial mock that returns a real-shaped store object, as epub-reader does:
useThemeStore: vi.fn(() => ({ currentTheme: ref({ isDark: false }) }))That removes the need for defensive optional-chaining in production code, and the watcher at line 132 can then track a real reactive dependency.
| extension: 'html', | ||
| label: () => $gettext('HTML file'), | ||
| newFileMenu: { | ||
| menuTitle() { |
There was a problem hiding this comment.
[Cleanup] menuTitle() duplicates the label of the same extension entry — both return $gettext('HTML file'). If the label is ever renamed, a developer who updates label but misses menuTitle will silently show the old string in the 'New >' create menu while all other surfaces show the new one.
The text-editor avoids this by deriving menuTitle from extensionItem.label directly. Consider the same pattern here, or at minimum extract a shared constant.
| color: '#e34c26', | ||
| defaultExtension: 'html', | ||
| meta: { | ||
| fileSizeLimit: 2000000 |
There was a problem hiding this comment.
[Cleanup] fileSizeLimit (2 000 000 bytes, here) and PREVIEW_SIZE_LIMIT (500 000 bytes, in helpers/preview.ts) are two independent hardcoded thresholds that govern the same file but have no enforced relationship. Currently the wrapper warns at 2 MB while the preview pauses at 500 KB, which is the intended order — but there is nothing to prevent a future edit from reversing them (e.g. raising PREVIEW_SIZE_LIMIT above 2 MB), and no comment explaining the dependency.
Consider adding a runtime assertion PREVIEW_SIZE_LIMIT < fileSizeLimit, or exporting PREVIEW_SIZE_LIMIT from helpers/preview.ts and referencing it here so the relationship is explicit.
… guard Address review findings: - Security: wrapWithPreviewCsp() matched <head>/<html> with a raw regex, so a crafted '<!-- <head> -->' could divert the CSP <meta> into a commented-out tag and leave the rendered preview without its self-protecting CSP. Matching now skips HTML comments and injects into the first real head/html. - Security: renderLargeAnyway was never reset, so a single 'show preview anyway' opt-in permanently disabled the large-file pause; a later external content change could auto-render another large/hostile document. The guard now re-arms on every content change. - Bug: the large-content watch path cleared previewContent but left a queued debounce timer that could still render the oversized content; it is now cancelled. - Bug: opting in now renders synchronously instead of through the 250ms debounce, so the preview pane no longer mounts empty. - Cleanup: de-duplicate the 'HTML file' label/menuTitle. Adds regression tests for the comment-evasion and guard re-arm behaviour. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: David Walter <david.walter@kiteworks.com>
|
|
Run |



Summary
Adds a new core app,
web-app-html-editor, that opens.html/.htm/.xhtmlfiles in a CodeMirror 6 source editor with a live, sandboxed preview. No WOPI and no
extra server: the file is loaded and saved over WebDAV by the standard
AppWrapper,the source is edited in CodeMirror, and the rendered result is shown in a strictly
sandboxed
<iframe srcdoc>.Approach
The app is intentionally thin and idiomatic. It mirrors
web-app-text-editor:defineWebApplicationregisters it by file extension and routes throughAppWrapperRoute. By declaring acurrentContentprop and emittingupdate:currentContent, it inherits the framework's WebDAV load/save, dirtytracking,
Ctrl+S, autosave, the unsaved-changes guard and error notifications, sonone of that is reimplemented. The app itself only composes:
HtmlEditorPane.vue- a CodeMirror 6 wrapper (HTML mode, line numbers, themefollows the active ownCloud theme). CodeMirror 6 is already in the workspace via
md-editor-v3, so the individual@codemirror/*packages are added at theversions already in the lockfile.
HtmlPreviewPane.vue- a sandboxedsrcdociframe.HtmlToolbar.vue- an Editor | Split | Preview view-mode toggle (CSS grid; thefilename, Save and action menu come from
AppTopBar).The app is enabled by default via the
appsarray inconfig/config.json.distandconfig/config.json.sample-ocis.Security
The preview renders attacker-influenceable HTML (a user may open a file shared to
them), so the preview is treated as untrusted:
sandbox="allow-scripts"with noallow-same-origin(opaqueorigin), so the preview cannot read the shell's cookies, storage or OIDC token,
cannot call the oCIS API as the user, and cannot script the parent.
allow-formsand
allow-popupsare deliberately omitted.default-src 'none'; form-action 'none'; base-uri 'none'; inlinescript/style and
data:/blob:only) is injected into thesrcdoc, so thepreview is network-isolated and does not depend on the deployment proxy CSP.
or hostile document cannot freeze the tab on open.
srcdoc-not-src).Trade-off: because the preview is network-isolated, documents that reference
external stylesheets/scripts/images will not load them. This is intentional for
a first version. The package ships
ARCHAEOLOGY.md,DECISIONS.mdandSECURITY-REVIEW.mddocumenting the design rationale and the threat model.Testing
contract,
srcdoc), the toolbar, the CSP-injection helper, and the large-filepreview pause.
pnpm --filter html-editor test:unitis green (27 tests).vue-tsc --noEmit,eslintandprettierare clean..htmlfile (the app ispicked up as the default opener), live edit updating the preview, and saving back
over WebDAV.
Notes
MIME type), matching
web-app-text-editor.