feat(web-app-external): handle Collabora UI_InsertGraphic and UI_InsertFile postMessages#13658
Conversation
|
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. |
50f5d95 to
8eeedfc
Compare
I wonder if the workflow can be approved here |
|
@pedropintosilva thank you for this PR. We're currently in a process of rewriting our CI to GH actions. I would kindly ask you to have a little patience right now. As soon as the E2E tests pipelines are moved to GH action, this PR would need to be rebased and we could then run the workflows. I'll let you know as soon as it is ready. |
|
@LukasHirt , JFYI I have signed the CLA, thanks. |
|
@pedropintosilva we have migrated our test suite to GH actions. Please, do a rebase so that this PR picks up on that. I will approve the workflows to run afterwards. |
…rtFile postMessages
Add frontend handling for Collabora's remote file insertion and document
comparison features. When oCIS sets EnableInsertRemoteFile and
EnableInsertRemoteImage in the WOPI CheckFileInfo response, Collabora
shows new menu items that send UI_InsertGraphic and UI_InsertFile
postMessages to the parent window.
- Add Host_PostmessageReady handshake: reply to App_LoadingStatus with
Host_PostmessageReady so Collabora accepts Action postMessages.
- Handle UI_InsertGraphic: open a file picker modal filtered to image
MIME types, resolve the selected file to a signed WebDAV download URL,
and send Action_InsertGraphic back to the Collabora iframe.
- Handle UI_InsertFile: read callback and mimeTypeFilter from the
Collabora message, open the file picker accordingly, and send back
the appropriate Action (Action_InsertMultimedia or
Action_CompareDocuments).
- Create InsertRemoteFileModal.vue: new modal component that embeds the
oCIS file browser in embed mode, resolves the picked file to a
download URL via clientService.webdav.getFileUrl(), and calls back
with { filename, url }.
- Replace catchClickMicrosoftEdit with a unified handleAppMessage
listener that handles all app iframe postMessages (UI_Edit,
App_LoadingStatus, UI_InsertGraphic, UI_InsertFile).
Companion server-side PR: owncloud/ocis#12192
Signed-off-by: Pedro Pinto Silva <pedro.silva@collabora.com>
8eeedfc to
dc15f7d
Compare
|
@LukasHirt , I think the workflows might need approval |
|
@pedropintosilva could you please do one more rebase. We now removed the sonarqube step from CI and use app instead which does not block external contributions. Rebasing should then make the CI finally run as expected. Sorry for the obstacles… |
DeepDiver1975
left a comment
There was a problem hiding this comment.
Thanks @pedropintosilva — nice feature and the manual end-to-end evidence (Compare Documents + image insert) is great to see. Reviewing as maintainer. I'm requesting changes: there are a couple of real security gaps around postMessage origin handling, a duplication concern, and a direct collision with the open PR #13906 that need resolving before this can merge.
Security — postMessage origin handling
-
Incoming editor messages are not origin-checked.
handleAppMessageparses and acts on anywindowmessageevent regardless ofevent.origin. It will triggerloadAppUrl.perform('write'), open the insert modal, and (via the handshake) postHost_PostmessageReadyin response to messages from any frame on the page. Any embedded/third-party frame can drive these flows. The companion PR #13906 explicitly guards this withif (appOrigin && event.origin \!== appOrigin) return. Please add the same origin check at the top ofhandleAppMessage. -
postMessageToAppposts with target origin'*'. This broadcasts the message to whatever origin currently occupies the iframe. ForAction_InsertGraphic/Action_InsertMultimediathe payload contains a signed WebDAV download URL — posting it with'*'risks leaking a credentialed URL to an unexpected origin. Derive the editor origin fromappUrland post to that specific origin (again, this is exactly what #13906 does with itsappOrigincomputed). -
InsertRemoteFileModal.vuedrops theverifyMessageOrigincheck. This component is a near-verbatim copy of the existingweb-pkg/src/components/Modals/FilePickerModal.vue, but it omits theverifyMessageOrigin(origin)guard thatFilePickerModalruns at the top of bothonFilePickandonCancel. Without it, a message handler acting onowncloud-embed:file-pick/owncloud-embed:cancelaccepts those events from any origin. Please restore theuseEmbedMode().verifyMessageOrigin(origin)check in both handlers.
Duplication / reuse
InsertRemoteFileModal.vue duplicates ~95% of FilePickerModal.vue (same iframe embed setup, embed-target=file, onLoad focus, the file-pick/cancel listeners). The only real delta is "resolve to a download URL and call onSelect" vs. "open the editor route". Consider factoring the shared embed-picker scaffolding into a composable or extending FilePickerModal with a pluggable "on pick" strategy, rather than maintaining two copies that can (and already did, re: the origin check) drift apart.
Collision with #13906 (same area — flagged in the task)
PR #13906 ("enable Collabora Save As / export to storage") is open and modifies the same App.vue: it also adds the Host_PostmessageReady handshake, useModals/dispatchModal, the iframe ref, the onBeforeUnmount listener cleanup, and replaces catchClickMicrosoftEdit with a unified switch handler — but with origin checking and a specific target origin. These two PRs will conflict and currently diverge on the security-critical origin handling. They should be reconciled: ideally land the hardened handshake/handler infrastructure once (the #13906 version with origin checks) and layer the insert handlers on top, so we don't ship two competing implementations of the same channel.
Behaviour notes / smaller points
UI_Editis now gated ondetermineOpenAsPreview(appName)inside the handler (previously the listener was only attached in that case). Net behaviour looks equivalent, good — just calling it out.- The duplicate
ref="appIframeRef"on both the GET and POST iframes is OK because they're mutually exclusive viav-if, but it reads oddly; a single shared ref or distinct refs would be clearer. IMAGE_MIME_TYPESworks as a filter because the embed list filter matches against bothresource.extensionandresource.mimeType— confirmed, so MIME values are fine here.
Tests & CI
- The build check is currently failing (red) — please get CI green.
- There are no unit tests for the new postMessage handlers (
handleAppMessagedispatch, the handshake, modal wiring) or forInsertRemoteFileModal. Given this is security-sensitivepostMessagecode, please add coverage — at minimum an origin-rejection test once the origin guards are in place.web-app-external/tests/unit/app.spec.tsis the natural home.
Changelog
changelog/unreleased/enhancement-collabora-insert-remote-file.md is present and follows the template/format. 👍
Summary: solid feature with working manual verification, but please (1) origin-check incoming messages, (2) post Action messages to the specific editor origin (not '*'), (3) restore verifyMessageOrigin in the modal, (4) reconcile with #13906, and (5) green CI + add handler tests. Happy to re-review once those are addressed.
Add frontend handling for Collabora's remote file insertion and document
comparison features. When oCIS sets EnableInsertRemoteFile and
EnableInsertRemoteImage in the WOPI CheckFileInfo response, Collabora
shows new menu items that send UI_InsertGraphic and UI_InsertFile
postMessages to the parent window.
Add Host_PostmessageReady handshake: reply to App_LoadingStatus with
Host_PostmessageReady so Collabora accepts Action postMessages.
Handle UI_InsertGraphic: open a file picker modal filtered to image
MIME types, resolve the selected file to a signed WebDAV download URL,
and send Action_InsertGraphic back to the Collabora iframe.
Handle UI_InsertFile: read callback and mimeTypeFilter from the
Collabora message, open the file picker accordingly, and send back
the appropriate Action (Action_InsertMultimedia or
Action_CompareDocuments).
Create InsertRemoteFileModal.vue: new modal component that embeds the
oCIS file browser in embed mode, resolves the picked file to a
download URL via clientService.webdav.getFileUrl(), and calls back
with { filename, url }.
Replace catchClickMicrosoftEdit with a unified handleAppMessage
listener that handles all app iframe postMessages (UI_Edit,
App_LoadingStatus, UI_InsertGraphic, UI_InsertFile).
Companion server-side PR: owncloud/ocis#12192
Signed-off-by: Pedro Pinto Silva pedro.silva@collabora.com
and insert images from cloud storage: