feat: add permission auditor tool #515
Conversation
Complete implementation for bounty cortexlinux#446: - Scans for dangerous permissions (777, world-writable) - Explains security risks in plain English - Suggests context-aware permission fixes - Safe operation with dry-run mode by default - Docker/container UID mapping support The tool is placed in tools/permission-auditor/ for easy testing. All source code, tests, and documentation included. Closes cortexlinux#446
CLA Verification FailedThe following contributors have not signed the Contributor License Agreement:
How to Sign
This check runs automatically. Maintainers can update |
📝 WalkthroughWalkthroughAdds a new Linux Permission Auditor tool with CLI and Docker support that scans filesystems and containers for dangerous permissions, explains risks, suggests safe permissions, generates non-destructive fix commands (dry-run/backups), and includes installation, Docker examples, and extensive tests and helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Handler
participant FS as FileSystem Scanner
participant Detector as Permission Detector
participant Explainer as Explainer/Suggester
participant Fixer as Fix Engine
participant Docker as Docker Scanner
participant Reporter as Reporter
User->>CLI: run perm-audit (path, flags)
CLI->>FS: scan_directory(path, recursive, max_depth)
activate FS
FS->>Detector: check_file_permissions(file)
Detector-->>FS: finding metadata (path, perms, uid/gid, severity)
FS-->>CLI: findings[]
deactivate FS
alt Docker scanning enabled
CLI->>Docker: scan_docker_containers()
activate Docker
Docker-->>CLI: docker_findings[]
deactivate Docker
end
loop per finding
CLI->>Explainer: explain_issue(finding)
Explainer-->>CLI: plain-English explanation
CLI->>Explainer: suggest_safe_permissions(finding)
Explainer-->>CLI: recommended chmod/chown and rationale
end
alt --fix / --interactive
CLI->>Fixer: apply_single_fix / apply_bulk_fixes (dry_run/backups)
activate Fixer
Fixer-->>CLI: preview or result (status, command, applied)
deactivate Fixer
end
CLI->>Reporter: generate_text_report / generate_json_report
Reporter-->>User: formatted report + suggested commands
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
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: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Fix all issues with AI Agents 🤖
In @tools/permission-auditor/demos/showcase.py:
- Around line 84-91: The bare except in the JSON parsing block swallows all
exceptions; change it to catch json.JSONDecodeError (e.g., "except
json.JSONDecodeError as e") and print the error message and the offending output
(referencing result.stdout and parsed) so parsing failures are visible; if you
also want to surface unexpected issues, add a separate "except Exception as e"
that logs the exception and re-raises KeyboardInterrupt/SystemExit if detected,
rather than using a bare except.
In @tools/permission-auditor/examples/advanced.Dockerfile:
- Around line 69-73: The Dockerfile creates a real setuid-root binary
(/app/scripts/suid_test) by compiling /app/test_suid.c and setting chmod 4755,
which is a real privilege escalation risk; replace this with a safe test
artifact: do not compile or install a binary that spawns a shell—either create a
non-executable placeholder file or a harmless dummy executable (e.g., touch
/app/scripts/suid_test and set benign content) or simply touch a file and set
the mode to mimic detection (chmod 4755 on a non-functional placeholder) so the
permission-auditor can detect the pattern without introducing an exploitable
program.
- Around line 32-35: The Dockerfile copies Python packages into /root/.local and
sets PATH/PYTHONPATH to /root/.local, which will be inaccessible to the non-root
runtime user appuser; change the build step to install packages into a shared
location (e.g., /opt/.local or system site-packages) or chown the copied
/root/.local to appuser and update PATH/PYTHONPATH to that location so appuser
can run them; specifically adjust the COPY/ENV lines that reference /root/.local
and ensure the build used pip install --user is replaced or mirrored to a
directory owned by appuser (or use pip install --target /opt/.local) and update
PATH and PYTHONPATH accordingly.
- Around line 81-84: The COPY and RUN that install and chmod
docker-entrypoint.sh are placed after the USER appuser switch so they fail as a
non-root user; move the COPY --chown=appuser:appgroup docker-entrypoint.sh
/usr/local/bin/ and the RUN chmod +x /usr/local/bin/docker-entrypoint.sh lines
to before the USER appuser directive (you can keep ENTRYPOINT
["docker-entrypoint.sh"] where it is), ensuring the file is owned and executable
as intended.
In @tools/permission-auditor/README.md:
- Around line 43-49: Update the README clone instructions to reference the
repository in this organization instead of the external URL: replace the git
clone URL `https://github.com/altynai9128/permission-auditor2.git` with
`https://github.com/cortexlinux/cortex.git`, and update the subsequent
navigation step to `cd cortex/tools/permission-auditor/`; alternatively, remove
the clone step and add instructions to run the tool in-place from the
checked-out monorepo (e.g., `cd tools/permission-auditor && python3
src/auditor.py`) so users know the correct path within this repository.
In @tools/permission-auditor/scripts/demo.sh:
- Line 13: The script performs cd "$DEMO_DIR" without checking success; update
demo.sh to ensure the directory change succeeds by adding explicit error
handling after the cd (e.g., check the exit status and print a clear error
including DEMO_DIR and exit non‑zero) or enable a fail-fast mode at the top of
the script so failures abort immediately; make the change around the cd
"$DEMO_DIR" line in demo.sh and ensure any subsequent commands only run if the
cd succeeded.
In @tools/permission-auditor/scripts/docker-entrypoint.sh:
- Around line 6-13: The script uses unquoted $HOST_UID and $HOST_GID and doesn't
validate them; update the conditional and commands to quote variables (e.g.,
"$HOST_UID", "$HOST_GID") and validate they are numeric before running
usermod/groupmod/chown to avoid injection or word-splitting. Specifically,
change the if to check non-empty quoted values and add a numeric check like [[
"$HOST_UID" =~ ^[0-9]+$ ]] && [[ "$HOST_GID" =~ ^[0-9]+$ ]] before calling sudo
usermod -u "$HOST_UID" appuser, sudo groupmod -g "$HOST_GID" appgroup and sudo
chown -R "$HOST_UID":"$HOST_GID" /app; if validation fails, emit a clear warning
and skip the remapping. Ensure error handling remains (capture failures and log
warnings) but use the quoted variables everywhere.
In @tools/permission-auditor/scripts/install.sh:
- Around line 29-31: The install.sh script uses a relative path
"../src/auditor.py" which fails if the script is invoked from a different
working directory; update install.sh to resolve its own directory (use
${BASH_SOURCE[0]} or similar to compute script_dir) and use
script_dir/../src/auditor.py as the source for the copy to
/usr/local/bin/perm-audit, then keep the sudo chmod +x /usr/local/bin/perm-audit
step; reference the existing ../src/auditor.py string and the perm-audit target
when making the change.
In @tools/permission-auditor/src/auditor.py:
- Around line 35-62: The load_config function references undefined symbols
EXCLUDE_PATHS and Colors and is never invoked; move the entire load_config
definition to appear after the Colors class and the EXCLUDE_PATHS constant so
those names are defined before use, and then call load_config(...) from main()
(replace any inline default config use in main with the returned config) so the
function is actually used; alternatively, if you prefer not to move it, update
load_config to avoid direct references to EXCLUDE_PATHS and Colors (use literals
or defer resolution) and add a call to load_config in main() to load and apply
configuration.
- Around line 1212-1214: The code mistakenly performs a self-import ("from
auditor import apply_selected_fixes") from inside the same module which will
raise ImportError; remove that import statement and call the local function
apply_selected_fixes(all_findings, indices, dry_run=True) directly (ensure the
apply_selected_fixes function defined earlier in the file is available).
- Around line 32-33: The file contains duplicate imports of json and Path (the
symbols json and Path are imported twice); remove the repeated import lines
shown in this diff (the second "import json" and "from pathlib import Path") so
each symbol is imported only once, and verify there are no other duplicate
import statements for json or Path elsewhere in auditor.py.
- Around line 679-680: Replace the bare "except:" blocks at the three try/except
sites (around lines 391, 679, and 705) with a proper exception handler that
catches Exception only (e.g., "except Exception as e:") and log the error (using
the module's logger/audit logger via logger.exception or logger.error with the
exception) instead of silently passing; ensure you do not swallow
SystemExit/KeyboardInterrupt by not catching BaseException and, where the
original intent was to ignore recoverable errors, explicitly handle or comment
why the exception is safe to ignore.
In @tools/permission-auditor/src/auditor.py.save:
- Around line 277-278: The line merges a comment and code causing a SyntaxError;
split them into two lines so the comment stands alone and the conditional
executes: place the comment "# Skip excluded paths" on its own line, then add
"if should_skip_path(current_path):" on the next line followed by the indented
"return". Ensure indentation matches surrounding block and references the
existing should_skip_path and current_path variables.
- Around line 1-14: Remove the editor backup file auditor.py.save from the repo
and stop tracking such backups: delete auditor.py.save (or run git rm --cached
auditor.py.save if you need to preserve it locally), add the pattern *.save to
.gitignore to prevent future backups from being committed, and commit the
changes; ensure the canonical auditor.py (without .save) remains in place if
that is the intended source file.
In @tools/permission-auditor/test_permissions/check_perms.py:
- Around line 1-4: Add a module-level docstring at the top of this script
describing its purpose as a permission-auditing diagnostic utility (what it
checks, expected inputs/outputs, and any important behavior), placed immediately
after the shebang and before the imports; ensure the docstring is a proper
triple-quoted string and concise so the module complies with the "docstrings
required for all public APIs" guideline.
In @tools/permission-auditor/tests/integration_test.py:
- Around line 36-59: The test_world_writable function is missing a return type
hint and contains unreachable code after the finally block; add a return type
annotation (def test_world_writable() -> bool:) and remove the dead print/return
lines following the finally so the function ends after the cleanup/unlink in
finally and the intended return True/False remains inside the try/except/logic;
locate and update the function named test_world_writable accordingly.
- Around line 9-34: The test_777_file function lacks a return type hint and
contains unreachable dead code at the end; update the signature to include a
return type (def test_777_file() -> bool:) and remove the trailing unreachable
print("❌ test_777_file: FAILED") and return False lines, and ensure all control
paths return a bool (e.g., keep the existing return True in the try and add an
explicit return False in an except block or after the try/finally as
appropriate).
In @tools/permission-auditor/tests/test_fix_commands.py:
- Around line 52-59: The test assumes apply_single_fix returns 'DRY_RUN' but
apply_single_fix (in auditor.py) can return 'DRY_RUN_NEEDS_SUDO' when running as
non-root; update the assertion in test_fix_commands.py (the block that calls
apply_single_fix with dry_run=True) to accept either 'DRY_RUN' or
'DRY_RUN_NEEDS_SUDO' (e.g., check that result['status'] is in a set { 'DRY_RUN',
'DRY_RUN_NEEDS_SUDO' }) so the test passes in both root and non-root
environments while leaving the rest of the command/permission assertions
unchanged.
🟡 Minor comments (8)
tools/permission-auditor/scripts/demo.sh-13-13 (1)
13-13:cdwithout error handling can cause script to run in wrong directory.If the
cdfails, the script continues in the original directory, potentially creating files in unexpected locations.🔎 Proposed fix
-cd "$DEMO_DIR" +cd "$DEMO_DIR" || { echo "Failed to enter demo directory"; exit 1; }tools/permission-auditor/src/auditor.py-32-33 (1)
32-33: Duplicate imports.
jsonandPathare imported twice - once at lines 20 and 24, and again here at lines 32-33.-import json -from pathlib import Pathtools/permission-auditor/demos/showcase.py-84-91 (1)
84-91: Bareexceptclause swallows all exceptions.Using a bare
except:catches everything includingKeyboardInterruptandSystemExit, making debugging difficult and potentially hiding real issues.🔎 Proposed fix
try: import json parsed = json.loads(result.stdout) print(f"✅ JSON output is valid") print(f" Found {len(parsed.get('findings', []))} issues") print(f" Tool version: {parsed.get('metadata', {}).get('version', 'unknown')}") - except: + except json.JSONDecodeError as e: print("⚠️ Could not parse JSON output") + print(f" Error: {e}")tools/permission-auditor/scripts/install.sh-29-31 (1)
29-31: Relative path makes script fail if not run fromscripts/directory.The path
../src/auditor.pyis relative to CWD, not the script location. If the user runsbash tools/permission-auditor/scripts/install.shfrom the repo root, the copy will fail.🔎 Proposed fix
+# Get the directory where this script is located +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +AUDITOR_SRC="${SCRIPT_DIR}/../src/auditor.py" + +# Verify source exists +if [ ! -f "$AUDITOR_SRC" ]; then + echo -e "${RED}Error: Cannot find auditor.py at ${AUDITOR_SRC}${NC}" + exit 1 +fi + # Copy to /usr/local/bin -sudo cp ../src/auditor.py /usr/local/bin/perm-audit +sudo cp "$AUDITOR_SRC" /usr/local/bin/perm-audit sudo chmod +x /usr/local/bin/perm-audittools/permission-auditor/scripts/demo.sh-48-60 (1)
48-60: Hardcoded path and unused variable.
- The path
/opt/permission-auditor-final/src/auditor.pyis hardcoded and won't work in other environments.json_outputis set but never used (as flagged by ShellCheck SC2034).🔎 Proposed fix
+# Use script's location to find auditor +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +AUDITOR_PATH="${SCRIPT_DIR}/../src/auditor.py" + # Run auditor in dry-run mode echo "2. Running Permission Auditor (dry-run mode)..." echo "" echo "--- DRY RUN SCAN ---" -python3 /opt/permission-auditor-final/src/auditor.py . -r --fix +python3 "$AUDITOR_PATH" . -r --fix echo "--- END DRY RUN ---" echo "" # Show single command fix echo "3. Demonstrating single command fixes..." echo "" echo "For each issue, the tool generates a safe fix command:" echo "" # Get the JSON output to parse -echo "Getting detailed JSON report..." -json_output=$(python3 /opt/permission-auditor-final/src/auditor.py . -r --json 2>/dev/null) +# Note: JSON output demonstration removed as it wasn't being usedCommittable suggestion skipped: line range outside the PR's diff.
tools/permission-auditor/src/auditor.py-679-680 (1)
679-680: Bareexceptclause hides errors and catches unintended exceptions.This pattern appears multiple times in the file (lines 391, 679, 705). Bare
except:catches everything includingKeyboardInterrupt,SystemExit, and programming errors.🔎 Proposed fix
- except: - pass + except (OSError, IOError): + passtools/permission-auditor/tests/test_fix_commands.py-52-59 (1)
52-59: Status assertion may need to includeDRY_RUN_NEEDS_SUDO.Based on the
apply_single_fixAPI inauditor.py, when running as non-root and sudo is required, the status isDRY_RUN_NEEDS_SUDO, notDRY_RUN. The current assertion on line 55 only checks forDRY_RUN, which could cause test failures in non-root environments.🔎 Proposed fix
- assert result['status'] == 'DRY_RUN', f"Expected DRY_RUN, got {result['status']}" + assert result['status'] in ['DRY_RUN', 'DRY_RUN_NEEDS_SUDO'], \ + f"Expected DRY_RUN or DRY_RUN_NEEDS_SUDO, got {result['status']}"tools/permission-auditor/scripts/docker-entrypoint.sh-6-13 (1)
6-13: Quote variables and validate inputs for robustness.The unquoted
$HOST_UIDand$HOST_GIDvariables could cause issues if they contain unexpected characters. Additionally, validating that these are numeric values would prevent command injection risks.🔎 Proposed fix
-if [ -n "$HOST_UID" ] && [ -n "$HOST_GID" ]; then +# Validate and apply UID/GID mapping +if [ -n "$HOST_UID" ] && [ -n "$HOST_GID" ]; then + # Validate inputs are numeric + if ! echo "$HOST_UID" | grep -qE '^[0-9]+$' || ! echo "$HOST_GID" | grep -qE '^[0-9]+$'; then + echo "Error: HOST_UID and HOST_GID must be numeric" + exit 1 + fi + echo "Mapping container user to host UID/GID: $HOST_UID:$HOST_GID" - sudo usermod -u $HOST_UID appuser 2>/dev/null || echo "Warning: Cannot change UID" - sudo groupmod -g $HOST_GID appgroup 2>/dev/null || echo "Warning: Cannot change GID" + sudo usermod -u "$HOST_UID" appuser 2>/dev/null || echo "Warning: Cannot change UID" + sudo groupmod -g "$HOST_GID" appgroup 2>/dev/null || echo "Warning: Cannot change GID" - sudo chown -R $HOST_UID:$HOST_GID /app 2>/dev/null || true + sudo chown -R "$HOST_UID:$HOST_GID" /app 2>/dev/null || true fi
🧹 Nitpick comments (16)
tools/permission-auditor/tests/test-docker.sh (1)
13-20: Quote command substitutions to prevent word splitting.The unquoted
$(id -u)and$(id -g)command substitutions could cause issues if the output contains spaces or special characters (though unlikely for UID/GID).🔎 Proposed fix
docker run --rm \ - -e HOST_UID=$(id -u) \ - -e HOST_GID=$(id -g) \ + -e HOST_UID="$(id -u)" \ + -e HOST_GID="$(id -g)" \ -v /tmp:/host-tmp \ permission-auditor:test /app -r -dAs per static analysis hints (Shellcheck SC2046)
tools/permission-auditor/scripts/quick-test.sh (1)
55-72: Consider capturing exit status from all tests.Currently, only
BASIC_TEST_RESULTaffects the final exit code. Earlier test failures (Tests 1-4) are reported but don't influence the script's exit status, which could cause CI/CD pipelines to miss failures.🔎 Proposed fix to track all test results
+OVERALL_RESULT=0 + # Test 1: Help echo "1. Testing help..." python3 src/auditor.py --help > /dev/null 2>&1 if [ $? -eq 0 ]; then echo "✅ Help works" else echo "❌ Help failed" + OVERALL_RESULT=1 fi # Test 2: Basic scan echo "" echo "2. Testing basic scan..." python3 src/auditor.py . > /dev/null 2>&1 if [ $? -eq 0 ] || [ $? -eq 1 ]; then echo "✅ Basic scan works" else echo "❌ Basic scan failed" + OVERALL_RESULT=1 fi # Test 3: Create test file and scan echo "" echo "3. Creating test file..." TEST_FILE="/tmp/test-perm-audit-$$.txt" echo "test-content" > "$TEST_FILE" chmod 777 "$TEST_FILE" echo "Scanning test file..." output=$(python3 src/auditor.py "$TEST_FILE" 2>&1) if echo "$output" | grep -q "CRITICAL"; then echo "✅ Found 777 permission issue" else echo "❌ Did not find issue" echo "Output: $output" + OVERALL_RESULT=1 fi # Test 4: Test world-writable detection echo "" echo "4. Testing world-writable detection..." echo "test" > "/tmp/test-666-$$.txt" chmod 666 "/tmp/test-666-$$.txt" output=$(python3 src/auditor.py "/tmp/test-666-$$.txt" 2>&1) if echo "$output" | grep -q "HIGH\|WORLD_WRITABLE"; then echo "✅ Found world-writable issue" else echo "❌ Did not find world-writable issue" + OVERALL_RESULT=1 fi # Cleanup rm -f "$TEST_FILE" "/tmp/test-666-$$.txt" # Test 5: Run Python unit tests echo "" echo "5. Running unit tests..." if [ -f "tests/test_basic.py" ]; then python3 tests/test_basic.py BASIC_TEST_RESULT=$? + if [ $BASIC_TEST_RESULT -ne 0 ]; then + OVERALL_RESULT=1 + fi else echo "⚠️ test_basic.py not found, skipping" BASIC_TEST_RESULT=0 fi echo "" echo "=== TEST COMPLETE ===" # Exit with worst result -if [ $BASIC_TEST_RESULT -ne 0 ]; then - exit $BASIC_TEST_RESULT -fi +exit $OVERALL_RESULTtools/permission-auditor/tests/integration_test.py (1)
61-72: Consider handling assertion failures gracefully.Currently, if an assertion fails in a test function, it will raise
AssertionErrorand terminate the test runner without executing remaining tests. Consider wrapping test execution in try/except to ensure all tests run.🔎 Proposed fix for robust test execution
for test in tests: - if test(): - passed += 1 + try: + if test(): + passed += 1 + except AssertionError as e: + print(f"❌ {test.__name__}: FAILED - {e}") + except Exception as e: + print(f"❌ {test.__name__}: ERROR - {e}") print(f"\nResults: {passed}/{len(tests)} tests passed") exit(0 if passed == len(tests) else 1)tools/permission-auditor/tests/test_version.py (2)
7-23: Hardcoded version strings make tests fragile.The version
'1.0.0'is hardcoded in multiple test functions. If the version changes inauditor.py, these tests will fail silently or require manual updates.Consider importing the version constant from the auditor module:
from auditor import VERSION # Then use VERSION in assertionsAlso, per coding guidelines, type hints are required. Add return type hints:
-def test_version_flag(): +def test_version_flag() -> bool:
71-92: Test runner lacks exception handling for individual tests.If a test function raises an exception, the entire test run will abort. Consider wrapping each test call in a try/except to ensure all tests run:
🔎 Proposed improvement
passed = 0 for test in tests: - if test(): - passed += 1 + try: + if test(): + passed += 1 + except Exception as e: + print(f"❌ Test {test.__name__} raised exception: {e}")tools/permission-auditor/demos/showcase.py (1)
14-15: Missing type hints and docstring content.Per coding guidelines, type hints are required for Python code and docstrings are required for public APIs.
-def run_showcase(): - """Run the full showcase.""" +def run_showcase() -> None: + """ + Run the full showcase demonstrating Permission Auditor features. + + Creates a temporary directory with test files having various dangerous + permissions, runs the auditor in multiple modes, and cleans up on exit. + """tools/permission-auditor/src/auditor.py (2)
350-362: Missing blank lines between function definitions (PEP 8).Per PEP 8 and the coding guidelines, top-level function definitions should be separated by two blank lines.
'results': results } + + def apply_selected_fixes(findings, indices, dry_run=True):
399-479: Missing type hints on public API functions.Per coding guidelines, type hints are required. The
check_file_permissionsfunction has a type hint forfilepathbut is missing the return type.-def check_file_permissions(filepath: str): +def check_file_permissions(filepath: str) -> dict | None:Similar updates needed for other public functions like
explain_issue,suggest_safe_permissions,generate_text_report, etc.tools/permission-auditor/scripts/install.sh (1)
1-44: Consider adding sudo check and uninstall option.The script assumes sudo is available but doesn't verify. Also, users may want to uninstall.
🔎 Optional enhancements
# Add after Python check: # Check sudo access if ! sudo -v 2>/dev/null; then echo -e "${RED}Error: This script requires sudo privileges${NC}" exit 1 fi # Add at end of file: echo "" echo -e "To uninstall:" echo -e " sudo rm /usr/local/bin/perm-audit"tools/permission-auditor/tests/test_basic.py (2)
11-24: Missing type hints on test functions.Per coding guidelines, type hints are required in Python code.
🔎 Proposed fix
-def test_imports(): +def test_imports() -> bool: """Test that all required modules can be imported."""
76-103: Moveshutilimport to module level.The
shutilimport on line 101 should be at the top of the file with other imports for better readability and to avoid repeated import overhead if this function is called multiple times.🔎 Proposed fix
import os import sys import tempfile +import shutil # Add src directory to path sys.path.insert(0, os.path.join(os.path.dirname(__file__), '../src'))And remove line 101:
finally: # Cleanup - import shutil if os.path.exists(temp_dir): shutil.rmtree(temp_dir)tools/permission-auditor/tests/test_fix_commands.py (1)
1-10: Imports and setup are appropriate.The
statmodule is imported but appears unused in this file. Consider removing it.🔎 Proposed fix
import os import tempfile -import stat import systools/permission-auditor/tests/test_full_features.py (4)
286-291: Avoid bareexceptclause.The bare
except:on line 290 catches all exceptions includingKeyboardInterruptandSystemExit, which can mask issues. Catch specific exceptions instead.🔎 Proposed fix
def get_safe_username(): try: import pwd return pwd.getpwuid(os.getuid()).pw_name - except: + except (ImportError, KeyError, OSError): return "testuser"
348-356: Another bareexceptclause to address.Same issue as above - prefer specific exception types.
🔎 Proposed fix
try: # Check if we own the file or are root stat_info = os.stat(temp_path) if os.getuid() == 0 or stat_info.st_uid == os.getuid(): can_write = True - except: + except (OSError, PermissionError): pass
392-397: Third bareexceptclause in cleanup logic.While cleanup code often uses broad exception handling, it's still better to be specific.
🔎 Proposed fix
try: for f in os.listdir(dir_name): if f.startswith(base_name + '.perm-backup-'): os.unlink(os.path.join(dir_name, f)) - except: + except OSError: pass
400-463: Docker support test has redundant import.Line 404 imports
check_docker_availableandscan_docker_containersbutcheck_docker_availableis already imported at the top of the file (line 24). Remove the redundant import.🔎 Proposed fix
def test_requirement_5_docker_support(): """Test 5: Handles Docker/container UID mapping.""" print_header("TEST 5: DOCKER/CONTAINER SUPPORT") - from auditor import check_docker_available, scan_docker_containers + from auditor import scan_docker_containers print("Testing Docker availability check...")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
tools/permission-auditor/README.mdtools/permission-auditor/config/default.jsontools/permission-auditor/demos/showcase.pytools/permission-auditor/examples/advanced.Dockerfiletools/permission-auditor/scripts/demo.shtools/permission-auditor/scripts/docker-entrypoint.shtools/permission-auditor/scripts/install.shtools/permission-auditor/scripts/quick-test.shtools/permission-auditor/src/auditor.pytools/permission-auditor/src/auditor.py.savetools/permission-auditor/test_permissions/check_perms.pytools/permission-auditor/test_permissions/dangerous_file.txttools/permission-auditor/test_permissions/test_function.pytools/permission-auditor/test_permissions/world_writable.txttools/permission-auditor/tests/__init__.pytools/permission-auditor/tests/integration_test.pytools/permission-auditor/tests/test-docker.shtools/permission-auditor/tests/test_basic.pytools/permission-auditor/tests/test_fix_commands.pytools/permission-auditor/tests/test_full_features.pytools/permission-auditor/tests/test_version.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
tools/permission-auditor/tests/__init__.pytools/permission-auditor/test_permissions/check_perms.pytools/permission-auditor/demos/showcase.pytools/permission-auditor/tests/integration_test.pytools/permission-auditor/tests/test_basic.pytools/permission-auditor/tests/test_version.pytools/permission-auditor/tests/test_fix_commands.pytools/permission-auditor/tests/test_full_features.pytools/permission-auditor/test_permissions/test_function.pytools/permission-auditor/src/auditor.py
{setup.py,setup.cfg,pyproject.toml,**/__init__.py}
📄 CodeRabbit inference engine (AGENTS.md)
Use Python 3.10 or higher as the minimum supported version
Files:
tools/permission-auditor/tests/__init__.py
🧬 Code graph analysis (5)
tools/permission-auditor/tests/test_basic.py (1)
tools/permission-auditor/src/auditor.py (4)
check_file_permissions(399-479)scan_directory(491-541)explain_issue(547-594)suggest_safe_permissions(596-729)
tools/permission-auditor/tests/test_version.py (1)
cortex/sandbox/docker_sandbox.py (1)
test(483-639)
tools/permission-auditor/tests/test_fix_commands.py (1)
tools/permission-auditor/src/auditor.py (2)
apply_single_fix(94-268)suggest_safe_permissions(596-729)
tools/permission-auditor/test_permissions/test_function.py (1)
tools/permission-auditor/src/auditor.py (1)
check_file_permissions(399-479)
tools/permission-auditor/src/auditor.py (4)
cortex/kernel_features/llm_device.py (2)
open(158-159)read(120-129)cortex/env_manager.py (1)
load(571-595)cortex/sandbox/sandbox_executor.py (1)
failed(79-81)tools/permission-auditor/tests/test_full_features.py (1)
main(613-682)
🪛 LanguageTool
tools/permission-auditor/README.md
[style] ~8-~8: Consider using a different verb for a more formal wording.
Context: ...es. This tool helps identify and safely fix such problems. ## ✨ Features - ✅ **Da...
(FIX_RESOLVE)
🪛 Shellcheck (0.11.0)
tools/permission-auditor/tests/test-docker.sh
[warning] 16-16: Quote this to prevent word splitting.
(SC2046)
[warning] 17-17: Quote this to prevent word splitting.
(SC2046)
tools/permission-auditor/scripts/demo.sh
[warning] 13-13: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 60-60: json_output appears unused. Verify use (or export if used externally).
(SC2034)
tools/permission-auditor/scripts/quick-test.sh
[warning] 18-18: This $? refers to a condition, not a command. Assign to a variable to avoid it being overwritten.
(SC2319)
🔇 Additional comments (23)
tools/permission-auditor/tests/__init__.py (1)
1-1: LGTM!Empty
__init__.pyfiles are the correct, idiomatic way to establish a Python package in modern versions. This properly structures thetestsdirectory as a package, enabling imports for the test modules.tools/permission-auditor/test_permissions/check_perms.py (1)
5-23: LGTM - Diagnostic logic is clear and correct.The diagnostic output format is well-structured and provides all necessary permission details for testing purposes.
tools/permission-auditor/tests/test-docker.sh (2)
1-12: LGTM - Test structure is clear.The numbered test sections with descriptive echo statements provide good visibility into test progress.
21-36: Good test coverage with proper cleanup.The test suite covers interactive mode, JSON output, and actual fixing operations. The cleanup step with
docker stopensures no lingering containers.tools/permission-auditor/scripts/quick-test.sh (3)
1-22: LGTM - Test structure is sound.The shellcheck warning (SC2319) at line 18 is a false positive. The
$?variable is checked immediately in the conditional, so there's no risk of it being overwritten before use. The logic correctly accepts exit codes 0 or 1 for the basic scan.
24-38: Effective 777 permission detection test.Good use of
$$for unique temp file names and helpful diagnostic output on failure.
40-53: LGTM - World-writable detection is properly tested.The test correctly sets 666 permissions and checks for appropriate severity indicators. Cleanup is comprehensive.
tools/permission-auditor/tests/integration_test.py (1)
1-8: LGTM - Module setup is clean.Module docstring and imports are appropriate for integration testing.
tools/permission-auditor/config/default.json (2)
1-28: Configuration structure looks solid.The severity mappings, safe permissions, and exclude patterns are well-defined and appropriate for a permission auditing tool.
8-8: These configuration keys are not used in the code and do not cause any inconsistency.The
check_dockeranddocker.scan_containerssettings in the configuration file are unused. Docker container scanning is controlled exclusively by the--dockercommand-line argument, not by configuration settings. The config keys appear to be legacy or planned functionality that was never implemented. No action needed.Likely an incorrect or invalid review comment.
tools/permission-auditor/tests/test_basic.py (4)
1-10: Module setup looks good, but consider a minor improvement.The
sys.pathmanipulation is acceptable for test discovery. However, importingshutilat the module level (it's currently imported insidetest_directory_scan) would be cleaner.
26-49: Test correctly validates 777 detection with proper cleanup.Good use of
finallyblock for cleanup. Consider adding type hint for consistency.
51-74: World-writable detection test is well-structured.Proper isolation with temp files and cleanup. Same minor note about type hints applies.
105-121: Test runner implementation is correct.Exit codes are properly set based on test results. This approach is straightforward and effective.
tools/permission-auditor/tests/test_fix_commands.py (4)
12-22: Good defensive fallback chain for username retrieval.The multiple fallback methods handle various edge cases (containers, CI environments). Missing type hint and return type.
74-141: Test cases cover good variety of file types.The test matrix covers scripts, configs, logs, regular files, and directories. This provides solid coverage for the permission suggestion logic.
188-227: Special files test validates system file handling.Testing against
/etc/shadow,/etc/sudoers, and/etc/passwdwith mock findings is appropriate for unit testing the suggestion logic without requiring actual system file access.
229-245: Test runner follows consistent pattern with other test files.Exit code handling is correct.
tools/permission-auditor/tests/test_full_features.py (2)
1-28: Comprehensive import set for full feature testing.All necessary functions are imported from the auditor module. The import of
Colorsfor formatting is a nice touch for readable test output.
613-693: Well-structured main function with proper error handling.Good use of structured test result collection, summary output, and appropriate exit codes. The
KeyboardInterrupthandling is a nice touch.tools/permission-auditor/scripts/docker-entrypoint.sh (1)
15-15: Correct use ofexecfor entrypoint.Using
exec "$@"properly replaces the shell with the command, ensuring signal handling works correctly in Docker.tools/permission-auditor/README.md (1)
1-18: Documentation is clear and well-structured.The problem statement effectively explains the motivation. The features list covers all bounty requirements. Good use of emoji for visual clarity.
tools/permission-auditor/examples/advanced.Dockerfile (1)
51-67: Demo permission artifacts are well-documented.Good demonstration of various permission scenarios for testing the auditor. The comments make the intent clear.
| # Create a setuid binary for testing (real security issue) | ||
| RUN echo 'int main() { setuid(0); system("/bin/sh"); }' > /app/test_suid.c && \ | ||
| gcc /app/test_suid.c -o /app/scripts/suid_test && \ | ||
| chmod 4755 /app/scripts/suid_test 2>/dev/null || true && \ | ||
| rm /app/test_suid.c |
There was a problem hiding this comment.
Remove the setuid binary example - this creates a real privilege escalation vulnerability.
Even for demonstration purposes, compiling and installing a setuid-root binary that spawns a shell is dangerous. If this image is accidentally deployed or the binary is extracted, it provides a trivial privilege escalation path. Use a mock/dummy binary or document the risk pattern without creating an actual exploit.
🔎 Proposed fix - use a safe alternative
-# Create a setuid binary for testing (real security issue)
-RUN echo 'int main() { setuid(0); system("/bin/sh"); }' > /app/test_suid.c && \
- gcc /app/test_suid.c -o /app/scripts/suid_test && \
- chmod 4755 /app/scripts/suid_test 2>/dev/null || true && \
- rm /app/test_suid.c
+# Create a setuid binary placeholder for testing (safe - does nothing harmful)
+RUN echo 'int main() { return 0; }' > /app/test_suid.c && \
+ gcc /app/test_suid.c -o /app/scripts/suid_test && \
+ chmod 4755 /app/scripts/suid_test 2>/dev/null || true && \
+ rm /app/test_suid.cOr better yet, skip creating a real binary entirely and just touch a file with setuid permissions for detection testing:
-# Create a setuid binary for testing (real security issue)
-RUN echo 'int main() { setuid(0); system("/bin/sh"); }' > /app/test_suid.c && \
- gcc /app/test_suid.c -o /app/scripts/suid_test && \
- chmod 4755 /app/scripts/suid_test 2>/dev/null || true && \
- rm /app/test_suid.c
+# Create a file with setuid bit for testing detection (safe - not executable code)
+RUN touch /app/scripts/suid_test_marker && \
+ chmod 4755 /app/scripts/suid_test_marker 2>/dev/null || true📝 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.
| # Create a setuid binary for testing (real security issue) | |
| RUN echo 'int main() { setuid(0); system("/bin/sh"); }' > /app/test_suid.c && \ | |
| gcc /app/test_suid.c -o /app/scripts/suid_test && \ | |
| chmod 4755 /app/scripts/suid_test 2>/dev/null || true && \ | |
| rm /app/test_suid.c | |
| # Create a setuid binary placeholder for testing (safe - does nothing harmful) | |
| RUN echo 'int main() { return 0; }' > /app/test_suid.c && \ | |
| gcc /app/test_suid.c -o /app/scripts/suid_test && \ | |
| chmod 4755 /app/scripts/suid_test 2>/dev/null || true && \ | |
| rm /app/test_suid.c |
| # Create a setuid binary for testing (real security issue) | |
| RUN echo 'int main() { setuid(0); system("/bin/sh"); }' > /app/test_suid.c && \ | |
| gcc /app/test_suid.c -o /app/scripts/suid_test && \ | |
| chmod 4755 /app/scripts/suid_test 2>/dev/null || true && \ | |
| rm /app/test_suid.c | |
| # Create a file with setuid bit for testing detection (safe - not executable code) | |
| RUN touch /app/scripts/suid_test_marker && \ | |
| chmod 4755 /app/scripts/suid_test_marker 2>/dev/null || true |
🤖 Prompt for AI Agents
In @tools/permission-auditor/examples/advanced.Dockerfile around lines 69-73,
The Dockerfile creates a real setuid-root binary (/app/scripts/suid_test) by
compiling /app/test_suid.c and setting chmod 4755, which is a real privilege
escalation risk; replace this with a safe test artifact: do not compile or
install a binary that spawns a shell—either create a non-executable placeholder
file or a harmless dummy executable (e.g., touch /app/scripts/suid_test and set
benign content) or simply touch a file and set the mode to mimic detection
(chmod 4755 on a non-functional placeholder) so the permission-auditor can
detect the pattern without introducing an exploitable program.
| # Add entrypoint script for UID/GID mapping | ||
| COPY --chown=appuser:appgroup docker-entrypoint.sh /usr/local/bin/ | ||
| RUN chmod +x /usr/local/bin/docker-entrypoint.sh | ||
| ENTRYPOINT ["docker-entrypoint.sh"] |
There was a problem hiding this comment.
Dockerfile ordering issue: COPY and RUN after USER switch will fail.
Lines 82-83 attempt to copy and chmod a file in /usr/local/bin/ after switching to appuser. The non-root user cannot write to /usr/local/bin/ or change permissions there. Move these commands before USER appuser.
🔎 Proposed fix - reorder commands
+# Add entrypoint script for UID/GID mapping (before USER switch)
+COPY docker-entrypoint.sh /usr/local/bin/
+RUN chmod +x /usr/local/bin/docker-entrypoint.sh && \
+ chown appuser:appgroup /usr/local/bin/docker-entrypoint.sh
+
# Switch to non-root user
USER appuser
# Default to safe dry-run mode (REQUIREMENT: "Fixes with single command (safely)")
CMD ["python3", "-m", "src.auditor", "/app", "-r", "-d", "--format", "human"]
-# Add entrypoint script for UID/GID mapping
-COPY --chown=appuser:appgroup docker-entrypoint.sh /usr/local/bin/
-RUN chmod +x /usr/local/bin/docker-entrypoint.sh
ENTRYPOINT ["docker-entrypoint.sh"]Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @tools/permission-auditor/examples/advanced.Dockerfile around lines 81-84,
The COPY and RUN that install and chmod docker-entrypoint.sh are placed after
the USER appuser switch so they fail as a non-root user; move the COPY
--chown=appuser:appgroup docker-entrypoint.sh /usr/local/bin/ and the RUN chmod
+x /usr/local/bin/docker-entrypoint.sh lines to before the USER appuser
directive (you can keep ENTRYPOINT ["docker-entrypoint.sh"] where it is),
ensuring the file is owned and executable as intended.
| def load_config(config_path=None): | ||
| """Load configuration from JSON file.""" | ||
| default_config = { | ||
| "settings": { | ||
| "default_scan_path": ".", | ||
| "recursive_scan": True, | ||
| "max_depth": 8, | ||
| "exclude_patterns": EXCLUDE_PATHS, | ||
| "safe_permissions": { | ||
| "directories": "755", | ||
| "regular_files": "644", | ||
| "executable_files": "750", | ||
| "sensitive_files": "600" | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if config_path and Path(config_path).exists(): | ||
| try: | ||
| with open(config_path, 'r') as f: | ||
| user_config = json.load(f) | ||
| # Merge with default config | ||
| if 'permission_auditor' in user_config: | ||
| return user_config['permission_auditor'] | ||
| except Exception as e: | ||
| print(f"{Colors.YELLOW}[!] Config error: {e}, using defaults{Colors.END}") | ||
|
|
||
| return default_config |
There was a problem hiding this comment.
load_config function references undefined symbols and is never called.
- Line 42 references
EXCLUDE_PATHSwhich is defined later at line 77 - Line 60 references
Colors.YELLOWwhich is defined later at line 65 - This function is defined but never used in
main()or elsewhere
This will cause a NameError if the function is ever called. Either:
- Move this function after the
Colorsclass andEXCLUDE_PATHSdefinition - Remove the function if it's not needed
- Or use the function in
main()
🤖 Prompt for AI Agents
In @tools/permission-auditor/src/auditor.py around lines 35-62, The load_config
function references undefined symbols EXCLUDE_PATHS and Colors and is never
invoked; move the entire load_config definition to appear after the Colors class
and the EXCLUDE_PATHS constant so those names are defined before use, and then
call load_config(...) from main() (replace any inline default config use in main
with the returned config) so the function is actually used; alternatively, if
you prefer not to move it, update load_config to avoid direct references to
EXCLUDE_PATHS and Colors (use literals or defer resolution) and add a call to
load_config in main() to load and apply configuration.
| # Skip excluded paths if should_skip_path(current_path): | ||
| return |
There was a problem hiding this comment.
Syntax error: comment and code merged on the same line.
This line has a broken statement where a comment and code are merged:
# Skip excluded paths if should_skip_path(current_path):This will cause a SyntaxError when the file is executed. The code after the comment is treated as part of the comment.
🤖 Prompt for AI Agents
In @tools/permission-auditor/src/auditor.py.save around lines 277-278, The line
merges a comment and code causing a SyntaxError; split them into two lines so
the comment stands alone and the conditional executes: place the comment "# Skip
excluded paths" on its own line, then add "if should_skip_path(current_path):"
on the next line followed by the indented "return". Ensure indentation matches
surrounding block and references the existing should_skip_path and current_path
variables.
| #!/usr/bin/env python3 | ||
| import os | ||
| import stat | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add module docstring.
Per coding guidelines, docstrings are required for all public APIs. Add a module-level docstring explaining the purpose of this diagnostic script.
🔎 Proposed fix
#!/usr/bin/env python3
+"""Diagnostic script to check permissions of test files."""
import os
import statBased on coding guidelines: "Docstrings required for all public APIs"
🤖 Prompt for AI Agents
In @tools/permission-auditor/test_permissions/check_perms.py around lines 1-4,
Add a module-level docstring at the top of this script describing its purpose as
a permission-auditing diagnostic utility (what it checks, expected
inputs/outputs, and any important behavior), placed immediately after the
shebang and before the imports; ensure the docstring is a proper triple-quoted
string and concise so the module complies with the "docstrings required for all
public APIs" guideline.
| #!/usr/bin/env python3 | ||
| import sys | ||
| sys.path.insert(0, '../src') | ||
|
|
||
| import auditor | ||
|
|
||
| # Test the function directly | ||
| print("Direct function test:") | ||
| result1 = auditor.check_file_permissions("dangerous_file.txt") | ||
| print(f"dangerous_file.txt: {result1}") | ||
|
|
||
| result2 = auditor.check_file_permissions("world_writable.txt") | ||
| print(f"world_writable.txt: {result2}") | ||
|
|
||
| result3 = auditor.check_file_permissions("dangerous_dir") | ||
| print(f"dangerous_dir: {result3}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Test lacks setup, assertions, and proper structure.
This test script has several issues:
- Fragile path:
sys.path.insert(0, '../src')depends on the current working directory being the test_permissions folder - Missing test files: The test assumes
dangerous_file.txt,world_writable.txt, anddangerous_direxist but doesn't create them - No assertions: Just prints results without validating correctness
- No exit code: Cannot determine pass/fail programmatically
🔎 Proposed improvement
#!/usr/bin/env python3
+"""Test check_file_permissions function directly."""
+
import sys
-sys.path.insert(0, '../src')
+import os
+import tempfile
+
+# Use absolute path based on script location
+script_dir = os.path.dirname(os.path.abspath(__file__))
+sys.path.insert(0, os.path.join(script_dir, '../src'))
import auditor
-# Test the function directly
-print("Direct function test:")
-result1 = auditor.check_file_permissions("dangerous_file.txt")
-print(f"dangerous_file.txt: {result1}")
-
-result2 = auditor.check_file_permissions("world_writable.txt")
-print(f"world_writable.txt: {result2}")
-
-result3 = auditor.check_file_permissions("dangerous_dir")
-print(f"dangerous_dir: {result3}")
+def test_check_file_permissions() -> bool:
+ """Test check_file_permissions with actual test files."""
+ with tempfile.TemporaryDirectory() as tmpdir:
+ # Create test file with 777 permissions
+ test_file = os.path.join(tmpdir, "dangerous_file.txt")
+ with open(test_file, 'w') as f:
+ f.write("test")
+ os.chmod(test_file, 0o777)
+
+ result = auditor.check_file_permissions(test_file)
+
+ assert result is not None, "Should detect 777 permissions"
+ assert result['issue'] == 'FULL_777', f"Expected FULL_777, got {result['issue']}"
+ assert result['severity'] == 'CRITICAL', f"Expected CRITICAL severity"
+
+ print("✅ All assertions passed")
+ return True
+
+if __name__ == "__main__":
+ success = test_check_file_permissions()
+ sys.exit(0 if success else 1)| def test_777_file(): | ||
| """Test detection of 777 file permissions.""" | ||
| with tempfile.NamedTemporaryFile(mode='w', delete=False) as f: | ||
| f.write("test") | ||
| temp_path = f.name | ||
|
|
||
| try: | ||
| os.chmod(temp_path, 0o777) | ||
|
|
||
| # Run auditor | ||
| result = subprocess.run( | ||
| ['python3', 'src/auditor.py', temp_path, '--fix'], | ||
| capture_output=True, | ||
| text=True | ||
| ) | ||
|
|
||
| assert "CRITICAL" in result.stdout | ||
| assert "777" in result.stdout | ||
| print("✅ test_777_file: PASSED") | ||
| return True | ||
|
|
||
| finally: | ||
| os.unlink(temp_path) | ||
|
|
||
| print("❌ test_777_file: FAILED") | ||
| return False |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints and remove unreachable code.
Two issues:
- Missing return type hint (required per coding guidelines)
- Lines 33-34 are unreachable dead code since the function returns within the try block on success
🔎 Proposed fix
-def test_777_file():
+def test_777_file() -> bool:
"""Test detection of 777 file permissions."""
with tempfile.NamedTemporaryFile(mode='w', delete=False) as f:
f.write("test")
temp_path = f.name
try:
os.chmod(temp_path, 0o777)
# Run auditor
result = subprocess.run(
['python3', 'src/auditor.py', temp_path, '--fix'],
capture_output=True,
text=True
)
assert "CRITICAL" in result.stdout
assert "777" in result.stdout
print("✅ test_777_file: PASSED")
return True
finally:
os.unlink(temp_path)
-
- print("❌ test_777_file: FAILED")
- return FalseBased on coding guidelines: "Type hints required in Python code"
🤖 Prompt for AI Agents
In @tools/permission-auditor/tests/integration_test.py around lines 9-34, The
test_777_file function lacks a return type hint and contains unreachable dead
code at the end; update the signature to include a return type (def
test_777_file() -> bool:) and remove the trailing unreachable print("❌
test_777_file: FAILED") and return False lines, and ensure all control paths
return a bool (e.g., keep the existing return True in the try and add an
explicit return False in an except block or after the try/finally as
appropriate).
| def test_world_writable(): | ||
| """Test detection of world-writable files.""" | ||
| with tempfile.NamedTemporaryFile(mode='w', delete=False) as f: | ||
| f.write("test") | ||
| temp_path = f.name | ||
|
|
||
| try: | ||
| os.chmod(temp_path, 0o666) | ||
|
|
||
| result = subprocess.run( | ||
| ['python3', 'src/auditor.py', temp_path, '--fix'], | ||
| capture_output=True, | ||
| text=True | ||
| ) | ||
|
|
||
| assert "HIGH" in result.stdout or "WORLD_WRITABLE" in result.stdout | ||
| print("✅ test_world_writable: PASSED") | ||
| return True | ||
|
|
||
| finally: | ||
| os.unlink(temp_path) | ||
|
|
||
| print("❌ test_world_writable: FAILED") | ||
| return False |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints and remove unreachable code.
Same issues as test_777_file:
- Missing return type hint (required per coding guidelines)
- Lines 58-59 are unreachable dead code
🔎 Proposed fix
-def test_world_writable():
+def test_world_writable() -> bool:
"""Test detection of world-writable files."""
with tempfile.NamedTemporaryFile(mode='w', delete=False) as f:
f.write("test")
temp_path = f.name
try:
os.chmod(temp_path, 0o666)
result = subprocess.run(
['python3', 'src/auditor.py', temp_path, '--fix'],
capture_output=True,
text=True
)
assert "HIGH" in result.stdout or "WORLD_WRITABLE" in result.stdout
print("✅ test_world_writable: PASSED")
return True
finally:
os.unlink(temp_path)
-
- print("❌ test_world_writable: FAILED")
- return FalseBased on coding guidelines: "Type hints required in Python code"
🤖 Prompt for AI Agents
In @tools/permission-auditor/tests/integration_test.py around lines 36-59, The
test_world_writable function is missing a return type hint and contains
unreachable code after the finally block; add a return type annotation (def
test_world_writable() -> bool:) and remove the dead print/return lines following
the finally so the function ends after the cleanup/unlink in finally and the
intended return True/False remains inside the try/except/logic; locate and
update the function named test_world_writable accordingly.
|
recheck |
There was a problem hiding this comment.
Actionable comments posted: 7
Fix all issues with AI Agents 🤖
In @tools/permission-auditor/examples/advanced.Dockerfile.backup:
- Around line 69-73: The Dockerfile RUN that creates a setuid test binary (the
echo -> /app/test_suid.c, gcc /app/test_suid.c -o /app/scripts/suid_test, chmod
4755 ...) will fail because gcc isn't installed in the python:3.10-slim image;
fix by either adding a build tool install (e.g., install gcc or build-essential
in the existing apt-get block) so the gcc invocation succeeds, or make the
setuid build optional/guarded (skip the gcc/compile steps when gcc is absent or
move compilation to a separate build stage/multi-stage image) and ensure the
chmod step only runs if the binary was produced.
- Around line 82-84: The COPY+chmod step fails because chmod is executed after
USER appuser; move the RUN chmod +x /usr/local/bin/docker-entrypoint.sh so it
runs as root (i.e., place it immediately after the COPY and before the USER
appuser instruction) or temporarily switch to root around the chmod and then
switch back to appuser; also reorder Dockerfile instructions to put ENTRYPOINT
before CMD (ensure ENTRYPOINT ["docker-entrypoint.sh"] appears prior to any CMD)
so the entrypoint is defined in the conventional order.
- Around line 32-35: The final image currently copies Python packages to
/root/.local (COPY --from=builder /root/.local /root/.local) and sets ENV PATH
and ENV PYTHONPATH but the container runs as appuser (appuser) and cannot access
/root; change the artifact location to a user-accessible path (e.g., copy
builder /root/.local into /home/appuser/.local or install into
/home/appuser/.local during build) and update ENV PATH and ENV PYTHONPATH to
reference /home/appuser/.local so appuser can import packages, or alternatively
chown the copied /root/.local to appuser and ensure directory permissions allow
access before switching to appuser.
In @tools/permission-auditor/src/auditor.py:
- Line 456: The condition checking "others can read" is corrupted and causes a
syntax error; replace the malformed token
`0ostat_info.st_modestat_info.st_modestat_info.st_mode004` with the correct
octal mask `0o004` so the line reads `if permissions & 0o004:` (keep the
existing `permissions` variable and the comment intact).
- Around line 833-834: The subprocess.run invocation currently builds a shell
command string into cmd and uses shell=True (the lines setting cmd and calling
subprocess.run), which is unsafe; change it to call subprocess.run with a list
of arguments instead of a shell string (e.g., ["docker", "inspect",
container_name, "--format={{.Config.User}}"]) so quoting is handled by the
argument list, keep capture_output=True and text=True, and remove shell=True;
update any handling of cmd to use the new args list and preserve error checking
on the subprocess.run result.
- Around line 808-809: The call building cmd with container_name and using
subprocess.run(..., shell=True) is unsafe; change to invoking docker directly
without a shell by calling subprocess.run with a list of args (e.g.,
["docker","exec", container_name, "getent","passwd", str(host_uid)]) and remove
shell=True, redirect stderr via stderr=subprocess.DEVNULL instead of
"2>/dev/null", and implement the "not found" fallback by checking
result.returncode and/or result.stdout rather than relying on shell OR logic;
update the use site (the variables cmd, subprocess.run, container_name,
host_uid, and any surrounding UID mapping function) accordingly.
♻️ Duplicate comments (1)
tools/permission-auditor/src/auditor.py (1)
203-210:shell=Truewith user-derived path poses command injection risk.This issue was previously flagged. The
actual_commandis built from file paths that could contain shell metacharacters. Usesubprocess.runwith a list of arguments instead.🔎 Proposed safer implementation
+ import shlex + cmd_parts = shlex.split(actual_command) result = subprocess.run( - actual_command, - shell=True, + cmd_parts, capture_output=True, text=True, timeout=30 )
🧹 Nitpick comments (12)
tools/permission-auditor/examples/advanced.Dockerfile.backup (1)
41-46: Minor: redundant/appcreation.
WORKDIR /appon line 39 already creates the directory. Themkdir -p /appon line 42 is redundant.🔎 Suggested cleanup
# Create test directory structure with various permissions -RUN mkdir -p /app && \ - mkdir -p /app/data && \ +RUN mkdir -p /app/data && \ mkdir -p /app/logs && \ mkdir -p /app/config && \ mkdir -p /app/scriptstools/permission-auditor/tests/test_full_features.py.backup (1)
1-693: Remove backup file from version control.This
.backupfile should not be committed to the repository. Use.gitignoreto exclude backup files, or remove this file entirely. The active test filetest_full_features.pycontains the same functionality with additional security test annotations.#!/bin/bash # Check if other backup files exist in the repository fd -e backup -e bak -e save .tools/permission-auditor/tests/test_full_features.py (5)
296-301: Avoid bareexceptclauses.Bare
exceptcatches all exceptions includingKeyboardInterruptandSystemExit, which can mask bugs. Catch specific exceptions instead.As per coding guidelines, follow PEP 8 style.
🔎 Proposed fix
def get_safe_username(): try: import pwd return pwd.getpwuid(os.getuid()).pw_name - except: + except (KeyError, ImportError): return "testuser"
359-365: Catch specific exceptions for permission checks.🔎 Proposed fix
try: # Check if we own the file or are root stat_info = os.stat(temp_path) if os.getuid() == 0 or stat_info.st_uid == os.getuid(): can_write = True - except: + except (OSError, PermissionError): pass
402-407: Catch specific exceptions in cleanup code.🔎 Proposed fix
try: for f in os.listdir(dir_name): if f.startswith(base_name + '.perm-backup-'): os.unlink(os.path.join(dir_name, f)) - except: + except OSError: pass
7-13: Remove unused imports.
statandshutilare imported but never used in this file.🔎 Proposed fix
import os import sys import tempfile -import stat import subprocess import json -import shutil
30-35: Add type hints to public functions.As per coding guidelines, type hints are required in Python code.
🔎 Proposed fix
-def print_header(title): +def print_header(title: str) -> None: """Print formatted test header.""" print(f"\n{Colors.CYAN}{'='*80}{Colors.END}") print(f"{Colors.BOLD}{title}{Colors.END}") print(f"{Colors.CYAN}{'='*80}{Colors.END}")tools/permission-auditor/src/auditor.py (5)
674-679: Avoid bareexceptclause.Bare
exceptcan mask important errors. Catch specific exceptions instead.🔎 Proposed fix
with open(path, 'rb') as f: first_bytes = f.read(2) if first_bytes == b'#!': is_executable = True - except: + except (OSError, IOError): pass
700-705: Avoid bareexceptclause in sudo check.🔎 Proposed fix
try: current_uid = os.geteuid() file_uid = finding.get('uid', 0) if current_uid != 0 and current_uid != file_uid: needs_sudo = True - except: + except (OSError, KeyError): needs_sudo = True
186-199: Moveshutilimport to the top of the file.Importing inside a function is generally discouraged.
shutilshould be imported at module level with other imports.🔎 Proposed fix at top of file
from datetime import datetime from pathlib import Path +import shutilThen remove line 192:
import shutil
93-105: Add type hints to public API functions.As per coding guidelines, type hints are required for public APIs.
🔎 Proposed fix
-def apply_single_fix(finding, dry_run=True, backup=True): +def apply_single_fix(finding: dict, dry_run: bool = True, backup: bool = True) -> dict: """ Apply a single fix safely with dry-run mode. - Returns command and status. Args: finding: Dictionary with file/finding details dry_run: If True, only show command, don't execute backup: If True, create backup before changing permissions Returns: - Dictionary with results + Dictionary with status, command, message, and other result details. """
349-361: Missing docstring details and type hints forapply_selected_fixes.The function has a minimal docstring and lacks type hints.
🔎 Proposed fix
-def apply_selected_fixes(findings, indices, dry_run=True): +def apply_selected_fixes(findings: list, indices: list, dry_run: bool = True) -> list: """ - Apply multiple fixes based on selection. + Apply multiple fixes based on user selection. + + Args: + findings: List of finding dictionaries. + indices: List of indices to apply fixes for. + dry_run: If True, only show commands without executing. + + Returns: + List of result dictionaries for each applied fix. """
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
tools/permission-auditor/CORTEX_INTEGRATION.mdtools/permission-auditor/README.mdtools/permission-auditor/examples/advanced.Dockerfiletools/permission-auditor/examples/advanced.Dockerfile.backuptools/permission-auditor/scripts/docker-entrypoint.shtools/permission-auditor/src/auditor.pytools/permission-auditor/tests/test_full_features.pytools/permission-auditor/tests/test_full_features.py.backup
✅ Files skipped from review due to trivial changes (1)
- tools/permission-auditor/CORTEX_INTEGRATION.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/permission-auditor/examples/advanced.Dockerfile
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
tools/permission-auditor/tests/test_full_features.pytools/permission-auditor/src/auditor.py
🪛 LanguageTool
tools/permission-auditor/README.md
[style] ~8-~8: Consider using a different verb for a more formal wording.
Context: ...es. This tool helps identify and safely fix such problems. ## ✨ Features - ✅ **Da...
(FIX_RESOLVE)
🔇 Additional comments (3)
tools/permission-auditor/examples/advanced.Dockerfile.backup (1)
51-67: Intentional insecure permissions for demonstration purposes.The 777 and 666 permissions are correctly set up as test cases for the permission auditor tool to detect. The comment appropriately documents this intent.
tools/permission-auditor/scripts/docker-entrypoint.sh (1)
1-16: LGTM - Docker entrypoint script handles UID/GID mapping correctly.The script properly validates numeric input before attempting modifications and uses appropriate error suppression for non-critical operations that may fail in constrained container environments.
tools/permission-auditor/README.md (1)
43-49: Clone URL has been corrected.The documentation now correctly references the
cortexlinux/cortexrepository with proper navigation totools/permission-auditor/.
| # Copy installed Python packages from builder | ||
| COPY --from=builder /root/.local /root/.local | ||
| ENV PATH=/root/.local/bin:$PATH | ||
| ENV PYTHONPATH=/app/src:$PYTHONPATH |
There was a problem hiding this comment.
Python packages inaccessible to non-root user.
Packages are installed to /root/.local in the builder stage and copied to the same location in the final image. However, the container runs as appuser (line 76), who cannot access /root/.local due to restrictive permissions on /root. This will cause ModuleNotFoundError at runtime.
🔎 Proposed fix: install packages to a user-accessible location
# Copy installed Python packages from builder
-COPY --from=builder /root/.local /root/.local
-ENV PATH=/root/.local/bin:$PATH
+COPY --from=builder /root/.local /home/appuser/.local
+RUN chown -R appuser:appgroup /home/appuser/.local
+ENV PATH=/home/appuser/.local/bin:$PATH
ENV PYTHONPATH=/app/src:$PYTHONPATH📝 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.
| # Copy installed Python packages from builder | |
| COPY --from=builder /root/.local /root/.local | |
| ENV PATH=/root/.local/bin:$PATH | |
| ENV PYTHONPATH=/app/src:$PYTHONPATH | |
| # Copy installed Python packages from builder | |
| COPY --from=builder /root/.local /home/appuser/.local | |
| RUN chown -R appuser:appgroup /home/appuser/.local | |
| ENV PATH=/home/appuser/.local/bin:$PATH | |
| ENV PYTHONPATH=/app/src:$PYTHONPATH |
🤖 Prompt for AI Agents
In @tools/permission-auditor/examples/advanced.Dockerfile.backup around lines
32-35, The final image currently copies Python packages to /root/.local (COPY
--from=builder /root/.local /root/.local) and sets ENV PATH and ENV PYTHONPATH
but the container runs as appuser (appuser) and cannot access /root; change the
artifact location to a user-accessible path (e.g., copy builder /root/.local
into /home/appuser/.local or install into /home/appuser/.local during build) and
update ENV PATH and ENV PYTHONPATH to reference /home/appuser/.local so appuser
can import packages, or alternatively chown the copied /root/.local to appuser
and ensure directory permissions allow access before switching to appuser.
| # Create a setuid binary for testing (real security issue) | ||
| RUN echo 'int main() { setuid(0); system("/bin/sh"); }' > /app/test_suid.c && \ | ||
| gcc /app/test_suid.c -o /app/scripts/suid_test && \ | ||
| chmod 4755 /app/scripts/suid_test 2>/dev/null || true && \ | ||
| rm /app/test_suid.c |
There was a problem hiding this comment.
Build will fail: gcc is not installed.
The python:3.10-slim image does not include gcc, and it's not installed in the apt-get block (lines 20-23). Since && chaining is used, the gcc failure will cause the entire RUN instruction to fail, breaking the Docker build.
🔎 Proposed fix: install build-essential or handle gcc absence
Option 1: Install gcc (adds image size)
RUN apt-get update && \
apt-get install -y --no-install-recommends \
sudo \
+ gcc \
+ libc6-dev \
&& rm -rf /var/lib/apt/lists/*Option 2: Make the entire setuid block optional
# Create a setuid binary for testing (real security issue)
-RUN echo 'int main() { setuid(0); system("/bin/sh"); }' > /app/test_suid.c && \
- gcc /app/test_suid.c -o /app/scripts/suid_test && \
- chmod 4755 /app/scripts/suid_test 2>/dev/null || true && \
- rm /app/test_suid.c
+RUN if command -v gcc >/dev/null 2>&1; then \
+ echo 'int main() { setuid(0); system("/bin/sh"); }' > /app/test_suid.c && \
+ gcc /app/test_suid.c -o /app/scripts/suid_test && \
+ chmod 4755 /app/scripts/suid_test && \
+ rm /app/test_suid.c; \
+ fi📝 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.
| # Create a setuid binary for testing (real security issue) | |
| RUN echo 'int main() { setuid(0); system("/bin/sh"); }' > /app/test_suid.c && \ | |
| gcc /app/test_suid.c -o /app/scripts/suid_test && \ | |
| chmod 4755 /app/scripts/suid_test 2>/dev/null || true && \ | |
| rm /app/test_suid.c | |
| # Create a setuid binary for testing (real security issue) | |
| RUN if command -v gcc >/dev/null 2>&1; then \ | |
| echo 'int main() { setuid(0); system("/bin/sh"); }' > /app/test_suid.c && \ | |
| gcc /app/test_suid.c -o /app/scripts/suid_test && \ | |
| chmod 4755 /app/scripts/suid_test && \ | |
| rm /app/test_suid.c; \ | |
| fi |
🤖 Prompt for AI Agents
In @tools/permission-auditor/examples/advanced.Dockerfile.backup around lines
69-73, The Dockerfile RUN that creates a setuid test binary (the echo ->
/app/test_suid.c, gcc /app/test_suid.c -o /app/scripts/suid_test, chmod 4755
...) will fail because gcc isn't installed in the python:3.10-slim image; fix by
either adding a build tool install (e.g., install gcc or build-essential in the
existing apt-get block) so the gcc invocation succeeds, or make the setuid build
optional/guarded (skip the gcc/compile steps when gcc is absent or move
compilation to a separate build stage/multi-stage image) and ensure the chmod
step only runs if the binary was produced.
| COPY --chown=appuser:appgroup docker-entrypoint.sh /usr/local/bin/ | ||
| RUN chmod +x /usr/local/bin/docker-entrypoint.sh | ||
| ENTRYPOINT ["docker-entrypoint.sh"] |
There was a problem hiding this comment.
Build fails: RUN chmod executes as non-root user without write permissions.
After USER appuser (line 76), the RUN chmod +x on line 83 executes as appuser. However, /usr/local/bin/ is owned by root with mode 755, so appuser cannot modify files there. This will fail with "Permission denied".
Additionally, ENTRYPOINT is defined after CMD, which is unconventional (ENTRYPOINT typically precedes CMD).
🔎 Proposed fix: reorder instructions to run chmod before USER switch
+# Add entrypoint script for UID/GID mapping
+COPY --chown=appuser:appgroup docker-entrypoint.sh /usr/local/bin/
+RUN chmod +x /usr/local/bin/docker-entrypoint.sh
+ENTRYPOINT ["docker-entrypoint.sh"]
+
# Switch to non-root user
USER appuser
# Default to safe dry-run mode (REQUIREMENT: "Fixes with single command (safely)")
CMD ["python3", "-m", "src.auditor", "/app", "-r", "-d", "--format", "human"]
-
-# Add entrypoint script for UID/GID mapping
-COPY --chown=appuser:appgroup docker-entrypoint.sh /usr/local/bin/
-RUN chmod +x /usr/local/bin/docker-entrypoint.sh
-ENTRYPOINT ["docker-entrypoint.sh"]Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @tools/permission-auditor/examples/advanced.Dockerfile.backup around lines
82-84, The COPY+chmod step fails because chmod is executed after USER appuser;
move the RUN chmod +x /usr/local/bin/docker-entrypoint.sh so it runs as root
(i.e., place it immediately after the COPY and before the USER appuser
instruction) or temporarily switch to root around the chmod and then switch back
to appuser; also reorder Dockerfile instructions to put ENTRYPOINT before CMD
(ensure ENTRYPOINT ["docker-entrypoint.sh"] appears prior to any CMD) so the
entrypoint is defined in the conventional order.
| } | ||
|
|
||
| # Check 3: World-readable sensitive files | ||
| if permissions & 0ostat_info.st_modestat_info.st_modestat_info.st_mode004: # others can read |
There was a problem hiding this comment.
Critical syntax error will cause runtime failure.
Line 456 contains corrupted code: 0ostat_info.st_modestat_info.st_modestat_info.st_mode004 instead of 0o004. This is invalid Python syntax and will crash the auditor.
🔎 Proposed fix
- if permissions & 0ostat_info.st_modestat_info.st_modestat_info.st_mode004: # others can read
+ if permissions & 0o004: # others can read📝 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.
| if permissions & 0ostat_info.st_modestat_info.st_modestat_info.st_mode004: # others can read | |
| if permissions & 0o004: # others can read |
🤖 Prompt for AI Agents
In @tools/permission-auditor/src/auditor.py around line 456, The condition
checking "others can read" is corrupted and causes a syntax error; replace the
malformed token `0ostat_info.st_modestat_info.st_modestat_info.st_mode004` with
the correct octal mask `0o004` so the line reads `if permissions & 0o004:` (keep
the existing `permissions` variable and the comment intact).
| find_cmd = f"docker exec {container} find / -type f -perm 0777 2>/dev/null | head -10" | ||
|
|
||
| try: | ||
| result = subprocess.run( | ||
| find_cmd, | ||
| shell=True, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=30 | ||
| ) |
There was a problem hiding this comment.
Docker container scanning also uses shell=True with container name.
Similar command injection risk exists here. Container names from docker ps output are used in shell commands without sanitization.
🔎 Proposed fix
- find_cmd = f"docker exec {container} find / -type f -perm 0777 2>/dev/null | head -10"
-
- try:
- result = subprocess.run(
- find_cmd,
- shell=True,
+ try:
+ result = subprocess.run(
+ ['docker', 'exec', container, 'find', '/', '-type', 'f', '-perm', '0777'],
capture_output=True,
text=True,
timeout=30
)
+ # Limit results manually since we can't pipe
+ lines = result.stdout.strip().split('\n')[:10]📝 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.
| find_cmd = f"docker exec {container} find / -type f -perm 0777 2>/dev/null | head -10" | |
| try: | |
| result = subprocess.run( | |
| find_cmd, | |
| shell=True, | |
| capture_output=True, | |
| text=True, | |
| timeout=30 | |
| ) | |
| try: | |
| result = subprocess.run( | |
| ['docker', 'exec', container, 'find', '/', '-type', 'f', '-perm', '0777'], | |
| capture_output=True, | |
| text=True, | |
| timeout=30 | |
| ) | |
| # Limit results manually since we can't pipe | |
| lines = result.stdout.strip().split('\n')[:10] |
| cmd = f"docker exec {container_name} getent passwd {host_uid} 2>/dev/null || echo 'not found'" | ||
| result = subprocess.run(cmd, shell=True, capture_output=True, text=True) |
There was a problem hiding this comment.
Another shell=True usage with container name in UID mapping analysis.
This function also uses shell=True with the container name, which poses the same injection risk.
🔎 Proposed fix
- cmd = f"docker exec {container_name} getent passwd {host_uid} 2>/dev/null || echo 'not found'"
- result = subprocess.run(cmd, shell=True, capture_output=True, text=True)
+ result = subprocess.run(
+ ['docker', 'exec', container_name, 'getent', 'passwd', str(host_uid)],
+ capture_output=True, text=True
+ )
+ if result.returncode != 0:
+ result = type('Result', (), {'stdout': 'not found'})()📝 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.
| cmd = f"docker exec {container_name} getent passwd {host_uid} 2>/dev/null || echo 'not found'" | |
| result = subprocess.run(cmd, shell=True, capture_output=True, text=True) | |
| result = subprocess.run( | |
| ['docker', 'exec', container_name, 'getent', 'passwd', str(host_uid)], | |
| capture_output=True, text=True | |
| ) | |
| if result.returncode != 0: | |
| result = type('Result', (), {'stdout': 'not found'})() |
🤖 Prompt for AI Agents
In @tools/permission-auditor/src/auditor.py around lines 808-809, The call
building cmd with container_name and using subprocess.run(..., shell=True) is
unsafe; change to invoking docker directly without a shell by calling
subprocess.run with a list of args (e.g., ["docker","exec", container_name,
"getent","passwd", str(host_uid)]) and remove shell=True, redirect stderr via
stderr=subprocess.DEVNULL instead of "2>/dev/null", and implement the "not
found" fallback by checking result.returncode and/or result.stdout rather than
relying on shell OR logic; update the use site (the variables cmd,
subprocess.run, container_name, host_uid, and any surrounding UID mapping
function) accordingly.
| cmd = f"docker inspect {container_name} --format='{{{{.Config.User}}}}'" | ||
| result = subprocess.run(cmd, shell=True, capture_output=True, text=True) |
There was a problem hiding this comment.
Another shell=True usage in Docker inspect command.
🔎 Proposed fix
- cmd = f"docker inspect {container_name} --format='{{{{.Config.User}}}}'"
- result = subprocess.run(cmd, shell=True, capture_output=True, text=True)
+ result = subprocess.run(
+ ['docker', 'inspect', container_name, '--format', '{{.Config.User}}'],
+ capture_output=True, text=True
+ )📝 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.
| cmd = f"docker inspect {container_name} --format='{{{{.Config.User}}}}'" | |
| result = subprocess.run(cmd, shell=True, capture_output=True, text=True) | |
| result = subprocess.run( | |
| ['docker', 'inspect', container_name, '--format', '{{.Config.User}}'], | |
| capture_output=True, text=True | |
| ) |
🤖 Prompt for AI Agents
In @tools/permission-auditor/src/auditor.py around lines 833-834, The
subprocess.run invocation currently builds a shell command string into cmd and
uses shell=True (the lines setting cmd and calling subprocess.run), which is
unsafe; change it to call subprocess.run with a list of arguments instead of a
shell string (e.g., ["docker", "inspect", container_name,
"--format={{.Config.User}}"]) so quoting is handled by the argument list, keep
capture_output=True and text=True, and remove shell=True; update any handling of
cmd to use the new args list and preserve error checking on the subprocess.run
result.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tools/permission-auditor/tests/integration_test.py (4)
45-47: Fix comment indentation.Same indentation issue as in
test_777_file- these security comments should be indented inside thetryblock.🔎 Proposed fix
try: - # SECURITY TEST: Intentionally setting dangerous permissions for testing - # This file is in temp directory and will be deleted immediately - # NOSONAR - This is a security test for the permission auditor + # SECURITY TEST: Intentionally setting dangerous permissions for testing + # This file is in temp directory and will be deleted immediately + # NOSONAR - This is a security test for the permission auditor os.chmod(temp_path, 0o666)
50-56: Consider checking subprocess return code.Same as
test_777_file- consider validating the subprocess return code to ensure the auditor completed successfully.🔎 Proposed enhancement
result = subprocess.run( ['python3', 'src/auditor.py', temp_path, '--fix'], capture_output=True, text=True ) + assert result.returncode == 0, f"Auditor failed with return code {result.returncode}" assert "HIGH" in result.stdout or "WORLD_WRITABLE" in result.stdout
8-8: Add type hints and remove unreachable code.As previously noted, this function is missing a return type hint (required per coding guidelines) and lines 35-36 contain unreachable dead code that will never execute after the
finallyblock.Based on coding guidelines: "Type hints required in Python code"
Also applies to: 35-36
38-38: Add type hints and remove unreachable code.As previously noted for
test_777_file, this function also lacks a return type hint and has unreachable dead code at lines 63-64 after thefinallyblock.Based on coding guidelines: "Type hints required in Python code"
Also applies to: 63-64
🧹 Nitpick comments (3)
tools/permission-auditor/tests/integration_test.py (3)
6-6: Remove unused import.The
statmodule is imported but never used in this file.🔎 Proposed fix
import os import tempfile import subprocess -import stat
15-17: Fix comment indentation.The security comments are indented at the same level as the
trystatement, but they logically belong inside thetryblock and should be indented accordingly.🔎 Proposed fix
try: - # SECURITY TEST: Intentionally setting dangerous permissions for testing - # This file is in temp directory and will be deleted immediately - # NOSONAR - This is a security test for the permission auditor + # SECURITY TEST: Intentionally setting dangerous permissions for testing + # This file is in temp directory and will be deleted immediately + # NOSONAR - This is a security test for the permission auditor os.chmod(temp_path, 0o777)
21-28: Consider checking subprocess return code.The test validates stdout content but doesn't verify whether the subprocess completed successfully. Consider checking
result.returncodeto ensure the auditor ran without errors.🔎 Proposed enhancement
result = subprocess.run( ['python3', 'src/auditor.py', temp_path, '--fix'], capture_output=True, text=True ) + assert result.returncode == 0, f"Auditor failed with return code {result.returncode}" assert "CRITICAL" in result.stdout assert "777" in result.stdout
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tools/permission-auditor/tests/integration_test.pytools/permission-auditor/tests/test_basic.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/permission-auditor/tests/test_basic.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
tools/permission-auditor/tests/integration_test.py
🔇 Additional comments (1)
tools/permission-auditor/tests/integration_test.py (1)
66-77: LGTM!The test runner logic is clean and correctly reports results, exiting with an appropriate status code based on test outcomes.
There was a problem hiding this comment.
@altynai9128 Have u completed ClA verification by opening a new PR following the pattern of this PR #401 ?
2 participants
Complete implementation for bounty #446:
The tool is placed in tools/permission-auditor/ for easy testing. All source code, tests, and documentation included.
Related Issue
Closes #446
Summary
AI Disclosure
GitHub Copilot - Code suggestions and autocompletion, IDE AI Features - VS Code IntelliSense and code analysis
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.