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.

Sign up for GitHub

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
Merged

fix(logger): Prevent infinite recursion with log.condition => users or matches #50026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions build/integration/features/log-condition.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
# SPDX-License-Identifier: AGPL-3.0-or-later
Feature: log-condition

Background:
Given invoking occ with "config:system:set log.condition matches 0 users 0 --value admin"
Then the command was successful

Scenario: Accessing /status.php with log.condition
When requesting "/status.php" with "GET"
Then the HTTP status code should be "200"

Scenario: Accessing /index.php with log.condition
When requesting "/index.php" with "GET"
Then the HTTP status code should be "200"

Scenario: Accessing /remote.php/webdav with log.condition
When requesting "/remote.php/webdav" with "GET"
Then the HTTP status code should be "401"

Scenario: Accessing /remote.php/dav with log.condition
When requesting "/remote.php/dav" with "GET"
Then the HTTP status code should be "401"

Scenario: Accessing /ocs/v1.php with log.condition
When requesting "/ocs/v1.php" with "GET"
Then the HTTP status code should be "200"

Scenario: Accessing /ocs/v2.php with log.condition
When requesting "/ocs/v2.php" with "GET"
Then the HTTP status code should be "404"

Scenario: Accessing /public.php/webdav with log.condition
When requesting "/public.php/webdav" with "GET"
Then the HTTP status code should be "401"

Scenario: Accessing /public.php/dav with log.condition
When requesting "/public.php/dav" with "GET"
Then the HTTP status code should be "503"
Comment on lines +37 to +39
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:

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");

12 changes: 12 additions & 0 deletions lib/private/Log.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
class Log implements ILogger, IDataLogger {
private ?bool $logConditionSatisfied = null;
private ?IEventDispatcher $eventDispatcher = null;
private int $nestingLevel = 0;

public function __construct(
private IWriter $logger,
Expand Down Expand Up @@ -192,6 +193,11 @@ public function log(int $level, string $message, array $context = []): void {
}

public function getLogLevel(array $context, string $message): int {
if ($this->nestingLevel > 1) {
return ILogger::WARN;
}

$this->nestingLevel++;
/**
* @psalm-var array{
* shared_secret?: string,
Expand Down Expand Up @@ -242,6 +248,7 @@ public function getLogLevel(array $context, string $message): int {

// if log condition is satisfied change the required log level to DEBUG
if ($this->logConditionSatisfied) {
$this->nestingLevel--;
return ILogger::DEBUG;
}

Expand All @@ -256,6 +263,7 @@ public function getLogLevel(array $context, string $message): int {
* once this is met -> change the required log level to debug
*/
if (in_array($context['app'], $logCondition['apps'] ?? [], true)) {
$this->nestingLevel--;
return ILogger::DEBUG;
}
}
Expand All @@ -268,6 +276,7 @@ public function getLogLevel(array $context, string $message): int {

// Invalid configuration, warn the user and fall back to default level of WARN
error_log('Nextcloud configuration: "loglevel" is not a valid integer');
$this->nestingLevel--;
return ILogger::WARN;
}

Expand All @@ -281,12 +290,15 @@ public function getLogLevel(array $context, string $message): int {
if (!isset($option['apps']) && !isset($option['loglevel']) && !isset($option['message'])) {
/* Only user and/or secret are listed as conditions, we can cache the result for the rest of the request */
$this->logConditionSatisfied = true;
$this->nestingLevel--;
return ILogger::DEBUG;
}
$this->nestingLevel--;
return $option['loglevel'] ?? ILogger::DEBUG;
}
}

$this->nestingLevel--;
return ILogger::WARN;
}

Expand Down
Loading