Skip to content

fix: 2 improvements across 2 files#7551

Open
tomaioo wants to merge 2 commits into
freelawproject:mainfrom
tomaioo:improve/quality/missing-error-handling-in-clipboard-copy
Open

fix: 2 improvements across 2 files#7551
tomaioo wants to merge 2 commits into
freelawproject:mainfrom
tomaioo:improve/quality/missing-error-handling-in-clipboard-copy

Conversation

@tomaioo

@tomaioo tomaioo commented Jul 6, 2026

Copy link
Copy Markdown

Summary

fix: 2 improvements across 2 files

Problem

Severity: Medium | File: cl/assets/static-global/js/alpine/composables/clipboard.js:L28

The copyToClipboard method in the clipboard composable does not handle potential errors from navigator.clipboard.writeText(). This API can fail in insecure contexts or when permission is denied, causing an unhandled promise rejection. Additionally, the temporary textarea element is created but never appended to the DOM or cleaned up, which could cause memory leaks.

Solution

Wrap the clipboard operation in a try/catch block, handle the promise rejection, and properly clean up the temporary element. Consider using document.createElement and removing it after use, or better yet, avoid creating a DOM element entirely since navigator.clipboard.writeText() accepts a string directly.

Changes

  • cl/assets/static-global/js/alpine/composables/clipboard.js (modified)
  • cl/alerts/templates/docket_alert_unsubscription_email.html (modified)

tomaioo added 2 commits July 6, 2026 11:08
- Quality: Missing Error Handling in Clipboard Copy Operation
- Quality: Potential XSS via HTML Injection in Template

Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
- Quality: Missing Error Handling in Clipboard Copy Operation
- Quality: Potential XSS via HTML Injection in Template

Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mlissner

mlissner commented Jul 6, 2026

Copy link
Copy Markdown
Member

@claude review

@mlissner mlissner requested a review from albertisfu July 6, 2026 19:03
@mlissner mlissner moved this to To Do in Sprint (Web Team) Jul 6, 2026
@mlissner mlissner added the slow-review Volunteer-led work that is scheduled for review soon, but not immediately label Jul 6, 2026
Comment on lines 20 to 28
{% for rd in de.recap_documents.all %}
{% if forloop.first %}
{% if rd.description %}
{{ rd.description|safe }}
{{ rd.description }}
{% else %}
{{ de.description|safe|default:""|safe }}
{{ de.description|default:"" }}
{% endif %}
{% endif %}
{% endfor %}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The PR silently removes |safe from three template expressions in docket_alert_unsubscription_email.html (rd.description, de.description, and docket|best_case_name), but the sibling docket_alert_email.html — the primary alert this unsubscribe email is paired with — still applies |safe to the exact same expressions at lines 24, 26, 51, 90, 92, as do ~24 other templates project-wide. Since PACER case names and RECAP descriptions legitimately contain HTML (e.g. <i> around party names, <b><font color="red">…</font></b> for administrative flags), a docket named Smith v. <i>Jones</i> will now render in the unsubscribe email as literal Smith v. &lt;i&gt;Jones&lt;/i&gt; — visible escaped markup in a user-facing email. If XSS hardening was the intent, it needs to be applied consistently across all docket alert templates, and the PR description should mention it (currently it only discusses the clipboard.js change).

Extended reasoning...

What the bug is

Three |safe filters are removed from cl/alerts/templates/docket_alert_unsubscription_email.html:

  • L23: {{ rd.description|safe }}{{ rd.description }}
  • L25: {{ de.description|safe|default:""|safe }}{{ de.description|default:"" }}
  • L35: {{ docket|best_case_name|safe }}{{ docket|best_case_name }}

Under Django autoescape, this causes any HTML in those fields to be rendered as escaped text in the resulting email.

Why existing code doesn't prevent it

The best_case_name filter (cl/custom_filters/templatetags/text_filters.py:165) is registered with @register.filter(is_safe=True), but that flag is not the same as mark_safe(). is_safe=True is Django's promise that the filter will not introduce unsafe characters in already-safe input; it does not mark the underlying TextField (case_name / case_name_full / case_name_short on Docket) as safe. Contrast with v_wrapper in the same module which explicitly uses mark_safe(). This is why every other template in the codebase applies |safe after best_case_name — the raw value is auto-escaped without it.

Project-wide convention

The sibling cl/alerts/templates/docket_alert_email.html — the primary alert email; the unsubscribe email is triggered from a link inside it — applies |safe to the identical expressions:

  • L24: {{ rd.description|safe }}
  • L26: {{ de.description|safe|default:""|safe }}
  • L51/L90/L92: {{ docket|best_case_name|safe }}

Grep confirms ~24 templates project-wide follow this convention (docket_alert_new.html, includes/docket_alert_case_name.html, docket_li.html, opinions.html, recap_document.html, oral_argument.html, top_prayers.html, etc.). The removed |safe is the outlier, not a reform.

Step-by-step proof

  1. A user is subscribed to a bankruptcy docket with case_name = 'Advantage LLC <b><font color="red">Jointly Administered under 23-90886.</font></b>' (this exact HTML pattern appears in real PACER fixtures in cl/lib/tests.py:1554-1562).
  2. The user clicks the unsubscribe link inside the alert email (which rendered the name correctly because docket_alert_email.html still uses |safe).
  3. docket_alert_unsubscription_email.html renders. Under Django autoescape and without |safe, {{ docket|best_case_name }} produces the escaped string.
  4. The h2 header in the delivered email reads:
    You have been unsubscribed from Advantage LLC &lt;b&gt;&lt;font color=&quot;red&quot;&gt;Jointly Administered under 23-90886.&lt;/font&gt;&lt;/b&gt; (23-90886).
  5. Same divergence occurs in the preheader for any RECAP entry description containing HTML (e.g. italicized text).

The result is that two emails from the same alert system, populated from the same fields, display the same case name inconsistently.

Impact and fix

User-visible regression in production email rendering for any docket whose case_name or associated RECAP descriptions contain HTML — common in bankruptcy and consolidated cases with red-flagged PACER text. The PR description only documents the clipboard.js change; the template edit is unexplained, so this looks inadvertent.

Fix: revert the three template edits (restore |safe on all three expressions). If the goal was actually XSS hardening against stored PACER content, that needs to be a repo-wide effort applied consistently to docket_alert_email.html, the includes/docket_alert_case_name.html partial, and the other ~20 alert / opinion / audio templates — not a one-file silent change bundled with an unrelated clipboard fix.

@mlissner mlissner moved this from To Do to Blocked in Sprint (Web Team) Jul 6, 2026
@mlissner

mlissner commented Jul 6, 2026

Copy link
Copy Markdown
Member

Hello, @tomaioo, and thanks for this. Claude flagged an issue. Can you please respond or remove your tweak?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

slow-review Volunteer-led work that is scheduled for review soon, but not immediately

Projects

Status: Blocked

Development

Successfully merging this pull request may close these issues.

4 participants