fix(security): validate federation server against trusted-servers list (OC10-100)

[DeepDiver1975]

Summary

• Replaces the always-true isServerAllowed() stub in FederationService with a real check against ownCloud's federation trusted-servers list (OCA\Federation\TrustedServers)
• When the federation app is not installed (TrustedServers is null), all remote federation requests are denied by default (secure default)
• URL comparison strips trailing slashes and checks both http/https scheme variants to tolerate minor admin-input differences
• Explicitly registers FederationService in the DI container so TrustedServers can be injected when the federation app is available

Fixes OC10-100 (SSRF via federated endpoint, CVSS 7.1).

Test plan

• Unit tests added for isServerAllowed() covering: null TrustedServers → deny, exact match, trailing slash stripping (single and multiple), http↔https scheme swap, untrusted server → false, empty list → false
• Run phpunit --testsuite unit --filter testIsServerAllowed — all 7 tests pass
• Run full unit suite — all existing tests pass
• Integration: deploy with federation app enabled, verify trusted remote server works; verify untrusted server is denied with a log entry
• Integration: deploy with federation app disabled, verify all remote federation requests are denied

🤖 Generated with Claude Code

[DeepDiver1975]

Code Review — PR #598

Overview

Fixes an SSRF vulnerability where isServerAllowed() always returned true, allowing the federated endpoint to proxy requests to any arbitrary server. The fix injects TrustedServers from ownCloud's federation app and validates incoming server URLs against it. Good, targeted security fix with solid test coverage.

---

Code Quality

lib/FederationService.php

• The isServerAllowed() implementation is correct and well-structured. The null-guard, normalization, and scheme-swap logic are all sound.
• The multi-line docblock on isServerAllowed() explains why the scheme swap exists — appropriate given it's a non-obvious invariant.
• Minor: The aligned assignment operator style ($this->logger = $logger) deviates from the rest of the codebase (surrounding code uses unaligned assignments). Not a blocker, but inconsistent.

lib/AppInfo/Application.php

• One concern: \OC::$server->query(...) is used inside the closure instead of the $server variable already in scope via use ($server). These should be the same object in normal operation, but mixing \OC::$server (global static) with the injected $server is inconsistent and harder to test. Should be $server->query(...).

tests/unit/FederationServiceTest.php

• The 7 new test cases cover the meaningful branches well: null guard, exact match, trailing slash (single and multiple), scheme swap both ways, untrusted, and empty list.
• testIsServerAllowedReturnsFalseWhenListIsEmpty and testIsServerAllowedReturnsFalseForUntrustedServer are effectively identical — both mock isTrustedServer → false with no other distinction. One could be dropped without losing coverage.

---

Potential Issues

1. \OC::$server vs injected $server (Application.php): Prefer $server->query(...) over \OC::$server->query(...) for consistency and testability.

2. No path-prefix handling: If a trusted server is stored as https://cloud.example.com/owncloud, rtrim($remote, '/') won't normalize it to match correctly. Worth noting as a known limitation.

3. isTrustedServer called twice on mismatch: Once with the normalized URL, once with the scheme-swapped URL. Since this likely does a database lookup, minor performance concern in hot paths. Not critical.

---

Suggestions

• Application.php: Replace \OC::$server->query(...) with $server->query(...).
• FederationServiceTest.php: Remove one of the two redundant false-return tests.
• Consider noting the path-prefix limitation as a known non-goal in a comment or follow-up ticket.

---

Security Assessment

The fix correctly closes the SSRF: the default is now deny-all (null TrustedServers), and the allowlist check is authoritative. The scheme-swap tolerance is a reasonable UX accommodation that doesn't weaken security meaningfully.

Verdict: Approve with minor fix — the \OC::$server inconsistency in Application.php should be addressed before merging.

[DeepDiver1975]

Code Review — PR #598 (updated)

Overview

Fixes SSRF in the federated endpoint by replacing the always-true isServerAllowed() stub with a real check against ownCloud's TrustedServers list. Three files changed: the service implementation, its DI wiring, and its tests. Previous review issues resolved.

---

Code Quality

lib/FederationService.php — Clean.

• isServerAllowed() logic is correct: null-guard → normalize → exact check → scheme-swap → second check.
• substr offsets are correct (https:// = 8 chars, http:// = 7 chars).
• The multi-line docblock is justified — the two-pass scheme-swap is non-obvious.
• Aligned assignment style in the constructor is cosmetically inconsistent with the rest of the file, but harmless.

lib/AppInfo/Application.php — Good, previous \OC::$server issue resolved.

• $server->query(...) is now used consistently throughout.

tests/unit/FederationServiceTest.php — Good, redundant test removed.

• 6 isServerAllowed tests cover: null guard, exact match, single slash strip, multiple slash strip, http→https swap, https→http swap, untrusted server.

---

Remaining Minor Points

1. $server->query() vs $container->query(): $server is $container->getServer(), so this queries the server-level DIC rather than the app container. Fine for a core class like TrustedServers, just worth noting as a deviation from the conventional app-container query pattern.

2. Known limitation (acceptable): Path-prefixed installs (e.g. https://cloud.example.com/owncloud) aren't handled by trailing-slash stripping alone. Pre-existing edge case, not introduced here.

3. No use import for TrustedServers in Application.php: Referenced as a fully-qualified class name inside the closure rather than via a use statement, slightly inconsistent with the use OCA\Richdocuments\FederationService import above. Not a blocker.

---

Security Assessment

• Default deny (null TrustedServers) is correct.
• The allowlist check is authoritative — no bypass path visible.
• The scheme-swap tolerance doesn't weaken security: both variants must be untrusted to block a request.

Verdict: Ready to merge. The two issues from the first review are resolved. Remaining points are cosmetic or pre-existing.

[jvillafanez]

I'd prefer for the admin to provide the list of trusted servers via configuration (either system config or app config). This way, the app remains independent (not relying on the federation app)

[DeepDiver1975]

The list of trusted servers is already known to the system. Maintaining another list is just pure pain and absolutely not user friendly.

[jvillafanez]

The problems I see are:

• The trusted servers are in the "federation" app, which can be switched off any time
• According to the description, the "federation" app syncs user directories for auto-completion. It doesn't say anything about syncing or retrieving files from other servers. It should be possible to access to federated shares without the "federation" app, so it makes sense for the "richdocuments" app to use this feature for those same federated shares.
• The list will only include servers which we can exchange users with (which is the purpose of the "federation" app). This is likely a much smaller set of servers than the ones used to get files from.
• Exchanging users seems to be a requirement now.

It would be much easier if the trusted servers were part of core

[DeepDiver1975]

• The trusted servers are in the "federation" app, which can be switched off any time

yes - and in the sense of 'federation deactivated' in this scenario: no collabora federation will work as well. But well - there is also the federatedfilesahring app. As usual a super messy design over all ....

* According to the description, the "federation" app syncs user directories for auto-completion. It doesn't say anything about syncing or retrieving files from other servers. It should be possible to access to federated shares without the "federation" app, so it makes sense for the "richdocuments" app to use this feature for those same federated shares.

One could repurpose the federation app ....

* The list will only include servers which we can exchange users with (which is the purpose of the "federation" app). This is likely a much smaller set of servers than the ones used to get files from.

* Exchanging users seems to be a requirement now.

It would be much easier if the trusted servers were part of core

This is absolutely true - nothing to argue - but we will not change this as of now.