Skip to content

Conversation

@sharoneyal
Copy link
Collaborator

@sharoneyal sharoneyal commented Oct 29, 2025

User description

  • Allow only toml files (and for global settings, also read and merge from env.)
  • Override merging behavior in case of non scalar values
  • Improve security of what is allowed in toml files

PR Type

Enhancement, Bug fix


Description

  • Implement custom TOML loader for enhanced security and controlled merging

  • Disable default Dynaconf loaders to prevent duplicate file loading

  • Add comprehensive security validation to block dangerous directives

  • Enforce .toml file extension and file size limits (100MB max)

  • Replace list/dict append behavior with overwrite for consistent merging

  • Apply custom loader consistently across configuration loading points


Diagram Walkthrough

flowchart LR
  A["Dynaconf Configuration"] -->|"uses custom loader"| B["custom_merge_loader.py"]
  B -->|"validates security"| C["validate_file_security"]
  B -->|"loads & merges"| D["TOML Files"]
  C -->|"blocks forbidden keys"| E["includes/preload/loaders"]
  D -->|"enforces constraints"| F["File size & extension checks"]
  B -->|"replaces values"| G["Accumulated Data"]
  G -->|"updates settings"| H["Dynaconf Object"]
Loading

File Walkthrough

Relevant files
Enhancement
custom_merge_loader.py
New secure custom TOML loader with validation                       

pr_agent/custom_merge_loader.py

  • New custom TOML loader module with security-focused design
  • Implements controlled merging that replaces (not appends) list/dict
    values
  • Validates file security by blocking forbidden directives (includes,
    preload, loaders, etc.)
  • Enforces constraints: .toml file extension only, max 100MB file size,
    max 50 nesting depth
  • Handles multiple settings files with proper error handling and logging
+167/-0 
Configuration changes
config_loader.py
Configure custom loader for global settings                           

pr_agent/config_loader.py

  • Disable default Dynaconf loaders to prevent duplicate file loading
  • Configure custom merge loader and environment variable loader
  • Set root_path to settings folder for Dynaconf.find_file()
    functionality
  • Add load_dotenv=False security setting to prevent .env file loading
  • Apply dynconf_kwargs configuration to global_settings initialization
+9/-2     
utils.py
Apply custom loader to repo settings                                         

pr_agent/git_providers/utils.py

  • Apply custom merge loader configuration to repository-specific
    settings
  • Disable default loaders and use only custom_merge_loader
  • Add envvar_prefix=False to prevent environment variable interference
  • Replace merge_enabled=False with merge_enabled=True for proper section
    merging
  • Maintain load_dotenv=False security setting
+8/-1     
Error handling
pr_config.py
Add error handling and custom loader to config tool           

pr_agent/tools/pr_config.py

  • Wrap Dynaconf initialization in try-except for error handling
  • Apply custom merge loader configuration to configuration.toml loading
  • Add security settings: load_dotenv=False and envvar_prefix=False
  • Return empty dict on exception instead of failing
  • Improve robustness when loading configuration files
+18/-2   

@qodo-free-for-open-source-projects
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Loader override risk

Description: Blocking the 'loaders' key in TOML while also relying on Dynaconf's 'loaders'
configuration at initialization may break user configs silently; validate this is intended
and documented to avoid configuration injection via user-controlled files in other
contexts.
custom_merge_loader.py [146-161]

Referred Code
# Check at the top level and in all sections
def check_dict(data, path="", max_depth=MAX_DEPTH):
    if max_depth <= 0:
        raise SecurityError(
            f"Maximum nesting depth exceeded at {path}. "
            f"Possible attempt to cause stack overflow."
        )

    for key, value in data.items():
        full_path = f"{path}.{key}" if path else key

        if key.lower() in forbidden_keys_to_reasons:
            raise SecurityError(
                f"Security error in {filename}: "
                f"Forbidden directive '{key}' found at {full_path}. Reason: {forbidden_keys_to_reasons[key.lower()]}"
            )
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Consistent Naming Conventions

Objective: All new variables, functions, and classes must follow the project's established naming
standards

Status: Passed

No Dead or Commented-Out Code

Objective: Keep the codebase clean by ensuring all submitted code is active and necessary

Status: Passed

When relevant, utilize early return

Objective: In a code snippet containing multiple logic conditions (such as 'if-else'), prefer an
early return on edge cases than deep nesting

Status: Passed

Robust Error Handling

Objective: Ensure potential errors and edge cases are anticipated and handled gracefully throughout
the code

Status:
Silent Exceptions: The loader swallows exceptions when 'silent' is True and only logs them, which
may hide failures during config loading without clear propagation or fallback.

Referred Code
except Exception as e:
    if not silent:
        raise e
    get_logger().exception(f"Exception loading settings file: {settings_file}. Skipping.")
Single Responsibility for Functions

Objective: Each function should have a single, well-defined responsibility

Status:
Multiple Concerns: The 'load' function performs file discovery, security validation, parsing,
accumulation, and settings mutation, potentially combining multiple responsibilities.

