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

Escape dictionary curly bracket before renderingin HTML #42

Merged
merged 8 commits into from
May 6, 2022

Conversation

chadell
Copy link
Contributor

@chadell chadell commented May 4, 2022

Addresses issue #41

Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

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

I'm not sure this is the "best" solution to the issue - I'm concerned that it seems like we're double-interpreting a variable value somewhere in here, and this is maybe only a bandaid over a more fundamental design issue. Might be worth looking at how render_diff is wrapping this text into format_html() and see if that needs to be revisited somehow instead.

@chadell
Copy link
Contributor Author

chadell commented May 5, 2022

The result that we send to format_html(result) contains this HTML: <li class="diff-added">cfs: {'asw_owner': ''}</li>

>>> from django.utils.html import format_html
>>> result = "<li class='diff-added'>cfs: {'asw_owner': ''}</li>"
>>> format_html(result)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.6/site-packages/django/utils/html.py", line 115, in format_html
    return mark_safe(format_string.format(*args_safe, **kwargs_safe))
KeyError: "'asw_owner'"

From docs, format_html(format_string, *args, **kwargs) takes that as a format string.
So, my suggestion would be to start using mark_safe instead.

@chadell chadell requested a review from glennmatthews May 5, 2022 17:33
@chadell
Copy link
Contributor Author

chadell commented May 5, 2022

Any concern/idea about bandit issue with cross-site scripting?

@glennmatthews
Copy link
Contributor

Any concern/idea about bandit issue with cross-site scripting?

Could certainly try a test or two with objects in the diff whose string representation is HTML or JS?

@chadell
Copy link
Contributor Author

chadell commented May 6, 2022

In 6e3c3c0 I replaced all the f-strings with input data by format_html what enforces escape to protect against XSS, and then when we reused this "safe" HTML code, we use the mark_safe to not escape it again.

Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

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

Can we have a unit test case or two get added for render_diff? Otherwise looks good to me!

@chadell chadell merged commit b533bad into nautobot:develop May 6, 2022
@chadell chadell deleted the issue41-dict-keyerror branch May 6, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants