Skip to content

fix: support federation between systems in subdirectories - #41599

Merged
DeepDiver1975 merged 2 commits into
masterfrom
federation-subdirs
Jun 8, 2026
Merged

fix: support federation between systems in subdirectories#41599
DeepDiver1975 merged 2 commits into
masterfrom
federation-subdirs

Conversation

@phil-davis

Copy link
Copy Markdown
Contributor

testRemoteUrl was only checking for a responding ownCloud at the provided host. But if the server is installed in a subdirectory then the requests were not sent there. For example:

Now it checks for a response from http://mydomain.com/cloud/status.php

testRemoteUrl was only checking for a responding ownCloud at the
provided host. But if the server is installed in a subdirectory
then the requests were not sent there. For example:
- the cloud server is at http://mydomain.com/cloud
- testRemoteUrl checked for a repsonse from http://mydomain.com/status.php

Now it checks for a response from http://mydomain.com/cloud/status.php
@phil-davis

Copy link
Copy Markdown
Contributor Author

I found this problem because in the acceptance test CI with GitHub workflows I have 2 ownCloud core servers running. One is at localhost/server and the other is at localhost/federated

I had to make this fix/enhancement so that the two core servers would find each other. I imagine that this would also be a problem for a production site that had cloud storage servers at locations like:

somedomain.com.au/sydneycloud

otherdomain.com/chicagocloud

and tried to make federated connections between them.

@DeepDiver1975 DeepDiver1975 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — fix: support federation between systems in subdirectories

Overview: Fixes testRemoteUrl to include the URL path component when constructing the remote URL to test, so servers installed in subdirectories (e.g. mydomain.com/cloud) are correctly probed at mydomain.com/cloud/status.php instead of mydomain.com/status.php.

Correctness

The fix is correct for the stated case. Both the with-scheme and without-scheme branches (if/else) are updated consistently, and the if ($parsed_path !== null) guard correctly skips appending when there is no path.

Edge case: trailing slash on path

If $remote is mydomain.com/cloud/ (trailing slash), parse_url returns $parsed_path = '/cloud/'. After appending, $remote becomes mydomain.com/cloud/. The downstream testUrl call then constructs https://mydomain.com/cloud//status.php (double slash), which may fail depending on the remote server's handling of double slashes.

Suggested guard in both branches:

if ($parsed_path !== null) {
    $remote .= \rtrim($parsed_path, '/');
}

Tests

Good use of @dataProvider — reusing providesRemoteAddress across both testRemoteWithValidHttps and testRemoteWithWorkingHttp is clean. Consider adding a trailing-slash case to the provider ('owncloud.com/cloud/') to cover the edge case above.

Changelog

Present and correctly formatted. ✅

Summary

Aspect Assessment
Correctness ✅ Fixes the stated bug
Edge cases Minor: trailing slash on path produces double slash in URL
Tests Good coverage; one additional case recommended
Changelog ✅ Present

Verdict: Nearly ready to merge. The trailing-slash edge case is minor but easy to fix with rtrim.

@DeepDiver1975

Copy link
Copy Markdown
Contributor

@phil-davis one minor review item - thank you

@phil-davis
phil-davis requested a review from DeepDiver1975 June 8, 2026 14:25
@phil-davis phil-davis changed the title fix: support federation between systems in subdirectories Jun 8, 2026
@phil-davis

Copy link
Copy Markdown
Contributor Author

I edited the PR title to start "bugfix" because I saw that #41603 seems to require "Bugfix" in the changelog.

Are there different rules for what goes in the changelog, and what goes in commit messages?

@DeepDiver1975

@DeepDiver1975

Copy link
Copy Markdown
Contributor

Don't get confused with calens and semantic commit messages.
We might want to consolidate the configurations.

But "fix:" is correct for the PR title

@phil-davis phil-davis changed the title bugfix: support federation between systems in subdirectories Jun 8, 2026

@DeepDiver1975 DeepDiver1975 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — fix: support federation between systems in subdirectories

Overview: testRemoteUrl() in External\Manager was discarding the URL path when constructing the probe URL, so a server at mydomain.com/cloud was probed at mydomain.com/status.php instead of mydomain.com/cloud/status.php. The fix preserves the path component.

Correctness

The fix adds $parsed_path extraction and appends it (with trailing slash stripped) to the reconstructed host string. The same pattern is applied in both the if (\is_string($parsed_host)) branch (scheme-bearing URLs) and the else branch (bare host/path strings prepended with http://). Symmetric fix. ✅

\rtrim($parsed_path, '/'): Prevents double slashes when calens appends /status.php downstream. ✅

$parsed_path !== null guard: parse_url returns null for the path component when there is no path. The guard prevents concatenating a null. ✅

Edge case — path /: A URL like http://mydomain.com/ yields $parsed_path = '/', which after rtrim(..., '/') becomes ''. This is correct — the root path contributes nothing to the base. ✅

Tests

providesRemoteAddress() adds three cases:

  • 'owncloud.com' — bare host (existing behaviour) ✅
  • 'owncloud.com/cloud' — host + path without trailing slash ✅
  • 'owncloud.com/cloud/' — host + path with trailing slash (tests the rtrim) ✅

Both testRemoteWithValidHttps and testRemoteWithWorkingHttp are converted to data-provider tests covering all three cases. ✅

Summary

Aspect Assessment
Path preserved in probe URL ✅ Both URL branches fixed symmetrically
Trailing slash stripped rtrim prevents double slash
Null path guard ✅ Correct
Tests ✅ Three cases including trailing-slash variant
Changelog ✅ Present

Verdict: Ready to merge.

@DeepDiver1975
DeepDiver1975 merged commit 9b2278d into master Jun 8, 2026
13 of 14 checks passed
@DeepDiver1975
DeepDiver1975 deleted the federation-subdirs branch June 8, 2026 15:00
DeepDiver1975 added a commit that referenced this pull request Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants