Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make federation address book sync work with allow_local_remote_servers = false #48451

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Sep 30, 2024

  • Resolves: federation address book sync does not work with allow_local_remote_servers = false

Summary

Client.preventLocalAddress expects an absolute URL, which means the base_uri option cannot be used.

Regression from #46002

TODO

  • CI
  • Review

Checklist

@kesselb kesselb force-pushed the bug/noid/federated-addressbook-sync-without-localaddressallowed branch from f4d1897 to c7f8a9f Compare September 30, 2024 11:09
@kesselb kesselb changed the title fix: wip fix: make federation address book sync work with allow_local_remote_servers = false Sep 30, 2024
@kesselb kesselb added this to the Nextcloud 31 milestone Sep 30, 2024
@kesselb kesselb marked this pull request as ready for review September 30, 2024 11:12
@kesselb kesselb self-assigned this Sep 30, 2024

$options = [
'auth' => [$userName, $sharedSecret],
'base_uri' => $url,
'body' => $this->buildSyncCollectionRequestBody($syncToken),
'headers' => ['Content-Type' => 'application/xml']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also add 'nextcloud' => ['allow_local_address' => true], to always allow private IP ranges for address book federation. But we should also enable it for the initial handshake then, and therefore that's something for a follow-up.

@kesselb kesselb force-pushed the bug/noid/federated-addressbook-sync-without-localaddressallowed branch from c7f8a9f to a16eb6c Compare September 30, 2024 12:15
@kesselb
Copy link
Contributor Author

kesselb commented Sep 30, 2024

ClientTest is failing:

1) Test\Http\Client\ClientTest::testPreventLocalAddressDisabledByGlobalConfig with data set #14 ('!@#$', true)
OCP\Http\Client\LocalServerException: Could not detect any host

/home/runner/actions-runner/_work/server/server/lib/private/Http/Client/Client.php:163
/home/runner/actions-runner/_work/server/server/tests/lib/TestCase.php:233
/home/runner/actions-runner/_work/server/server/tests/lib/Http/Client/ClientTest.php:165

2) Test\Http\Client\ClientTest::testPreventLocalAddressDisabledByOption with data set #14 ('!@#$', true)
OCP\Http\Client\LocalServerException: Could not detect any host

/home/runner/actions-runner/_work/server/server/lib/private/Http/Client/Client.php:163
/home/runner/actions-runner/_work/server/server/tests/lib/TestCase.php:233
/home/runner/actions-runner/_work/server/server/tests/lib/Http/Client/ClientTest.php:176

The above URL is invalid, and guzzle/curl will reject it with:

cURL error 3: URL using bad/illegal format or missing URL (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for !@#$

If we keep a16eb6c, then the notable change is that the request is just rejected a bit earlier. Though, it might be nicer to use a different/new exception for it.

Opinions?

@kesselb kesselb force-pushed the bug/noid/federated-addressbook-sync-without-localaddressallowed branch from a16eb6c to 5576f05 Compare October 1, 2024 15:55
…ervers = false

Client.preventLocalAddress expects an absolute URL, which means the base_uri option cannot be used.

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
This change should make it easier to spot wrong uses of the HTTP client on development setups where allow_local_remote_servers is usually true.

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@kesselb kesselb force-pushed the bug/noid/federated-addressbook-sync-without-localaddressallowed branch from 5576f05 to 6be0043 Compare October 1, 2024 16:00
Copy link
Contributor

@pabzm pabzm left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! 👍

@kesselb kesselb merged commit 1c1f488 into master Oct 1, 2024
177 checks passed
@kesselb kesselb deleted the bug/noid/federated-addressbook-sync-without-localaddressallowed branch October 1, 2024 18:41
@kesselb
Copy link
Contributor Author

kesselb commented Oct 1, 2024

/backport 8708164 to stable30

@kesselb
Copy link
Contributor Author

kesselb commented Oct 1, 2024

/backport 8708164 to stable29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants