fix(builtin): derive server mode and public URL from IURLGenerator rather than browser context#5639
Draft
joshtrichards wants to merge 19 commits intomainfrom
Draft
fix(builtin): derive server mode and public URL from IURLGenerator rather than browser context#5639joshtrichards wants to merge 19 commits intomainfrom
joshtrichards wants to merge 19 commits intomainfrom
Conversation
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
…ing set Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
…ctivity Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.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.
Summary
The built-in CODE server (richdocumentscode) configuration had a cluster of related bugs rooted in a single misconception: the admin UI was constructing the CODE proxy URL client-side from
window.location.hostand using it for both detection and storage. This only worked by coincidence on the simplest single-host setups and broke silently under reverse proxies, split DNS, or any deployment where the internal and public URLs differ.This PR fixes all related issues and takes advantage of CODE's
ProxyPrefixmechanism — which makes the/hosting/discoveryresponse request-context-aware — to establish a clean, correct architecture for both browser-based and CLI-based setup paths.Bugs Fixed
Bug 1 — Builtin mode detection used a browser-constructed URL (
AdminSettings.vueL834–835)checkIfDemoServerIsActive()constructedCODEUrlfromwindow.location.host + generateFilePath(...)and compared it to the server-storedwopi_url. These are conceptually different things that only matched by coincidence. Any reverse proxy or split DNS broke the comparison, causing the "builtin" radio button to never be selected even with a working CODE installation.Bug 2 —
setBuiltinServer()wrote a browser-constructed URL aswopi_urlwopi_urlis the internal server-to-server URL; it must be derived fromIURLGeneratorserver-side. Storing a browser-constructed value here caused Nextcloud to attempt internal connections via the public URL, which fails or performs unnecessarily in many network configurations.Bug 3 —
checkSettings()never re-evaluatedserverModeserverModewas determined once inbeforeMount()from the initial state snapshot.checkSettings()fetched fresh settings asynchronously but never calledcheckIfDemoServerIsActive()afterwards, leaving the displayed radio button stale.Bug 4 —
autoConfigurePublicUrl()poisonedpublic_wopi_urlin CLI contextFor the built-in CODE server,
public_wopi_urlshould always equal Nextcloud's own public origin — CODE has no separate hostname. ButautoConfigurePublicUrl()derived it server-side from the discoveryurlsrc, which for builtin CODE is request-context-aware viaProxyPrefix. In CLI context (occ), this reflectedoverwrite.cli.url(often internal/localhost) rather than the true public URL, causingSERVER_STATE_BROWSER_CONNECTION_ERROReven when the server-side connection was healthy.Architecture Changes
server_modeis now first-class configA new
server_modeconfig key (builtin|custom|demo) is stored explicitly. The frontend reads it directly — no URL string comparison needed, no guessing.wopi_urlfor builtin is derived at runtime, never storedAppConfig::getCollaboraUrlInternal()now derives the proxy URL viaIURLGeneratoron every call for builtin mode. This means it automatically stays correct after domain migrations without reconfiguration.public_wopi_urlfor builtin is derived fromIURLGenerator, not discoveryFor builtin,
AppConfig::getCollaboraUrlPublic()returnsdomainOnly(IURLGenerator::getAbsoluteURL('/'))— Nextcloud's own public origin. This is correct in both HTTP and CLI contexts, providedoverwrite.cli.urlis set (a standard Nextcloud prerequisite already required for cron jobs, notifications, and otherocccommands).autoConfigurePublicUrl()is skipped for builtinSince
public_wopi_urlis derived dynamically for builtin, discovery-based detection is neither needed nor correct.ConnectivityService::autoConfigurePublicUrl()is now a no-op whenserver_mode === 'builtin'.Browser self-healing via
ProxyPrefixcheckFrontend()now fetches/hosting/discoverydirectly (same-origin for builtin, fully readable) and extracts theurlsrcorigin. Because CODE'sProxyPrefixmechanism rewritesurlsrcto reflect the request's host and scheme, the browser always receives the correct public origin. If it differs from the storedpublic_wopi_url(e.g. after a domain migration), it is persisted back to the server automatically.CLI builtin setup via
--builtinflagocc richdocuments:activate-confignow accepts--builtin. It derives bothwopi_urlandpublic_wopi_urlfromIURLGenerator, verifies internal connectivity, and fails fast with an actionable message if the derived URL looks internal (indicatingoverwrite.cli.urlis not set correctly).Changes
lib/AppConfig.phpSERVER_MODEconstant; injectIURLGenerator; addgetServerMode(),isBuiltinServer(),getBuiltinServerUrl(); updategetCollaboraUrlInternal()andgetCollaboraUrlPublic()to derive values dynamically for builtin modelib/Service/ConnectivityService.phpautoConfigurePublicUrl()returns early for builtin modelib/Controller/SettingsController.phpserver_modeandbuiltin_server_urltogetSettingsData(); updatesetSettings()to accept and handleserver_mode; activate builtin mode server-side viaIURLGeneratorlib/Settings/Admin.phpserver_modeandbuiltin_server_urlin initial statelib/Command/ActivateConfig.php--builtinflag with full builtin setup path; derive and validate public URL fromIURLGenerator; fail fast with actionable error if URL looks internalsrc/components/AdminSettings.vuecheckIfDemoServerIsActive()to readserver_modedirectly; replacesetBuiltinServer()to delegate URL derivation to the server; fixcheckSettings()to re-evaluateserverModeafter async fetch; replacecheckFrontend()with discovery-based self-healing for builtin; remove all client-sideCODEUrlconstructionTesting
Browser-based setup (fresh install):
public_wopi_urlis correctly detected viacheckFrontend()server_modefrom serverCLI-based setup:
occ richdocuments:activate-config --builtinwithoverwrite.cli.urlcorrectly set; verify both URLs are correctoverwrite.cli.urlset to localhost; verify command fails fast with actionable error messageocc richdocuments:activate-config --wopi-url=...(custom server path); verify existing behaviour unchangedSelf-healing:
public_wopi_urlis updated automatically without manual reconfigurationRegression:
server_mode=custom,autoConfigurePublicUrl()still runs,public_wopi_urlstill set from discoveryTODO
Checklist