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

Add flask.Markup XSS plugin #877

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

raj3shp
Copy link
Contributor

@raj3shp raj3shp commented Apr 3, 2022

Hi,
I would like to contribute another plugin based on Flask's security consideration about calling flask.Markup on user submitted data.

Cheers!

@sigmavirus24
Copy link
Member

I feel as if this should be not in core but a separate plugin that users can install

@raj3shp
Copy link
Contributor Author

raj3shp commented Apr 4, 2022

Hi @sigmavirus24 ,
Thanks for your comment. I consider this to be flask equivalent of django_mark_safe pluging in bandit core. Could you please elaborate the reason behind your thinking? Also regarding, "not in core separate plugin that users can install" -where are such plugins be hosted?

@Daverball
Copy link

Daverball commented Sep 19, 2023

Considering flask.Markup is just an alias for markupsafe.Markup, which is much more broadly used than flask itself (it's also used in jinja, which already has its own bandit rules), it makes a lot more sense to implement this relative to markupsafe rather than flask, although it should be fine to support the alias as well.

But I would concur that it makes sense to add this plugin to bandit. I don't see why this should be separate, considering the existing plugins in core.

That being said the implementation could be greatly improved. This rule is much too broad. It's always fine to pass a literal/uninterpolated string into Markup. I also don't think it's necessary to mark unescape as dangerous, since it should be fine as long as you don't pass the unescaped string into another function which should only accept safe markup.

@Daverball
Copy link

Daverball commented Sep 19, 2023

@ericwb @lukehinds @sigmavirus24 As long as there's some interest, considering the precedent set by django/jinja/mako, I'd be happy to take a stab at an implementation for markupsafe.Markup with fewer false positives. I.e. allowing literal/uninterpolated strings to be passed into Markup and ignoring use of unescape, since with consistent type annotations unsafe use of unescaped markup should be caught by type checkers, since only Markup is safe to render unescaped, str is not.

Copy link
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

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

Please add to the functional tests that verify this new plugin.

bandit/plugins/flask_markup_xss.py Outdated Show resolved Hide resolved
bandit/plugins/flask_markup_xss.py Outdated Show resolved Hide resolved
bandit/plugins/flask_markup_xss.py Outdated Show resolved Hide resolved
Copy link
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

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

So it seems that Flask removed the deprecated Flask.markup. Flask advises to now use:
from markupsafe import Markup

Importing escape and Markup from flask is deprecated. Import them directly from markupsafe instead. [#4996](https://github.com/pallets/flask/pull/4996)

https://flask.palletsprojects.com/en/stable/changes/

@raj3shp
Copy link
Contributor Author

raj3shp commented Jan 22, 2025

So it seems that Flask removed the deprecated Flask.markup. Flask advises to now use: from markupsafe import Markup

Importing escape and Markup from flask is deprecated. Import them directly from markupsafe instead. [#4996](https://github.com/pallets/flask/pull/4996)

https://flask.palletsprojects.com/en/stable/changes/

Ah, thanks! I guess this plugin is no longer needed in context of flask. I will close the PR. Let me know if you find it useful to have a separate plugin for usage of markupsafe.Markup on formatted strings or dynamic variables. It would produce lots of false positives though.

@Daverball
Copy link

I can try to port the rule I wrote for ruff to bandit, if there's interest.

https://docs.astral.sh/ruff/rules/unsafe-markup-use/

Either with or without the corresponding settings. I'm personally not a huge fan of the whitelist, but it was requested by the community.

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.

4 participants