Next release#1677
Open
jokob-sk wants to merge 4 commits into
Open
Conversation
- Implemented the REST Import plugin (rest_import.py) to handle importing data from REST APIs. - Added functionality for configurable HTTP methods, authentication types, and custom headers. - Included error handling for various HTTP response statuses and connection issues. - Created unit tests for the plugin covering header building, path resolution, MAC validation, record mapping, and authentication methods. - Ensured that module-level side effects are patched during tests to prevent live interactions.
…into next_release
Contributor
📝 WalkthroughWalkthroughAdds a new ChangesREST Import Plugin
Modal UI Fixes and Dev Docs
Sequence Diagram(s)sequenceDiagram
participant main as main()
participant process_import as process_import()
participant make_request as make_request()
participant resolve_path as resolve_path()
participant map_record as map_record()
participant Plugin_Objects as Plugin_Objects
main->>process_import: decoded import cfg (url, auth, field mappings)
process_import->>make_request: url, method, ssl_verify, auth, headers, post_body
make_request-->>process_import: parsed JSON response or None
process_import->>resolve_path: response data, device_list_path
resolve_path-->>process_import: records list or None
loop for each record
process_import->>map_record: record, cfg, mac_field, ip_field, fake_mac
map_record-->>process_import: mapped fields dict or None
process_import->>Plugin_Objects: add_object(mapped fields)
end
main->>Plugin_Objects: write_result_file()
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front/plugins/rest_import/README.md (1)
79-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the standard
### Other infosection format.The README currently uses
### Notes; plugin READMEs in this repo should use the standardized### Other infosection and fields for consistency.📝 Suggested patch
-### Notes - -- Version: 1.0.0 -- Author: `jokob-sk` -- Records with missing or invalid MAC addresses are skipped unless **Generate Fake MAC** is enabled -- Each import definition executes independently; failed imports do not block others +### Notes + +- Records with missing or invalid MAC addresses are skipped unless **Generate Fake MAC** is enabled +- Each import definition executes independently; failed imports do not block others + +### Other info + +- Version: 1.0.0 +- Author: [jokob-sk](https://github.com/jokob-sk) +- Release Date: 17-Jun-2026Based on learnings: plugin READMEs under
front/plugins/**/README.mdshould include a### Other infosection with standardized labels.🤖 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 `@front/plugins/rest_import/README.md` around lines 79 - 85, Replace the `### Notes` section header with `### Other info` to align with the standardized plugin README format used across the repository. Keep all the content (Version, Author, and the behavioral notes about MAC addresses and import definitions) exactly as is, only changing the section header name.Source: Learnings
🧹 Nitpick comments (3)
.gemini/skills/plugin-development/plugin-skill.md (1)
7-7: ⚡ Quick winAlign step 5 with imperative verb pattern for consistency.
Steps 1–4 use imperative verbs ("Assess," "Read," "Confirm," "Ask"), but step 5 uses passive phrasing ("code placed in"). For consistency and clarity, rephrase as an imperative statement.
✨ Proposed fix for consistency
-5. code placed in `front/plugins/<new plugin name>` +5. Place code in `front/plugins/<new plugin 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 @.gemini/skills/plugin-development/plugin-skill.md at line 7, In the plugin-skill.md file at step 5, the phrasing "code placed in" uses passive voice which is inconsistent with the imperative verb pattern used in steps 1-4 ("Assess," "Read," "Confirm," "Ask"). Rephrase step 5 to use an imperative verb form that matches the pattern of the previous steps, such as starting with an action verb like "Place" or "Create" instead of the passive construction.front/js/settings_utils.js (1)
1178-1178: ⚡ Quick winRemove debug console.log statement.
This debug log will execute on every form field generation, creating unnecessary console output. Please remove or comment it out before merging.
🧹 Proposed cleanup
- console.log(setType);🤖 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 `@front/js/settings_utils.js` at line 1178, Remove the console.log(setType) debug statement from the code. This statement is executing on every form field generation and creating unnecessary console output. Simply delete the line containing console.log(setType) to clean up the debug code before merging.front/php/templates/modals.php (1)
132-132: 💤 Low valueConsider moving inline styles to a CSS class.
The inline styles work correctly for enabling scroll behavior, but extracting them to a CSS class in
app.csswould improve maintainability and reusability.♻️ Proposed refactor
In
front/css/app.css, add:`#modal-form-plc` { overflow-y: auto; max-height: 60vh; padding: 0 15px; }Then update this line to:
-<div id="modal-form-plc" style="overflow-y: auto; max-height: 60vh; padding: 0 15px;"></div> +<div id="modal-form-plc"></div>🤖 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 `@front/php/templates/modals.php` at line 132, The div element with id="modal-form-plc" contains inline styles for overflow-y, max-height, and padding that should be externalized for better maintainability and reusability. Remove the style attribute from the modal-form-plc div element in the modals.php file and add a corresponding CSS rule in front/css/app.css targeting the `#modal-form-plc` selector with the three style properties (overflow-y: auto, max-height: 60vh, and padding: 0 15px).
🤖 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 `@front/plugins/rest_import/config.json`:
- Around line 470-485: The RSTIMPRT_password and RSTIMPRT_bearer_token input
fields in the configuration are currently rendering as plain text inputs, which
exposes sensitive values during editing. Modify the elementOptions for both the
RSTIMPRT_password function (around line 470) and the RSTIMPRT_bearer_token
function (around line 508) to add an input type property set to "password" in
their respective elementOptions arrays to mask the input values on screen.
In `@front/plugins/rest_import/README.md`:
- Around line 66-77: Add a language identifier to the fenced code block that
contains the OPNsense DHCP configuration example. Change the opening triple
backticks (```) to include a language identifier (```text) to comply with
markdownlint rule MD040, which requires all fenced code blocks to have a
specified language for proper syntax highlighting and linting compliance.
In `@front/plugins/rest_import/rest_import.py`:
- Around line 117-118: Before calling map_record() in the loop that iterates
through records at line 117-118, add a guard check to ensure each record is a
dictionary object. If the record is not a dict (e.g., it's a scalar or list),
skip processing that record or handle it appropriately. Apply the same guard
check to the other location mentioned in the comment around lines 232-235 where
map_record() is also called.
- Around line 59-61: The `decode_settings_base64()` function's return value is
not validated before being passed to `process_import()`, which will cause
failures when cfg.get() is called on malformed or non-dict data. Add validation
after decoding the raw_config in the loop to ensure cfg is a valid dictionary
with expected structure, then either skip invalid configs with appropriate
logging or raise an informative error before calling process_import(cfg,
plugin_objects). Apply this validation consistently to all occurrences mentioned
in the comment including the loop at lines 59-61 and the related code at lines
67-82.
- Around line 206-208: The exception handler catching
requests.exceptions.RequestException is logging the raw exception object `e`
which can leak sensitive endpoint data like URLs and query strings. Replace the
direct logging of the exception in the mylog call with a generic error message
that does not include the exception details, ensuring only safe context
information is logged without exposing the actual request exception content.
---
Outside diff comments:
In `@front/plugins/rest_import/README.md`:
- Around line 79-85: Replace the `### Notes` section header with `### Other
info` to align with the standardized plugin README format used across the
repository. Keep all the content (Version, Author, and the behavioral notes
about MAC addresses and import definitions) exactly as is, only changing the
section header name.
---
Nitpick comments:
In @.gemini/skills/plugin-development/plugin-skill.md:
- Line 7: In the plugin-skill.md file at step 5, the phrasing "code placed in"
uses passive voice which is inconsistent with the imperative verb pattern used
in steps 1-4 ("Assess," "Read," "Confirm," "Ask"). Rephrase step 5 to use an
imperative verb form that matches the pattern of the previous steps, such as
starting with an action verb like "Place" or "Create" instead of the passive
construction.
In `@front/js/settings_utils.js`:
- Line 1178: Remove the console.log(setType) debug statement from the code. This
statement is executing on every form field generation and creating unnecessary
console output. Simply delete the line containing console.log(setType) to clean
up the debug code before merging.
In `@front/php/templates/modals.php`:
- Line 132: The div element with id="modal-form-plc" contains inline styles for
overflow-y, max-height, and padding that should be externalized for better
maintainability and reusability. Remove the style attribute from the
modal-form-plc div element in the modals.php file and add a corresponding CSS
rule in front/css/app.css targeting the `#modal-form-plc` selector with the three
style properties (overflow-y: auto, max-height: 60vh, and padding: 0 15px).
🪄 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: 9d6408b1-c7ba-4825-862e-f4950dab9ea4
📒 Files selected for processing (9)
.gemini/skills/plugin-development/plugin-skill.mdfront/css/app.cssfront/js/modal.jsfront/js/settings_utils.jsfront/php/templates/modals.phpfront/plugins/rest_import/README.mdfront/plugins/rest_import/config.jsonfront/plugins/rest_import/rest_import.pytest/plugins/test_rest_import.py
Comment on lines
+470
to
+485
| "function": "RSTIMPRT_password", | ||
| "type": { | ||
| "dataType": "string", | ||
| "elements": [ | ||
| { | ||
| "elementType": "input", | ||
| "elementOptions": [ | ||
| { | ||
| "placeholder": "Password or API Secret" | ||
| }, | ||
| { | ||
| "cssClasses": "col-sm-10" | ||
| } | ||
| ], | ||
| "transformers": [] | ||
| } |
Contributor
There was a problem hiding this comment.
Mask secret fields in the popup form.
RSTIMPRT_password and RSTIMPRT_bearer_token are currently plain text inputs, which exposes sensitive values during editing.
🔐 Suggested patch
{
"function": "RSTIMPRT_password",
"type": {
"dataType": "string",
"elements": [
{
"elementType": "input",
"elementOptions": [
+ {
+ "type": "password"
+ },
{
"placeholder": "Password or API Secret"
},
{
"cssClasses": "col-sm-10"
}
],
"transformers": []
}
]
},
@@
{
"function": "RSTIMPRT_bearer_token",
"type": {
"dataType": "string",
"elements": [
{
"elementType": "input",
"elementOptions": [
+ {
+ "type": "password"
+ },
{
"placeholder": "Bearer token"
},
{
"cssClasses": "col-sm-10"
}
],
"transformers": []
}
]
},Also applies to: 508-523
🤖 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 `@front/plugins/rest_import/config.json` around lines 470 - 485, The
RSTIMPRT_password and RSTIMPRT_bearer_token input fields in the configuration
are currently rendering as plain text inputs, which exposes sensitive values
during editing. Modify the elementOptions for both the RSTIMPRT_password
function (around line 470) and the RSTIMPRT_bearer_token function (around line
508) to add an input type property set to "password" in their respective
elementOptions arrays to mask the input values on screen.
Comment on lines
+66
to
+77
| ``` | ||
| Name: OPNsense DHCP | ||
| URL: https://firewall/api/dnsmasq/leases/search | ||
| Method: GET | ||
| Auth Type: basic | ||
| Username: admin | ||
| Device List Path: rows | ||
| MAC Address: hwaddr | ||
| IP Address: address | ||
| Device Name: hostname | ||
| Vendor: mac_info | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language to the fenced configuration block.
The fenced block at Line 66 has no language identifier, matching the markdownlint MD040 warning.
📝 Suggested patch
-```
+```text
Name: OPNsense DHCP
URL: https://firewall/api/dnsmasq/leases/search
Method: GET
Auth Type: basic
Username: admin
Device List Path: rows
MAC Address: hwaddr
IP Address: address
Device Name: hostname
Vendor: mac_info</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @front/plugins/rest_import/README.md around lines 66 - 77, Add a language
identifier to the fenced code block that contains the OPNsense DHCP
configuration example. Change the opening triple backticks () to include a language identifier (text) to comply with markdownlint rule MD040, which
requires all fenced code blocks to have a specified language for proper syntax
highlighting and linting compliance.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- cr-comment:v1:3a910b619fdb2af36c9e5dda -->
_Source: Linters/SAST tools_
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+59
to
+61
| for raw_config in import_configs: | ||
| cfg = decode_settings_base64(raw_config) | ||
| process_import(cfg, plugin_objects) |
Contributor
There was a problem hiding this comment.
Validate decoded import configs before processing.
If decode_settings_base64() returns malformed/non-dict data, process_import() will fail on cfg.get(...) and abort the run.
🛠️ Suggested patch
- for raw_config in import_configs:
- cfg = decode_settings_base64(raw_config)
- process_import(cfg, plugin_objects)
+ for idx, raw_config in enumerate(import_configs):
+ cfg = decode_settings_base64(raw_config)
+ if not isinstance(cfg, dict):
+ mylog('none', [f'[{pluginName}] Skipping import #{idx + 1} - invalid config payload'])
+ continue
+ process_import(cfg, plugin_objects)Also applies to: 67-82
🤖 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 `@front/plugins/rest_import/rest_import.py` around lines 59 - 61, The
`decode_settings_base64()` function's return value is not validated before being
passed to `process_import()`, which will cause failures when cfg.get() is called
on malformed or non-dict data. Add validation after decoding the raw_config in
the loop to ensure cfg is a valid dictionary with expected structure, then
either skip invalid configs with appropriate logging or raise an informative
error before calling process_import(cfg, plugin_objects). Apply this validation
consistently to all occurrences mentioned in the comment including the loop at
lines 59-61 and the related code at lines 67-82.
Comment on lines
+117
to
+118
| for idx, record in enumerate(records): | ||
| result = map_record(name, idx, record, cfg, mac_field, ip_field, fake_mac) |
Contributor
There was a problem hiding this comment.
Guard against non-object records before mapping.
map_record() expects a dict (record.get(...)). If the resolved list contains scalars/lists, this raises and stops processing.
🛠️ Suggested patch
for idx, record in enumerate(records):
+ if not isinstance(record, dict):
+ mylog('none', [f'[{pluginName}] Skipped record {idx} - record is not an object for "{name}"'])
+ skipped += 1
+ continue
result = map_record(name, idx, record, cfg, mac_field, ip_field, fake_mac)
if result is None:
skipped += 1
continueAlso applies to: 232-235
🤖 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 `@front/plugins/rest_import/rest_import.py` around lines 117 - 118, Before
calling map_record() in the loop that iterates through records at line 117-118,
add a guard check to ensure each record is a dictionary object. If the record is
not a dict (e.g., it's a scalar or list), skip processing that record or handle
it appropriately. Apply the same guard check to the other location mentioned in
the comment around lines 232-235 where map_record() is also called.
Comment on lines
+206
to
+208
| except requests.exceptions.RequestException as e: | ||
| mylog('none', [f'[{pluginName}] Request error for "{name}": {e}']) | ||
| return None |
Contributor
There was a problem hiding this comment.
Avoid logging raw request exception text.
RequestException messages can include full URLs/query strings, which risks leaking sensitive endpoint data into logs.
🔐 Suggested patch
- except requests.exceptions.RequestException as e:
- mylog('none', [f'[{pluginName}] Request error for "{name}": {e}'])
+ except requests.exceptions.RequestException as e:
+ mylog('none', [f'[{pluginName}] Request error for "{name}" ({e.__class__.__name__})'])
return None📝 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.
Suggested change
| except requests.exceptions.RequestException as e: | |
| mylog('none', [f'[{pluginName}] Request error for "{name}": {e}']) | |
| return None | |
| except requests.exceptions.RequestException as e: | |
| mylog('none', [f'[{pluginName}] Request error for "{name}" ({e.__class__.__name__})']) | |
| return None |
🤖 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 `@front/plugins/rest_import/rest_import.py` around lines 206 - 208, The
exception handler catching requests.exceptions.RequestException is logging the
raw exception object `e` which can leak sensitive endpoint data like URLs and
query strings. Replace the direct logging of the exception in the mylog call
with a generic error message that does not include the exception details,
ensuring only safe context information is logged without exposing the actual
request exception content.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests