Enable ruff security linting and fix findings#109
Conversation
📝 WalkthroughWalkthroughThis PR hardens the documentation processing pipeline by adding defusedxml and switching XML parsing to DefusedET, improves FileLock error logging, annotates asciidoctor/pandoc subprocess calls with ChangesSecurity Hardening and Linting Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 `@scripts/rhoso_adoc_docs_to_text.py`:
- Line 2082: Add a bounded timeout to the external conversion subprocesses so
they cannot hang indefinitely: update the subprocess.run(...) calls that invoke
asciidoctor_cmd (line with subprocess.run(asciidoctor_cmd, ...)) and the other
conversion runs (the subprocess.run calls at the locations using pandoc_cmd /
similar command variables) to include a timeout parameter (e.g., timeout=300)
and handle subprocess.TimeoutExpired where these runs occur (wrap in try/except
or propagate with clear logging) so batch processing remains resilient.
- Around line 1369-1370: The current broad except in the block that calls
self.lock_path.unlink() (which logs via LOG.debug(f"Could not remove lock file
{self.lock_path}: {e}")) should be narrowed to only catch OSError to avoid
hiding unrelated bugs; replace "except Exception as e" with "except OSError as
e" (keeping the same LOG.debug call and message) in the method that removes the
lock file so only OS-level unlink failures are handled.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 810098d1-e2d0-4bb2-8a21-ab6344b85d7e
📒 Files selected for processing (3)
.pre-commit-config.yamlrequirements.txtscripts/rhoso_adoc_docs_to_text.py
| except Exception as e: | ||
| LOG.debug(f"Could not remove lock file {self.lock_path}: {e}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify broad exception catches in this file and inspect nearby context.
rg -n -C2 'except Exception as e' scripts/rhoso_adoc_docs_to_text.pyRepository: openstack-lightspeed/rag-content
Length of output: 3731
Catch OSError (not Exception) when removing the lock file
self.lock_path.unlink() is only meant to “best-effort” clean up OS-level failures; catching Exception here can hide unrelated bugs.
Proposed fix
- except Exception as e:
+ except OSError as e:
LOG.debug(f"Could not remove lock file {self.lock_path}: {e}")📝 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.
| except Exception as e: | |
| LOG.debug(f"Could not remove lock file {self.lock_path}: {e}") | |
| except OSError as e: | |
| LOG.debug(f"Could not remove lock file {self.lock_path}: {e}") |
🧰 Tools
🪛 Ruff (0.15.14)
[warning] 1369-1369: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@scripts/rhoso_adoc_docs_to_text.py` around lines 1369 - 1370, The current
broad except in the block that calls self.lock_path.unlink() (which logs via
LOG.debug(f"Could not remove lock file {self.lock_path}: {e}")) should be
narrowed to only catch OSError to avoid hiding unrelated bugs; replace "except
Exception as e" with "except OSError as e" (keeping the same LOG.debug call and
message) in the method that removes the lock file so only OS-level unlink
failures are handled.
| str(input_for_conversion.absolute()), | ||
| ] | ||
| subprocess.run(asciidoctor_cmd, check=True, capture_output=True) | ||
| subprocess.run(asciidoctor_cmd, check=True, capture_output=True) # noqa: S603 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# AST-based verification: list subprocess.run calls missing a `timeout=` kwarg.
python - <<'PY'
import ast
from pathlib import Path
p = Path("scripts/rhoso_adoc_docs_to_text.py")
tree = ast.parse(p.read_text(encoding="utf-8"))
for node in ast.walk(tree):
if isinstance(node, ast.Call):
fn = node.func
is_subprocess_run = (
isinstance(fn, ast.Attribute)
and fn.attr == "run"
and isinstance(fn.value, ast.Name)
and fn.value.id == "subprocess"
)
if is_subprocess_run:
has_timeout = any(k.arg == "timeout" for k in node.keywords if k.arg)
if not has_timeout:
print(f"subprocess.run without timeout at line {node.lineno}")
PYRepository: openstack-lightspeed/rag-content
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("scripts/rhoso_adoc_docs_to_text.py")
lines = path.read_text(encoding="utf-8").splitlines()
targets = [2082, 2112, 2278, 2324]
for t in targets:
start = max(1, t-8)
end = min(len(lines), t+8)
print(f"\n--- {path}:{t} (show {start}-{end}) ---")
for i in range(start, end+1):
print(f"{i:5d}: {lines[i-1]}")
PYRepository: openstack-lightspeed/rag-content
Length of output: 4030
Add explicit timeouts to Asciidoctor/Pandoc conversion subprocesses
subprocess.run(...) calls at lines 2082, 2112, 2278, and 2324 run external conversions without any timeout=, so they can hang indefinitely. Add a timeout to bound failures and keep batch processing resilient.
Proposed fix
+SUBPROCESS_TIMEOUT_SECONDS = 600
...
- subprocess.run(asciidoctor_cmd, check=True, capture_output=True) # noqa: S603
+ subprocess.run(
+ asciidoctor_cmd,
+ check=True,
+ capture_output=True,
+ timeout=SUBPROCESS_TIMEOUT_SECONDS,
+ ) # noqa: S603
...
- subprocess.run(pandoc_cmd, check=True, capture_output=True) # noqa: S603
+ subprocess.run(
+ pandoc_cmd,
+ check=True,
+ capture_output=True,
+ timeout=SUBPROCESS_TIMEOUT_SECONDS,
+ ) # noqa: S603
...
result = subprocess.run( # noqa: S603
- asciidoctor_cmd, check=True, capture_output=True, text=True
+ asciidoctor_cmd,
+ check=True,
+ capture_output=True,
+ text=True,
+ timeout=SUBPROCESS_TIMEOUT_SECONDS,
)
...
subprocess.run( # noqa: S603
- pandoc_cmd, check=True, capture_output=True, text=True
+ pandoc_cmd,
+ check=True,
+ capture_output=True,
+ text=True,
+ timeout=SUBPROCESS_TIMEOUT_SECONDS,
)🤖 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 `@scripts/rhoso_adoc_docs_to_text.py` at line 2082, Add a bounded timeout to
the external conversion subprocesses so they cannot hang indefinitely: update
the subprocess.run(...) calls that invoke asciidoctor_cmd (line with
subprocess.run(asciidoctor_cmd, ...)) and the other conversion runs (the
subprocess.run calls at the locations using pandoc_cmd / similar command
variables) to include a timeout parameter (e.g., timeout=300) and handle
subprocess.TimeoutExpired where these runs occur (wrap in try/except or
propagate with clear logging) so batch processing remains resilient.
|
CI error is unrelated, this should fix it: #110 |
Upgrade ruff from v0.10.0 to v0.15.4 and enable Bandit security rules (--extend-select S). Resolved issues: - S314: Replace xml.etree.ElementTree.fromstring() with defusedxml to prevent XXE attacks - S603: Suppress false positives on subprocess.run() calls that use static command lists - S110: Log debug message on lock file removal failure instead of silently swallowing the exception
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/rhoso_adoc_docs_to_text.py (1)
1369-1370:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNarrow exception type to
OSError.The
unlink()call only raisesOSError(and subclasses likeFileNotFoundError). Catching the broaderExceptioncan mask unrelated bugs such asKeyboardInterruptor programming errors.Proposed fix
- except Exception as e: + except OSError as e: LOG.debug(f"Could not remove lock file {self.lock_path}: {e}")🤖 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 `@scripts/rhoso_adoc_docs_to_text.py` around lines 1369 - 1370, The except block that logs failures removing the lock file currently catches all Exceptions; change it to catch only OSError (or FileNotFoundError if you prefer more specificity) so unrelated exceptions aren’t swallowed. Locate the unlink() call that uses self.lock_path (the try/except surrounding unlink()) and replace "except Exception as e" with "except OSError as e" and keep the existing LOG.debug message referencing self.lock_path and the error.
🤖 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.
Duplicate comments:
In `@scripts/rhoso_adoc_docs_to_text.py`:
- Around line 1369-1370: The except block that logs failures removing the lock
file currently catches all Exceptions; change it to catch only OSError (or
FileNotFoundError if you prefer more specificity) so unrelated exceptions aren’t
swallowed. Locate the unlink() call that uses self.lock_path (the try/except
surrounding unlink()) and replace "except Exception as e" with "except OSError
as e" and keep the existing LOG.debug message referencing self.lock_path and the
error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6608b34-a520-4b86-a6e7-56226d54a588
📒 Files selected for processing (3)
.pre-commit-config.yamlrequirements.txtscripts/rhoso_adoc_docs_to_text.py
✅ Files skipped from review due to trivial changes (1)
- requirements.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- .pre-commit-config.yaml
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lpiwowar, umago The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm cancel |
|
/lgtm |
openshift-merge-bot
Bot
merged commit 135b955
into
openstack-lightspeed:main
2 participants
Upgrade ruff from
v0.10.0tov0.15.4and enable Bandit security rules (--extend-select S). Resolved issues:S314: Replacexml.etree.ElementTree.fromstring()with defusedxml to prevent XXE attacksS603: Suppress false positives onsubprocess.run()calls that use static command listsS110: Log debug message on lock file removal failure instead of silently swallowing the exceptionSummary by CodeRabbit
Bug Fixes
Chores