fix: support continuation lines in scan_sudoers#122
Conversation
📝 WalkthroughWalkthroughThis PR adds line continuation support to the sudoers scanner. The ChangesSudoers line continuation support
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
=======================================
Coverage ? 47.90%
=======================================
Files ? 2
Lines ? 382
Branches ? 0
=======================================
Hits ? 183
Misses ? 199
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
fixes #121 |
Cause: The scan_sudoers parser did not handle continuation lines in /etc/sudoers Consequence: The /etc/sudoers file was not parsed correctly, and the role would think it needed to change /etc/sudoers, and so was not idempotent. Fix: scan_sudoers is fixed to correctly process continuation lines. Result: The role is idempotent if the existing /etc/sudoers file uses continuation lines. Signed-off-by: Rich Megginson <rmeggins@redhat.com>
richm
force-pushed
the
fix-continuation-lines
branch
from
b61f705 to
60ef18c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@library/scan_sudoers.py`:
- Around line 225-231: get_includes currently parses the raw sudoers text before
continuation-line normalization, so any `#include/`#includedir split across lines
with a trailing backslash are missed; update get_includes to perform the same
continuation-folding used later (the re.sub r"(?<!\\)\\\n" replacement on
all_lines) before scanning for include/#includedir directives so folded lines
produce correct include tokens, preserving escaped backslashes as the existing
regex does.
In `@tests/tests_scan_sudoers.yml`:
- Around line 81-83: Extend the continuation-lines test that currently asserts
"__expected_scan_sudoers_parsed_continations == ansible_facts.sudoers" by adding
two checks: (1) a failure-path case that supplies a sudoers input with an
escaped trailing backslash (so the line must NOT be continued) and asserts the
parsed result differs from the happy-path expected value (use a distinct
expected variable, e.g., __expected_scan_sudoers_escaped_backslash, and assert
it != ansible_facts.sudoers or compare to that escaped expectation), and (2) an
idempotence check that runs the role a second time and asserts no changes (use
the existing play host result facts and add an assert that the second run’s
changed count is 0 or that ansible_facts.sudoers still equals the happy-path
expected). Target the same test blocks that use ansible_facts.sudoers and
__expected_scan_sudoers_parsed_continations so the new asserts are colocated
with the original continuation test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 100ede1b-d4a5-4fa4-b7ce-f62e2038cf37
📒 Files selected for processing (2)
library/scan_sudoers.pytests/tests_scan_sudoers.yml
| # process continuation lines - from the man page for sudoers: | ||
| # Long lines can be continued with a backslash (‘\’) as the last | ||
| # character on the line. | ||
| # replace backslash followed by a newline with an empty string, but only if the backslash | ||
| # is not escaped (preceded by a backslash) | ||
| # I guess someone might have a legitimate use case for ending a line with a backslash . . . | ||
| all_lines = re.sub(r"(?<!\\)\\\n", "", all_lines) |
There was a problem hiding this comment.
Fold continuation lines before include discovery.
At Line 215, get_includes(path) parses the raw file before the normalization added at Line 231. If #include/#includedir uses continuation syntax, those includes are missed and child sudoers files are not scanned.
Proposed fix
+def _fold_continuations(text):
+ # Fold unescaped backslash-newline continuations
+ return re.sub(r"(?<!\\)\\\n", "", text)
+
def get_includes(path):
# Get includes
fp = open(path, "r")
- sudoers_file = fp.read()
+ sudoers_file = _fold_continuations(fp.read())
fp.close()
@@
- all_lines = re.sub(r"(?<!\\)\\\n", "", all_lines)
+ all_lines = _fold_continuations(all_lines)🧰 Tools
🪛 Ruff (0.15.13)
[warning] 226-226: Comment contains ambiguous ‘ (LEFT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF003)
[warning] 226-226: Comment contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@library/scan_sudoers.py` around lines 225 - 231, get_includes currently
parses the raw sudoers text before continuation-line normalization, so any
`#include/`#includedir split across lines with a trailing backslash are missed;
update get_includes to perform the same continuation-folding used later (the
re.sub r"(?<!\\)\\\n" replacement on all_lines) before scanning for
include/#includedir directives so folded lines produce correct include tokens,
preserving escaped backslashes as the existing regex does.
There was a problem hiding this comment.
yeah - this parser is not ideal - I want to replace it all with cvtsudoers, but mapping the output of cvtsudoers to the format expected by the role input is pretty tricky - I really don't want to spend any more time fixing the existing parsing code than is necessary - I think
- support for continuation lines is necessary
- it is highly unlikely that an include directive will use a continuation line, therefore, unnecessary
There was a problem hiding this comment.
@richm, understood — the parser is a known limitation and the continuation-line fix you've added is sufficient for the current goal of restoring idempotence. The get_includes edge case is an acceptable deferral given how uncommon a continuation-split #include directive would be in practice.
Would you like me to open a follow-up GitHub issue to track the planned migration from the current parser to cvtsudoers?
✏️ Learnings added
Learnt from: richm
Repo: linux-system-roles/sudo PR: 122
File: library/scan_sudoers.py:225-231
Timestamp: 2026-05-22T18:12:36.843Z
Learning: In the `linux-system-roles/sudo` repository, the `library/scan_sudoers.py` module is a known imperfect parser. The maintainer (richm) plans to eventually replace it entirely with `cvtsudoers`, but mapping `cvtsudoers` output to the role's expected input format is non-trivial. Incremental fixes to the existing parser should be kept minimal — only what is strictly necessary for correctness. For example, fixing continuation-line handling in `get_includes()` for `#include`/`#includedir` directives was explicitly deemed unnecessary because such usage is highly unlikely in practice.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| - name: Assert that scan_sudoers parsed the sudoers file correctly with continuation lines | ||
| assert: | ||
| that: __expected_scan_sudoers_parsed_continations == ansible_facts.sudoers |
There was a problem hiding this comment.
Add failure-path and idempotence checks to this continuation test.
This assertion only validates the happy path once. Add (1) a failure scenario (e.g., escaped trailing backslash that should not continue) and (2) a second role run with an idempotence assertion.
Minimal extension sketch
+ - name: Run the role a second time (idempotence check)
+ include_tasks: tasks/run_role_with_clear_facts.yml
+ vars:
+ sudo_rewrite_default_sudoers_file: true
+ sudo_remove_unauthorized_included_files: true
+ sudo_sudoers_files:
+ - path: /etc/sudoers
+ aliases: "{{ aliases_in_role_input_format }}"
+ register: __second_run
+
+ - name: Assert idempotence on second run
+ assert:
+ that:
+ - not __second_run.changedAs per coding guidelines, tests in tests/tests_*.yml should verify both success and failure scenarios and be idempotent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tests_scan_sudoers.yml` around lines 81 - 83, Extend the
continuation-lines test that currently asserts
"__expected_scan_sudoers_parsed_continations == ansible_facts.sudoers" by adding
two checks: (1) a failure-path case that supplies a sudoers input with an
escaped trailing backslash (so the line must NOT be continued) and asserts the
parsed result differs from the happy-path expected value (use a distinct
expected variable, e.g., __expected_scan_sudoers_escaped_backslash, and assert
it != ansible_facts.sudoers or compare to that escaped expectation), and (2) an
idempotence check that runs the role a second time and asserts no changes (use
the existing play host result facts and add an assert that the second run’s
changed count is 0 or that ansible_facts.sudoers still equals the happy-path
expected). Target the same test blocks that use ansible_facts.sudoers and
__expected_scan_sudoers_parsed_continations so the new asserts are colocated
with the original continuation test.
There was a problem hiding this comment.
yeah - this parser is not ideal - I want to replace it all with cvtsudoers, but mapping the output of cvtsudoers to the format expected by the role input is pretty tricky - I really don't want to spend any more time fixing the existing parsing code than is necessary
There was a problem hiding this comment.
@richm, that's a completely reasonable call — the fix addresses the immediate idempotence bug without over-investing in a parser that's slated for replacement. I'll drop the suggestion.
If you'd like, I can open a follow-up issue to track the cvtsudoers-based migration so it isn't lost. Just let me know!
✏️ Learnings added
Learnt from: richm
Repo: linux-system-roles/sudo PR: 122
File: tests/tests_scan_sudoers.yml:81-83
Timestamp: 2026-05-22T18:12:48.559Z
Learning: In the `linux-system-roles/sudo` role, the `scan_sudoers` parser (library/scan_sudoers.py) is intentionally minimal and the maintainer (richm) plans to replace it entirely with `cvtsudoers`. Do not suggest expanding or improving the existing parser beyond critical bug fixes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
[citest] |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
library/scan_sudoers.py (1)
225-231:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply continuation folding to
get_includes()as well.The
get_includes()function at line 126 reads and parses the sudoers file before continuation folding is applied. If#includeor#includedirdirectives use continuation syntax, they won't be detected and child sudoers files will be missed.This issue was already flagged in a previous review.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@library/scan_sudoers.py` around lines 225 - 231, get_includes() currently parses the raw sudoers text before folding continuations, so `#include/`#includedir lines that use a backslash newline are missed; apply the same continuation-folding regex used for all_lines (re.sub(r"(?<!\\)\\\n", "", ...)) to the file contents inside get_includes() immediately after reading the file and before searching for include directives, so continuation-joined lines are detected while preserving escaped backslashes.tests/tests_scan_sudoers.yml (1)
81-83: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAdd failure scenarios and idempotence checks.
This test only validates the happy path. Per coding guidelines, tests must verify both success and failure scenarios and must be idempotent.
Missing test coverage:
- Failure scenario: Test that escaped backslashes (e.g.,
\\at end of line) do NOT trigger continuation- Idempotence: Run the role a second time and assert no changes
As per coding guidelines, tests in
tests/tests_*.ymlshould verify both success and failure scenarios and be idempotent - running twice should not cause failures.This issue was already flagged in a previous review.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tests_scan_sudoers.yml` around lines 81 - 83, The test only covers the happy path; add a failure-case assertion and an idempotence check: extend the playbook/tasks around the existing task that asserts __expected_scan_sudoers_parsed_continations == ansible_facts.sudoers by adding a new expected fixture (or inline expected data) that includes a line ending with an escaped backslash (\\) and assert that this escaped backslash does NOT produce a continuation entry in ansible_facts.sudoers (use a negative assert comparing the failure-expected structure), and then add a second run of the role (or a handler to re-run the relevant task) and assert no changes occurred on the second run (e.g., assert result.changed is false or assert that ansible_run_changes == 0) so the test is idempotent; reference the existing variable __expected_scan_sudoers_parsed_continations and ansible_facts.sudoers to locate where to add these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/tests_scan_sudoers.yml`:
- Line 68: Replace the incorrect empty-separator join in the template so alias
value lists are comma-separated; change the expression using itemvals.1
(currently "{{ itemvals.1 | join("") }}") to join with ", " (e.g., "{{
itemvals.1 | join(', ') }}") so multi-item lists like ['/bin/ls','/bin/cat']
render as "/bin/ls, /bin/cat" instead of being concatenated into
"/bin/ls/bin/cat".
---
Duplicate comments:
In `@library/scan_sudoers.py`:
- Around line 225-231: get_includes() currently parses the raw sudoers text
before folding continuations, so `#include/`#includedir lines that use a backslash
newline are missed; apply the same continuation-folding regex used for all_lines
(re.sub(r"(?<!\\)\\\n", "", ...)) to the file contents inside get_includes()
immediately after reading the file and before searching for include directives,
so continuation-joined lines are detected while preserving escaped backslashes.
In `@tests/tests_scan_sudoers.yml`:
- Around line 81-83: The test only covers the happy path; add a failure-case
assertion and an idempotence check: extend the playbook/tasks around the
existing task that asserts __expected_scan_sudoers_parsed_continations ==
ansible_facts.sudoers by adding a new expected fixture (or inline expected data)
that includes a line ending with an escaped backslash (\\) and assert that this
escaped backslash does NOT produce a continuation entry in ansible_facts.sudoers
(use a negative assert comparing the failure-expected structure), and then add a
second run of the role (or a handler to re-run the relevant task) and assert no
changes occurred on the second run (e.g., assert result.changed is false or
assert that ansible_run_changes == 0) so the test is idempotent; reference the
existing variable __expected_scan_sudoers_parsed_continations and
ansible_facts.sudoers to locate where to add these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 017b7e3d-e3af-443e-89cb-09c66847c3fc
📒 Files selected for processing (2)
library/scan_sudoers.pytests/tests_scan_sudoers.yml
| {{ alias.0 }} {{ itemvals.0 }}{{ space }}={{ space }}{{ itemvals.1 | join("") }} | ||
| {{ alias.0 }} \ | ||
| {{ itemvals.0 }}{{ space }}={{ space }}\ | ||
| {{ itemvals.1 | join("") }} |
There was a problem hiding this comment.
Fix incorrect list concatenation in alias values.
Line 68 uses join("") which concatenates list items with no separator. For multi-item lists like ['/bin/ls', '/bin/cat'], this produces invalid sudoers syntax /bin/ls/bin/cat instead of the required /bin/ls, /bin/cat.
The test currently passes only because all test data uses single-item lists, hiding this bug.
🔧 Proposed fix
- {{ itemvals.1 | join("") }}
+ {{ itemvals.1 | join(", ") }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{ itemvals.1 | join("") }} | |
| {{ itemvals.1 | join(", ") }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tests_scan_sudoers.yml` at line 68, Replace the incorrect
empty-separator join in the template so alias value lists are comma-separated;
change the expression using itemvals.1 (currently "{{ itemvals.1 | join("") }}")
to join with ", " (e.g., "{{ itemvals.1 | join(', ') }}") so multi-item lists
like ['/bin/ls','/bin/cat'] render as "/bin/ls, /bin/cat" instead of being
concatenated into "/bin/ls/bin/cat".
There was a problem hiding this comment.
this is true, but in the case of the test, there is only one value, so the join("") just converts the list with one element to a string.
There was a problem hiding this comment.
@richm, understood — since the test data has single-item lists, join("") is effectively equivalent to join(", ") here and the output is correct. My concern was about future-proofing in case multi-item lists are added, but it doesn't affect the current test.
Would you like me to open a follow-up issue to track making the join separator explicit (, ) as a defensive improvement for when test data is extended?
2 participants
Cause: The scan_sudoers parser did not handle continuation lines in /etc/sudoers
Consequence: The /etc/sudoers file was not parsed correctly, and the role would
think it needed to change /etc/sudoers, and so was not idempotent.
Fix: scan_sudoers is fixed to correctly process continuation lines.
Result: The role is idempotent if the existing /etc/sudoers file uses
continuation lines.
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by CodeRabbit
Improvements
Tests