Add standalone OWASP cheat sheet refresh script - #952
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThis PR adds ChangesCheatsheets Update Script
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
d08d662 to
716f8b2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/update-cheatsheets.sh (2)
23-24: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winBackup filename collision within the same second.
date +%Y%m%d%H%M%Shas 1-second resolution; two runs within the same second silently overwrite each other's backup.🛡️ Add uniqueness
-BACKUP_FILE="${DB_PATH}.$(date +%Y%m%d%H%M%S).bak" +BACKUP_FILE="${DB_PATH}.$(date +%Y%m%d%H%M%S)_$$.bak"🤖 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/update-cheatsheets.sh` around lines 23 - 24, The backup creation in the update-cheatsheets script can collide when run multiple times within the same second because BACKUP_FILE is based only on date +%Y%m%d%H%M%S. Update the BACKUP_FILE naming logic in the script so each run produces a unique filename, for example by adding higher-resolution time, the process ID, or another unique suffix, and keep the cp backup step using that new unique name.
14-16: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFragile dependency-check proxy.
Using
import flasksuccess as a stand-in for "all deps installed" means new/updated packages inrequirements.txtwon't get installed ifflaskis already present in the venv from a prior run.♻️ Simpler and more robust alternative
-if ! python -c "import flask" >/dev/null 2>&1; then - pip install -r "$ROOT_DIR/requirements.txt" -fi +pip install -q -r "$ROOT_DIR/requirements.txt"
pip installis idempotent and fast when nothing changed, so always running it avoids silently skipping updated dependencies.🤖 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/update-cheatsheets.sh` around lines 14 - 16, The dependency check in the update-cheatsheets.sh script is too narrow because it only tests flask via the python import gate, so updated or newly added packages in requirements.txt can be skipped. Remove the import-based conditional around the pip install step and always run the requirements installation in the script flow so dependency updates are applied reliably; the relevant logic is the shell block that wraps pip install -r "$ROOT_DIR/requirements.txt".
🤖 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.
Nitpick comments:
In `@scripts/update-cheatsheets.sh`:
- Around line 23-24: The backup creation in the update-cheatsheets script can
collide when run multiple times within the same second because BACKUP_FILE is
based only on date +%Y%m%d%H%M%S. Update the BACKUP_FILE naming logic in the
script so each run produces a unique filename, for example by adding
higher-resolution time, the process ID, or another unique suffix, and keep the
cp backup step using that new unique name.
- Around line 14-16: The dependency check in the update-cheatsheets.sh script is
too narrow because it only tests flask via the python import gate, so updated or
newly added packages in requirements.txt can be skipped. Remove the import-based
conditional around the pip install step and always run the requirements
installation in the script flow so dependency updates are applied reliably; the
relevant logic is the shell block that wraps pip install -r
"$ROOT_DIR/requirements.txt".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 4a99f980-fd4a-4622-a8a0-b5532012fd97
📒 Files selected for processing (1)
scripts/update-cheatsheets.sh
76283a2 to
cdf6c55
Compare
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/update-cheatsheets.sh`:
- Around line 8-15: Update the virtual-environment setup in
update-cheatsheets.sh to validate $VENV_DIR/bin/python rather than only the
directory, recreate the environment when that executable is missing, and use the
absolute $VENV_DIR/bin/python for Python and pip operations so system
executables cannot be selected.
- Around line 23-28: Replace the raw cp-based backup in the backup creation flow
with an existing virtualenv Python script using sqlite3.Connection.backup() to
copy DB_PATH to BACKUP_FILE, and wait for that process to complete before
reopening DB_PATH. Afterward, run PRAGMA integrity_check against BACKUP_FILE and
fail if the check is not successful, preserving the existing backup failure
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 08927877-75c1-4288-b289-949e2a2a7b1d
📒 Files selected for processing (1)
scripts/update-cheatsheets.sh
|
Heads-up: #950 (mapping fixtures) just merged. Rebase onto latest |
1 similar comment
|
Heads-up: #950 (mapping fixtures) just merged. Rebase onto latest |
|
Please rebase onto latest Also see review notes on the backup/ |
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Review — cheat sheet refresh script (#952)
Small standalone script — good split from #471. Please rebase (~28 behind) and fix the backup integrity check before merge.
Blocker
In the Python backup snippet, integrity check runs after the with sqlite3.connect(...) context exits, so dst_conn is already closed:
with sqlite3.connect(src) as src_conn, sqlite3.connect(dst) as dst_conn:
src_conn.backup(dst_conn)
cur = dst_conn.cursor() # closedMove PRAGMA integrity_check inside the with, or reopen dst for the check.
Non-blocking
- Mirror
scripts/update-cwe.shmore closely where possible (CRE_NO_NEO4J, simplercpbackup is fine for SQLite cache if you prefer) - Always
pip install -r requirements.txton every run is heavy; consider install-if-missing like other scripts (optional) - Add trailing newline at EOF
- After #950, confirm this script’s link-normalization assumptions still match cheat-sheet URL shape
Happy to approve once rebase + backup fix are in.
ca9da5b to
8d6ca21
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
- Replace SQLite online backup with a simple `cp` backup (matches update-cwe.sh) - Remove stamp-file optimisation to keep unconditional pip install - Keep environment variables to disable gap analysis and embeddings - Keep link normalization to convert GitHub URLs to official HTML pages - Ensure trailing newline at EOF This addresses all points from PR review OWASP#952.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/update-cheatsheets.sh (2)
41-52: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMatch canonical GitHub file URLs before normalizing Cheat Sheet links.
If cached rows use
/blob/master/cheatsheets/..., theLIKEselector misses them and normalizes0rows. Include that/blob/form in the selection before extracting the filename.🤖 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/update-cheatsheets.sh` around lines 41 - 52, Update the row-selection query using github_prefix so it matches both canonical GitHub tree URLs and /blob/master/cheatsheets/ file URLs before normalization. Preserve the existing filename extraction and normalization flow, ensuring cached blob-form rows are included.
44-59: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject empty filenames before constructing the HTML URL.
link LIKE prefix%also matches the exact.../cheatsheets/prefix;os.path.basename(link)then returns an empty filename andhtml_namebecomes.html. Skip links without a non-empty filename before updating the row.🤖 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/update-cheatsheets.sh` around lines 44 - 59, In the loop over rows returned by the node query, validate the basename-derived filename before constructing html_name or executing the update. Skip entries where os.path.basename(link) is empty, while preserving the existing URL conversion for links with non-empty filenames.
♻️ Duplicate comments (1)
scripts/update-cheatsheets.sh (1)
10-18:⚠️ Potential issue | 🟡 MinorReapply the virtual-environment executable check.
The current code checks only whether
"$VENV_DIR"is a directory. An incomplete environment can pass this check.sourcecan then fail, orpipandpythoncan resolve to system executables. Check"$VENV_DIR/bin/python"and invoke that absolute interpreter for pip and both Python commands. This finding duplicates the previous review comment, but it remains present in the current code.Suggested fix
-if [[ ! -d "$VENV_DIR" ]]; then +if [[ ! -x "$VENV_DIR/bin/python" ]]; then echo "Creating virtual environment in $VENV_DIR" + rm -rf -- "$VENV_DIR" python3 -m venv "$VENV_DIR" fi -source "$VENV_DIR/bin/activate" - echo "Installing Python runtime dependencies" -pip install -r "$ROOT_DIR/requirements.txt" +"$VENV_DIR/bin/python" -m pip install -r "$ROOT_DIR/requirements.txt" ... -python "$ROOT_DIR/cre.py" --cheatsheets_in --cache_file "$CACHE_FILE" +"$VENV_DIR/bin/python" "$ROOT_DIR/cre.py" --cheatsheets_in --cache_file "$CACHE_FILE" ... -python - "$CACHE_FILE" <<'PY' +"$VENV_DIR/bin/python" - "$CACHE_FILE" <<'PY'Also applies to: 29-29, 32-32
🤖 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/update-cheatsheets.sh` around lines 10 - 18, Update the virtual-environment setup check in the script to validate "$VENV_DIR/bin/python" rather than only the directory, recreating the environment when that executable is missing. Use the absolute "$VENV_DIR/bin/python" for pip installation and both Python command invocations, while preserving activation and existing behavior.
🤖 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/update-cheatsheets.sh`:
- Around line 20-23: Replace the raw cp operation in the CACHE_FILE backup block
with a SQLite-consistent backup using sqlite3.Connection.backup(), and run
PRAGMA integrity_check before or after the backup to validate it. Ensure the
backup handles live WAL state safely and preserves the existing success message
and conditional flow.
---
Outside diff comments:
In `@scripts/update-cheatsheets.sh`:
- Around line 41-52: Update the row-selection query using github_prefix so it
matches both canonical GitHub tree URLs and /blob/master/cheatsheets/ file URLs
before normalization. Preserve the existing filename extraction and
normalization flow, ensuring cached blob-form rows are included.
- Around line 44-59: In the loop over rows returned by the node query, validate
the basename-derived filename before constructing html_name or executing the
update. Skip entries where os.path.basename(link) is empty, while preserving the
existing URL conversion for links with non-empty filenames.
---
Duplicate comments:
In `@scripts/update-cheatsheets.sh`:
- Around line 10-18: Update the virtual-environment setup check in the script to
validate "$VENV_DIR/bin/python" rather than only the directory, recreating the
environment when that executable is missing. Use the absolute
"$VENV_DIR/bin/python" for pip installation and both Python command invocations,
while preserving activation and existing behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: c54d97ea-1d0a-4795-8871-449a3e611acf
📒 Files selected for processing (1)
scripts/update-cheatsheets.sh
- Replace SQLite online backup with a simple `cp` backup (matches update-cwe.sh) - Remove stamp-file optimisation to keep unconditional pip install - Keep environment variables to disable gap analysis and embeddings - Keep link normalization to convert GitHub URLs to official HTML pages - Ensure trailing newline at EOF This addresses all points from PR review OWASP#952.
d16fd2b to
6918afd
Compare
- Replace unsafe `cp` with sqlite3.Connection.backup() to handle WAL files and ensure consistency for live databases. - Move PRAGMA integrity_check inside the connection context to avoid using a closed connection (fixes the blocker from PR review OWASP#952). - Keep stamp‑file optimisation for pip install (optional but harmless). - Ensure trailing newline and preserve link-normalisation logic.
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Re-review
Prior blocker fixed — thanks. Integrity check now runs inside the dst_conn context after src_conn.backup(dst_conn), so dst_conn is still open. Online backup is a solid choice for WAL-aware SQLite.
CI green, MERGEABLE, single-file script. Optional nit: add a trailing newline at EOF.
Approve. Happy to rebase-merge when you want it landed (or after a light rebase onto latest main — currently ~11 behind, unlikely to conflict).
|
Maintainer note (2026-08-09): Merge decision on this PR is postponed ~1 week (target revisit ~2026-08-16). Context for that revisit: July call was to park #952/#953/#960 as GSoC reference (only #950 was meant to land first); #954 has since merged and already normalizes cheat sheet URLs in the parser, so the SQL normalize step here may be redundant. Options then: keep parked, shrink to backup+ No action needed from the author until we come back to it. Approval stands technically; product/stack decision is deferred. |
|
@Bornunique911 thank you for the cheat-sheet refresh script updates and for keeping CI green. Merge decision on #952 remains parked (revisit with the end-of-August #471 / fixture reshape batch, alongside the July park note). Appreciate you sticking with the focused PR pattern. |
- Replace SQLite online backup with a simple `cp` backup (matches update-cwe.sh) - Remove stamp-file optimisation to keep unconditional pip install - Keep environment variables to disable gap analysis and embeddings - Keep link normalization to convert GitHub URLs to official HTML pages - Ensure trailing newline at EOF This addresses all points from PR review OWASP#952.
- Replace unsafe `cp` with sqlite3.Connection.backup() to handle WAL files and ensure consistency for live databases. - Move PRAGMA integrity_check inside the connection context to avoid using a closed connection (fixes the blocker from PR review OWASP#952). - Keep stamp‑file optimisation for pip install (optional but harmless). - Ensure trailing newline and preserve link-normalisation logic.
a1e765d to
eedd526
Compare
2 participants
Summary
This PR is split out from the larger issue-471 review flow to make review smaller and more focused.
It adds a standalone script for refreshing OWASP Cheat Sheet data and normalizing cheat sheet links in the local cache.
Issue reference:
Problem Fixed
The earlier refresh-scripts review became too large because it was mixed with broader OWASP importer and follow-up work.
For this part of the work, the useful standalone contribution is:
Solution
This PR adds a single standalone script:
scripts/update-cheatsheets.shThe script:
--cheatsheets_inTests
Reviewer Notes
This PR is intentionally narrow because it was split to reduce review size:
This PR is meant to be reviewed as a standalone operational helper.