Skip to content

Conversation

joshtrichards
Copy link
Member

@joshtrichards joshtrichards commented Jan 31, 2025

Summary

#43650 added logging of the host that triggers the LocalServerException. It also added the port to that log output.

However the logged port is misleading and logging it does not make sense in this context because... it's not the actual port specified in the request.

The port here is merely the one that happens to be the current one in the foreach iterator when the IP in question triggers isLocalAddress($ip) to be true. (We check a list of ports that is essentially {requestPort + 80 + 443}. Ultimately that'll be 80 every time which, again, has nothing to do with the current request.

So either don't log any port or, since we do have the request port, go ahead and log it for the admin since it's more relevant.

P.S. Hmm. Actually upon reflection I think we can probably just move the entire foreach ($ports as $port) { below the isLocalAddress() check too. The ports list is only used for properly populating the CURLOPT_RESOLVE. The ports themselves are not used for the check itself so no reason to run the check within the port combo iterator.

foreach ($ports as $port) {
$curlResolves["$hostName:$port"] = [];

TODO

  • ...

Checklist

Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
@joshtrichards joshtrichards added bug 3. to review Waiting for reviews labels Jan 31, 2025
@joshtrichards joshtrichards added this to the Nextcloud 32 milestone Jan 31, 2025
@joshtrichards
Copy link
Member Author

/backport to stable31

@joshtrichards
Copy link
Member Author

/backport to stable30

@joshtrichards
Copy link
Member Author

/backport to stable29

@Altahrim
Copy link
Collaborator

I think it makes sense to do what's in your "P.S.": check IP addresses first and populates curl options later.
It could allow to fix the TODO too, maybe in a second time

This was referenced Aug 22, 2025
This was referenced Sep 2, 2025
This was referenced Sep 25, 2025
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
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.

3 participants