[fix] Validated extra file paths #400#401
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)**/*.{py,html}📄 CodeRabbit inference engine (Custom checks)
Files:
🧠 Learnings (1)📚 Learning: 2026-03-13T05:53:38.499ZApplied to files:
🔇 Additional comments (3)
📝 WalkthroughWalkthrough
Estimated code review effort: 3 (Moderate) | ~20 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (7 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 |
Extra file paths were written into generated archives and later applied by device agents. The backend now rejects malformed paths during validation so bogus archive entries do not reach downstream tools. Closes #400
ea67865 to
a70204a
Compare
Flake8 E203 Error in
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@netjsonconfig/backends/base/backend.py`:
- Around line 186-190: Update the JsonSchemaError message in the path validation
logic to state that paths may also be rejected for invalid components, including
empty, "." or ".." components, while retaining the existing allowed-character
guidance. Keep the validation behavior and error path unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a34681c3-ed02-4eba-884e-db59c07793c8
📒 Files selected for processing (2)
netjsonconfig/backends/base/backend.pytests/test_base.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Kilo Code Review
⚠️ CI failures not shown inline (4)
GitHub Actions: Netjsonconfig CI Build / Python==3.10: [fix] Validated extra file paths #400
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.10.20/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.10.20/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.20/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.20/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.20/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.10.20/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Isort check successful!
--- /home/runner/work/netjsonconfig/netjsonconfig/tests/test_base.py .731206+00:00
+++ /home/runner/work/netjsonconfig/netjsonconfig/tests/test_base.py .552740+00:00
@@ -44,11 +44,15 @@
{
"files": [
{"path": "/etc/config/network", "mode": "0644", "contents": ""},
{"path": "etc/openvpn/client.conf", "mode": "0644", "contents": ""},
{"path": "{{cert_path_abc123}}", "mode": "0644", "contents": ""},
- {"path": "/etc/{{cert_path_abc123}}", "mode": "0644", "contents": ""},
+ {
+ "path": "/etc/{{cert_path_abc123}}",
+ "mode": "0644",
+ "contents": "",
+ },
]
}
)
b.validate()
ERROR: Black check failed! Hint: did you forget to run openwisp-qa-format?
./netjsonconfig/backends/base/backend.py:203:26: E203 whitespace before ':'
ERROR: Flake8 check failed!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
File manage.py not found, skipping Make Migration Check.
##[error]Process completed with exit code 1.
GitHub Actions: Netjsonconfig CI Build / 3_Python==3.11.txt: [fix] Validated extra file paths #400
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.11.15/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Isort check successful!
--- /home/runner/work/netjsonconfig/netjsonconfig/tests/test_base.py .253388+00:00
+++ /home/runner/work/netjsonconfig/netjsonconfig/tests/test_base.py .926620+00:00
@@ -44,11 +44,15 @@
{
"files": [
{"path": "/etc/config/network", "mode": "0644", "contents": ""},
{"path": "etc/openvpn/client.conf", "mode": "0644", "contents": ""},
{"path": "{{cert_path_abc123}}", "mode": "0644", "contents": ""},
- {"path": "/etc/{{cert_path_abc123}}", "mode": "0644", "contents": ""},
+ {
+ "path": "/etc/{{cert_path_abc123}}",
+ "mode": "0644",
+ "contents": "",
+ },
]
}
)
b.validate()
ERROR: Black check failed! Hint: did you forget to run openwisp-qa-format?
./netjsonconfig/backends/base/backend.py:203:26: E203 whitespace before ':'
ERROR: Flake8 check failed!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
File manage.py not found, skipping Make Migration Check.
##[error]Process completed with exit code 1.
GitHub Actions: Netjsonconfig CI Build / 4_Python==3.12.txt: [fix] Validated extra file paths #400
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.12.13/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Isort check successful!
--- /home/runner/work/netjsonconfig/netjsonconfig/tests/test_base.py .223354+00:00
+++ /home/runner/work/netjsonconfig/netjsonconfig/tests/test_base.py .405500+00:00
@@ -44,11 +44,15 @@
{
"files": [
{"path": "/etc/config/network", "mode": "0644", "contents": ""},
{"path": "etc/openvpn/client.conf", "mode": "0644", "contents": ""},
{"path": "{{cert_path_abc123}}", "mode": "0644", "contents": ""},
- {"path": "/etc/{{cert_path_abc123}}", "mode": "0644", "contents": ""},
+ {
+ "path": "/etc/{{cert_path_abc123}}",
+ "mode": "0644",
+ "contents": "",
+ },
]
}
)
b.validate()
ERROR: Black check failed! Hint: did you forget to run openwisp-qa-format?
./netjsonconfig/backends/base/backend.py:203:26: E203 whitespace before ':'
ERROR: Flake8 check failed!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
File manage.py not found, skipping Make Migration Check.
##[error]Process completed with exit code 1.
GitHub Actions: Netjsonconfig CI Build / Python==3.12: [fix] Validated extra file paths #400
Conclusion: failure
##[group]Run ./run-qa-checks
�[36;1m./run-qa-checks�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.12.13/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib
##[endgroup]
SUCCESS: Blank endline check successful!
SUCCESS: Isort check successful!
--- /home/runner/work/netjsonconfig/netjsonconfig/tests/test_base.py .223354+00:00
+++ /home/runner/work/netjsonconfig/netjsonconfig/tests/test_base.py .405500+00:00
@@ -44,11 +44,15 @@
{
"files": [
{"path": "/etc/config/network", "mode": "0644", "contents": ""},
{"path": "etc/openvpn/client.conf", "mode": "0644", "contents": ""},
{"path": "{{cert_path_abc123}}", "mode": "0644", "contents": ""},
- {"path": "/etc/{{cert_path_abc123}}", "mode": "0644", "contents": ""},
+ {
+ "path": "/etc/{{cert_path_abc123}}",
+ "mode": "0644",
+ "contents": "",
+ },
]
}
)
b.validate()
ERROR: Black check failed! Hint: did you forget to run openwisp-qa-format?
./netjsonconfig/backends/base/backend.py:203:26: E203 whitespace before ':'
ERROR: Flake8 check failed!
SUCCESS: ReStructuredText check successful!
Commit validation: successful!
File manage.py not found, skipping Make Migration Check.
##[error]Process completed with exit code 1.
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
Mark all user-facing strings as translatable using the Django i18n framework in Django pull requests
Files:
tests/test_base.pynetjsonconfig/backends/base/backend.py
🧠 Learnings (1)
📚 Learning: 2026-03-13T05:53:38.499Z
Learnt from: stktyagi
Repo: openwisp/netjsonconfig PR: 355
File: tests/openwrt/test_default.py:0-0
Timestamp: 2026-03-13T05:53:38.499Z
Learning: Enforce importing ValidationError from netjsonconfig.exceptions instead of jsonschema.exceptions across the codebase. This aligns with project conventions (including tests like tests/openwrt/test_default.py) and should be validated by CI. When reviewing Python files, flag any imports like from jsonschema.exceptions import ValidationError and suggest from netjsonconfig.exceptions import ValidationError (or equivalent relative import) to maintain consistency.
Applied to files:
tests/test_base.pynetjsonconfig/backends/base/backend.py
🪛 GitHub Actions: Netjsonconfig CI Build / 1_Python==3.10.txt
netjsonconfig/backends/base/backend.py
[error] 203-203: Black check failed (formatting). Hint: did you forget to run openwisp-qa-format?
[error] 203-203: Flake8 check failed: E203 whitespace before ':'.
🪛 GitHub Actions: Netjsonconfig CI Build / 2_Python==3.13.txt
netjsonconfig/backends/base/backend.py
[error] 203-203: Black check failed. Hint: did you forget to run openwisp-qa-format?
[error] 203-203: Flake8 check failed: E203 whitespace before ':'.
🪛 GitHub Actions: Netjsonconfig CI Build / 3_Python==3.11.txt
netjsonconfig/backends/base/backend.py
[error] 203-203: Flake8/pycodestyle reported: E203 whitespace before ':' at line 203 column 26.
🪛 GitHub Actions: Netjsonconfig CI Build / 4_Python==3.12.txt
netjsonconfig/backends/base/backend.py
[error] 203-203: Black check failed. Hint: did you forget to run openwisp-qa-format?
[error] 203-203: Flake8 check failed: E203 whitespace before ':'.
🪛 GitHub Actions: Netjsonconfig CI Build / Python==3.10
netjsonconfig/backends/base/backend.py
[error] 203-203: Flake8 check failed: E203 whitespace before ':'
🪛 GitHub Actions: Netjsonconfig CI Build / Python==3.11
tests/test_base.py
[error] 44-49: Black check failed for step './run-qa-checks'. Hint: did you forget to run openwisp-qa-format? Formatting mismatch in test data structure (dict/list formatting around the '/etc/{{cert_path_abc123}}' entry).
netjsonconfig/backends/base/backend.py
[error] 203-203: Flake8/pycodestyle error: E203 whitespace before ':'. (at :203:26) Flake8 check failed in step './run-qa-checks'.
🪛 GitHub Actions: Netjsonconfig CI Build / Python==3.12
netjsonconfig/backends/base/backend.py
[error] 203-203: Black check failed. Hint: did you forget to run openwisp-qa-format?
[error] 203-203: Black/formatting error: E203 whitespace before ':'.
🪛 GitHub Actions: Netjsonconfig CI Build / Python==3.13
netjsonconfig/backends/base/backend.py
[error] 203-203: Black check failed. Hint: did you forget to run openwisp-qa-format?
[error] 203-203: Black/formatting reported: E203 whitespace before ':'.
🔇 Additional comments (2)
netjsonconfig/backends/base/backend.py (1)
19-22: LGTM!Also applies to: 165-167, 192-215
tests/test_base.py (1)
7-12: LGTM!Also applies to: 41-75
github-project-automation
Bot
moved this to In progress
in OpenWISP Priorities for next releases
|
coverage: 99.152% (+0.009%) from 99.143% — issues/400-reject-invalid-file-paths into master |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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)
netjsonconfig/backends/base/backend.py (2)
179-183: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject multiple leading slashes as empty path components.
lstrip("/")removes all leading separators, so//etc/passwdand///etc/passwdpass the component check._process_files()strips only one slash, leaving an absolute archive member path.Proposed fix
- components = path.lstrip("/").split("/") + components = ( + path[1:].split("/") if path.startswith("/") else path.split("/") + )🤖 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 `@netjsonconfig/backends/base/backend.py` around lines 179 - 183, Update the path component validation in _process_files so leading separators are not all removed before validation; preserve enough of the original path to detect empty components created by multiple leading slashes, while retaining valid single-leading-slash handling. Ensure paths such as //etc/passwd and ///etc/passwd are rejected before archive processing.
204-215: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate placeholder bodies before normalizing file paths.
netjsonconfig/backends/base/backend.py:204-215The current check accepts any{{...}}span, so{{}},{{foo;reboot}}, or{{foo/bar}}collapse toxand pass_file_path_re. Restrict the placeholder body to the documented\w+form, allowing only surrounding whitespace.🤖 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 `@netjsonconfig/backends/base/backend.py` around lines 204 - 215, Update the placeholder handling in the file-path validation loop to inspect the body between `{{` and `}}` before replacing it with `x`. Accept only a nonempty `\w+` placeholder with optional surrounding whitespace, reject bodies containing separators, semicolons, or other characters, and preserve the existing `_file_path_re` validation for the normalized path.
🤖 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 `@netjsonconfig/backends/base/backend.py`:
- Around line 179-183: Update the path component validation in _process_files so
leading separators are not all removed before validation; preserve enough of the
original path to detect empty components created by multiple leading slashes,
while retaining valid single-leading-slash handling. Ensure paths such as
//etc/passwd and ///etc/passwd are rejected before archive processing.
- Around line 204-215: Update the placeholder handling in the file-path
validation loop to inspect the body between `{{` and `}}` before replacing it
with `x`. Accept only a nonempty `\w+` placeholder with optional surrounding
whitespace, reject bodies containing separators, semicolons, or other
characters, and preserve the existing `_file_path_re` validation for the
normalized path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a13a5f86-e59a-409f-a0ac-d05dfe392fd6
📒 Files selected for processing (1)
netjsonconfig/backends/base/backend.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
Mark all user-facing strings as translatable using the Django i18n framework in Django pull requests
Files:
netjsonconfig/backends/base/backend.py
🧠 Learnings (1)
📚 Learning: 2026-03-13T05:53:38.499Z
Learnt from: stktyagi
Repo: openwisp/netjsonconfig PR: 355
File: tests/openwrt/test_default.py:0-0
Timestamp: 2026-03-13T05:53:38.499Z
Learning: Enforce importing ValidationError from netjsonconfig.exceptions instead of jsonschema.exceptions across the codebase. This aligns with project conventions (including tests like tests/openwrt/test_default.py) and should be validated by CI. When reviewing Python files, flag any imports like from jsonschema.exceptions import ValidationError and suggest from netjsonconfig.exceptions import ValidationError (or equivalent relative import) to maintain consistency.
Applied to files:
netjsonconfig/backends/base/backend.py
🔇 Additional comments (1)
netjsonconfig/backends/base/backend.py (1)
19-22: LGTM!Also applies to: 165-168, 188-189
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Reviewed by step-3.7-flash · Input: 65.1K · Output: 4.2K · Cached: 173.1K |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai resolve. |
github-project-automation
Bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
|
✅ Action performedComments resolved and changes approved. |
github-project-automation
Bot
moved this from Reviewer approved
to Done
in OpenWISP Priorities for next releases
2 participants
Checklist
Reference to Existing Issue
Closes #400.
Description of Changes
This change validates extra file paths during backend validation.
Malformed paths are rejected before they can be written into generated
configuration archives. Normal absolute and relative paths remain valid,
while whitespace, shell characters, empty path parts, and
.or..components are rejected.
Internal template placeholders are still accepted when they are properly
wrapped in
{{and}}, because some validation flows run before thefull runtime context is available.
Screenshot
Not applicable.