Referred Code
def load(obj, env=None, silent=True, key=None, filename=None):
    """
    Load and merge TOML configuration files into a Dynaconf settings object using a secure, in-house loader.
    This loader:
    - Replaces list and dict fields instead of appending/updating (non-default Dynaconf behavior).
    - Enforces several security checks (e.g., disallows includes/preloads and enforces .toml files).
    - Supports optional single-key loading.
    Args:
        obj: The Dynaconf settings instance to update.
        env: The current environment name (upper case). Defaults to 'DEVELOPMENT'. Note: currently unused.
        silent (bool): If True, suppress exceptions and log warnings/errors instead.
        key (str | None): Load only this top-level key (section) if provided; otherwise, load all keys from the files.
        filename (str | None): Custom filename for tests (not used when settings_files are provided).
    Returns:
        None
    """

    MAX_TOML_SIZE_IN_BYTES = 100 * 1024 * 1024 # Prevent out of mem. exceptions by limiting to 100 MBs which is sufficient for upto 1M lines

    # Get the list of files to load
    # TODO: hasattr(obj, 'settings_files') for some reason returns False. Need to use 'settings_file'


 ... (clipped 70 lines)
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-free-for-open-source-projects
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Centralize the duplicated Dynaconf configuration

The dynconf_kwargs dictionary is defined in three separate files with slight
differences. This should be refactored into a single, centralized helper
function to avoid code duplication and improve maintainability.

Examples:

pr_agent/config_loader.py [12-16]
dynconf_kwargs = {'core_loaders': [], # DISABLE default loaders, otherwise will load toml files more than once.
                           'loaders': ['pr_agent.custom_merge_loader', 'dynaconf.loaders.env_loader'], # Use a custom loader to merge sections, but overwrite their overlapping values. Also support ENV variables to take precedence.
                           'root_path': join(current_dir, "settings"), #Used for Dynaconf.find_file() - So that root path points to settings folder, since we disabled all core loaders.
                           'merge_enabled': True  # In case more than one file is sent, merge them. Must be set to True, otherwise, a .toml file with section [XYZ] overwrites the entire section of a previous .toml file's [XYZ] and we want it to only overwrite the overlapping fields under such section
                           }
pr_agent/git_providers/utils.py [41-45]
                        dynconf_kwargs = {'core_loaders': [],  # DISABLE default loaders, otherwise will load toml files more than once.
                             'loaders': ['pr_agent.custom_merge_loader'],
                             # Use a custom loader to merge sections, but overwrite their overlapping values. Don't involve ENV variables.
                             'merge_enabled': True  # Merge multiple files; ensures [XYZ] sections only overwrite overlapping keys, not whole sections.
                         }

Solution Walkthrough:

Before:

# In pr_agent/config_loader.py
dynconf_kwargs = {
    'core_loaders': [],
    'loaders': ['pr_agent.custom_merge_loader', 'dynaconf.loaders.env_loader'],
    'root_path': '...',
    'merge_enabled': True
}
global_settings = Dynaconf(..., **dynconf_kwargs)

# In pr_agent/git_providers/utils.py
dynconf_kwargs = {
    'core_loaders': [],
    'loaders': ['pr_agent.custom_merge_loader'],
    'merge_enabled': True
}
new_settings = Dynaconf(..., **dynconf_kwargs)
# ... similar duplication in pr_agent/tools/pr_config.py

After:

# In a central location, e.g., pr_agent/config_loader.py
def get_dynaconf_kwargs(use_env_loader=False, root_path=None):
    kwargs = {
        'core_loaders': [],
        'loaders': ['pr_agent.custom_merge_loader'],
        'merge_enabled': True
    }
    if use_env_loader:
        kwargs['loaders'].append('dynaconf.loaders.env_loader')
    if root_path:
        kwargs['root_path'] = root_path
    return kwargs

# In pr_agent/config_loader.py
dynconf_kwargs = get_dynaconf_kwargs(use_env_loader=True, root_path=...)
global_settings = Dynaconf(..., **dynconf_kwargs)

# In pr_agent/git_providers/utils.py
dynconf_kwargs = get_dynaconf_kwargs()
new_settings = Dynaconf(..., **dynconf_kwargs)
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies significant code duplication of the dynconf_kwargs dictionary across three files, and centralizing this logic would improve maintainability and reduce future inconsistencies.

Medium
Learned
best practice
Safer exception handling logs

Avoid logging raw exception details which may include sensitive paths; log a
generic message and chain the original exception when re-raising.

pr_agent/custom_merge_loader.py [89-92]

 except Exception as e:
     if not silent:
-        raise e
-    get_logger().exception(f"Exception loading settings file: {settings_file}. Skipping.")
+        raise RuntimeError(f"Failed loading settings file: {settings_file}") from e
+    get_logger().error("Failed loading settings file. Skipping.", artifact={"file": str(settings_file)})
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Improve error handling to avoid leaking secrets and provide actionable messages.

Low
General
Simplify attribute access with getattr

Refactor the attribute access for settings_files to use chained getattr calls,
which simplifies the code and improves readability.

pr_agent/custom_merge_loader.py [28-30]

-# TODO: hasattr(obj, 'settings_files') for some reason returns False. Need to use 'settings_file'
-settings_files = obj.settings_files if hasattr(obj, 'settings_files') else (
-    obj.settings_file) if hasattr(obj, 'settings_file') else []
+settings_files = getattr(obj, 'settings_files', getattr(obj, 'settings_file', []))
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion improves code readability by using a more concise and Pythonic getattr pattern, but it's a minor style improvement as the original code is functionally correct.

Low
  • More
  • Author self-review: I have reviewed the PR code suggestions, and addressed the relevant ones.

@sharoneyal sharoneyal merged commit d141e4f into main Oct 30, 2025
2 checks passed
@sharoneyal sharoneyal deleted the es/use_custom_toml_handler_for_dynaconf branch October 30, 2025 09:56
@bar-qodo
Copy link

bar-qodo commented Nov 6, 2025

/agentic_review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants