Skip to content

Fix file.managed omitting mode from changes when creating a file - #70084

Open
HarnageaGabriel wants to merge 1 commit into
saltstack:masterfrom
HarnageaGabriel:fix/69969-check-file-meta-newfile-mode
Open

Fix file.managed omitting mode from changes when creating a file#70084
HarnageaGabriel wants to merge 1 commit into
saltstack:masterfrom
HarnageaGabriel:fix/69969-check-file-meta-newfile-mode

Conversation

@HarnageaGabriel

Copy link
Copy Markdown

Summary

  • check_file_meta() in salt/modules/file.py has two early-return points that fire when the target file does not yet exist (lstats falsy). Both skipped the entire mode/user/group comparison block, so file.managed only ever reported {'newfile': name} (test=True) or a diff (real run) when creating a file — never the mode being applied. When the file already existed, mode/user/group changes were reported correctly.
  • This makes it impossible to audit, from state output alone, what permissions a newly-created file will get (e.g. mode: '0600' vs an unspecified umask-derived mode produce byte-identical changes).
  • Fix: on the new-file paths, report the requested mode/user/group directly (there's nothing to diff against), guarded the same way as the existing-file case (skipped on Windows). Both early-return points are updated so behavior is consistent regardless of new_file_diff.

Fixes #69969

Test plan

  • Added unit tests in tests/pytests/unit/modules/file/test_file_check.py covering a nonexistent target path with mode/user/group specified, for both new_file_diff=True/False, and for the ignore_ordering/tuple-return path.
  • Verified existing-file behavior is unchanged (no regression).
  • Added changelog/69969.fixed.md.

Note: ran pytest tests/pytests/unit/modules/file/test_file_check.py on Windows; the OS-gated tests (skip_on_windows) skip there since mode/user/group aren't applicable on Windows — the fix's logic was additionally verified directly against check_file_meta() with salt.utils.platform.is_windows() patched to False, confirming the new/existing-file paths behave as expected.

🤖 Generated with Claude Code

@HarnageaGabriel
HarnageaGabriel requested a review from a team as a code owner August 18, 2026 19:56
@welcome

welcome Bot commented Aug 18, 2026

Copy link
Copy Markdown

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here's some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject.pdl@broadcom.com. We're glad you've joined our community and look forward to doing awesome things with you!

check_file_meta() had two early-return points that fired when a
target file does not yet exist (lstats falsy). Both skipped the
mode/user/group comparison block entirely, so file.managed only
ever reported {'newfile': name} or a diff on creation — never the
mode (or user/group) being applied. Existing-file mode changes were
already reported correctly.

Report the requested mode/user/group directly on the new-file paths
(guarded the same way as the existing-file case, skipped on
Windows), since there is no prior state to diff against.

Fixes saltstack#69969
@HarnageaGabriel
HarnageaGabriel force-pushed the fix/69969-check-file-meta-newfile-mode branch from cf40ea9 to b860413 Compare August 19, 2026 19:33

@twangboy twangboy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's make these changes on the 3008.x branch, please.

Comment thread salt/modules/file.py
if group is not None:
changes["group"] = group
if mode is not None:
changes["mode"] = mode

@twangboy twangboy Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1. Unhandled Defaults / Inherited Permissions

Relying strictly on if variable is not None: only captures parameters explicitly passed to check_file_meta. If a user relies on Salt defaults or inherited OS permissions (leaving user, group, or mode as None), these details won't be recorded in changes during file creation. Consider resolving default values or falling back to stats data when these variables are None.

2. Code Duplication

This exact non-Windows metadata check is duplicated verbatim at lines 6322–6328. Refactoring this into a local helper function will keep the logic DRY and consistent across both branches.

Suggested Refactor

def _add_file_meta_changes(changes_dict, stats_data=None):
    if salt.utils.platform.is_windows():
        return

    # Fall back to stats/defaults if parameters were omitted
    target_user = user or (stats_data.get("user") if stats_data else None)
    target_group = group or (stats_data.get("group") if stats_data else None)
    target_mode = mode or (stats_data.get("mode") if stats_data else None)

    for key, val in [("user", target_user), ("group", target_group), ("mode", target_mode)]:
        if val is not None:
            changes_dict[key] = val

You can then simplify both conditional blocks in check_file_meta:

# Line 6216
if not lstats and not new_file_diff:
    changes["newfile"] = name
    _add_file_meta_changes(changes, lstats)
# Line 6321
if not lstats:
    _add_file_meta_changes(changes, lstats)

You'll also need to add some tests for when the default values are used to make sure the user/group/mode values are reported as changes.

@twangboy twangboy added the test:full Run the full test suite label Aug 24, 2026
@twangboy twangboy added this to the Argon v3008.3 milestone Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

file.managed omits mode from changes when creating a file (test=True and real runs), making created-file permissions unauditable

2 participants