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

Log host/ip for access violation #43650

Merged

Conversation

Commifreak
Copy link
Contributor

@Commifreak Commifreak commented Feb 19, 2024

Summary

During checking some Host violates local access rules I noticed, that there is no value being logged. This PR adds the affected Host/IP to the Exception.

Checklist

@solracsf solracsf added this to the Nextcloud 29 milestone Feb 19, 2024
@solracsf solracsf added the 3. to review Waiting for reviews label Feb 19, 2024
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Makes sense

@Commifreak
Copy link
Contributor Author

Makes sense

I also added the hostName and port to the message.

@skjnldsv
Copy link
Member

There were 3 failures:

1) lib\Http\Client\DnsPinMiddlewareTest::testRejectIPv4
Failed asserting that exception message 'Host "192.168.0.1" (www.example.com:80) violates local access rules' contains 'Host violates local access rules'.

2) lib\Http\Client\DnsPinMiddlewareTest::testRejectIPv6
Failed asserting that exception message 'Host "fd12:3456:789a:1::1" (ipv6.example.com:80) violates local access rules' contains 'Host violates local access rules'.

3) lib\Http\Client\DnsPinMiddlewareTest::testRejectCanonicalName
Failed asserting that exception message 'Host "192.168.0.2" (www.example.com:80) violates local access rules' contains 'Host violates local access rules'.

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 23, 2024
@Commifreak
Copy link
Contributor Author

Commifreak commented Feb 23, 2024

Uhm... Thanks for bringing that up. Iam not well experienced with testst and it feels wrong to hard-code the to-test IPs to the expectExceptionMessage function. Is there a way to define that the expected exception message contains not the exact string but the part violates local access rules?

EDIT: Sorry, just read the PHPUnit docs right, that IS actually a part of a string... I will update the test to only contain the last part.

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Feb 23, 2024
@skjnldsv skjnldsv force-pushed the log_host_ip_for_access_violation branch from 84f7712 to 51739ad Compare February 24, 2024 10:34
@skjnldsv skjnldsv merged commit e79d426 into nextcloud:master Feb 24, 2024
150 of 157 checks passed
Copy link

welcome bot commented Feb 24, 2024

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@Commifreak Commifreak deleted the log_host_ip_for_access_violation branch February 24, 2024 11:48
@blizzz blizzz mentioned this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants