Skip to content

Disallow logging.conf from elsewhere than the app folder #825

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

Open
Ickerday wants to merge 1 commit into
base: develop
Choose a base branch
from
+30 −0
Open

Disallow logging.conf from elsewhere than the app folder #825

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
6 changes: 6 additions & 0 deletions splunklib/searchcommands/environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ def configure_logging(logger_name, filename=None):
global _current_logging_configuration_file
filename = path.realpath(filename)

app_root_real = path.realpath(app_root)
if path.commonpath([filename, app_root_real]) != app_root_real: # pyright: ignore[reportUnknownArgumentType]

@mateusz834 mateusz834 Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If filename is relative here, what will happen? Does realpath always return the absolute path?

Docs does not seem to say anything about that:

def realpath(filename, *, strict=False):
    """Return the canonical path of the specified filename, eliminating any
symbolic links encountered in the path."""

It only follows symbolic links.

raise ValueError(
f'Logging configuration file "{filename}" is outside the app directory'
)

if filename != _current_logging_configuration_file:
working_directory = getcwd()
chdir(app_root)
Expand Down
24 changes: 24 additions & 0 deletions tests/unit/searchcommands/test_builtin_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,30 @@ def test_logging_configuration(self):
f"Expected ValueError, but logging_configuration={command.logging_configuration}"
)

# logging_configuration raises a value error for an existing absolute path outside the app directory (RCE guard)
try:
command.logging_configuration = os.path.join(package_directory, "__init__.py")

@mateusz834 mateusz834 Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TBH I don't understand what this even tests.

package_directory is the location of some file in the SDK.

Does that test even make sense, if run outside of splunk? Since that is what we do here.

except ValueError:
pass
except BaseException as e:
pytest.fail(f"Expected ValueError, but {type(e)} was raised")
else:
pytest.fail(
f"Expected ValueError, but logging_configuration={command.logging_configuration}"
)

# logging_configuration raises a value error when a relative path traverses outside the app directory (RCE guard)
try:
command.logging_configuration = os.path.join("..", "..", "..", "__init__.py")

@mateusz834 mateusz834 Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please add an additional test with an absolute path, one that points to an app dir (success), and the other one that points to a different place (failure).

except ValueError:
pass
except BaseException as e:
pytest.fail(f"Expected ValueError, but {type(e)} was raised")
else:
pytest.fail(
f"Expected ValueError, but logging_configuration={command.logging_configuration}"
)

def test_logging_level(self):
rebase_environment("app_without_logging_configuration")
command = StubbedSearchCommand()
Expand Down
Loading