Repository navigation
Conversation
moodyjmz
marked this pull request as draft
May 26, 2026 09:14
moodyjmz
force-pushed
the
feat/wopi-phase-5
branch
4 times, most recently
from
July 2, 2026 12:35
3d8b00a to
4ec1a9a
Compare
moodyjmz
force-pushed
the
feat/wopi-phase-5
branch
from
August 28, 2026 20:44
4ec1a9a to
4eb7bbd
Compare
6 of 8 tasks
Contributor
Author
|
(Carried over from #163, which is now closed in favor of this branch.) Found while running a live cross-replica EO test against this code: A real fix would derive the allowed origin from |
moodyjmz
force-pushed
the
feat/wopi-phase-5
branch
from
October 23, 2026 12:21
29d9e5d to
a015055
Compare
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
…tion Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
…n validation Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
A logged-in visitor to a share link always got a token minted from their own folder. When the file isn't in it, which is the normal case for a link someone sent them, that failed and openShare returned 403. Fall back to the share-scoped token a guest gets, with the link's download restriction, named after the user instead of the caller-supplied guestName. Logged-in users used to skip the link password entirely, which was only harmless because they never got a link-based token. Now only those who reach the file through their own folder skip it; anyone whose access is the link itself has to enter the password first. Until they have, every failure leads to the password challenge, so a missing path, an unsupported type or an unreadable share can't be told apart from an existing file. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
The backend notes said not re-reading the share on each WOPI request matched richdocuments, which in fact stores the share token and re-resolves the share every time; state the revocation gap plainly. KG2 described logged-in visitors as getting guest tokens, which was never the behaviour; describe what they get now. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
moodyjmz
force-pushed
the
feat/wopi-phase-5
branch
from
October 26, 2026 09:51
e16745e to
da23a84
Compare
Opening a file answered 415 "File type not supported by the editor" whenever discovery couldn't be read, so an unconfigured or unreachable editor server, including the 60 s after a failed fetch, looked like a file type problem. Check DiscoveryService::isAvailable() first and answer 503 "Editor server unavailable". On a password-protected link the visitor still gets the password challenge first. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
The share-link change left the spec matrix and an integration test claiming that logged-in visitors skip the link password. Replace them with integration cases for a logged-in user without own access, through a plain link (reaches the editor-URL code) and a password-protected one (303 to the password page), and say in the timestamp matrix row which editor path is integration- and which is unit-tested. Also pin the two pending-password refusals that had no unit test, an unreadable share and a NotPermitted path, so neither can turn back into a distinguishable 403. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
…accurately The README's CheckFileInfo flag table described rename and anonymity by "authenticated user" versus "guest", but logged-in users whose only access is a link now get a share-link token. The discovery failure marker comment didn't say that it depends on memcache.local: without one failures aren't remembered, and a Redis local cache shares it. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
The WOPI branch adds database migrations, a background job and an admin settings section, but kept main's version, so instances already running main never ran them. Bump it so they do, including the share_token migration that follows. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
UnusedClass, PossiblyUnusedMethod on constructors and PossiblyUnusedProperty on entities fire on code Nextcloud reaches by name (controllers, listeners, migrations, the DI container, Entity's magic accessors). They sat in the baseline file by file, so every new class or column had to be baselined too. Suppress them by scope in psalm.xml with the reason, and drop the 16 baseline entries they make redundant; the baseline now holds only findings still to fix. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Guest tokens weren't linked to their share. Every WOPI request resolved the file in the owner's folder with the rights stamped when the token was minted, so deleting the share, letting it expire, or removing edit or download rights had no effect for the 10 h token lifetime. Share-link tokens now store their share token (new share_token column). A new WopiAccessResolver turns a token into its file and current rights on every request, and every WOPI entry point and both write gates (PutFile, the POST operations) use it: - user tokens resolve through the editor's own folder, as before; - share-link tokens resolve through the live share: a deleted, expired or unreadable share answers 404, the file comes from the share's node (scoped to a shared folder), write requires the share's UPDATE permission now, and the share's download restriction applies now; - tokens from before this change that have neither a user nor a share are rejected; the visitor reopens the file. Logged-in users who open a file through a link now get their uid on the link token, so CheckFileInfo names them to the editor and co-editors instead of showing a guest. Versions and activity still don't record them as the author: WOPI requests set no user scope yet. Wopi::isGuest() and getUserForFileAccess() are replaced by isShareSession() (where access comes from) and isAnonymous() (whether a user is behind it), which a logged-in link visitor now answers differently. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Share-link tokens are now re-checked against the live share on every request; the README still said rights were fixed at token issue and that revocation waited for the token to expire. Also note that logged-in link visitors are named to the editor but not yet recorded as authors. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
/open/share/{shareToken} is public and every successful call stores a
WOPI token valid for 10 h, yet it had neither a rate limit nor share
token throttling: anyone holding a link could mint rows faster than the
hourly cleanup removes them, and unknown tokens could be guessed at full
speed.
Limit anonymous callers to 30 requests per minute and throttle unknown
share tokens under the officeOpenShare brute-force action, as core does
for its share pages.
Assisted-by: ClaudeCode:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
The hourly cleanup deleted at most 500 expired WOPI tokens per run and selected them without an index on expiry, while every share-link open adds a token. Busy links outgrew the cleanup and each run scanned the whole table. Index office_wopi.expiry, and delete in batches of 500 until nothing expired is left, bounded to 100 batches per table and run. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Verifying the editor's WOPI proof signatures needs an RSA public key built from the modulus and exponent in the discovery document. Nextcloud server only ships phpseclib 2, so bundle phpseclib 3, as richdocuments does, rather than hand-rolling the key encoding. Nextcloud loads apps/<app>/composer/autoload.php at boot and, when it exists, no longer registers the app's lib/ itself. Add it with an autoloader suffix so this app's composer autoloader class can't collide with another app's. Release packaging must run composer install --no-dev; a checkout without vendor/ gets a plain PSR-4 loader for the app's own classes, so only what needs a bundled library fails instead of the whole app. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
buildEditorUrl() swapped the WOPISrc origin for callback_url inline. WOPI proof verification needs the same rewrite to rebuild the URL the editor signed, so move it into DiscoveryService::editorReachableUrl(). Behaviour is unchanged; the existing buildEditorUrl tests cover it. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
The WOPI endpoints are public and authorised by the access token alone, and that token sits in the editor page of whoever opened the file. A visitor on a "hide download" share could copy it and fetch, or with write access overwrite, the file directly; hiding the download was cosmetic. When the editor's discovery publishes a <proof-key>, a new middleware on WopiController now requires a valid X-WOPI-Proof or X-WOPI-ProofOld (RSA-SHA256 over the access token, the request URL and X-WOPI-TimeStamp) from the current or previous key, as MS-WOPI specifies, and a timestamp no older than 20 minutes; anything else gets 403. Euro-Office signs whenever its WOPI support is on (its images generate the keys with WOPI_ENABLED) and Collabora always does. - The URL is checked both as Nextcloud sees it and in its callback_url form, since the editor signs the address it called. - The keys are remembered in app config and rechecked against discovery hourly, so verifying needs no discovery fetch per request even without memcache.local, and a failed discovery fetch keeps the last known keys in force instead of switching the check off. - A proof that fails against the known keys, or arrives while none are known, triggers one reread of discovery, at most every 5 minutes, so keys the editor regenerated are picked up without a key overlap. - Without the bundled phpseclib, requests are refused and the cause is logged. - Rejections are logged with file id and caller address. Nextcloud adds the request URL, token included, to every log line, so only the first rejection per 5 minutes is a warning; repeats go to debug. The verifier is tested against requests signed with Euro-Office's own proof code (tests/fixtures/wopi-proof-euro-office.json). Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Proof signatures only help when the editor publishes keys. For editors that don't, and as a second check for those that do, restrict the WOPI endpoints to the editor servers' addresses with the app config wopi_allowlist (IPs or CIDR ranges separated by commas or spaces; empty keeps them open, as before): occ config:app:set office wopi_allowlist --value="10.0.0.0/24" The caller address is the one Nextcloud resolves through its trusted proxies, and one it can't parse is rejected. An entry that doesn't parse is skipped and reported at most once an hour (every log line of a WOPI request carries its access token), so an allowlist with no valid entry rejects everyone instead of opening up. Rejections are warned about at most once per 5 minutes. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Proof verification only applies when the editor publishes proof keys, and the allowlist only when it's configured. With neither, anyone holding a document's access token can call the WOPI endpoints directly. Say so on the admin page, with both ways to fix it. The warning stays hidden while discovery is unreachable, since the editor's keys are unknown then. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Describe proof-key verification, the wopi_allowlist setting and the admin warning; list the new access, proof and allowlist classes; and add composer install --no-dev to the setup, since the app now ships phpseclib in vendor/. Note that the integration tests need an editor without proof keys, as they call the WOPI endpoints unsigned. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
A file shared with a user with download disabled could be opened in the editor and exported, printed or downloaded from there: user tokens always had hideDownload off. Set it when any share the user received the file through forbids downloading (the same file can reach a user along several paths), as richdocuments does, through OCP's ISharedStorage. The flag then hides export and print in the editor like it does for link shares. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
The file action was only loaded on LoadAdditionalScriptsEvent, which the logged-in Files app dispatches but public share pages don't, so link visitors never saw it and the public-share branch of file-actions.ts was unreachable. Public share pages render as a public files/index page; load the action there too, on the BeforeTemplateRenderedEvent core dispatches for every template response (checked against server master, 36.0.0 dev). Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
PutFile passed fopen('php://input') straight to putContent(). If the
stream couldn't be opened that was putContent(false), which stores an
empty file in place of the document. Answer 500 and leave the file
alone instead.
Assisted-by: ClaudeCode:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
The settings routes carried #[AuthorizedAdminSetting] for the Admin settings class, but that class isn't an IDelegatedSettings, so the attribute could never grant delegated admins access; psalm reported the mismatch. Drop it: without #[NoAdminRequired] the routes are admin-only by default, which is what they effectively were. Delegating them would let non-admins choose which server Nextcloud trusts as the editor, so that stays a deliberate decision for later. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Resolve the remaining type findings instead of baselining them: xpath() results checked as arrays (they can be null as well as false), a cached discovery document used only when it is a string, preg_replace()'s null on failure not passed on to trim()/rtrim(), expired-row ids returned as ints, a null offset passed to the query builder as 0, and the file size cast to int before the Range arithmetic. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
AGENTS.md says psalm findings get fixed, not baselined. Make every class final that nothing extends or mocks, type the class constants, mark overrides with #[\Override], drop redundant @var annotations, and make WopiLockMapper::upsertLock() void since no caller uses its result. The six classes the unit tests mock stay non-final, and entity properties set by Entity::fromRow() are exempt from the constructor check; both are named in psalm.xml with the reason. With that, psalm-baseline.xml is empty and removed, as on main. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
composer.json mapped the OCP and PSR stub packages in autoload-dev, so a developer's vendor/autoload.php (from a plain composer install, which phpunit and psalm need) carried them. Nextcloud loads that file through composer/autoload.php, so in a dev instance any OCP class not loaded yet came from the nextcloud/ocp stubs instead of the running server. Map the stubs in the unit-test bootstrap instead. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Nextcloud 35 and 36 ship phpseclib 3 in 3rdparty; the earlier commit that bundled it assumed the server still had phpseclib 2. A bundled copy is loaded through the app's composer autoloader, which composer registers ahead of the server's, so it would replace the server's phpseclib for every caller in that Nextcloud, at whatever version the app's lock pins. Keep phpseclib as a dev dependency for the unit tests and psalm, and drop composer/autoload.php, so Nextcloud registers the app's lib/ itself again and releases need no vendor/. The "library missing" refusal in the proof middleware goes with it. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
generateFileToken() gained a required hideDownload argument and upsertLock() no longer returns the lock; the integration tests, which no CI job runs, still used the old forms and failed with an ArgumentCountError and a call on null. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
…quests The suite needed a live Euro-Office: the open tests asserted the 415 of an editor that advertised no actions, and every WOPI call went unsigned, so an editor with proof keys turned them all into 403s. A StubEditor now serves discovery with a docx edit action and a proof key from php -S, and the test client signs WOPI requests with that key, using ext-openssl only so the signing shares no code with the app's verifier. A test checks its signed data against a proof Euro-Office's own code produced. The base class points the app at the stub for the run and puts the editor settings back afterwards. The open tests now follow the editor URL to CheckFileInfo, as the editor would, and WopiProofTest covers unsigned, foreign-key, stale and replayed-to-another-file proofs. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
The runner only worked from the host against the dev container, and composer test:integration just printed that and failed. It now runs phpunit where it is started (CI, or a shell in the server container), and OFFICE_IT_CONTAINER keeps the docker exec route. The server's test bootstrap loads every app in apps/, enabled or not, so on an install without files_external enabled its mount provider queried a table that was never created. Load the enabled apps instead, as the web server does. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
No workflow ran PHPUnit: test-unit.yml runs vitest only. Run the unit tests on each supported PHP version, and the integration suite against Nextcloud's newest supported server branch on SQLite, MySQL 8.4 and PostgreSQL 16, based on the organization's phpunit templates. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
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
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.
does MS WOPI support more