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(logger): Prevent infinite recursion with log.condition => users or matches #50026

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

nickvergessen
Copy link
Member

Summary

When we need to check the log condition for a user matches, there is a risk that something on the way checks the log level and would result in an infinite loop.
So we simply check if it's a nested call and use the default warning level in that case.

Checklist

When we need to check the log condition for a user matches,
there is a risk that something on the way checks the log level
and would result in an infinite loop.
So we simply check if it's a nested call and use the default
warning level in that case.

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen added bug 3. to review Waiting for reviews labels Jan 3, 2025
@nickvergessen nickvergessen added this to the Nextcloud 31 milestone Jan 3, 2025
@nickvergessen nickvergessen self-assigned this Jan 3, 2025
@nickvergessen nickvergessen requested review from artonge, skjnldsv and yemkareems and removed request for a team January 3, 2025 09:09
@nickvergessen
Copy link
Member Author

/backport to stable30

@nickvergessen
Copy link
Member Author

/backport to stable29

Comment on lines +37 to +39
Scenario: Accessing /public.php/dav with log.condition
When requesting "/public.php/dav" with "GET"
Then the HTTP status code should be "503"
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker:

Seems a bit odd to me to assert for a 503 status on a test that would cover not seeing a "fatal" error, what would the response code otherwise be?

Copy link
Member Author

Choose a reason for hiding this comment

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

Undefined array key 0 at \/home\/nickv\/Nextcloud\/31\/server\/apps\/dav\/appinfo\/v2\/publicremote.php#81

If I make /public.php/dav/files/foobar so the path matches the regex, it still 503's with "File does not exist" exception which is not handled/translated correctly:

"CustomMessage": "Exception thrown: Sabre\\DAV\\Exception\\NotFound"

But I didn't want to go much deeper (should have a 404 here instead, and I don't want to upload a real file for this simple test)

Copy link
Member Author

Choose a reason for hiding this comment

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

Translation happens here:

} catch (\Exception $e) {
$class = get_class($e);
$msg = $e->getMessage();
$this->logger->error($e->getMessage(), ['exception' => $e]);
throw new ServiceUnavailable("$class: $msg");

@nickvergessen nickvergessen merged commit 0f2b628 into master Jan 3, 2025
185 of 188 checks passed
@nickvergessen nickvergessen deleted the bugfix/noid/prevent-infitnite-loop branch January 3, 2025 11:11
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants