fix: Prevent stored XSS in Tag Cloud#41
Open
Flashl3opard wants to merge 2 commits intoalphaonelabs:mainfrom
Open
fix: Prevent stored XSS in Tag Cloud#41Flashl3opard wants to merge 2 commits intoalphaonelabs:mainfrom
Flashl3opard wants to merge 2 commits intoalphaonelabs:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis change adjusts end-of-file formatting in the HTML file by removing and re-adding the closing tag with modified trailing whitespace and newline structure. No functional or structural content changes are introduced. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 addresses a stored XSS risk in the activity tag cloud by removing inline JavaScript handlers and switching to DOM event listeners with data-* attributes. It also includes a set of front-end concurrency guards (action locks) and a back-end change to make /api/join idempotent when an enrollment already exists.
Changes:
- Replace inline
onclicktag-cloud handlers withdata-tag+addEventListenerbinding to prevent stored XSS. - Add client-side “in-flight” action locks to reduce double-submit/double-click behavior across several pages.
- Update
api_jointo detect existing enrollments and handle uniqueness conflicts explicitly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
public/index.html |
Removes inline tag-cloud handlers and binds clicks via addEventListener; expands escaping for tag/user strings. |
public/teach.html |
UI refactor + adds withActionLock and load versioning; adjusts hosted activities rendering. |
public/login.html |
Adds withActionLock around login/register submit flows; mostly formatting. |
public/course.html |
Adds withActionLock for joining activities; mostly formatting/structure changes. |
src/worker.py |
Makes /api/join return success when already enrolled and removes INSERT OR IGNORE in favor of explicit handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Flashl3opard
force-pushed
the
fix/xss-tag-inline-onclick
branch
from
fe7916b to
9fecd98
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@public/index.html`:
- Around line 121-139: The inputs lack accessible labels: add explicit labels
associated with the elements (use <label for="search">, <label
for="filter-type">, and <label for="filter-format"> or equivalent
aria-labelledby attributes) so screen readers and keyboard users get an
accessible name; if visual clutter is a concern use visually-hidden/sr-only
labels and ensure each label text describes the control (e.g., "Search
activities", "Filter by type", "Filter by format") and that the label 'for'
attribute matches the element ids search, filter-type, and filter-format
respectively.
In `@public/login.html`:
- Around line 89-97: Update the client-side password rule to match the server's
8-character minimum by changing the input with id "r-password": set minlength
from 6 to 8, update the placeholder from "min 6 characters" to "min 8
characters", and ensure any UI text (e.g., error container "register-err") or
client-side validation logic that references the 6-char rule is updated to
reflect the 8-character minimum so users are validated consistently before
submission.
- Around line 45-90: The labels next to inputs are not associated with their
controls; update each label element to include a matching for attribute pointing
to the corresponding input id (e.g., for="l-username", for="l-password",
for="r-name", for="r-username", for="r-email", for="r-password") so screen
readers and form focus work correctly; ensure each label exactly matches the
input id strings used in the form elements.
In `@public/teach.html`:
- Around line 72-173: Many labels are not associated with their controls which
breaks accessibility; update each <label> to include a matching for attribute
pointing to the corresponding control id (e.g., for="a-title" for the Title
input with id="a-title", for="a-desc" for textarea id="a-desc",
for="a-type"/"a-format"/"a-schedule"/"a-tags" for those selects/inputs, and for
the session form labels to match ids like s-activity, s-title, s-start, s-end,
s-location, s-desc); ensure any error/status paragraphs (act-err, act-ok) are
either referenced via aria-describedby on the form fields where applicable or
given role="status" to be announced by assistive tech.
- Around line 240-245: The code uses esc(preselect) (an HTML-entity escaper)
when building the selector for sel.querySelector, which can produce invalid CSS
selectors for certain inputs; replace esc(preselect) with CSS.escape(preselect)
when constructing the selector passed to document.querySelector/querySelectorAll
(i.e., in the block that reads params, preselect, sel, opt and calls
sel.querySelector('option[value="' + ... + '"]')), ensuring the selector
interpolation uses CSS.escape to safely escape any characters that would break
querySelector.
In `@src/worker.py`:
- Around line 991-993: The except block catching Exception as e currently checks
for the generic "UNIQUE" substring; update it to detect the specific D1
constraint for the enrollments table (e.g., check for "enrollments.activity_id,
enrollments.user_id" or the exact D1 format you observe such as
"enrollments(activity_id, user_id)") inside the except handling in src/worker.py
(the try/except around the enrollment insert). If that specific constraint is
present, return ok(None, "Already joined activity"); otherwise re-raise or
handle the error normally so other UNIQUE violations are not treated as
successful joins.
🪄 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: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5fcf3d2-324c-404a-b5be-56f3ea5c0805
📒 Files selected for processing (5)
public/course.htmlpublic/index.htmlpublic/login.htmlpublic/teach.htmlsrc/worker.py
Flashl3opard
force-pushed
the
fix/xss-tag-inline-onclick
branch
from
9fecd98 to
52cca0e
Compare
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
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.
Overview
This PR fixes a stored XSS vulnerability caused by injecting user-controlled tag names directly into inline JavaScript event handlers. A crafted tag name could break out of the JavaScript string context and execute arbitrary code in other users’ browsers.
Root Cause
Tag names were inserted into inline handlers as follows:
onclick="toggleTag('${name}')"Because HTML entity decoding happens before JavaScript execution, a tag such as:
could break out of the string context and execute arbitrary JavaScript.
Fix
This PR removes inline event handlers and replaces them with safe DOM-based event binding:
onclick="toggleTag('${...}')"data-tag="..."addEventListener()and retrieve values using.dataset.tagThis ensures user-controlled values are treated strictly as data and are never executed as code.
Changes
buildTagCloud()inpublic/index.htmlSummary
This PR fixes a stored cross-site scripting (XSS) vulnerability in the tag cloud feature where user-controlled tag names were injected into inline JavaScript event handlers.
Vulnerability Details
User-controlled tag names were previously embedded directly into inline
onclickattributes:An attacker could craft a malicious tag name (e.g.,
x');alert(1);//) that breaks out of the JavaScript string context and executes arbitrary code in other users' browsers.Solution Implemented
onclick="toggleTag('${esc(name)}')"from HTML generationdata-tag-nameattributes, treated as plain textaddEventListener()calls to programmatically bind click events, reading tag values fromelement.dataset.tagNamebuildTagCloud()function now attaches event listeners after DOM generation, ensuring tag names are never interpreted as codeFiles Modified
buildTagCloud()function to replace inline event handlers with data attributes and programmatic event bindingImpact
Tag values are now completely isolated from the JavaScript execution context, preventing any injection attacks regardless of tag name content. The fix maintains full functionality while eliminating the XSS vector.