-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
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>
/backport to stable30 |
/backport to stable29 |
Scenario: Accessing /public.php/dav with log.condition | ||
When requesting "/public.php/dav" with "GET" | ||
Then the HTTP status code should be "503" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translation happens here:
server/apps/dav/lib/Connector/Sabre/PublicAuth.php
Lines 80 to 84 in 3810770
} catch (\Exception $e) { | |
$class = get_class($e); | |
$msg = $e->getMessage(); | |
$this->logger->error($e->getMessage(), ['exception' => $e]); | |
throw new ServiceUnavailable("$class: $msg"); |
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