Using upload buttons for web server instead of file referencing - #1552
Using upload buttons for web server instead of file referencing#1552pUrGe12 wants to merge 12 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds API and web support for uploading scan input files, stores uploads temporarily with signed tokens, resolves them in ChangesFile Upload Feature
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/API.md`:
- Around line 78-82: The docs for the /upload/file multipart body currently say
"two fields" listing `file` and `param_name` but omit the required API key;
update the description for the /upload/file endpoint to explicitly state that an
authentication `key` (API key) must still be provided and may be sent as a
multipart form field, query parameter, or cookie according to the API auth
rules, and adjust the phrasing so the field list includes `key` alongside `file`
and `param_name` to prevent 401 confusion.
- Around line 85-100: Update the /upload/file docs to mention the 413 response
for oversized uploads: note that Flask's MAX_CONTENT_LENGTH enforces a 10 MB
limit and that requests exceeding it return HTTP 413 (Payload Too Large) in
addition to the existing HTTP 400 error cases (`upload_no_file`,
`upload_no_file_selected`, `upload_invalid_filename`,
`upload_file_type_not_allowed`); ensure the example and text around
`param_name`, `targets_list`/`usernames_list`/`passwords_list`, and
`allowed_upload_extensions` remain unchanged while adding the 413 entry to the
"Possible error responses" list.
In `@nettacker/api/engine.py`:
- Line 58: The module-level dict uploaded_files currently holds cross-request
state; change the upload flow to generate and return a unique upload_id from the
upload handler (e.g., the function handling /upload/file), store the uploaded
file(s) keyed by that upload_id in a scoped store instead of merging into
uploaded_files directly, require the client to pass that upload_id to the scan
creation handler (e.g., the function handling /new/scan) and, when creating the
new scan, pop() the store entry for that upload_id to consume it so files cannot
leak into subsequent requests; update any references to uploaded_files to use
the upload_id-based lookup and removal instead.
- Around line 255-256: Validate that the uploaded param is allowed by checking
param_name against the FILE_UPLOAD_PARAMS set before adding to uploaded_files:
in the code path where you call get_value(flask_request, "param_name") and then
access flask_request.files, ensure you only accept and store files whose
param_name is present in FILE_UPLOAD_PARAMS (reject or ignore others) so
uploaded_files cannot contain arbitrary keys that later get merged via
form_values.update(uploaded_files); apply the same validation in the identical
block around lines handling the second occurrence (the block referenced at
270-271).
In `@nettacker/web/static/js/main.js`:
- Around line 323-349: The upload handlers bound to $("#"+param+"_file") start
async AJAX requests to "/upload/file" but do not block the scan submission, so
add a simple upload-tracking mechanism: introduce a shared counter (e.g.
pendingUploads) incremented right before each $.ajax call in the change handler
for the inputs (targets_list_file, usernames_list_file, passwords_list_file,
read_from_file_file) and decremented in both .done and .fail callbacks; while
pendingUploads > 0 disable the submit control (e.g. the form submit button) and
re-enable it when pendingUploads reaches 0, and also make the form submit
handler check pendingUploads and prevent submission if >0 to cover race
conditions. Ensure the change handler still clears fileInput.value on failure
and updates statusSpan ("#"+param+"_status") as before.
🪄 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: d57c2083-2f62-4a42-856b-86490c62282c
📒 Files selected for processing (6)
docs/API.mdnettacker/api/engine.pynettacker/config.pynettacker/locale/en.yamlnettacker/web/static/index.htmlnettacker/web/static/js/main.js
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nettacker/api/engine.py (1)
330-351: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAlready-resolved upload files leak when a later key's token is invalid.
If a request supplies valid tokens for e.g.
targets_listandusernames_listbut the third key's token is invalid/expired, the function returns 400 at line 341/344/348 before reaching thetry/finallyat line 362-366. Files already appended touploaded_pathsfor the earlier keys are never deleted here — they only get swept once their own 15-minute TTL passes (line 264-286), not immediately.♻️ Wrap the whole resolution + construction in one try/finally
uploaded_paths = [] - for key in FILE_UPLOAD_PARAMS: - token = form_values.get(key) - if not token: - form_values.pop(key, None) - continue - try: - payload = _upload_token_serializer.loads(token, max_age=UPLOAD_TOKEN_TTL_SECONDS) - except BadData: - # tampered, forged, or expired token - return jsonify(structure(status="error", msg=_("upload_token_invalid"))), 400 - if not isinstance(payload, dict) or payload.get("param") != key: - # token was issued for a different parameter - return jsonify(structure(status="error", msg=_("upload_token_invalid"))), 400 - stored_name = secure_filename(payload.get("name", "")) - file_path = nettacker_path_config.tmp_dir / stored_name - if not stored_name or not file_path.is_file(): - return jsonify(structure(status="error", msg=_("upload_file_not_found"))), 400 - form_values[key] = str(file_path) - uploaded_paths.append(file_path) + try: + for key in FILE_UPLOAD_PARAMS: + token = form_values.get(key) + if not token: + form_values.pop(key, None) + continue + try: + payload = _upload_token_serializer.loads( + token, max_age=UPLOAD_TOKEN_TTL_SECONDS + ) + except BadData: + return jsonify(structure(status="error", msg=_("upload_token_invalid"))), 400 + if not isinstance(payload, dict) or payload.get("param") != key: + return jsonify(structure(status="error", msg=_("upload_token_invalid"))), 400 + stored_name = secure_filename(payload.get("name", "")) + file_path = nettacker_path_config.tmp_dir / stored_name + if not stored_name or not file_path.is_file(): + return jsonify(structure(status="error", msg=_("upload_file_not_found"))), 400 + form_values[key] = str(file_path) + uploaded_paths.append(file_path) + ... # remaining form_values setup, then Nettacker(...) inside this try + finally: + for file_path in uploaded_paths: + file_path.unlink(missing_ok=True)🤖 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 `@nettacker/api/engine.py` around lines 330 - 351, Wrap the entire upload-token resolution and subsequent request construction flow, including the FILE_UPLOAD_PARAMS loop, in the existing try/finally cleanup scope so every early invalid-token or missing-file return deletes paths already added to uploaded_paths. Preserve the current validation responses and ensure cleanup runs for all exits after uploaded_paths is initialized.
🧹 Nitpick comments (1)
nettacker/api/engine.py (1)
288-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
upload_file()lacks a docstring for this public endpoint.
new_scan()andcleanup_expired_uploads()both have docstrings; this new route handler doesn't. As per coding guidelines, "Keep functions small, use type hints where practical, and add docstrings for public APIs."Otherwise, the
param_nameallow-list check (line 293-294) correctly runs before touchingflask_request.files, addressing the earlier review comment on this endpoint.🤖 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 `@nettacker/api/engine.py` around lines 288 - 310, The public endpoint handler upload_file() lacks the required documentation. Add a concise docstring directly inside upload_file() describing its POST file-upload behavior and response purpose, without changing the existing validation or upload flow.Source: Coding guidelines
🤖 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 `@nettacker/api/engine.py`:
- Around line 61-70: Update _upload_token_serializer to use a persisted shared
secret, such as app.secret_key or the established configuration/environment
secret, instead of generating a new secrets.token_hex(32) value during process
startup. Preserve the existing salt and token TTL behavior so tokens remain
valid across workers and restarts.
---
Outside diff comments:
In `@nettacker/api/engine.py`:
- Around line 330-351: Wrap the entire upload-token resolution and subsequent
request construction flow, including the FILE_UPLOAD_PARAMS loop, in the
existing try/finally cleanup scope so every early invalid-token or missing-file
return deletes paths already added to uploaded_paths. Preserve the current
validation responses and ensure cleanup runs for all exits after uploaded_paths
is initialized.
---
Nitpick comments:
In `@nettacker/api/engine.py`:
- Around line 288-310: The public endpoint handler upload_file() lacks the
required documentation. Add a concise docstring directly inside upload_file()
describing its POST file-upload behavior and response purpose, without changing
the existing validation or upload flow.
🪄 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: 20fc2b87-97f6-4e9e-acfd-2bc6c45985d0
📒 Files selected for processing (4)
docs/API.mdnettacker/api/engine.pynettacker/locale/en.yamlnettacker/web/static/js/main.js
🚧 Files skipped from review as they are similar to previous changes (2)
- nettacker/locale/en.yaml
- docs/API.md
|
@codex review this PR using skill: https://github.com/nettacker-codex-ai/skills/blob/main/pr-review-recommender/SKILL.md |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24ece8c2b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| - [Purpose](#purpose) | ||
| - [Requests Structure](#requests-structure) | ||
| - [Upload File](#upload-file) |
There was a problem hiding this comment.
The reviewed commit 9d1383ddce47c4bc77a88ed59b1cd3e24fd59746 is unsigned (git log -1 --format='%G?' reports N), but this repository requires all commits to be signed; please rewrite or squash the PR commit with signing enabled before merge.
Useful? React with 👍 / 👎.
| for file_path in uploaded_paths: | ||
| file_path.unlink(missing_ok=True) |
There was a problem hiding this comment.
When the uploaded token is for read_from_file, the scanner has not consumed the file by the time this finally runs: initialization only validates the path, while module templates are expanded in the background thread started below. Unlinking every uploaded path here removes the wordlist before the fuzzer can read it, so web/API scans that use the new Wordlist upload fail once module execution begins; defer deleting read_from_file uploads until the scan has finished or load the contents before unlinking.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| $.extend(data, uploaded_tokens); |
There was a problem hiding this comment.
For the new usernames upload, this adds usernames_list without removing the usernames textarea value. That textarea contains whitespace by default in the page markup, and the core parser checks if options.usernames before elif options.usernames_list, so an apparently empty upload-only submission ignores the uploaded file and uses the whitespace value instead; trim/drop usernames whenever a username-list token is present.
Useful? React with 👍 / 👎.
| @app.route("/upload/file", methods=["POST"]) | ||
| def upload_file(): | ||
| api_key_is_valid(app, flask_request) | ||
| cleanup_expired_uploads() |
There was a problem hiding this comment.
Because cleanup only runs at the start of /upload/file, files from uploads that are never submitted, or superseded by selecting another file, are not removed when their 15-minute token expires unless a later upload request happens to trigger this call. The last batch of abandoned files can therefore remain in the tmp directory indefinitely, up to 10 MB each; run the sweep from a periodic/background path or delete superseded uploads explicitly.
Useful? React with 👍 / 👎.
2 participants
Proposed change
This PR adds a button to 4 sites which earlier used to reference the files in the server, for security hardening. Fixes #1550.
Type of change
Checklist
make pre-commitand confirm it didn't generate any warnings/changesmake testand I confirm all tests passed locallydocs/folder