Fix: guard Categories access in extract_capec_names (Issue #2487)#2517
Fix: guard Categories access in extract_capec_names (Issue #2487)#2517immortal71 wants to merge 22 commits intoOWASP:masterfrom
Conversation
Accessing catalog['Categories']['Category'] without guards caused an unhandled KeyError if the CAPEC JSON had no Categories section. Added defensive checks consistent with existing guards for Attack_Patterns and Attack_Pattern. Logs a warning and skips the categories block if the key is absent or malformed. Fixes OWASP#2488
There was a problem hiding this comment.
Pull request overview
Adds defensive handling in extract_capec_names to avoid crashing when CAPEC catalog JSON is missing or has malformed Categories data, while still extracting names from Attack_Pattern entries.
Changes:
- Guard access to
catalog["Categories"]["Category"]with presence/type checks. - Emit warnings (instead of raising) when
Categories/Categoryis missing or malformed, then continue processing.
scripts/capec_map_enricher.py
Outdated
| elif "Category" not in catalog["Categories"]: | ||
| logging.warning("No 'Category' key found in categories section") | ||
| elif not isinstance(catalog["Categories"]["Category"], list): | ||
| logging.warning("'Category' is not a list") | ||
| else: | ||
| for category in catalog["Categories"]["Category"]: | ||
| if "_ID" in category and "_Name" in category: | ||
| capec_id = int(category["_ID"]) | ||
| capec_name = category["_Name"] | ||
| capec_names[capec_id] = capec_name |
There was a problem hiding this comment.
catalog["Categories"] is assumed to be a mapping. If CAPEC JSON includes a malformed Categories value (e.g., null/None), the check "Category" not in catalog["Categories"] will raise TypeError and the function will still crash. Consider validating catalog.get("Categories") is a dict (or at least supports in/get) before checking for "Category", and log a dedicated warning when the section isn’t an object.
| elif "Category" not in catalog["Categories"]: | |
| logging.warning("No 'Category' key found in categories section") | |
| elif not isinstance(catalog["Categories"]["Category"], list): | |
| logging.warning("'Category' is not a list") | |
| else: | |
| for category in catalog["Categories"]["Category"]: | |
| if "_ID" in category and "_Name" in category: | |
| capec_id = int(category["_ID"]) | |
| capec_name = category["_Name"] | |
| capec_names[capec_id] = capec_name | |
| else: | |
| categories = catalog.get("Categories") | |
| # V2.2: Validate input structure before use to fail safely on malformed data | |
| if not isinstance(categories, dict): | |
| logging.warning("Invalid 'Categories' section in catalog; expected an object") | |
| elif "Category" not in categories: | |
| logging.warning("No 'Category' key found in categories section") | |
| else: | |
| category_entries = categories["Category"] | |
| if not isinstance(category_entries, list): | |
| logging.warning("'Category' is not a list") | |
| else: | |
| for category in category_entries: | |
| if "_ID" in category and "_Name" in category: | |
| capec_id = int(category["_ID"]) | |
| capec_name = category["_Name"] | |
| capec_names[capec_id] = capec_name |
scripts/capec_map_enricher.py
Outdated
| if "Categories" not in catalog: | ||
| logging.warning("No 'Categories' key found in catalog") | ||
| elif "Category" not in catalog["Categories"]: | ||
| logging.warning("No 'Category' key found in categories section") | ||
| elif not isinstance(catalog["Categories"]["Category"], list): | ||
| logging.warning("'Category' is not a list") | ||
| else: | ||
| for category in catalog["Categories"]["Category"]: | ||
| if "_ID" in category and "_Name" in category: | ||
| capec_id = int(category["_ID"]) | ||
| capec_name = category["_Name"] | ||
| capec_names[capec_id] = capec_name |
There was a problem hiding this comment.
The new defensive branches for missing/malformed Categories (Categories missing, Category missing, Category not a list) don’t appear to have unit tests in tests/scripts/capec_map_enricher_utest.py. Please add focused tests asserting the function returns the Attack_Pattern-derived mappings and emits the expected warnings for each of these cases.
scripts/capec_map_enricher.py
Outdated
| if "Categories" not in catalog: | ||
| logging.warning("No 'Categories' key found in catalog") | ||
| elif "Category" not in catalog["Categories"]: |
There was a problem hiding this comment.
PR description says “Unit tests covering missing/malformed Categories already exist”, but the current test suite (e.g., tests/scripts/capec_map_enricher_utest.py) doesn’t include cases for missing/malformed Categories or assertions for the new warning messages. Either update the PR description to match reality or include the missing tests as part of this PR.
…er; preserve behavior for tests
…ame_session cover, player index route test
…te_limiter prod bypass, schema changeset tests
…nd next_round, ip_helper socket branches
…ate_limiter coverage tests
…/1 to fix KeyError
… hacky multi-clause render
|
@sydseter the pr is ready for review !! |
|
@immortal71 there are a couple of good suggestions by @copilot code review[agent] that are worth implementing. Could you look at expanding the test coverage to cover the abuse cases that your code corrects? |
Apply Copilot suggestion: use catalog.get('Categories') + isinstance(dict)
check before accessing nested keys, preventing TypeError when Categories
is null or a non-dict value.
Add unit tests for all missing/malformed Categories edge cases:
- Categories key absent
- Categories is None
- Categories is non-dict (e.g. list)
- Category key missing from dict
- Category value is not a list
Closes OWASP#2487
|
@rewtd done, is this good to go ? |
Problem: scripts/capec_map_enricher.py raised an unhandled KeyError when the CAPEC JSON lacked the top-level 'Categories' section.\n\nFix: Add defensive checks to ensure 'Categories' and 'Category' exist and that 'Category' is a list before iterating. When absent or malformed, the function logs a warning and continues extracting names from 'Attack_Pattern' entries.\n\nTests: Unit tests covering missing/malformed Categories already exist and passed locally.