Reorganize profile section: Categorized Accordion Layout for Profiles & Scan Methods#1474
Reorganize profile section: Categorized Accordion Layout for Profiles & Scan Methods#1474prbhtkumr wants to merge 5 commits intoOWASP:masterfrom
Conversation
WalkthroughThe PR restructures profile and scan method selection UI from flat checkbox lists into category-grouped accordion layouts. Backend changes reorganize HTML generation logic in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR reorganizes the “New Scan” UI by rendering Profiles and Scan Methods into categorized, collapsible accordion sections, and updates frontend selection behavior so choosing a profile auto-selects its associated scan methods.
Changes:
- Update API HTML generators to output categorized accordion markup for profiles and scan methods.
- Update frontend JS to support category “select all” checkboxes, deduplicate selected profiles, and sync profile→module selection.
- Add/adjust tests and introduce new locale strings for category titles/descriptions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
nettacker/api/core.py |
Generates new categorized accordion HTML for profiles and scan methods, including category-level select-all controls. |
nettacker/web/static/js/main.js |
Updates selection collection and adds event handlers for accordion/category selection and profile→module syncing. |
nettacker/locale/en.yaml |
Adds new i18n strings for category titles/descriptions and “Select all”. |
tests/api/test_core.py |
Updates existing API HTML expectations to match new class names/structure. |
tests/api/test_reorganized_ui.py |
Adds tests covering categorization/removal of special profile keys and basic module categorization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for profile in cat_info["profiles"]: | ||
| label_type = cat_info["label"] | ||
| res += """ | ||
| <label><input id="{0}" type="checkbox" class="checkbox checkbox-{1}-profile" data-modules="{3}"> |
There was a problem hiding this comment.
Profiles that belong to multiple categories are rendered once per category using the same id attribute (id="{profile}"). This produces duplicate DOM ids (invalid HTML) and will break selectors like $("#" + moduleId) / $("#mixed_profile") (jQuery returns the first match only), causing inconsistent checkbox syncing when a profile appears in more than one panel.
Use unique ids per rendered checkbox (e.g., prefix/suffix with the category) and store the actual profile name in a separate attribute (value or data-profile) so request-building and module-sync logic can still identify the profile reliably.
| <label><input id="{0}" type="checkbox" class="checkbox checkbox-{1}-profile" data-modules="{3}"> | |
| <label><input id="{1}_{0}" type="checkbox" class="checkbox checkbox-{1}-profile" data-profile="{0}" data-modules="{3}"> |
| $(".checkbox-brute-profile").click(function () { | ||
| if (this.id === "brute") { | ||
| $(".checkbox-sm-brute-module").prop("checked", $(this).prop("checked")); | ||
| } | ||
| }); | ||
|
|
||
| $(".checkbox-vulnerability").click(function () { | ||
| $(".checkbox-vuln-module").prop("checked", $(this).prop("checked")); | ||
| $(".checkbox-scan-profile").click(function () { | ||
| if (this.id === "scan") { | ||
| $(".checkbox-sm-scan-module").prop("checked", $(this).prop("checked")); | ||
| } | ||
| }); | ||
|
|
||
| $(".checkbox-vuln-profile").click(function () { | ||
| if (this.id === "vulnerability" || this.id === "vuln") { | ||
| $(".checkbox-sm-vuln-module").prop("checked", $(this).prop("checked")); | ||
| } | ||
| }); |
There was a problem hiding this comment.
These handlers run for every profile checkbox in the category but only do something when the checkbox id happens to be exactly scan/brute/vulnerability/vuln. In the reorganized UI, selecting profiles is already handled by the #profiles ... change handler; keeping these special-case click handlers adds dead/duplicated logic and makes behavior harder to reason about.
Consider removing these click handlers and (if needed) implement any special “select all modules” behavior via the existing category-level checkboxes or within the single change handler.
| @@ -0,0 +1,64 @@ | |||
|
|
|||
| import pytest | |||
There was a problem hiding this comment.
pytest is imported but never used in this test module. This will be flagged by ruff/flake8-style unused-import checks.
Remove the unused import pytest (or use it explicitly if needed).
| import pytest |
| # 'mixed_profile' has 'a_scan' and 'b_brute', so it should appear in both categories. | ||
| mock_load_profiles.return_value = { | ||
| "mixed_profile": ["mod1_scan", "mod2_brute"], | ||
| "only_scan": ["mod3_scan"] | ||
| } | ||
|
|
||
| result = profiles() | ||
|
|
||
| # Check if 'mixed_profile' appears in the scan category | ||
| assert 'id="mixed_profile"' in result | ||
| # Check if 'mixed_profile' is listed with its modules | ||
| assert 'data-modules="mod1_scan,mod2_brute"' in result | ||
|
|
||
| # The HTML structure should have panels for scan, brute, and vuln | ||
| assert 'id="collapse_scan"' in result | ||
| assert 'id="collapse_brute"' in result | ||
| assert 'id="collapse_vuln"' in result | ||
|
|
There was a problem hiding this comment.
test_profiles_deduplication doesn’t actually verify deduplication or correct placement within categories; it only checks for a raw id="mixed_profile" substring, which will still pass even if the profile is rendered multiple times (and it will fail if ids are made unique to avoid invalid HTML).
Consider asserting that the profile appears under the expected category sections (e.g., within collapse_scan and collapse_brute) and/or that the rendered HTML does not contain duplicate ids for the same logical profile.
| # 'mixed_profile' has 'a_scan' and 'b_brute', so it should appear in both categories. | |
| mock_load_profiles.return_value = { | |
| "mixed_profile": ["mod1_scan", "mod2_brute"], | |
| "only_scan": ["mod3_scan"] | |
| } | |
| result = profiles() | |
| # Check if 'mixed_profile' appears in the scan category | |
| assert 'id="mixed_profile"' in result | |
| # Check if 'mixed_profile' is listed with its modules | |
| assert 'data-modules="mod1_scan,mod2_brute"' in result | |
| # The HTML structure should have panels for scan, brute, and vuln | |
| assert 'id="collapse_scan"' in result | |
| assert 'id="collapse_brute"' in result | |
| assert 'id="collapse_vuln"' in result | |
| # 'mixed_profile' has 'mod1_scan' and 'mod2_brute', so it should appear in both categories. | |
| mock_load_profiles.return_value = { | |
| "mixed_profile": ["mod1_scan", "mod2_brute"], | |
| "only_scan": ["mod3_scan"] | |
| } | |
| result = profiles() | |
| # The HTML structure should have panels for scan, brute, and vuln | |
| assert 'id="collapse_scan"' in result | |
| assert 'id="collapse_brute"' in result | |
| assert 'id="collapse_vuln"' in result | |
| # Ensure the logical profile is not duplicated via multiple identical IDs | |
| assert result.count('id="mixed_profile"') == 1 | |
| # Check that 'mixed_profile' is listed with its modules | |
| assert 'data-modules="mod1_scan,mod2_brute"' in result | |
| # Verify that the mixed profile appears under both scan and brute sections | |
| scan_index = result.index('id="collapse_scan"') | |
| brute_index = result.index('id="collapse_brute"') | |
| vuln_index = result.index('id="collapse_vuln"') | |
| scan_section = result[scan_index:brute_index] | |
| brute_section = result[brute_index:vuln_index] | |
| assert "mixed_profile" in scan_section | |
| assert "mixed_profile" in brute_section |
| <a class="label label-{1}">{0}</a></label> """.format( | ||
| profile, label | ||
| <div class="panel panel-default"> | ||
| <div class="panel-heading" style="cursor: pointer;" data-toggle="collapse" data-parent="#profile_accordion" href="#collapse_{0}"> |
There was a problem hiding this comment.
The collapse toggle is implemented by putting an href attribute on a <div class="panel-heading" ... href="#collapse_{0}">, but href is not a valid attribute for div elements. This is invalid HTML and can lead to inconsistent behavior across browsers/assistive tech.
Prefer data-target="#collapse_{0}" (or wrap the clickable area in an <a> element with href) while keeping data-toggle="collapse".
| <div class="panel-heading" style="cursor: pointer;" data-toggle="collapse" data-parent="#profile_accordion" href="#collapse_{0}"> | |
| <div class="panel-heading" style="cursor: pointer;" data-toggle="collapse" data-parent="#profile_accordion" data-target="#collapse_{0}"> |
| for cat_name, cat_info in categories.items(): | ||
| res += """ | ||
| <div class="panel panel-default"> | ||
| <div class="panel-heading" style="cursor: pointer;" data-toggle="collapse" data-parent="#scan_methods_accordion" href="#collapse_sm_{0}"> |
There was a problem hiding this comment.
Same issue as above for scan methods: href is attached to a <div class="panel-heading" ...>, which is invalid HTML. Use data-target or an <a href> toggle element to keep the collapse behavior valid and consistent.
| <div class="panel-heading" style="cursor: pointer;" data-toggle="collapse" data-parent="#scan_methods_accordion" href="#collapse_sm_{0}"> | |
| <div class="panel-heading" style="cursor: pointer;" data-toggle="collapse" data-parent="#scan_methods_accordion" data-target="#collapse_sm_{0}"> |
| res += """ | ||
| <label><input id="{0}" type="checkbox" class="checkbox checkbox-{1}-profile" data-modules="{3}"> | ||
| <a class="label label-{2}">{0}</a></label> | ||
| """.format(profile, cat_name, label_type, ",".join(all_profiles[profile])) |
There was a problem hiding this comment.
profile names and the data-modules attribute value are interpolated into HTML without escaping. Since module/profile identifiers ultimately come from YAML filenames/tags, a crafted name containing quotes/angle brackets could break attributes and allow HTML/JS injection in the web UI.
Escape values used in attributes/text (e.g., via html.escape(..., quote=True)) when rendering id, link text, and data-modules.
| res += """ | ||
| <label><input id="{0}" type="checkbox" class="checkbox checkbox-sm-{1}-module"> | ||
| <a class="label label-{2}">{0}</a></label> | ||
| """.format(module, cat_name, label_type) |
There was a problem hiding this comment.
Module names are interpolated into HTML (id and visible text) without escaping. While module ids are usually derived from internal filenames, escaping here would prevent attribute breakage/XSS if a module name ever contains special characters.
Consider applying HTML/attribute escaping when rendering {module} and the id attribute.
| $("#profiles input[type='checkbox']:checked").each(function () { | ||
| if ( | ||
| this.id !== "all_profiles" && | ||
| !$(this).hasClass("check-all-category") | ||
| ) { | ||
| p[n] = this.id; | ||
| n += 1; | ||
| } | ||
| }); | ||
| // Deduplicate profiles as they might appear in multiple categories | ||
| p = Array.from(new Set(p)); | ||
| var profiles = p.join(","); |
There was a problem hiding this comment.
The profile collection logic relies on this.id and then deduplicates ids via new Set(p). With the current backend change allowing a profile to be displayed in multiple categories, the DOM ends up with duplicate ids, and the current approach can't distinguish between multiple rendered checkboxes for the same logical profile.
Once profile checkboxes use unique DOM ids, collect the logical profile identifier from a dedicated attribute (value / data-profile) instead of id, and deduplicate based on that field.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
tests/api/test_reorganized_ui.py (2)
19-21: Assertion fordata-modulesmay be order-sensitive.The assertion
'data-modules="mod1_scan,mod2_brute"'assumes a specific ordering. If the module list iteration order changes (e.g., due to dict ordering or sorting), this test could fail. Consider using a more flexible assertion or ensuring the order is deterministic.💡 Alternative assertion approach
# Check that data-modules contains both modules (order-independent) assert 'data-modules=' in result assert 'mod1_scan' in result assert 'mod2_brute' in result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/test_reorganized_ui.py` around lines 19 - 21, The test currently asserts a specific ordering with assert 'data-modules="mod1_scan,mod2_brute"' which is brittle; update the check in tests/api/test_reorganized_ui.py (the assertion around 'mixed_profile' and result) to be order-independent by either (a) asserting the presence of 'data-modules=' and individually asserting both 'mod1_scan' and 'mod2_brute' are present in result, or (b) parsing the data-modules attribute from result (e.g., extract the attribute string and split on commas) and assert the set of modules equals {'mod1_scan','mod2_brute'} so the test no longer depends on ordering.
49-51: Weak assertion forcustom_vulnerabilitymapping.The assertion
'checkbox-sm-vuln-module' in resulton line 51 only verifies the class exists somewhere in the output, not thatcustom_vulnerabilityspecifically has this class. Sinceheartbleed_vulnalso produces this class (line 47), the assertion would pass even ifcustom_vulnerabilitywasn't mapped correctly.Consider a more targeted assertion that verifies the checkbox for
custom_vulnerabilityhas the vuln class:💡 More specific assertion
# Verify custom_vulnerability is in vuln category by checking adjacent HTML assert 'id="custom_vulnerability"' in result # Check that it appears near the vuln module class (within the vuln accordion panel) assert 'collapse_sm_vuln' in result # Panel exists🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/test_reorganized_ui.py` around lines 49 - 51, Replace the weak class-only check with a targeted assertion that the checkbox element for custom_vulnerability has the vuln class: locate the 'id="custom_vulnerability"' occurrence in the rendered HTML (use that string) and assert that the vuln-specific context appears next to it (for example the same element or nearby markup contains 'checkbox-sm-vuln-module' or that it is inside the vuln panel marker like 'collapse_sm_vuln'); ensure the assertion references both 'custom_vulnerability' and 'checkbox-sm-vuln-module' (or 'collapse_sm_vuln') together so the test verifies custom_vulnerability is actually mapped to the vuln category rather than just the class existing elsewhere.nettacker/api/core.py (2)
222-241: Duplicated category definitions betweenprofiles()andscan_methods().The category structure with titles, descriptions, and labels is defined identically in both functions. Consider extracting this to a shared helper function or module-level constant to improve maintainability and ensure consistency.
♻️ Proposed refactor
def _get_module_categories(): """Return the common category structure for profiles and scan methods.""" return { "scan": { "title": _("scan_modules_title"), "desc": _("scan_modules_desc"), "label": "success", }, "brute": { "title": _("brute_modules_title"), "desc": _("brute_modules_desc"), "label": "warning", }, "vuln": { "title": _("vuln_modules_title"), "desc": _("vuln_modules_desc"), "label": "danger", }, }Then in each function, add the
profilesormoduleslist:categories = _get_module_categories() for cat in categories: categories[cat]["profiles"] = [] # or "modules" = []Also applies to: 307-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/api/core.py` around lines 222 - 241, The category dict is duplicated in profiles() and scan_methods(); extract it into a single helper (e.g., _get_module_categories) or a module-level constant and have both functions call it to get the base structure, then add the function-specific list keys (profiles or modules) to each category; update profiles() and scan_methods() to call the helper and set categories[cat]["profiles"] = [] or categories[cat]["modules"] = [] as appropriate to avoid duplication and keep titles/descriptions/labels consistent.
328-333: Inconsistent handling of "vuln" suffix between functions.In
profiles()(line 250), the condition iselif cat == "vulnerability" or cat == "vuln", but here onlyelif cat == "vulnerability"is checked. This is actually correct because"vuln" in categoriesevaluates toTrueat line 330, so modules ending with_vulnare handled in theifbranch. However, the inconsistency makes the code harder to follow.Consider aligning the logic structure for clarity:
💡 Align with profiles() pattern
for sm in sorted(methods.keys()): cat = sm.split("_")[-1] if cat in categories: categories[cat]["modules"].append(sm) - elif cat == "vulnerability": + elif cat == "vulnerability" or cat == "vuln": categories["vuln"]["modules"].append(sm)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/api/core.py` around lines 328 - 333, The loop that assigns modules to categories in the function containing the for sm in sorted(methods.keys()) block is inconsistent with profiles(): it only checks elif cat == "vulnerability" while profiles() checks (cat == "vulnerability" or cat == "vuln"); make the logic consistent by checking both "vulnerability" and "vuln" in that elif (or otherwise explicitly handle "vuln" in the same branch) so modules ending with _vuln and _vulnerability are treated the same; update the conditional around categories["vuln"]["modules"].append(sm) accordingly and keep the same naming/branching style as profiles().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nettacker/api/core.py`:
- Around line 243-251: The code that derives categories from module names (loop
over all_profiles -> modules -> cats) only checks exact suffixes and ignores
variants like "bruteforce" or "version"; update the classification logic in that
block to normalize and map suffixes using substring/startswith checks (e.g., if
suffix.startswith("brute") or "brute" in suffix -> treat as "brute"; if
suffix.startswith("scan") or "scan" in suffix -> "scan"; if "vuln" in suffix or
suffix == "vulnerability" -> "vuln"; otherwise fall back to known categories or
a default) so profiles are appended to categories["..."]["profiles"]
consistently; modify the code that computes cats from modules (referenced
variables: all_profiles, modules, cats, categories, profile) to perform this
normalized mapping rather than exact equality checks.
In `@nettacker/locale/en.yaml`:
- Around line 12-18: Add the seven missing i18n keys (scan_modules_title,
scan_modules_desc, brute_modules_title, brute_modules_desc, vuln_modules_title,
vuln_modules_desc, select_all) to each of the 23 locale YAML files (ar, bn, de,
el, es, fa, fr, hi, hy, id, it, iw, ja, ko, nl, ps, pt-br, ru, tr, ur, uz, vi,
zh-cn); for each file add the same key names and either provide the correct
translated string or, if a translation is not available yet, copy the English
value from en.yaml as a placeholder so the keys exist and the _() lookups no
longer fall back or break. Ensure YAML formatting matches existing files (key:
value) and that the seven keys are placed alongside the other module-related
entries in each locale file.
In `@nettacker/web/static/js/main.js`:
- Around line 789-812: Event handlers for .checkbox-sm-vuln-module,
.checkbox-sm-scan-module and .checkbox-sm-brute-module are trying to uncheck
non-existent IDs (`#scan`, `#brute`, `#vulnerability`, `#vuln`); update those handlers
to target the actual category-level checkbox selectors used by the
generator—i.e., use the class-based category checkbox selector (class
"check-all-sm-category" with the appropriate data-category attribute) and the
existing .checkAll / .check-all-scans classes instead of the missing IDs so
unchecking a module properly unchecks its category checkbox (locate the handlers
for .checkbox-sm-vuln-module, .checkbox-sm-scan-module and
.checkbox-sm-brute-module and replace the ID selectors with the corresponding
".check-all-sm-category[data-category='...']" selectors for each category).
- Around line 679-709: The handler bound to "#profiles input[type='checkbox']"
calls $(this).data("modules").split(",") and later uses $(this).data("modules")
inside the checked-iteration, which will throw if data-modules is missing or
empty; update the handler to defensively read the attribute (e.g., var
modulesVal = $(this).data("modules"); if (!modulesVal) continue/skip or set
modules = []; and likewise when iterating "#profiles
input[type='checkbox']:checked" only process checkboxes that have a non-empty
data("modules") before calling .split(",") (use otherModules = otherModulesVal ?
otherModulesVal.split(",") : []), so moduleId comparisons and stillNeeded logic
skip missing data-modules entries.
- Around line 711-727: Remove the dead click handlers for
".checkbox-brute-profile", ".checkbox-scan-profile", and
".checkbox-vuln-profile" (the blocks checking this.id ===
"brute"/"scan"/"vulnerability") because profile checkbox IDs are actual profile
names from profiles() and those conditions are never true; either delete these
handlers or refactor them to use the existing data attributes (data-category or
data-modules) used by the ".check-all-category" change handler and the
individual profile change handler so category-level toggles and module-syncing
continue to work correctly (refer to the selectors ".check-all-category",
".checkbox-sm-*-module", and the profiles() generator in nettacker/api/core.py
when implementing the change).
In `@tests/api/test_reorganized_ui.py`:
- Around line 1-4: Remove the leading blank line at the top of the file and
reorder imports so standard library imports come before third-party/local ones:
place "from unittest.mock import patch" before "import pytest" and "from
nettacker.api.core import profiles, scan_methods" (or simply ensure
unittest.mock is listed prior to nettacker imports); ensure import ordering
conforms to isort/ruff expectations and run the formatter to validate.
---
Nitpick comments:
In `@nettacker/api/core.py`:
- Around line 222-241: The category dict is duplicated in profiles() and
scan_methods(); extract it into a single helper (e.g., _get_module_categories)
or a module-level constant and have both functions call it to get the base
structure, then add the function-specific list keys (profiles or modules) to
each category; update profiles() and scan_methods() to call the helper and set
categories[cat]["profiles"] = [] or categories[cat]["modules"] = [] as
appropriate to avoid duplication and keep titles/descriptions/labels consistent.
- Around line 328-333: The loop that assigns modules to categories in the
function containing the for sm in sorted(methods.keys()) block is inconsistent
with profiles(): it only checks elif cat == "vulnerability" while profiles()
checks (cat == "vulnerability" or cat == "vuln"); make the logic consistent by
checking both "vulnerability" and "vuln" in that elif (or otherwise explicitly
handle "vuln" in the same branch) so modules ending with _vuln and
_vulnerability are treated the same; update the conditional around
categories["vuln"]["modules"].append(sm) accordingly and keep the same
naming/branching style as profiles().
In `@tests/api/test_reorganized_ui.py`:
- Around line 19-21: The test currently asserts a specific ordering with assert
'data-modules="mod1_scan,mod2_brute"' which is brittle; update the check in
tests/api/test_reorganized_ui.py (the assertion around 'mixed_profile' and
result) to be order-independent by either (a) asserting the presence of
'data-modules=' and individually asserting both 'mod1_scan' and 'mod2_brute' are
present in result, or (b) parsing the data-modules attribute from result (e.g.,
extract the attribute string and split on commas) and assert the set of modules
equals {'mod1_scan','mod2_brute'} so the test no longer depends on ordering.
- Around line 49-51: Replace the weak class-only check with a targeted assertion
that the checkbox element for custom_vulnerability has the vuln class: locate
the 'id="custom_vulnerability"' occurrence in the rendered HTML (use that
string) and assert that the vuln-specific context appears next to it (for
example the same element or nearby markup contains 'checkbox-sm-vuln-module' or
that it is inside the vuln panel marker like 'collapse_sm_vuln'); ensure the
assertion references both 'custom_vulnerability' and 'checkbox-sm-vuln-module'
(or 'collapse_sm_vuln') together so the test verifies custom_vulnerability is
actually mapped to the vuln category rather than just the class existing
elsewhere.
🪄 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.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5a67839-98b6-46ed-8418-55fa2d10199f
📒 Files selected for processing (5)
nettacker/api/core.pynettacker/locale/en.yamlnettacker/web/static/js/main.jstests/api/test_core.pytests/api/test_reorganized_ui.py
| for profile in sorted(all_profiles.keys()): | ||
| modules = all_profiles[profile] | ||
| cats = set(m.split("_")[-1] for m in modules) | ||
|
|
||
| for cat in cats: | ||
| if cat in categories: | ||
| categories[cat]["profiles"].append(profile) | ||
| elif cat == "vulnerability" or cat == "vuln": | ||
| categories["vuln"]["profiles"].append(profile) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find modules that don't end with _scan, _brute, _vuln, or _vulnerability
ast-grep --pattern 'load_modules()'
# Alternatively, check actual module names
fd -e yaml . nettacker/modules/ --exec basename {} .yaml | while read mod; do
suffix="${mod##*_}"
if [[ "$suffix" != "scan" && "$suffix" != "brute" && "$suffix" != "vuln" && "$suffix" != "vulnerability" ]]; then
echo "Uncategorized module: $mod"
fi
doneRepository: OWASP/Nettacker
Length of output: 5248
🏁 Script executed:
sed -n '222,241p' nettacker/api/core.pyRepository: OWASP/Nettacker
Length of output: 622
🏁 Script executed:
sed -n '243,260p' nettacker/api/core.pyRepository: OWASP/Nettacker
Length of output: 701
Module suffix mismatch causes silent exclusion from profile categorization.
The category derivation logic at line 245 (m.split("_")[-1]) extracts the last underscore-separated segment. However, the code only recognizes specific suffixes: "scan", "brute", "vuln", or "vulnerability" (lines 247-249). Modules with other suffixes (e.g., "wp_xmlrpc_bruteforce" → "bruteforce", "ssl_weak_version" → "version") are silently skipped and will not appear in the profile accordion UI. Over 180 existing modules are affected by this.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nettacker/api/core.py` around lines 243 - 251, The code that derives
categories from module names (loop over all_profiles -> modules -> cats) only
checks exact suffixes and ignores variants like "bruteforce" or "version";
update the classification logic in that block to normalize and map suffixes
using substring/startswith checks (e.g., if suffix.startswith("brute") or
"brute" in suffix -> treat as "brute"; if suffix.startswith("scan") or "scan" in
suffix -> "scan"; if "vuln" in suffix or suffix == "vulnerability" -> "vuln";
otherwise fall back to known categories or a default) so profiles are appended
to categories["..."]["profiles"] consistently; modify the code that computes
cats from modules (referenced variables: all_profiles, modules, cats,
categories, profile) to perform this normalized mapping rather than exact
equality checks.
| scan_modules_title: Scan Modules | ||
| scan_modules_desc: (Information Gathering and Reconnaissance) | ||
| brute_modules_title: Brute Force Modules | ||
| brute_modules_desc: (Authentication Attacks) | ||
| vuln_modules_title: Vulnerability Modules | ||
| vuln_modules_desc: (Exploit and Vulnerability Detection) | ||
| select_all: Select all |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which locale files are missing the new keys
for key in scan_modules_title scan_modules_desc brute_modules_title brute_modules_desc vuln_modules_title vuln_modules_desc select_all; do
echo "=== Files missing key: $key ==="
for f in $(fd -e yaml . nettacker/locale/); do
if ! grep -q "^${key}:" "$f"; then
echo " $f"
fi
done
doneRepository: OWASP/Nettacker
Length of output: 4760
Add missing i18n keys to all 23 other locale files.
All 7 new keys (scan_modules_title, scan_modules_desc, brute_modules_title, brute_modules_desc, vuln_modules_title, vuln_modules_desc, select_all) are only present in en.yaml. They are missing from all other 23 locale files (ar, bn, de, el, es, fa, fr, hi, hy, id, it, iw, ja, ko, nl, ps, pt-br, ru, tr, ur, uz, vi, zh-cn). Since the code uses _() calls to translate these strings, users with non-English locale settings will see missing translations or key fallback behavior.
Add these keys to all 23 locale files with appropriate translations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nettacker/locale/en.yaml` around lines 12 - 18, Add the seven missing i18n
keys (scan_modules_title, scan_modules_desc, brute_modules_title,
brute_modules_desc, vuln_modules_title, vuln_modules_desc, select_all) to each
of the 23 locale YAML files (ar, bn, de, el, es, fa, fr, hi, hy, id, it, iw, ja,
ko, nl, ps, pt-br, ru, tr, ur, uz, vi, zh-cn); for each file add the same key
names and either provide the correct translated string or, if a translation is
not available yet, copy the English value from en.yaml as a placeholder so the
keys exist and the _() lookups no longer fall back or break. Ensure YAML
formatting matches existing files (key: value) and that the seven keys are
placed alongside the other module-related entries in each locale file.
| $(document).on("change", "#profiles input[type='checkbox']", function () { | ||
| if ($(this).hasClass("check-all-category") || this.id === "all_profiles") { | ||
| return; | ||
| } | ||
| var modules = $(this).data("modules").split(","); | ||
| var isChecked = $(this).prop("checked"); | ||
| for (var i = 0; i < modules.length; i++) { | ||
| var moduleId = modules[i]; | ||
| if (isChecked) { | ||
| $("#" + moduleId).prop("checked", true); | ||
| } else { | ||
| // Only uncheck if no other checked profile includes this module | ||
| var stillNeeded = false; | ||
| $("#profiles input[type='checkbox']:checked").each(function () { | ||
| if ( | ||
| this.id !== "all_profiles" && | ||
| !$(this).hasClass("check-all-category") | ||
| ) { | ||
| var otherModules = $(this).data("modules").split(","); | ||
| if (otherModules.indexOf(moduleId) !== -1) { | ||
| stillNeeded = true; | ||
| return false; | ||
| } | ||
| } | ||
| }); | ||
| if (!stillNeeded) { | ||
| $("#" + moduleId).prop("checked", false); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Potential error if data-modules attribute is missing or empty.
At line 683, $(this).data("modules").split(",") will throw an error if data-modules is undefined. Similarly at line 697, accessing .data("modules") on a checkbox without this attribute returns undefined, and calling .split(",") on it will throw TypeError: Cannot read property 'split' of undefined.
This could occur if:
- A profile checkbox somehow lacks the
data-modulesattribute - The inner loop (line 692) iterates over checkboxes that aren't profile checkboxes
🛡️ Defensive fix
$(document).on("change", "#profiles input[type='checkbox']", function () {
if ($(this).hasClass("check-all-category") || this.id === "all_profiles") {
return;
}
- var modules = $(this).data("modules").split(",");
+ var modulesData = $(this).data("modules");
+ if (!modulesData) return;
+ var modules = modulesData.split(",");
var isChecked = $(this).prop("checked");
for (var i = 0; i < modules.length; i++) {
var moduleId = modules[i];
if (isChecked) {
$("#" + moduleId).prop("checked", true);
} else {
// Only uncheck if no other checked profile includes this module
var stillNeeded = false;
$("#profiles input[type='checkbox']:checked").each(function () {
if (
this.id !== "all_profiles" &&
!$(this).hasClass("check-all-category")
) {
- var otherModules = $(this).data("modules").split(",");
+ var otherData = $(this).data("modules");
+ if (!otherData) return true; // continue
+ var otherModules = otherData.split(",");
if (otherModules.indexOf(moduleId) !== -1) {
stillNeeded = true;
return false;
}
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nettacker/web/static/js/main.js` around lines 679 - 709, The handler bound to
"#profiles input[type='checkbox']" calls $(this).data("modules").split(",") and
later uses $(this).data("modules") inside the checked-iteration, which will
throw if data-modules is missing or empty; update the handler to defensively
read the attribute (e.g., var modulesVal = $(this).data("modules"); if
(!modulesVal) continue/skip or set modules = []; and likewise when iterating
"#profiles input[type='checkbox']:checked" only process checkboxes that have a
non-empty data("modules") before calling .split(",") (use otherModules =
otherModulesVal ? otherModulesVal.split(",") : []), so moduleId comparisons and
stillNeeded logic skip missing data-modules entries.
| $(".checkbox-brute-profile").click(function () { | ||
| if (this.id === "brute") { | ||
| $(".checkbox-sm-brute-module").prop("checked", $(this).prop("checked")); | ||
| } | ||
| }); | ||
|
|
||
| $(".checkbox-vulnerability").click(function () { | ||
| $(".checkbox-vuln-module").prop("checked", $(this).prop("checked")); | ||
| $(".checkbox-scan-profile").click(function () { | ||
| if (this.id === "scan") { | ||
| $(".checkbox-sm-scan-module").prop("checked", $(this).prop("checked")); | ||
| } | ||
| }); | ||
|
|
||
| $(".checkbox-vuln-profile").click(function () { | ||
| if (this.id === "vulnerability" || this.id === "vuln") { | ||
| $(".checkbox-sm-vuln-module").prop("checked", $(this).prop("checked")); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if any module yaml files define profiles with these exact names
rg -l '"scan"|"brute"|"vuln"|"vulnerability"' --type yaml nettacker/modules/ | head -20
# Also check the profile loading logic
rg -n 'profiles.*=.*\[' nettacker/modules/ --type yaml | grep -E '"(scan|brute|vuln|vulnerability)"' | head -10Repository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Find HTML templates that might define profiles
fd -type f -name "*.html" nettacker/web/ | head -20Repository: OWASP/Nettacker
Length of output: 229
🏁 Script executed:
# Search for where profiles are defined or listed
rg -n "profile" nettacker/web/static/js/main.js -A 2 -B 2 | head -50Repository: OWASP/Nettacker
Length of output: 1597
🏁 Script executed:
# Look for checkbox elements with IDs mentioned in the review
rg -n "checkbox.*brute|checkbox.*scan|checkbox.*vuln|id.*=.*['\"]brute['\"]|id.*=.*['\"]scan['\"]" nettacker/web/ --type html --type jsRepository: OWASP/Nettacker
Length of output: 1242
🏁 Script executed:
# Check if there are any profile-related data structures or definitions
rg -n "brute.*profile|scan.*profile|profile.*brute|profile.*scan" nettacker/web/ -A 2 -B 2 | head -40Repository: OWASP/Nettacker
Length of output: 754
🏁 Script executed:
# Find the HTML file that contains the profile checkboxes
fd -type f -name "*.html" nettacker/web/Repository: OWASP/Nettacker
Length of output: 229
🏁 Script executed:
# Look at the index.html file to see how profiles are structured
rg -n "checkbox.*profile|profiles" nettacker/web/static/index.html -A 3 -B 1 | head -80Repository: OWASP/Nettacker
Length of output: 503
🏁 Script executed:
# Check if there are template files that generate these checkboxes
fd -type f -name "*.html" nettacker/web/templates/Repository: OWASP/Nettacker
Length of output: 229
🏁 Script executed:
# Find Python files that might define or render profiles
fd -type f -name "*.py" nettacker/web/Repository: OWASP/Nettacker
Length of output: 229
🏁 Script executed:
# Search for where the profile variable is generated in Python
rg -n "profile.*=" nettacker/web/ -A 2 | head -100Repository: OWASP/Nettacker
Length of output: 1893
🏁 Script executed:
# Look for profile definitions or profile data
rg -n "profiles\s*=" nettacker/ --include="*.py" -A 5 | head -60Repository: OWASP/Nettacker
Length of output: 500
🏁 Script executed:
# Find Python files in nettacker/web
find nettacker/web -name "*.py"Repository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Search for where the profile template variable is set
rg -n "profile\"\|\"profile" nettacker/web/ -A 3Repository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Look for Flask/Django views that render the index template
rg -n "render.*profile\|profile.*render" nettacker/web/ -A 2Repository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Let's look at the actual index.html file to see the full profile section
cat nettacker/web/static/index.html | grep -A 100 "id=\"profiles\"" | head -120Repository: OWASP/Nettacker
Length of output: 5305
🏁 Script executed:
# Find all Python files in the web directory
find nettacker/web -name "*.py" -type fRepository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Search more broadly for where profiles are generated
rg "profile\s*=" nettacker/ -lRepository: OWASP/Nettacker
Length of output: 114
🏁 Script executed:
# Look at the engine.py file to see how profiles are generated
rg -n "profile" nettacker/api/engine.py -B 2 -A 2 | head -60Repository: OWASP/Nettacker
Length of output: 330
🏁 Script executed:
# Search for the web views/routes that render the index template
find nettacker -name "*.py" -path "*/web/*" -type f | xargs grep -l "render\|template\|index"Repository: OWASP/Nettacker
Length of output: 43
🏁 Script executed:
# Look for where the profile HTML is generated
rg -n "checkbox.*profile\|checkbox-brute\|checkbox-scan\|checkbox-vuln" nettacker/Repository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Find where the profiles() function is defined
rg -n "def profiles" nettacker/Repository: OWASP/Nettacker
Length of output: 100
🏁 Script executed:
# Check the imports in engine.py to see where profiles comes from
head -50 nettacker/api/engine.py | rg -n "from\|import"Repository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Search for profile generation across the codebase
rg -n "def profiles\|profiles\s*=" nettacker/ -A 5Repository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Look at the profiles() function implementation
sed -n '209,300p' nettacker/api/core.pyRepository: OWASP/Nettacker
Length of output: 3127
🏁 Script executed:
# Check the entire profiles() function
rg -n "def profiles" nettacker/api/core.py -A 80Repository: OWASP/Nettacker
Length of output: 3311
🏁 Script executed:
# Look for actual profile names in the modules
find nettacker/modules -name "*.py" -type f | head -5Repository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Check what actual profiles exist
rg -n "load_profiles\|all_profiles\|profile.*=" nettacker/api/core.py -B 5 -A 10 | head -60Repository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Get the Nettacker class definition to understand profile loading
rg -n "class Nettacker\|def load_profiles" nettacker/ -A 20 | head -80Repository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Find where Nettacker class is defined
find nettacker -name "*.py" -type f | xargs grep -l "class Nettacker"Repository: OWASP/Nettacker
Length of output: 80
🏁 Script executed:
# Search for .py files that might define profiles
find nettacker/modules -name "*.py" | head -10Repository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Look for JSON or YAML profile definitions
find nettacker -name "*.json" -o -name "*.yaml" -o -name "*.yml" | head -20Repository: OWASP/Nettacker
Length of output: 561
🏁 Script executed:
# Look at the Nettacker class in app.py
rg -n "def load_profiles\|class Nettacker" nettacker/core/app.py -A 30 | head -100Repository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Search for any configuration files that might list profiles
find nettacker -name "profiles*" -o -name "*profile*" | grep -v "__pycache__" | grep -v ".pyc"Repository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Look for where profiles are actually stored or referenced
rg -n "\"drupal\"\|\"wordpress\"\|\"ssl\"" nettacker/ -lRepository: OWASP/Nettacker
Length of output: 41
🏁 Script executed:
# Look at the check-all-category handler that the review mentions
rg -n "check-all-category" nettacker/web/static/js/main.js -B 2 -A 10Repository: OWASP/Nettacker
Length of output: 2165
🏁 Script executed:
# Examine the full context around lines 711-727 and 739-745
sed -n '705,750p' nettacker/web/static/js/main.jsRepository: OWASP/Nettacker
Length of output: 1401
Remove these dead code click handlers or refactor for actual use.
These handlers check conditions like this.id === "brute" and this.id === "scan", but the id attributes on profile checkboxes are set to actual profile names (e.g., "drupal", "wordpress") generated by the profiles() function in nettacker/api/core.py. The conditional checks will never be true, making the inner logic unreachable.
Category-level selection is already handled properly by the .check-all-category change handler (line 739-745), which uses the data-category attribute to select all profiles in a category. Individual profile changes are handled by the change handler on line 679, which syncs modules via the data-modules attribute.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nettacker/web/static/js/main.js` around lines 711 - 727, Remove the dead
click handlers for ".checkbox-brute-profile", ".checkbox-scan-profile", and
".checkbox-vuln-profile" (the blocks checking this.id ===
"brute"/"scan"/"vulnerability") because profile checkbox IDs are actual profile
names from profiles() and those conditions are never true; either delete these
handlers or refactor them to use the existing data attributes (data-category or
data-modules) used by the ".check-all-category" change handler and the
individual profile change handler so category-level toggles and module-syncing
continue to work correctly (refer to the selectors ".check-all-category",
".checkbox-sm-*-module", and the profiles() generator in nettacker/api/core.py
when implementing the change).
| $(document).on("click", ".checkbox-sm-vuln-module", function () { | ||
| if (!$(this).is(":checked")) { | ||
| $(".checkAll").prop("checked", false); | ||
| $(".checkbox-vulnerability").prop("checked", false); | ||
| $("#vulnerability").prop("checked", false); | ||
| $("#vuln").prop("checked", false); | ||
| $(".check-all-scans").prop("checked", false); | ||
| } | ||
| }); | ||
|
|
||
| $(".checkbox-scan-module").click(function () { | ||
| $(document).on("click", ".checkbox-sm-scan-module", function () { | ||
| if (!$(this).is(":checked")) { | ||
| $(".checkAll").prop("checked", false); | ||
| $(".checkbox-scan").prop("checked", false); | ||
| $("#scan").prop("checked", false); | ||
| $(".check-all-scans").prop("checked", false); | ||
| } | ||
| }); | ||
|
|
||
| $(".checkbox-brute-module").click(function () { | ||
| $(document).on("click", ".checkbox-sm-brute-module", function () { | ||
| if (!$(this).is(":checked")) { | ||
| $(".checkAll").prop("checked", false); | ||
| $(".checkbox-brute").prop("checked", false); | ||
| $("#brute").prop("checked", false); | ||
| $(".check-all-scans").prop("checked", false); | ||
| } | ||
| }); |
There was a problem hiding this comment.
References to non-existent element IDs #scan, #brute, #vulnerability, #vuln.
These handlers attempt to uncheck elements with IDs #scan, #brute, #vulnerability, and #vuln, but according to the HTML generation in nettacker/api/core.py:296-371, no elements with these exact IDs are created. The scan_methods() function generates checkboxes with IDs matching module names (e.g., id="ssh_brute") and category-level checkboxes use class="check-all-sm-category" with data-category attributes.
These .prop('checked', false) calls silently no-op since the selectors match nothing.
🔧 Proposed fix - use category checkbox selectors
$(document).on("click", ".checkbox-sm-vuln-module", function () {
if (!$(this).is(":checked")) {
$(".checkAll").prop("checked", false);
- $("#vulnerability").prop("checked", false);
- $("#vuln").prop("checked", false);
+ $(".check-all-sm-category[data-category='vuln']").prop("checked", false);
$(".check-all-scans").prop("checked", false);
}
});
$(document).on("click", ".checkbox-sm-scan-module", function () {
if (!$(this).is(":checked")) {
$(".checkAll").prop("checked", false);
- $("#scan").prop("checked", false);
+ $(".check-all-sm-category[data-category='scan']").prop("checked", false);
$(".check-all-scans").prop("checked", false);
}
});
$(document).on("click", ".checkbox-sm-brute-module", function () {
if (!$(this).is(":checked")) {
$(".checkAll").prop("checked", false);
- $("#brute").prop("checked", false);
+ $(".check-all-sm-category[data-category='brute']").prop("checked", false);
$(".check-all-scans").prop("checked", false);
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nettacker/web/static/js/main.js` around lines 789 - 812, Event handlers for
.checkbox-sm-vuln-module, .checkbox-sm-scan-module and .checkbox-sm-brute-module
are trying to uncheck non-existent IDs (`#scan`, `#brute`, `#vulnerability`, `#vuln`);
update those handlers to target the actual category-level checkbox selectors
used by the generator—i.e., use the class-based category checkbox selector
(class "check-all-sm-category" with the appropriate data-category attribute) and
the existing .checkAll / .check-all-scans classes instead of the missing IDs so
unchecking a module properly unchecks its category checkbox (locate the handlers
for .checkbox-sm-vuln-module, .checkbox-sm-scan-module and
.checkbox-sm-brute-module and replace the ID selectors with the corresponding
".check-all-sm-category[data-category='...']" selectors for each category).
|
|
||
| import pytest | ||
| from unittest.mock import patch | ||
| from nettacker.api.core import profiles, scan_methods |
There was a problem hiding this comment.
Fix import ordering and remove leading blank line to resolve CI failures.
The pipeline failures indicate pre-commit hooks (isort, ruff, ruff-format) modified the file. The issues are:
- Line 1: Leading blank line should be removed
- Line 3: Standard library imports (
unittest.mock) should come before third-party/local imports (nettacker)
🔧 Proposed fix
-
import pytest
+
from unittest.mock import patch
+
from nettacker.api.core import profiles, scan_methods
-from unittest.mock import patch
-from nettacker.api.core import profiles, scan_methods📝 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.
| import pytest | |
| from unittest.mock import patch | |
| from nettacker.api.core import profiles, scan_methods | |
| import pytest | |
| from unittest.mock import patch | |
| from nettacker.api.core import profiles, scan_methods |
🧰 Tools
🪛 GitHub Actions: CI/CD
[error] 1-1: pre-commit hook 'isort' failed: files were modified by this hook (ruff and ruff-format also reformat the same file).
[error] 1-1: pre-commit hook 'ruff' failed: files were modified by this hook. Found 1 error (1 fixed, 0 remaining).
[error] 1-1: pre-commit hook 'ruff-format' failed: 1 file reformatted and other files left unchanged; pre-commit hook(s) made changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/api/test_reorganized_ui.py` around lines 1 - 4, Remove the leading
blank line at the top of the file and reorder imports so standard library
imports come before third-party/local ones: place "from unittest.mock import
patch" before "import pytest" and "from nettacker.api.core import profiles,
scan_methods" (or simply ensure unittest.mock is listed prior to nettacker
imports); ensure import ordering conforms to isort/ruff expectations and run the
formatter to validate.
Proposed change
This PR reorganizes the "New Scan" interface by grouping the numerous Profiles and Scan Methods into intuitive, collapsible categories. This change significantly reduces UI clutter and improves the overall user experience, especially when dealing with a large number of available modules.
Additionally, I have synchronized the frontend selection logic to mirror the backend's behavior: selecting a profile now automatically selects its constituent scan methods in the UI.
Key Changes
UI Reorganization:
Frontend-Backend Synchronization:
Localization & Standards:
Testing:
Fixes #781
Type of change
Checklist
make pre-commitand confirm it didn't generate any warnings/changesmake test, I confirm all tests passed locallydocs/folder