Skip to content

fix(federation): check a partner recipient's certificate strictly and refuse an answer for another account - #1288

Open
SudoThijn wants to merge 1 commit into
developmentfrom
fix/partner-certificate-trust
Open

SudoThijn wants to merge 1 commit into
developmentfrom
fix/partner-certificate-trust

Conversation

@SudoThijn

Copy link
Copy Markdown
Contributor

Summary

Closes #1257, which includes #1281. It hardens how the web app checks a partner recipient's certificate. The two open questions on the issue were answered by the backend lead (see the questions comment): the web app trusts its own server, so there is no browser-side root pin, and a leaf with cA=TRUE is refused without backwards compatibility.

Cloud id check

  • lookup (src/store/modules/federatedShare.js) refuses an answer whose cloud id doesn't match the requested one, or an empty requested id, with CloudIdMismatchError.
    • The requested id gets the server's stripShareLinkFragments normalisation (backslashes, /index.php, trailing slashes), and the scheme and host case are ignored on both sides, so every form the server accepts still matches.
  • The share form refuses a mismatch with the existing "could not be verified" message.
  • Share-save suspends that partner share with reason cloud_id_mismatch, as one whose certificate no longer verifies. It doesn't block the save or the offline queue: a mismatch there can only come from an honest server whose cloud id resolver rewrites the id, and it would happen on every save.

Verifier (src/crypto/federatedCertificate.js)

It now refuses what the browser extension's port refuses, plus a CA leaf:

  • a self-issued intermediate, and an empty canonical cloud id on either side;
  • a leaf with cA=TRUE (bad_chain), whatever its keyUsage says;
  • an outer signature algorithm that differs from the TBS one, and a signature with unused bits;
  • on the leaf only: more than one common name, an unknown string type or invalid UTF-8 (a leading BOM is kept, a printable-ASCII TeletexString accepted); a CA's common name is never read;
  • RSA-PSS parameters with a mask other than MGF1, a trailer other than 1, or unknown, repeated, out-of-order or empty fields;
  • any other parse failure as malformed instead of a TypeError.

canonicalCloudId folds only the host's case, so the case of a cloud id's path now counts.

Partner gone

PARTNER_GONE (src/services/shareSave.js) pairs each reason with its status: unknown_recipient 404, not_a_partner 403, bad_chain 502. The same reason on another status, such as a proxy's 500, is an ordinary error and suspends nothing.

Docs

  • openspec/specs/federated-sharing/spec.md: the lookup requirement lists the new refusals and the trust in the own server as a known limit, with two new scenarios; a cloud id mismatch suspends a share.
  • openspec/changes/sharing-atomic-share-save/: the same suspension rule and the reason/status pairing, with a scenario.
  • CHANGELOG [Unreleased] Security: the CA:TRUE leaf refusal and the suspension it causes, and a Web app partner certificate check: trust in the own server, and verifier strictness #1257 entry.

Related

Test plan

  • npx vitest run: 206 files, 1793 tests pass. Every guard has a test that fails without it, checked by reverting each one.
  • npm run lint (0 errors), format, stylelint, test:l10n, check:l10n-js and check:manifest pass.

… refuse an answer for another account

The web app refuses a certificate lookup answer whose cloud id isn't the one asked for, after the server's own normalisation. The share form refuses it; a share-save suspends that partner share as one whose certificate no longer verifies, so the save and the offline queue go on.

The verifier refuses what the browser extension's port refuses: a self-issued intermediate, an empty cloud id on either side, an outer signature algorithm that differs from the TBS one, a signature with unused bits, strict common-name rules on the leaf only, and strict RSA-PSS parameters. It also refuses a leaf marked cA=TRUE, without backwards compatibility, and folds only the host's case of a cloud id.

PARTNER_GONE pairs each reason with its status, so a reason on another status, such as a proxy's 500, is an ordinary error and suspends nothing.

The browser root pin is dropped: the web app trusts its own server, which the spec records as a known limit.

Closes #1257
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/keepiq @ 26d8047

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-manifest ✅
test-l10n ✅
format ✅
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 114/114
npm ✅ ✅ 660/660
app:check-code ⏭️
info.xml ✅
REUSE ✅
lockfile sync ✅
PHPUnit ✅ 2321/2321 · 3 skipped
Newman ✅ 177/177 · 20 skipped
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates ✅

Quality workflow — 2026-10-16 15:15 UTC

Download the full PDF report from the workflow artifacts.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant