Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite unsafe (XSS-prone) code in a safer way #7699

Open
7 tasks
WofWca opened this issue May 26, 2022 · 2 comments
Open
7 tasks

Rewrite unsafe (XSS-prone) code in a safer way #7699

WofWca opened this issue May 26, 2022 · 2 comments
Labels
enhancement Adding or requesting a new feature. help wanted Extra attention is needed.

Comments

@WofWca
Copy link
Contributor

WofWca commented May 26, 2022

Describe the problem

There is some code that is hard to check whether is safe or not. Namely:

  • Remaining mark_safe usage (e.g.
    @register.filter
    def add_site_url(content):
    """Automatically add site URL to any relative links or images."""
    parser = etree.HTMLParser(collect_ids=False)
    tree = etree.parse(StringIO(content), parser)
    for link in tree.findall("//a"):
    url = link.get("href")
    if url.startswith("/"):
    link.set("href", get_site_url(url))
    for link in tree.findall("//img"):
    url = link.get("src")
    if url.startswith("/"):
    link.set("src", get_site_url(url))
    return mark_safe(
    etree.tostring(
    tree.getroot(), pretty_print=True, method="html", encoding="unicode"
    )
    )
    ,
    mark_safe(
    value.encode("ascii", "xmlcharrefreplace").decode("ascii")
    ),
    , looks like rewriting translations.py is gonna be a pain:
    return mark_safe("".join(output))
    )
  • |safe usage in templates (e.g.
    {{ field.label|safe }}{% if field.field.required %}<span class="asteriskField">*</span>{% endif %}
    )
  • Direct HttpResponse usage (e.g.
    return HttpResponse("\n".join(result), content_type=f"{mime}; charset=utf-8")
    )
  • Maybe there's something in JavaScript. I saw innerHTML, for example. And a lot of $('....
  • Wherever there is escape(, it likely means that the code can be rewritten with format_html.
  • Try searching for [A-Z]\.format\( - most of occurrences need to be rewritten with format_html.
  • Considering how big the translation files are, maybe it's time to make a wrapper for gettext, trans and blocktrans which will escape the text by default.

And I might have missed something.

Describe the solution you'd like

Gradually make the code easier to assess for security, by removing |safe, mark_safe(, .innerHTML = and utilizing format_html, format_html_join, conditional_escape, etc.

Describe alternatives you've considered

No response

Screenshots

No response

Additional context

These fall under this: #7676, #7683, #7696.

@WofWca WofWca changed the title Remove unsafe (XSS-prone) asda Rewrite unsafe (XSS-prone) code in a safe way May 26, 2022
@nijel
Copy link
Member

nijel commented May 27, 2022

Direct HttpResponse usage

This one is for generated text/plain, not sure if there is a better alternative.

weblate/weblate/templates/bootstrap3/field.html

This one is based on django-crispy-forms, we have to follow what they do.

weblate/weblate/accounts/templatetags/site_url.py

This one does post-processing of HTML, I don't see a better alternative.

, looks like rewriting translations.py is gonna be a pain:

It is basically merging a list with HTML tags and list with text. I'd be a bit worried about the performance if this is migrated to format_html_join.

@WofWca WofWca changed the title Rewrite unsafe (XSS-prone) code in a safe way Rewrite unsafe (XSS-prone) code in a safer way May 27, 2022
@github-actions
Copy link

github-actions bot commented Jun 8, 2022

This issue has been automatically marked as stale because there wasn’t any recent activity.

It will be closed soon if no further action occurs.

Thank you for your contributions!

@github-actions github-actions bot added the wontfix Nobody will work on this. label Jun 8, 2022
@nijel nijel added enhancement Adding or requesting a new feature. help wanted Extra attention is needed. and removed wontfix Nobody will work on this. labels Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding or requesting a new feature. help wanted Extra attention is needed.
Projects
None yet
Development

No branches or pull requests

3 participants
@nijel @WofWca and others