Skip to content

fix: support continuation lines in scan_sudoers #122

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
richm wants to merge 1 commit into
base: main
Choose a base branch
from
+29 −5
Open

fix: support continuation lines in scan_sudoers #122

Show file tree
Hide file tree
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
7 changes: 7 additions & 0 deletions library/scan_sudoers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,13 @@ def get_config_lines(path, params):
sudoer_file["include_directories"] = includes["include_directories"]
except KeyError: # ignore keyerror
pass
# 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)
Comment on lines +225 to +231
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Copy link
Copy Markdown
Contributor Author

@richm richm May 22, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 22, 2026

Choose a reason for hiding this comment

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

@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.

# Work on each line of sudoers file
for sline in all_lines.strip().split("\n"):
line = sline.replace("\n", "").replace(
Expand Down
27 changes: 22 additions & 5 deletions tests/tests_scan_sudoers.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,20 @@
alias_names: "{{ alias_keys | zip(alias_keys) | flatten | list }}"
alias_vals: "{{ alias_values.values() | flatten | list }}"
names_vals: "{{ alias_names | zip(alias_vals) | list }}"
# This is the format that the role expects the aliases to be in
# mostly have to lowercase the keys to match the format of the role
aliases_in_role_input_format: "{{ dict(keys | zip(vals)) }}"
keys: "{{ alias_values | dict2items | map(attribute='key') | map('lower') | list }}"
vals: "{{ alias_values | dict2items | map(attribute='value') | list }}"
# This is what we expect to be the output of scan_sudoers after parsing
# the file with the continuation lines
__expected_scan_sudoers_parsed_continations:
all_scanned_files:
- /etc/sudoers
sudoers_files:
- aliases: "{{ aliases_in_role_input_format }}"
path: /etc/sudoers
user_specifications: []
tasks:
- name: Run tests
block:
Expand All @@ -49,7 +63,9 @@
{% for alias in names_vals %}
{% set itemvals = alias.1.values() | list %}
{% set space = ("NO_SPACES" in itemvals.0) | ternary("", " ") %}
{{ alias.0 }} {{ itemvals.0 }}{{ space }}={{ space }}{{ itemvals.1 | join("") }}
{{ alias.0 }} \
{{ itemvals.0 }}{{ space }}={{ space }}\
{{ itemvals.1 | join("") }}
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
{{ 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".

Copy link
Copy Markdown
Contributor Author

@richm richm May 22, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 22, 2026

Choose a reason for hiding this comment

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

@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?

{% endfor %}
mode: preserve

Expand All @@ -60,10 +76,11 @@
sudo_remove_unauthorized_included_files: true
sudo_sudoers_files:
- path: /etc/sudoers
aliases: "{{ aliases }}"
aliases: "{{ dict(keys | zip(vals)) }}"
keys: "{{ alias_values | dict2items | map(attribute='key') | map('lower') | list }}"
vals: "{{ alias_values | dict2items | map(attribute='value') | list }}"
aliases: "{{ aliases_in_role_input_format }}"

- name: Assert that scan_sudoers parsed the sudoers file correctly with continuation lines
assert:
that: __expected_scan_sudoers_parsed_continations == ansible_facts.sudoers
Comment on lines +81 to +83
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.changed

As 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.

Copy link
Copy Markdown
Contributor Author

@richm richm May 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 22, 2026

Choose a reason for hiding this comment

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

@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.


- name: Get sudoers
slurp:
Expand Down
Loading