Render HTML scan reports inline in browser instead of downloading#1370
Render HTML scan reports inline in browser instead of downloading#1370Pbhacks wants to merge 2 commits intoOWASP:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
Summary by CodeRabbit
WalkthroughUpdated MIME mapping for ".htm"; Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 updates the WebUI results download endpoint so HTML/HTM scan reports are rendered inline in the browser (instead of forcing a download), while keeping non-HTML results as attachments, and corrects the .htm MIME type mapping.
Changes:
- Serve
.html/.htmscan results withContent-Disposition: inlineinstead ofattachment. - Add/override response headers for rendered HTML reports (CSP, nosniff).
- Fix
.htmMIME type mapping totext/html.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
nettacker/api/engine.py |
Switches inline vs attachment behavior based on extension and adds security headers for inline-rendered HTML reports. |
nettacker/api/core.py |
Fixes the .htm entry in the MIME type map to return text/html. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/engine.py`:
- Around line 398-405: The Content-Security-Policy currently allows inline
scripts via "script-src 'unsafe-inline'" which permits XSS in inline HTML
reports; update the CSP assignment in engine.py (the
headers["Content-Security-Policy"] value) to disallow inline script
execution—e.g., remove 'unsafe-inline' and set a restrictive directive such as
"script-src 'none'" (or "script-src 'self' 'nonce-...'" only if you explicitly
implement and inject nonces), so inline JavaScript cannot run; ensure you update
any related tests or report generation that relied on inline scripts if you
choose 'none'.
- Around line 391-407: Replace the ad-hoc filename sanitization that sets
safe_filename = filename.split("/")[-1] with a call to
werkzeug.utils.secure_filename (e.g., safe_filename = secure_filename(filename)
or secure_filename(os.path.basename(filename))) before building
Content-Disposition; update both the inline and attachment branches that use
safe_filename so the header value is sanitized and cannot contain backslashes,
newlines, or header-breaking characters, leaving the rest of the headers logic
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e68c9916-42ef-42a6-888e-27f3753a913a
📒 Files selected for processing (2)
nettacker/api/core.pynettacker/api/engine.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nettacker/api/engine.py (1)
395-396:⚠️ Potential issue | 🟡 MinorAdd fallback for empty
safe_filename.If
secure_filename()strips all characters (e.g., for malformed paths),safe_filenamebecomes an empty string, resulting in an invalidContent-Dispositionheader. Thesanitize_report_path_filename()function at lines 222-224 already handles this pattern.🛡️ Proposed fix
safe_filename = secure_filename(os.path.basename(filename)) + if not safe_filename: + safe_filename = f"report{ext or '.txt'}" ext = os.path.splitext(filename)[1].lower()Note:
extextraction should move before the fallback, or use a separate call:+ ext = os.path.splitext(filename)[1].lower() safe_filename = secure_filename(os.path.basename(filename)) - ext = os.path.splitext(filename)[1].lower() + if not safe_filename: + safe_filename = f"report{ext or '.txt'}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/api/engine.py` around lines 395 - 396, secure_filename(os.path.basename(filename)) can return an empty string for malformed inputs, which will produce an invalid Content-Disposition; extract the extension first (ext = os.path.splitext(filename)[1].lower()) and then compute safe_filename = secure_filename(os.path.basename(filename)); if safe_filename is empty, fallback to using sanitize_report_path_filename(filename) or construct a default name using the extracted ext; update the code around safe_filename and ext (references: secure_filename, safe_filename, ext, sanitize_report_path_filename) so the fallback occurs after ext is computed and before using safe_filename in the Content-Disposition header.
🧹 Nitpick comments (1)
nettacker/api/engine.py (1)
388-389: Consider narrowing the exception type.The static analysis tool flags the broad
Exceptioncatch. While defensive for an API endpoint, catching specific database exceptions (e.g., from apsw or SQLAlchemy) would improve debuggability without losing robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/api/engine.py` around lines 388 - 389, Replace the broad "except Exception:" handler that returns jsonify(structure(...)) with catching the concrete database exception types used by your project (e.g., apsw.DatabaseError, sqlite3.DatabaseError, or SQLAlchemyError depending on the DB layer) so only DB-related errors are handled there; import the appropriate exception class(es) at the top of nettacker/api/engine.py and change the except to "except <DBException1>, <DBException2>:" and keep the same jsonify(structure(...)) response, and optionally add a narrow fallback except Exception as e that logs the unexpected error before re-raising or returning a 500.
🤖 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/engine.py`:
- Around line 391-394: The code assumes get_scan_result() returns None or a
(filename, file_content) tuple but it can return a structure dict on SQLite APSW
errors; update the handling around result (the output of get_scan_result) to
first check if isinstance(result, dict) and if so immediately return
jsonify(result) with the appropriate error status (e.g., 500) instead of
attempting tuple unpacking; otherwise keep the existing None check and unpack
filename, file_content as before (reference get_scan_result, structure, result,
filename, file_content, jsonify).
---
Duplicate comments:
In `@nettacker/api/engine.py`:
- Around line 395-396: secure_filename(os.path.basename(filename)) can return an
empty string for malformed inputs, which will produce an invalid
Content-Disposition; extract the extension first (ext =
os.path.splitext(filename)[1].lower()) and then compute safe_filename =
secure_filename(os.path.basename(filename)); if safe_filename is empty, fallback
to using sanitize_report_path_filename(filename) or construct a default name
using the extracted ext; update the code around safe_filename and ext
(references: secure_filename, safe_filename, ext, sanitize_report_path_filename)
so the fallback occurs after ext is computed and before using safe_filename in
the Content-Disposition header.
---
Nitpick comments:
In `@nettacker/api/engine.py`:
- Around line 388-389: Replace the broad "except Exception:" handler that
returns jsonify(structure(...)) with catching the concrete database exception
types used by your project (e.g., apsw.DatabaseError, sqlite3.DatabaseError, or
SQLAlchemyError depending on the DB layer) so only DB-related errors are handled
there; import the appropriate exception class(es) at the top of
nettacker/api/engine.py and change the except to "except <DBException1>,
<DBException2>:" and keep the same jsonify(structure(...)) response, and
optionally add a narrow fallback except Exception as e that logs the unexpected
error before re-raising or returning a 500.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a30b0c7a-b237-47ba-ad32-bd9c0d780ecd
📒 Files selected for processing (2)
nettacker/api/engine.pytests/api/test_core.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nettacker/api/engine.py (1)
391-395:⚠️ Potential issue | 🟠 MajorNormalize the APSW no-row case to 404 as well.
Per
nettacker/database/db.py:579-627,get_scan_result()returns a dict in the APSW branch when no report row is found, while the SQLAlchemy branch returnsNonefor the same condition. Returning500here makes a nonexistent report ID backend-dependent instead of consistently returning404.Best fix is to normalize
get_scan_result()so both branches useNonefor “not found”, and reserve the dict path for real server errors only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/api/engine.py` around lines 391 - 395, The APSW branch of get_scan_result() returns a dict for a "no row found" case, causing engine.py to treat that as a server error (500) while the SQLAlchemy branch returns None and yields 404; update get_scan_result() so both branches return None when a report/row is not found (reserve returning a dict only for real error payloads), then in the caller in engine.py keep the existing checks: treat None as 404 and dict as an actual error response—specifically modify the APSW path inside get_scan_result() to return None instead of a dict for the no-row condition so the result normalization is consistent across backends.
🧹 Nitpick comments (1)
nettacker/api/engine.py (1)
403-421: Please add an endpoint-level regression test for these headers.The bug fix lives in
/results/get, but the added test coverage described in this PR only checks the.htmalias inmime_types(). Add an API test that exercises this route and assertsinline+ CSP for.html/.htm, andattachmentfor non-HTML files, so the actual browser behavior can’t regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/api/engine.py` around lines 403 - 421, Add an endpoint-level regression test that calls the /results/get route and asserts the Content-Disposition and Content-Security-Policy behavior: request a .html and .htm result and assert headers include Content-Disposition with "inline" and the CSP string set in engine.py, and request a non-HTML file (e.g., .txt) and assert Content-Disposition is "attachment"; exercise the actual route handler (the /results/get endpoint) rather than only testing mime_types(), and make assertions against the Response.headers returned by the Flask test client so future regressions in the Response construction in engine.py are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@nettacker/api/engine.py`:
- Around line 391-395: The APSW branch of get_scan_result() returns a dict for a
"no row found" case, causing engine.py to treat that as a server error (500)
while the SQLAlchemy branch returns None and yields 404; update
get_scan_result() so both branches return None when a report/row is not found
(reserve returning a dict only for real error payloads), then in the caller in
engine.py keep the existing checks: treat None as 404 and dict as an actual
error response—specifically modify the APSW path inside get_scan_result() to
return None instead of a dict for the no-row condition so the result
normalization is consistent across backends.
---
Nitpick comments:
In `@nettacker/api/engine.py`:
- Around line 403-421: Add an endpoint-level regression test that calls the
/results/get route and asserts the Content-Disposition and
Content-Security-Policy behavior: request a .html and .htm result and assert
headers include Content-Disposition with "inline" and the CSP string set in
engine.py, and request a non-HTML file (e.g., .txt) and assert
Content-Disposition is "attachment"; exercise the actual route handler (the
/results/get endpoint) rather than only testing mime_types(), and make
assertions against the Response.headers returned by the Flask test client so
future regressions in the Response construction in engine.py are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f611dc66-ef10-4c77-9ffb-89eb26bbedb1
📒 Files selected for processing (1)
nettacker/api/engine.py
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/database/db.py (1)
598-609:⚠️ Potential issue | 🟠 MajorKeep not-found and file-read failures distinct.
get_scan_result()already returnsNonewhenopen()fails in both backends. ReusingNonehere meansnettacker/api/engine.py:get_result_content()will now emit 404 for an existing report whose file was deleted or became unreadable. Please return a distinct error signal for read failures so 404 stays reserved for a missing DB row.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/database/db.py` around lines 598 - 609, get_scan_result() currently returns None both when the DB row is missing and when opening the report file fails; change the file-read error path (the with open(str(filename), "rb") block where logger.error(messages("read_report_fail")).format(e) is called) to raise a distinct exception (e.g., ReadReportError or IOError) or return a distinct sentinel (e.g., a tuple with an explicit error tag) instead of None so nettacker/api/engine.py:get_result_content() can distinguish "not found" from "unreadable file"; ensure the chosen signal is documented and callers are updated to handle it.
🤖 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/engine.py`:
- Around line 386-390: The except block handling get_scan_result(scan_id) is
calling logger.error() on the module instead of the Logger instance; change that
call to log.error(...) (the instance returned by log = logger.get_logger()) so
the error is logged properly and the JSON error response from
jsonify(structure(...)) is returned; update the call in the except block to use
log.error and include the scan_id in the message as before.
---
Outside diff comments:
In `@nettacker/database/db.py`:
- Around line 598-609: get_scan_result() currently returns None both when the DB
row is missing and when opening the report file fails; change the file-read
error path (the with open(str(filename), "rb") block where
logger.error(messages("read_report_fail")).format(e) is called) to raise a
distinct exception (e.g., ReadReportError or IOError) or return a distinct
sentinel (e.g., a tuple with an explicit error tag) instead of None so
nettacker/api/engine.py:get_result_content() can distinguish "not found" from
"unreadable file"; ensure the chosen signal is documented and callers are
updated to handle it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da4f8895-9cda-4f98-8e06-e5f13ace3756
📒 Files selected for processing (3)
nettacker/api/engine.pynettacker/database/db.pytests/api/test_engine.py
✅ Files skipped from review due to trivial changes (1)
- tests/api/test_engine.py
|
@coderabbitai remember to check for: 1. signed commits. 2. conflicts with other open pull requests in this repo |
🧩 Analysis chain🏁 Script executed: Length of output: 21136
1. 🔏 Commit SignaturesThis PR fails pre-merge checks. Of the 5 commits in this PR, 4 are unsigned:
All commits must be GPG/SSH signed before this PR can be merged. Please sign your commits. You can set up commit signing by following GitHub's guide on signing commits. 2.
|
| PR | Author | Overlapping Files |
|---|---|---|
#1355 |
@jess-tech-lab |
nettacker/api/engine.py |
#1346 |
@xovishnukosuri |
nettacker/api/core.py, nettacker/api/engine.py |
#1326 |
@Tech-Psycho95 |
nettacker/api/core.py, nettacker/api/engine.py |
#1294 |
@kwei-zhang |
nettacker/api/engine.py, nettacker/database/db.py |
The highest-risk overlap is with #1326, which modifies both nettacker/api/engine.py and nettacker/api/core.py extensively (+139/-101 and +2/-1 lines respectively). Maintainers should be aware of potential conflicts when merging these PRs.
🧠 Learnings used
Learnt from: securestep9
Repo: OWASP/Nettacker PR: 0
File: :0-0
Timestamp: 2026-01-15T17:07:23.769Z
Learning: When securestep9 requests to check commit signatures in open PRs, always verify all commits are signed using GitHub CLI and report unsigned commits immediately with explicit notification that the PR fails pre-merge checks.
Learnt from: securestep9
Repo: OWASP/Nettacker PR: 0
File: :0-0
Timestamp: 2026-01-13T22:48:26.796Z
Learning: When reviewing any pull request in the OWASP/Nettacker repository, always remind the PR author to use signed commits and explicitly state that the PR fails pre-merge checks if any commits are unsigned. Tag the author by username and provide instructions on how to sign commits.
Learnt from: securestep9
Repo: OWASP/Nettacker PR: 0
File: :0-0
Timestamp: 2026-01-10T15:29:31.759Z
Learning: When reviewing any pull request, always check that all commits are signed (GPG/SSH signatures verified). Use GitHub CLI to verify commit signatures with: `gh api repos/OWNER/REPO/pulls/PR_NUMBER/commits --jq '.[] | {sha: .sha, author: .commit.author.name, verified: .commit.verification.verified, reason: .commit.verification.reason}'`. Report any unsigned commits to maintainers.
Learnt from: securestep9
Repo: OWASP/Nettacker PR: 0
File: :0-0
Timestamp: 2026-01-10T14:58:54.638Z
Learning: When reviewing a pull request, always analyze other open PRs in the repository to identify potential conflicts, such as duplicate changes to the same files, overlapping features, or merge conflicts. Use GitHub CLI to search for related PRs by file changes, keywords, or linked issues.
✅ Actions performed
Reviews resumed.
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
- Changed Content-Disposition from attachment to inline for .html/.htm - Added sandbox allow-scripts CSP directive with base-uri/frame-ancestors - Used secure_filename with empty-string fallback for safe filenames - Handle None (404) and dict (500) returns from get_scan_result() - Normalized APSW no-row case to return None (consistent with SQLAlchemy) - Fixed .htm mime type to text/html in core.py - Added endpoint-level regression tests for /results/get - Added .htm assertion to test_mime_types Fixes OWASP#1316
8696748 to
92c97bd
Compare
Proposed change
Currently, clicking a scan result link in the WebUI (e.g.
https://127.0.0.1:8080/results/get?id=2) triggers an automatic download of the HTML file instead of rendering it in the browser. This degrades user experience.This PR fixes the
/results/getendpoint so HTML reports render inline in the browser, with hardened security headers.Changes
nettacker/api/engine.py--/results/getendpoint:Content-Dispositionfromattachmenttoinlinefor.html/.htmfilessandbox allow-scriptsCSP directive (permits D3.js/jQuery visualizations, omitsallow-same-originto isolate origin)base-uri 'none'andframe-ancestors 'none'CSP directivessecure_filename(os.path.basename())for safe filename handling with empty-string fallbackContent-Dispositionper RFC 6266Nonereturn fromget_scan_result()with 404 responsedictreturn fromget_scan_result()with 500 responselog.error()on database exceptions for debuggabilityattachment)nettacker/database/db.py--get_scan_result():Noneinstead ofstructure(dict), consistent with SQLAlchemy branch (both yield 404 now)nettacker/api/core.py--mime_types():.htmmime type (was'.htm'instead of'text/html')tests/api/test_engine.py(new):/results/get: inline HTML+CSP, .htm variant, non-HTML attachment, 404 not-found, 500 dict-error, 400 missing-id, empty safe_filename fallbacktests/api/test_core.py:.htmassertion totest_mime_typesto prevent regressionsFixes #1316
Type of change
Checklist
make pre-commitand confirm it didn't generate any warnings/changesmake test, I confirm all tests passed locally