feat(admin): rework the health check and verify the protected-pad cookie - #179
Merged
Conversation
"Test Etherpad connection" only called listAllPads, so it reported success on instances where protected pads could never work: Nextcloud issues the Etherpad session cookie from its own response, and a browser only accepts a Domain= that covers the sending host. Nextcloud and Etherpad therefore have to share a parent domain, or run on the same host. The two derivations that existed disagreed in exactly that case. The admin page prefilled from the common parent of both hosts, while the runtime derived from the Etherpad host alone and could emit a Domain= the browser discards. CookieDomainPolicy now makes that decision once, and the runtime, the settings page and the health check all read the same object, so what is set and what is reported cannot drift apart. Where both worked before, the shared parent is the narrower of the two values, e.g. .pad.example.org instead of .example.org for a Nextcloud on cloud.pad.example.org. The verdict is a warning beside the connection result, never folded into it, and only evaluated when protected pads are enabled. It is pure string comparison with no I/O, so the settings page can show it without running a test first. Public suffixes are recognised by a conservative heuristic rather than a Public Suffix List lookup — the app ships no runtime dependencies to carry one — so that case reports "may be" instead of a verdict. Corrects the docs alongside: the cookie domain field is no longer marked optional, and README no longer suggests that unrelated registrable domains work with cookie.sameSite "None". SameSite governs whether an accepted cookie is sent cross-site, not which domain may be set. Closes #176
- The health check read the stored protected-pads toggle while testing the submitted form values, so an unsaved change was reported against the wrong configuration in both directions. It now uses the submitted setting, and no longer needs PadTypePolicy at all. - The legacy fallback for a domain stored before the "was configured" flag existed lived only in PadSessionService, so the admin page and the runtime still fed the shared policy different inputs. It moved into CookieDomainPolicy::storedValue(), which every caller goes through. - Saving now returns the recomputed protected_pads block and the page applies it, so a corrected domain no longer keeps showing the old warning, and a newly broken one shows up without a reload.
persist() always wrote etherpad_cookie_domain_configured=yes, discarding what the validator had determined. A client omitting the optional field therefore had an empty value pinned as a deliberate host-only cookie, while the save response still described a derived domain. The browser form always submits the field, so only the admin API path was affected — which does support omitted optional fields.
EtherpadHealthCheckService now returns the decision only; the controller, which already builds the payload, renders the text. That drops the CookieDomainMessages dependency, the cookieDomainMessage field on HealthCheckResult, and the corresponding constructor arguments in three test fixtures. The two health-check tests asserting the submitted protected-pads toggle duplicated the pair already covering it. Renamed the originals to say what they guard and removed the copies. The remaining case now saves no cookie domain, so it exercises the derived path rather than tripping over the fixture's configured one. Adds assertions for the protected_pads contract to the existing save and health controller tests, and corrects a comment in AdminSettings: the field shows the saved value, which is not always what gets sent — with identical hosts the policy uses a host-only cookie regardless.
The connection test checks the base URL, API key and API URL, but sat at the bottom of the page behind three unrelated sections. It now follows the fields it verifies, with its own result area, so a freshly entered server can be checked without scrolling past everything else. Diagnostics keeps the consistency check and the pending-delete retry. The section hint covered both checks in one sentence and is split accordingly; the connection half now says that it works on the entered values whether or not they were saved, which is the reason to have the button there. Result areas share one callout shape — coloured rule, tinted panel — in green for success, red for failure and amber for the cookie warning. A check still running has no verdict, so it stays uncoloured. The rule carries the colour rather than the text: Nextcloud's success green is marginal as body text, and the messages already state the outcome in words, so colour never carries it alone.
One combined verdict hid what it did not cover. The test only contacts the API host, so a typo in the base URL — which every pad link in the browser uses — still read as a clean pass. The result is now a list with a line each for API reachability, the API key, the base URL and the protected-pads session cookie. The protected pads line carries the wording that used to sit in a separate callout, so the outcome and its caveat no longer appear as two disconnected messages. The base URL line is new and the only one doing extra I/O: a short GET against the browser-facing URL. It never fails the test. Nextcloud may be unable to reach that URL by design — split-horizon DNS, an egress firewall, a proxy serving only outside traffic — so it warns and says so rather than crying wolf on a correct instance. Any answer counts as reachable, including 401/403/404, since that already proves the name resolves and something listens. The standalone cookie warning stays for the page load and after saving, where no list exists; the two swap places so the same statement is never shown twice.
Every check maps to exactly one input, so the verdict now appears there: a green tick where things are fine, the reason itself where they are not. The list under the button remains only as a fallback for lines belonging to no single field. An empty API URL falls back to the base URL, so the API line points at whichever field actually supplied the address. The summary said "Etherpad connection test successful", which reads as "everything is fine" when it only meant the request went through — the very confusion the per-check list exists to remove. It now reports what was found: all checks passed, or some settings need attention. The metrics it used to append moved into the API line, where the target belongs anyway.
Only the tick is green now; the text uses the muted body colour. The success colour is too light to read as body text — something this branch already noted before using it for a whole line anyway. The fallback list under the button could never render: every check carries a field and every field has a slot. Removed, along with its markup, styles and the row builder. The cookie verdict had two mechanisms — a standalone callout and a field result. Saving now answers in the same shape the connection test uses, the initial page render writes into the same slot, and the protected-pads line is built once in CookieDomainMessages for both callers. One path, so the same statement can no longer appear twice or drift. Two health-check tests covered the same scenario as their neighbours from a second angle; merged into the originals.
A wrong domain told the admin it was wrong and left them to work out the right one. The policy now derives the domain these two hosts could share and carries it on the decision, so the message ends with "… would cover both". Nothing is suggested when there is nothing to suggest — unrelated hosts, or a shared parent that looks like a public suffix. The no-common-parent message also names the third way out it was missing: a proxy that already serves Etherpad under the Nextcloud domain works as soon as that address is the base URL, because pad links use it and the cookie follows.
- The base URL check assumed request() returns a response for 4xx/5xx, but Nextcloud's client throws — which is why EtherpadClient already recovers it. A reachable host answering 404 was therefore reported as "did not answer". It now uses the same recovery, and reports the original transport error rather than the recovery failure when there is no response at all. Tests cover both shapes. - A skipped base URL line claimed the same field as the API line, so with no separate API URL "Same as the API URL" overwrote the API verdict. Skipped lines carry no field now. - Field verdicts were cleared only once a response arrived, leaving green ticks beside "Testing…" and surviving a failed save. Both actions clear them before the request; the consistency check does not, since it changes nothing. Covered with a deferred request rather than an immediate one. - A rejected API key or an unreachable API host threw without a field, so the most important failure appeared only in the summary. The existing classification now also yields the field: the key for auth failures, the effective API host field for transport errors and 404. - Removed a duplicated pendingActions assignment.
- Any 4xx now warns instead of counting as reachable. A base URL with a wrong path answers 404 while every pad link built from it is broken, and the summary still said all checks passed. - The health result could contradict itself: the service produced the protected-pads line while the controller held the decision, so a warning decision could sit beside an empty check list and an "all passed" summary. The service returns the decision and the other lines; the controller appends the protected-pads line once and feeds the same list to both the summary and the payload. CookieDomainMessages is no longer a dependency of the health service. - The error mapper test now asserts the field it forwards, and that it is omitted when there is none. Removals: the old-markup fallbacks for the two status areas, since template and bundle ship together; the health_ok string, its data attribute and catalog entries, now that the endpoint always sends its own summary; and the paragraph in api-reference.md that repeated the cookie explanation from README.
The previous cleanup spliced the new text in without removing what it replaced, leaving the protected_pads block twice and the long cookie paragraph still there. Worse, the indentation put protected_pads under the 502 response, which carries only message and an optional field. One block now, correctly nested, pointing at README for the reasoning; the 502 note says explicitly that nothing else is on that response. HealthCheckResult loses the defaults on cookieDomain and checks — both call sites pass them, and without a default nobody can build a half-filled result by accident.
The hint and the field were derived by two separate matchers over the same error strings, so a new Etherpad message could produce a correct hint with the wrong field. classifyFailure() decides once and both are looked up from the reason; the two test providers become one table asserting message, hint and field together. Adds CookieDomainMessagesTest: a provider drives the policy into each warning it can produce and asserts the rendered line is a warning, is not blank and names the hosts or domain involved, without pinning the wording. asCheckItem() now falls back to the hosts sentence when a reason has no text, so a reason added later cannot appear as a blank warning. Adds AdminSettingsTest for the initial page render — the warning must be there before anyone runs a test, which is why the check does no I/O — with stubs for OCP\Util and OCP\Settings\ISettings. Removals: the empty-base-URL branch and its translations, unreachable because the validator rejects it; a test whose EtherpadClient mock broke the guaranteed return shape; the unused lineById() helper; a stale comment claiming a 404 proves reachability. The redirect test asserted nothing about redirects and now pins the request options instead.
Merging the two matchers dropped a generic case: the old field logic marked the key field for any message containing "api key" or "apikey", while the new classification only recognised the specific wordings and 401/403. "API key file could not be read" therefore lost its field. A last-resort reason restores it — placed last so it can never shadow a more specific reason and its hint, and carrying no hint of its own since there is nothing to advise. The fallback for an unmapped cookie reason now says the domain could not be verified rather than only naming the hosts, so a forgotten mapping reads as a gap instead of hiding behind a bare fact.
Code Review by Qodo
1.
|
PR Summary by QodoRework connection test into per-field checks; unify cookie-domain logic
AI Description
Diagram
High-Level Assessment
Files changed (43)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
The check contacted an admin-supplied URL with Nextcloud's local-address protection switched off and followed up to three redirects. The exemption is the actual bypass; the redirects made it steerable by whoever controls the configured host, without any admin involvement. Both are gone. A 3xx now counts as reachable, which is all this line ever claimed, and the body is streamed rather than buffered so a hostile host cannot dump data into the request within the timeout. Dropping the exemption is also the more accurate check: the base URL is what a browser opens, so an address only this server can reach is broken regardless. A blocked address says exactly that instead of the generic "did not answer", which would send the admin hunting a network fault that does not exist. The check no longer skips when the base URL matches the API URL. That shortcut hid the case the strict policy exists for, since the API call may well succeed against a loopback address no browser can use. Both lines then land on the same field, so the renderer keeps the more severe verdict instead of whichever arrived last.
Nextcloud refusing to contact a local address says nothing about whether users can: an internal or split-DNS deployment serves such addresses to browsers every day. The line now reports that the check was blocked and asks the admin to confirm their users can reach it, instead of asserting that browsers cannot open pads there. Drops the documented skip path, which no longer exists, and the note about proxies redirecting http to https — the validator already requires https for the base URL.
A transport failure can carry a long tail of internal hostnames and addresses into the admin panel. The base URL check already capped its detail; the API failure did not. Both use the same limit now, since both end up in the same place. The cut happens after classification, not before: a keyword can sit behind it, and losing it would drop the hint and the field along with it.
"saved or not" read as an afterthought. That the test works on the entered values follows from "the values above", so the clause was carrying its weight in awkwardness rather than information.
The English source read as a chain of clauses, and the translations followed it word for word – "the cookie follows" became "das Cookie folgt", "la cookie funcionará", "le cookie suivra", none of which is how any of them would be put. Simplifying the source removed the cause rather than patching each translation: it now names the subject, says plainly that the browser cannot send the cookie, and lists the two ways out as separate sentences. Spanish used "dominio principal" for parent domain, which reads as "main domain"; it is "dominio padre común" now, in the message and in the field hint. Also picks up smaller wording fixes: "mit den oben eingetragenen Werten", "con los valores indicados arriba", "hébergez les deux sur le même hôte".
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
1 participant
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.
The connection test gave one verdict for everything and only ever contacted the API host. It now reports per checked part – API, API key, base URL, session cookie – each shown at the field it came from, with a green tick where things are fine and the reason where they are not.
Two gaps that hid behind the single verdict:
The base URL was never checked. Nothing contacts it except the browser, so a typo there broke every pad link while the test stayed green. It is checked now, but never fails the test: Nextcloud may be unable to reach the public URL by design (split-horizon DNS, egress firewall), so an unreachable base URL warns and says so.
Protected pads could be impossible and nobody said so. They need a session cookie valid for both hosts. Nextcloud issues it from its own response, and a browser only accepts a
Domain=covering the sending host – so the two must share a parent domain or run on the same host.SameSitedoes not lift that restriction.Checking it surfaced a real bug: two derivations existed and disagreed in exactly the failing case. The settings page prefilled from the common parent of both hosts, while the runtime derived from the Etherpad host alone and could emit a
Domain=the browser discards.CookieDomainPolicymakes that decision once; runtime, settings page and health check read the same object. Where both worked before, the shared parent is the narrower of the two values. When a configured domain is wrong, the message names one that would work.The summary no longer says "successful" – that read as an all-clear when it only meant the request went through. It now reports what was found, and a failure marks the input that caused it instead of appearing only at the bottom.
Docs corrected alongside: the cookie domain field is no longer marked optional, and README no longer suggests unrelated registrable domains work with
cookie.sameSite: "None".Closes #176