Skip to content

fix(env_service): prevent arbitrary module loading via env_type validation #47

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
sebastiondev wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(env_service): prevent arbitrary module loading via env_type validation #47

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
25 changes: 25 additions & 0 deletions env_service/env_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import asyncio
from dataclasses import dataclass
import importlib
import re
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 15, 2026

Choose a reason for hiding this comment

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

high

The code on line 114 (not shown in this diff hunk) uses importlib.util.spec_from_file_location. However, importlib.util is not automatically imported when you import importlib. This will result in an AttributeError at runtime when attempting to register an environment. You should explicitly import importlib.util.

Suggested change
import re
import re
import importlib.util

import os
import sys
import time
Expand Down Expand Up @@ -58,6 +59,28 @@ def ensure_env(name: str, rel_path: str) -> None:
if PROJECT_ROOT not in sys.path:
sys.path.insert(0, PROJECT_ROOT)

ALLOWED_ENV_TYPES = frozenset(
d.name
for d in Path(
os.path.join(os.path.dirname(__file__), "environments"),
).iterdir()
if d.is_dir() and not d.name.startswith("_")
)
Comment on lines +62 to +68
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 15, 2026

Choose a reason for hiding this comment

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

medium

The initialization of ALLOWED_ENV_TYPES can be simplified by using pathlib.Path more idiomatically. Combining os.path.join and os.path.dirname inside a Path constructor is redundant when Path(__file__).parent provides the same functionality more cleanly.

ALLOWED_ENV_TYPES = frozenset(
    d.name
    for d in (Path(__file__).parent / "environments").iterdir()
    if d.is_dir() and not d.name.startswith("_")
)



def _validate_env_type(env_type: str) -> None:
"""Validate that env_type is a known, safe environment name."""
if not re.match(r"^[a-zA-Z0-9_]+$", env_type):
raise ValueError(
f"Invalid env_type: {env_type!r}. "
"Must contain only alphanumeric characters and underscores.",
)
if env_type not in ALLOWED_ENV_TYPES:
raise ValueError(
f"Unknown env_type: {env_type!r}. "
f"Available: {sorted(ALLOWED_ENV_TYPES)}",
)


def import_and_register_env(env_name, env_file=None):
"""
Expand All @@ -71,6 +94,7 @@ def import_and_register_env(env_name, env_file=None):
Returns:
The registered environment class or None on failure.
"""
_validate_env_type(env_name)
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 15, 2026

Choose a reason for hiding this comment

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

security-high high

While env_name is now validated, the env_file parameter in import_and_register_env is still used to construct a file path without validation. If an attacker can influence this parameter (e.g., through future API changes or other entry points), they could perform path traversal (e.g., by passing ../../some_module). It is recommended to apply the same character validation to env_file to ensure it remains within the intended environment directory.

    _validate_env_type(env_name)
    if env_file is not None and not re.match(r"^[a-zA-Z0-9_]+$", env_file):
        raise ValueError(
            f"Invalid env_file: {env_file!r}. "
            "Must contain only alphanumeric characters and underscores.",
        )

try:
if env_file is None:
env_file = f"{env_name}_env"
Expand Down Expand Up @@ -181,6 +205,7 @@ def get_remote_env_cls(self, env_type: str):
Returns:
The remote environment class.
"""
_validate_env_type(env_type)
if env_type in self.remote_env:
return self.remote_env[env_type]

Expand Down