Skip to content

fix: preserve omitted auth config fields on partial updates #7755

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

Merged
wxg0103 merged 1 commit into from
Nov 9, 2026
Merged

fix: preserve omitted auth config fields on partial updates #7755

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
2 changes: 2 additions & 0 deletions apps/portal/serializers/portal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ def edit(self, instance, with_valid=True):
file_fields = ["logo", "id", "create_time", "update_time"]
for field, value in instance.items():
if hasattr(portal, field) and field not in file_fields:
if field == "auth_config" and isinstance(value, dict):
value = {**(portal.auth_config or {}), **value}
setattr(portal, field, value)
for field_name in ["logo"]:
if field_name in instance:

@shaohuzhang1 shaohuzhang1 Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here is an analysis of the provided code snippet with identified irregularities, potential issues, and optimization suggestions:

1. Security Risk: Insecure Direct Object Reference (IDOR) / Mass Assignment

  • Issue: The code iterates over instance.items() and applies setattr if hasattr(portal, field) is true. Even though file_fields excludes some keys, this pattern is generally dangerous. It allows an attacker or malformed input to modify any attribute on the portal object that isn't explicitly blocklisted. If auth_config contains sensitive keys (e.g., is_admin, permissions), merging user-provided dicts without sanitization could lead to privilege escalation or configuration tampering.
  • Suggestion: Maintain a whitelist of allowed fields instead of a blacklist. Only iterate over fields you explicitly intend to update. 
    ALLOWED_FIELDS = {"name", "description", "theme", "auth_config"} # example

for field, value in instance.items():
    if field not in ALLOWED_FIELDS:
        continue
    if hasattr(portal, field):
        # Your logic here
        setattr(portal, field, value)

2. Logic Error: Conditional Field Handling After General Loop

  • Issue: The special handling for "logo" appears to be in a separate loop (for field_name in ["logo"]) after the main setattr loop. However, "logo" is excluded from the main loop via file_fields. This is okay, but if there are other fields needing special processing later, the structure is fragile. More importantly, auth_config is handled inline, while logo is handled separately. This inconsistency makes maintenance harder.
  • Suggestion: Group all special-case handling together or handle all updates uniformly unless the field requires entirely different logic (e.g., file uploads vs. JSON config).

3. Deep Merge Issue: Shallow Dictionary Merge

  • Issue: The line {**(portal.auth_config or {}), **value} performs a shallow merge. If auth_config is a nested dictionary (common for auth configurations, e.g., {"oidc": {"client_id": "...", "secret": "..."}}), updating {"oidc": {"enabled": True}} will wipe out existing keys inside oidc (like client_id or secret).
  • Suggestion: Use a deep merge utility or deepmerge library if partial updates to nested configs are intended. 
    import deepmerge
...
if field == "auth_config" and isinstance(value, dict):
    current = portal.auth_config or {}
    portal.auth_config = deepmerge.always_merger.merge(current, value)

4. Type Safety & Validation

  • Issue: There’s no validation that value is acceptable before setting it. Especially for auth_config, ensure the final merged dict is valid before persisting. An invalid merge could break authentication.
  • Suggestion: Validate the resulting auth_config after merging.

5. Minor Code Style & Clarity

  • Suggestion: Extract the special-case logic into a helper method or a more explicit conditional structure for readability. 
    if field == "auth_config" and isinstance(value, dict):
    merged_config = {**(portal.auth_config or {}), **value}
    # Optional: validate merged_config here
    setattr(portal, field, merged_config)
else:
    setattr(portal, field, value)

✅ Recommended Refactored Code

def edit(self, instance, with_valid=True):
    # Whitelist of fields that can be directly set
    ALLOWED_DIRECT_FIELDS = {"name", "description", "title"}  # Add your actual fields here
    FILE_FIELDS = {"logo", "id", "create_time", "update_time"}
    
    for field, value in instance.items():
        if field in FILE_FIELDS:
            continue
                
        if field == "auth_config" and isinstance(value, dict):
            # Note shallow merge caveat - use deep merge if needed
            current_config = portal.auth_config or {}
            new_config = {**current_config, **value}
            
            # TODO: Add validation step here
            
            portal.auth_config = new_config
            
        elif field in ALLOWED_DIRECT_FIELDS:
            setattr(portal, field, value)
        else:
            # Log warning or ignore unexpected fields
            logger.warning(f"Ignoring unknown field in edit payload: {field}")

    # Handle special case for logo separately if needed
    if "logo" in instance:
        # Existing logo handling logic
        pass

Key Takeaways:

  1. Replace blacklisting with whitelisting.
  2. Consider deep merging for nested dictionaries like auth_config.
  3. Avoid using generic setattr loops without strict control over allowed fields.

Expand Down
Loading