feat(security): use richdocuments.federation_allowlist system config for federation server validation

[DeepDiver1975]

Summary

• Replaces `OCA\Federation\TrustedServers` with a plain PHP array in `config.php` as the sole authority for permitted federation servers
• Admins configure the allowlist via `richdocuments.federation_allowlist` system config key — no federation app dependency required
• Absent, empty, or non-array values deny all federation requests (secure default)
• Allowlist entries are plain domain names (no scheme); both http and https requests are accepted for any listed domain

Config

```php
// config/config.php
'richdocuments.federation_allowlist' => [
'collab1.example.com',
'collab2.example.com',
],
```

Test plan

• Unit tests cover: absent key, non-array value, exact match, trailing slash (single/multiple), https request, http request, untrusted server
• `make test-php-style` passes
• CI: PHP Code Style, Phan, PHPStan all green
• Integration: add entry to `config.php`, verify trusted remote works; verify unlisted remote is denied with log entry
• Integration: omit key entirely, verify all federation requests denied

🤖 Generated with Claude Code

[DeepDiver1975]

Code Review

Overview

This PR replaces the OCA\Federation\TrustedServers dependency from PR #598 with a standalone richdocuments.federation_allowlist system config array, removing the hard runtime dependency on the federation app being installed. Admins configure allowed federation endpoints directly in config.php.

3 PHP files changed + 2 documentation files added.

---

lib/FederationService.php

Clean. The isServerAllowed() rewrite is correct:

• Deny-all default when key is absent or non-array ✓
• Trailing-slash stripping on both $remote and each allowlist entry ✓
• HTTP↔HTTPS scheme swapping to tolerate minor mismatches ✓
• No Phan suppression comments needed (IConfig is a core interface) ✓

---

lib/AppInfo/Application.php

Clean. DI registration simplified to four core dependencies; the isInstalled('federation') conditional and null-TrustedServers pattern from PR #598 are gone entirely.

---

tests/unit/FederationServiceTest.php

9 focused tests covering all branches of isServerAllowed(). The IConfig mock setup is correct.

Minor: testIsServerAllowedReturnsFalseWhenKeyIsAbsent and testIsServerAllowedReturnsFalseWhenListIsEmpty are cosmetically redundant — both stub getSystemValue to return [] and both assert false. The distinction (absent key vs. explicitly empty array) is meaningful to document, so keeping both is reasonable; just worth noting they exercise identical code paths.

---

Overall

No security issues, no correctness issues, no style concerns. The implementation is minimal and correct. The only question is whether the cosmetic test redundancy warrants removing one test — I'd lean toward keeping both as documentation of intent.

[jvillafanez]

This is what I had in mind.

[jvillafanez]

If we don't care about the protocol, it might be easier to just ask for the trusted host (just oc.server.prv instead of https://oc.server.prv). Stripping the protocol from the incoming remote and then checking for the matching host should be easier, even if we would need to ensure it's http or https.

[DeepDiver1975]

Code Review

Overview

Replaces the isServerAllowed() stub (returned true unconditionally) with a real implementation backed by a richdocuments.federation_allowlist system config array of plain domain names. The scheme is stripped from the incoming URL before matching, so a single entry covers both http:// and https://. DI wiring and tests updated accordingly.

---

lib/FederationService.php

Correct and clean. Two observations:

• preg_replace null return: preg_replace() can return null on error. With a literal pattern this is impossible in practice, but a (string) cast makes the type intent explicit and silences static analysis:

  $domain = \rtrim((string)\preg_replace('|^https?://|', '', $remote), '/');

• Path-prefixed installs: After scheme stripping, https://cloud.example.com/owncloud becomes cloud.example.com/owncloud and won't match a bare domain entry. Worth a note in the docblock as a known limitation.

---

tests/unit/FederationServiceTest.php

Good coverage. One cleanup: testIsServerAllowedReturnsFalseWhenKeyIsAbsent and testIsServerAllowedReturnsFalseWhenListIsEmpty both stub getSystemValue to return [] and assert false — identical code paths. One can be dropped.

---

PR Description

Stale — needs updating:

1. Config example still shows scheme-prefixed URLs (https://collab1.example.com) — should be plain domain names.
2. Test plan still mentions http→https swap / https→http swap — that logic was removed.

---

Security

No issues. Domain-only matching is slightly more robust than the previous scheme-swap approach.

---

Summary

One code fix ((string) cast), PR description update, and one redundant test to drop.

[jvillafanez]

Just a small thing, but it looks good.

[jvillafanez]

I'm not sure if this is needed because it should be auto-wired by the DI