feat(server): integrate OnlyOffice document editing#15148
Open
dadastory wants to merge 6 commits into
Open
Conversation
Add OnlyOffice Document Server integration for editing office attachments (docx/xlsx/pptx, ...), gated behind an experimental feature flag (default off). - backend plugins/onlyoffice: signed editor config, token-authenticated file download for the Document Server, JWT-verified callback that writes edits back as content-addressed blobs, per-attachment version manifest, and a standalone editor page. Supports 7 interaction modes, UI-language alignment and share-permission alignment. - frontend view-extensions/onlyoffice: attachment toolbar entry + in-app version panel, wired through a feature flag and the view manager. No blocksuite core schema changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a backend OnlyOffice plugin with config, controller endpoints, service logic, and editor-page bootstrap, plus frontend feature-flag wiring, toolbar actions, and a version-history panel for attachment editing. ChangesOnlyOffice Integration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/backend/server/src/plugins/onlyoffice/controller.ts`:
- Around line 158-176: The forcesave endpoint and delete-version endpoint (at
lines 215-235) are using `@Get` decorators for operations that mutate state, which
violates REST conventions and creates security vulnerabilities. Change the `@Get`
decorators to `@Post` decorators for both the forcesave method and the
delete-version method. Then update all callers of these endpoints in
packages/backend/server/src/plugins/onlyoffice/editor-page.ts,
packages/backend/server/src/plugins/onlyoffice/versions-page.ts, and
packages/frontend/core/src/blocksuite/view-extensions/onlyoffice/version-panel.ts
to send requests using method: 'POST' instead of GET.
In `@packages/backend/server/src/plugins/onlyoffice/editor-page.ts`:
- Around line 77-104: The polling loop that waits for the save result may
complete without ever receiving a blobId (in case of slow callbacks or
failures), but the code still proceeds to close the editor window, causing the
user's edits to appear lost. Add a flag variable (e.g., saved) that tracks
whether the postMessage with the new blobId was successfully sent inside the
condition checking jr && jr.blobId. Then, only close the editor window if this
flag is true, ensuring the window remains open when save confirmation is
missing.
In `@packages/backend/server/src/plugins/onlyoffice/service.ts`:
- Around line 555-625: The recordVersion method performs a non-atomic
read-modify-write sequence on manifest data without any serialization mechanism,
allowing concurrent status-2 and status-6 callbacks to race and overwrite each
other's changes. Add a locking or serialization mechanism that ensures only one
recordVersion call executes at a time for a given combination of workspaceId,
docId, and blockId. Use a Map-based lock (keyed by these three parameters) or a
task queue to serialize manifest updates and prevent concurrent modifications
from corrupting the version history.
- Around line 596-598: The hardDeleteBlob method calls at multiple locations
(around lines 596-598, 616-618, and 631-637) are deleting content-addressed
blobs without verifying that no other attachments or documents still reference
them. Before calling hardDeleteBlob for the stale blob, add a reference safety
check to ensure the blob is not shared by any other documents or attachments in
the workspace. Only proceed with the hard deletion if the blob has no remaining
references, otherwise use a soft delete approach or implement a garbage
collection mechanism that safely handles shared blobs.
- Around line 711-719: The fetch call to the OnlyOffice CommandService.ashx
endpoint does not validate the response status, so non-2xx responses are
silently treated as success. After the fetch call (within the try block), check
if the response status indicates failure by validating the response.ok property
or response.status, and if the response indicates an error, throw an error with
appropriate details. This ensures that failed force-save operations are properly
caught by the existing catch block and logged as warnings instead of being
silently treated as successful.
- Around line 659-664: The deleteVersion method currently deletes any provided
blobId without verifying it exists in the manifest.versions array first,
creating a security vulnerability where arbitrary workspace blobs can be
deleted. After filtering manifest.versions to remove the matching blobId and
before calling this.blobStorage.delete, add a check to verify that the filter
actually removed an entry (meaning the blobId was present in manifest.versions).
Only proceed with the blob deletion if the blobId was found and removed from the
manifest, otherwise reject the operation to prevent deletion of blobs not
associated with this attachment.
In `@packages/frontend/core/src/blocksuite/view-extensions/onlyoffice/toolbar.ts`:
- Around line 143-152: The onMessage message handler validates the origin but
does not verify that the message came from the specific popup window instance,
creating a security vulnerability where any same-origin window could send a
matching affine-onlyoffice-saved message and mutate the attachment reference.
Add a check for e.source in the validation condition of the onMessage function
to ensure the message originated from the specific popup window that was created
(you will need to ensure the popup window reference is captured when the window
is opened and then compare e.source against that reference in the validation
logic). This same source validation should also be applied at the other location
mentioned in the comment (lines 165-166).
In
`@packages/frontend/core/src/blocksuite/view-extensions/onlyoffice/version-panel.ts`:
- Around line 151-167: The _load() method sets the error property when a fetch
fails but never clears it when the load succeeds, causing stale error messages
to persist in the UI even after a successful version load. Add code at the
beginning of the try block in the _load() method to reset this.error to null or
an empty string before attempting to load the versions. This ensures that
successful loads clear any previous error state.
- Around line 247-255: The "Switch to" button in the version panel is being
rendered without checking write permissions, exposing a non-functional action in
read-only contexts. In the conditional rendering block around the switch
statement (where isCur determines whether to show "Current" or the switch
button), add a check for this.canWrite to the condition that displays the button
with the `@click`=${() => this._switch(v)} handler. The button should only render
when both canWrite is true AND isCur is false, so the button rendering logic
needs to be gated by the canWrite permission check alongside the existing isCur
check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9b14f90-9694-453e-ac57-6d432f522936
📒 Files selected for processing (15)
packages/backend/server/src/app.module.tspackages/backend/server/src/plugins/onlyoffice/config.tspackages/backend/server/src/plugins/onlyoffice/controller.tspackages/backend/server/src/plugins/onlyoffice/editor-page.tspackages/backend/server/src/plugins/onlyoffice/index.tspackages/backend/server/src/plugins/onlyoffice/service.tspackages/backend/server/src/plugins/onlyoffice/types.tspackages/backend/server/src/plugins/onlyoffice/versions-page.tspackages/frontend/core/src/blocksuite/block-suite-editor/lit-adaper.tsxpackages/frontend/core/src/blocksuite/manager/view.tspackages/frontend/core/src/blocksuite/view-extensions/onlyoffice/index.tspackages/frontend/core/src/blocksuite/view-extensions/onlyoffice/toolbar.tspackages/frontend/core/src/blocksuite/view-extensions/onlyoffice/version-panel.tspackages/frontend/core/src/components/hooks/affine/use-ai-specs.tspackages/frontend/core/src/modules/feature-flag/constant.ts
- use POST for mutating endpoints (forcesave, delete-version) + update callers - editor page: keep window open and surface error if save-back is unconfirmed - serialize per-attachment manifest read-modify-write to avoid lost history - soft-delete superseded/removed version blobs (content-addressed, may be shared) - deleteVersion: reject blob ids not present in the attachment manifest - forcesave: treat non-2xx / error responses as failures - toolbar: bind save-back postMessage to the actual popup (check e.source) - version panel: reset error after a successful load; gate "Switch to" on write - remove dead standalone versions-page (replaced by the in-app panel) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/backend/server/src/plugins/onlyoffice/service.ts (1)
781-785: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed when no allowed Document Server host is configured.
When neither
documentServerUrlnorinternalUrlparses to a valid host,allowedHostsis empty and theallowedHosts.length &&guard skips validation entirely, lettingapplyCallbackfetch an arbitrarydownloadUrl. Because the fetch here is the plainglobalThis.fetch(intentionally not SSRF-guarded, per the comment at lines 409-411), this becomes an SSRF vector against internal services if the integration is ever in a partially-configured state. Reject instead of allowing through.🛡️ Fail closed on empty allowlist
- if (allowedHosts.length && !allowedHosts.includes(target.host)) { + if (!allowedHosts.length || !allowedHosts.includes(target.host)) { throw new BadRequest( `OnlyOffice callback url host not allowed: ${target.host}` ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/backend/server/src/plugins/onlyoffice/service.ts` around lines 781 - 785, The validation guard in the OnlyOffice callback host check currently skips validation entirely when allowedHosts is empty (due to the allowedHosts.length && condition), which creates an SSRF vulnerability. Remove the allowedHosts.length check from the condition so that the validation always runs regardless of whether allowedHosts is populated, ensuring the BadRequest is thrown when allowedHosts is empty or when the target.host is not in the allowedHosts array. This ensures the code fails closed when no Document Server hosts are configured rather than allowing arbitrary downloadUrls to be fetched.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/backend/server/src/plugins/onlyoffice/service.ts`:
- Around line 24-26: The manifestLocks in-memory Map only serializes operations
within a single Node process, which causes race conditions and data corruption
in multi-instance deployments when concurrent status-2/status-6 callbacks for
the same attachment land on different replicas. Replace the in-memory
Promise-based locking mechanism in the manifestLocks field with the distributed
Mutex primitive (backed by Redis via Locker) that already exists in the codebase
and is used by other services like payment and copilot. Apply this distributed
Mutex to both the manifest lock operations and the serialization logic
referenced in lines 544-565 to ensure cross-instance consistency. Alternatively,
if single-instance deployment is required, document this constraint clearly in
the class documentation.
---
Outside diff comments:
In `@packages/backend/server/src/plugins/onlyoffice/service.ts`:
- Around line 781-785: The validation guard in the OnlyOffice callback host
check currently skips validation entirely when allowedHosts is empty (due to the
allowedHosts.length && condition), which creates an SSRF vulnerability. Remove
the allowedHosts.length check from the condition so that the validation always
runs regardless of whether allowedHosts is populated, ensuring the BadRequest is
thrown when allowedHosts is empty or when the target.host is not in the
allowedHosts array. This ensures the code fails closed when no Document Server
hosts are configured rather than allowing arbitrary downloadUrls to be fetched.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a08b73f-762c-43ea-86dc-4c22b14b60c3
📒 Files selected for processing (5)
packages/backend/server/src/plugins/onlyoffice/controller.tspackages/backend/server/src/plugins/onlyoffice/editor-page.tspackages/backend/server/src/plugins/onlyoffice/service.tspackages/frontend/core/src/blocksuite/view-extensions/onlyoffice/toolbar.tspackages/frontend/core/src/blocksuite/view-extensions/onlyoffice/version-panel.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/frontend/core/src/blocksuite/view-extensions/onlyoffice/toolbar.ts
- packages/frontend/core/src/blocksuite/view-extensions/onlyoffice/version-panel.ts
- packages/backend/server/src/plugins/onlyoffice/editor-page.ts
Replace the in-process promise-chain lock with the Redis-backed Mutex so manifest read-modify-write is serialized across server instances, not just within a single Node process. Prevents concurrent status-2/6 save callbacks landing on different replicas from corrupting version history.
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## canary #15148 +/- ##
==========================================
- Coverage 58.09% 51.86% -6.23%
==========================================
Files 3296 3235 -61
Lines 188605 187371 -1234
Branches 27217 25104 -2113
==========================================
- Hits 109567 97188 -12379
- Misses 75273 86869 +11596
+ Partials 3765 3314 -451
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Import the repeat directive from lit/directives/repeat.js instead of lit-html (not a direct dependency) to satisfy the import-x/no-extraneous-dependencies lint rule. - Document that OnlyOffice drops embedded fonts on save, so the stored blob can be much smaller than the original without any data loss.
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
2 participants
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.
What
Adds optional OnlyOffice Document Server integration so users can view and edit office attachments (
.docx/.xlsx/.pptx, …) directly from AFFiNE, gated behind an experimentalfeature flag (off by default).
Why
Office documents are a common attachment type, but currently they can only be downloaded. This lets self-hosted instances (with an OnlyOffice Document Server) open them in-place for viewing, editing, reviewing,
commenting and form-filling.
How it works
packages/backend/server/src/plugins/onlyoffice/, new NestJS module):/blobsroute requires a user session).sha256scheme as blocksuite), never overwriting in place.packages/frontend/core/src/blocksuite/view-extensions/onlyoffice/, new):ToolbarModuleExtension(the same DI mechanism the built-in AI/image toolbars use).Configuration
Enabled per-deployment via config / env (all default to disabled/empty):
onlyoffice.enabledonlyoffice.documentServerUrlonlyoffice.internalUrlonlyoffice.callbackHostonlyoffice.jwtSecretThe UI entry only appears when the
enable_onlyofficeexperimental feature flag is turned on (off by default).Scope / safety
app.module.ts, view manager,use-ai-specs.ts,lit-adaper.tsx,feature-flag/constant.ts).Notes / open questions for maintainers
Summary by CodeRabbit
Release Notes
enable_onlyofficefeature toggle.