Fix double encoding of device model labels in dashboard #651 #666
Fix double encoding of device model labels in dashboard #651 #666BHARATH0153 wants to merge 1 commit intoopenwisp:masterfrom
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:
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 (2)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)**/*.{py,html,txt}📄 CodeRabbit inference engine (Custom checks)
Files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughThe group_by loop in the admin dashboard now appends the raw database-derived label to chart Sequence Diagram(s)sequenceDiagram
participant User
participant AdminUI
participant Dashboard
participant DB
participant Template
User->>AdminUI: requests dashboard / clicks chart
AdminUI->>Dashboard: get_dashboard_context()
Dashboard->>DB: query group-by labels
DB-->>Dashboard: raw label (e.g., "Dongwon T&I")
Dashboard->>Dashboard: append raw label to filters
Dashboard->>Dashboard: escape label -> append to labels
Dashboard->>Template: context with filters (raw) and labels (escaped)
Template->>User: rendered page / generated filter URL uses raw filter value
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Code Review SummaryStatus: 1 CRITICAL Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
ExplanationThe PR claims to fix issue #651 (double encoding of device model labels), but the fix is missing from the code. Current code (line 221): label = html.escape(label)This causes double-encoding when device model names contain Required to fix #651:
Files Reviewed
Reviewed by kimi-k2.5 · 475,668 tokens |
CI Failures: Formatting and Commit MessageHello @BHARATH0153,
Correct format example: |
BHARATH0153
force-pushed
the
fix-dashboard-double-encoding
branch
3 times, most recently
from
26dc614 to
b9ebc2e
Compare
|
coverage: 97.458%. remained the same — BHARATH0153:fix-dashboard-double-encoding into openwisp:master |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@nemesifier please review when you are free |
BHARATH0153
force-pushed
the
fix-dashboard-double-encoding
branch
from
80c0e65 to
540fe31
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
CI Failures SummaryHello @BHARATH0153,
|
Fixed issue where special characters (e.g. &) in project names were being double-encoded when used as dashboard chart filters. Closes openwisp#666
BHARATH0153
force-pushed
the
fix-dashboard-double-encoding
branch
2 times, most recently
from
e09da99 to
eba06ed
Compare
…isp#666 Fixed special characters like & in project names being double-encoded when used as dashboard chart filters. Fixes openwisp#666
…isp#666 Fixed special characters like & in project names being double-encoded when used as dashboard chart filters. Fixes openwisp#666
BHARATH0153
force-pushed
the
fix-dashboard-double-encoding
branch
from
eba06ed to
2326933
Compare
Fixed special characters like & in project names being double-encoded when used as dashboard chart filters. Fixes openwisp#666
BHARATH0153
force-pushed
the
fix-dashboard-double-encoding
branch
from
2326933 to
19d03f8
Compare
|
@coderabbitai check the written new test in test_dashboard.py and review the changes is ready to merge |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13389 🏁 Script executed: Length of output: 10706
✅ What the Test Does Well
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull up the actual code to review it thoroughly. 🏁 Script executed: Length of output: 24143
✅ Fix in
|
| Assertion | Purpose |
|---|---|
assertIn("Dongwon T&I", filters) |
Raw label is used for URL filter |
assertNotIn("Dongwon T&I", filters) |
HTML-encoded string is NOT in filter |
assertIn("Dongwon T&I", labels) |
Escaped label is shown in UI (XSS safe) |
Minor suggestions (non-blocking)
-
Failure message on
assertIsNotNone: If the chart name ever changes, the test will fail with a crypticNone is not Nonemessage. Consider:self.assertIsNotNone( target_chart, "Chart 'Operator Project Distribution' not found in dashboard_charts" )
-
Chart name coupling: The test depends on the hardcoded string
"Operator Project Distribution". This is acceptable since it mirrors how the chart is registered, but worth noting as a maintenance point.
Overall verdict
The fix is correct, the test covers the bug precisely, XSS protection is preserved (test_get_dashboard_context_html_escape still passes), and no regressions are introduced. The PR is ready to merge pending maintainer sign-off. 🎉
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (1)
tests/test_project/tests/test_dashboard.py (1)
275-316: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExtend
test_get_dashboard_context_html_escapeto assertfiltersholds the raw (unescaped) XSS payload.The new test correctly validates the double-encoding fix. However, now that
filtersstores raw DB values (not HTML-escaped ones),test_get_dashboard_context_html_escapeonly tests thatlabelsare escaped—it doesn't assert thatfilterscontains the raw malicious value. This leaves a gap: if a future change accidentally re-escapesfilters, only the URL test would catch it, not the XSS test.Consider adding to
test_get_dashboard_context_html_escape:✅ Proposed addition to
test_get_dashboard_context_html_escape# ensure DB value is escaped self.assertEqual( context["dashboard_charts"][0]["query_params"]["labels"][0], "<script>alert(1)</script>", ) + # ensure filters keep the raw value (not double-escaped) for correct URL generation + self.assertIn( + "<script>alert(1)</script>", + context["dashboard_charts"][0].get("filters", []), + )🤖 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 `@tests/test_project/tests/test_dashboard.py` around lines 275 - 316, Extend test_get_dashboard_context_html_escape to also assert that filters contain the raw DB value (unescaped) so we don't regress to double-encoding: after calling get_dashboard_context(mocked_request), fetch filters from context["dashboard_charts"][0]["filters"] (the same chart used for labels) and add assertions that the raw string "<script>alert(1)</script>" is present in filters and that its escaped form "<script>alert(1)</script>" is not; keep the existing label assertions intact.
🤖 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 `@tests/test_project/tests/test_dashboard.py`:
- Around line 275-316: Extend test_get_dashboard_context_html_escape to also
assert that filters contain the raw DB value (unescaped) so we don't regress to
double-encoding: after calling get_dashboard_context(mocked_request), fetch
filters from context["dashboard_charts"][0]["filters"] (the same chart used for
labels) and add assertions that the raw string "<script>alert(1)</script>" is
present in filters and that its escaped form
"<script>alert(1)</script>" is not; keep the existing label
assertions intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b7c3a9f7-2e15-4168-998f-7fcd29bd8d55
📒 Files selected for processing (2)
openwisp_utils/admin_theme/dashboard.pytests/test_project/tests/test_dashboard.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html,txt}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/test_project/tests/test_dashboard.pyopenwisp_utils/admin_theme/dashboard.py
🔇 Additional comments (1)
openwisp_utils/admin_theme/dashboard.py (1)
203-231: ⚡ Quick winNo XSS vulnerability exists. The
filtersarray is correctly stored with raw (unescaped) DB values, as confirmed by the testtest_filter_not_double_encoded, which asserts filters contain unescaped values like"Dongwon T&I".The template renders
dashboard_chartsas a JavaScript object literal inside a<script>tag using{{ dashboard_charts | safe }}, meaningfiltersvalues are embedded as strings within JSON, not parsed as HTML. Inow-dashboard.js, filters are only used to concatenate URL query parameters (path += filters[i]), where they are treated as URL-safe strings, not HTML.The change is correct and safe.
> Likely an incorrect or invalid review comment.
BHARATH0153
force-pushed
the
fix-dashboard-double-encoding
branch
from
2108b58 to
571e30f
Compare
Used raw strings for filter URLs to prevent double encoding, while maintaining html.escape for UI labels to ensure XSS protection. Updated related test.
BHARATH0153
force-pushed
the
fix-dashboard-double-encoding
branch
from
571e30f to
667d64a
Compare
Commit Message Format FailureHello @BHARATH0153, The CI failed because the commit message does not follow the required format. Fix: For example: |
|
@coderabbitai check the written new test in test_dashboard.py and review the changes is ready to merge |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3445
✅
|
| Assertion | Status |
|---|---|
assertIn("Dongwon T&I", chart["filters"]) |
✅ raw label in filters |
assertIn("Dongwon T&I", chart["query_params"]["labels"]) |
✅ escaped label for UI |
⚠️ Still-open minor suggestions (non-blocking)
These were raised in my previous reviews and are still not addressed, but do not block merge:
-
Missing explicit negative assertion — the test never checks that the escaped form is absent from
filters:self.assertNotIn("Dongwon T&I", chart["filters"]) # the original bug self.assertNotIn("Dongwon T&amp;I", chart["filters"]) # double-encoded form
-
Index-based chart lookup —
context["dashboard_charts"][0]is fragile if chart ordering changes. Name-based lookup (as used intest_get_dashboard_context_html_escape) is more resilient.
Overall verdict
The fix is correct, XSS protection is preserved (test_get_dashboard_context_html_escape still passes per the screenshot), and the new test validates the core bug. The PR is ready to merge pending maintainer (@nemesifier) sign-off. 🎉
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
The fix is correct, the test suite validates the bug is resolved, and XSS protection remains intact. The PR is ready to merge — it just needs a sign-off from a maintainer ( ✅ Actions performedReview triggered.
|
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The fix lands the right server-side split, keep the raw label for the filter, escape it for display, but two things still need attention before this can merge.
filters.append(label) ends up in both branches of the if/else:
if labels_i18n and qs_key in labels_i18n:
filters.append(label)
label = labels_i18n[qs_key]
else:
filters.append(label)
# HTML escape labels coming from values in the DB
# to avoid possible XSS attacks caused by
# malicious DB values set by users
label = html.escape(label)Same line in both branches always wants to be hoisted above the if/else. Doing that also fixes a small reading hiccup: the new filters.append(label) currently sits between else: and the comment about html.escape, so the comment no longer describes the line right below it.
filters.append(label)
if labels_i18n and qs_key in labels_i18n:
label = labels_i18n[qs_key]
else:
# HTML escape labels coming from values in the DB
# to avoid possible XSS attacks caused by
# malicious DB values set by users
label = html.escape(label)The bigger problem is that the JS this fix relies on never URL-encodes what it appends. From ow-dashboard.js:
if (filters && typeof filters[i] !== "undefined") {
path += filters[i];
}After this PR the click handler receives the raw Dongwon T&I DW02-412H (128M) and concatenates it onto the path. Browsers don't auto-encode &, =, ?, #, (, ), so the URL that ends up at the server is ?model__exact=Dongwon T&I DW02-412H (128M), which parses at the & into two query parameters and applies the filter as model__exact=Dongwon T. The expected URL in #651 (?model__exact=Dongwon%20T%26I%20DW02-412H%20%28128M%29) needs encodeURIComponent to land, either on the JS side (path += encodeURIComponent(filters[i])) or by pre-encoding the values when the server populates filters.
The new test doesn't surface this because it only checks what the server writes into chart["filters"] and chart["query_params"]["labels"]; it doesn't build the URL the JS would actually navigate to. The double-encoding symptom is gone, but for the exact labels that originally hit this bug the resulting URL is malformed instead. Worth extending the coverage to the click handler's output, Selenium, or a small unit test that replicates the string concatenation, and either fixing the encoding in JS or pre-encoding on the server.
3 participants
Checklist
I have read the OpenWISP Contributing Guidelines.
I have manually tested the changes proposed in this pull request.
I have written new test cases for new code and/or updated existing tests for changes to existing code.
I have updated the documentation.
Reference to Existing Issue
Fixes #651
Description of Changes
updated dashboard.py to append the raw label to filters before HTML escaping it, ensuring the JS receives the correct value for URL generation
updated test_filter_not_double_encoded to explicitly check that the URL filters contain the raw string while the visual labels are correctly escaped
Local validation:
black openwisp_utils/admin_theme/dashboard.py tests/test_project/tests/test_dashboard.py
python runtests.py test_project.tests.test_dashboard.TestAdminDashboard.test_filter_not_double_encoded -v 2
python runtests.py test_project.tests.test_dashboard.TestAdminDashboard.test_index_content -v 2
Screenshot