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

Application.load_config_file loads all files with same basename as given file #850

Open
maxnoe opened this issue Jul 11, 2023 · 3 comments

Comments

@maxnoe
Copy link
Contributor

maxnoe commented Jul 11, 2023

Even if load_config_file is given an absolute path to a config file, it will load all files that have the same basename that are in that directory.

This is highly surprising and potentially a security issue.

Example:

from pathlib import Path
from traitlets import Integer
from traitlets.config import Application
from tempfile import TemporaryDirectory


class Foo(Application):
    bar = Integer(0).tag(config=True)

    def start(self):
        print(self.bar)


if __name__ == "__main__":
    with TemporaryDirectory() as tmpdir:
        tmpdir = Path(tmpdir)
        json_path = tmpdir / "foo.json"
        py_path = tmpdir / "foo.py"

        # valid python, invalid
        py_path.write_text("c.Foo.bar = 10")
        json_path.write_text("Invalid json")

        app = Foo()
        app.load_config_file(py_path)
        app.start()

        # other way around
        py_path.write_text("raise Exception('You loaded the python file!')")
        json_path.write_text('{"Foo": {"bar": 11}}')

        app = Foo()
        app.load_config_file(json_path)
        app.start()
@maxnoe
Copy link
Contributor Author

maxnoe commented Jul 11, 2023

Example without the errors, just showing that two files are loaded:

from pathlib import Path
from traitlets import Integer
from traitlets.config import Application
from tempfile import TemporaryDirectory


class Foo(Application):
    bar = Integer(0).tag(config=True)

    def start(self):
        print(self.bar)


if __name__ == "__main__":
    with TemporaryDirectory() as tmpdir:
        tmpdir = Path(tmpdir)
        json_path = tmpdir / "foo.json"
        py_path = tmpdir / "foo.py"

        # valid python, invalid
        py_path.write_text("c.Foo.bar = 10")
        json_path.write_text('{"Foo": {"bar": 11}}')

        app = Foo()
        app.load_config_file(py_path)
        print(app.loaded_config_files)
        app.start()

@StFroese
Copy link

The file extension returned by splittext is completely unused here

def load_config_file(self, filename, path=None):
"""Load config files by filename and path."""
filename, ext = os.path.splitext(filename)
new_config = Config()
for config, fname in self._load_config_files(
filename,
path=path,
log=self.log,
raise_config_file_errors=self.raise_config_file_errors,
):
new_config.merge(config)

Maybe add a keyword argument to the functionignore_ext=False and check

if ignore_ext:
    new_config.merge(config)
elif fname == filename:
    new_config.merge(config)

@maxnoe
Copy link
Contributor Author

maxnoe commented Jul 12, 2023

@StFroese that would still load the config though. Better just not remove the extension in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants