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

S308 (mark_safe) doesn't detects decorator usage and imports from another place #9780

Closed
ashrub-holvi opened this issue Feb 2, 2024 · 2 comments · Fixed by #9887
Closed
Assignees
Labels
rule Implementing or modifying a lint rule

Comments

@ashrub-holvi
Copy link

Hi, thank you for the cool project!

I am looking to suspicious-mark-safe-usage (S308) check. Here I see it's implemented, but something named "S703: django_mark_safe" is not, not sure what does it means, but looks like S308 works only if mark_safe is imported from django.utils.safestring and used as a function:

  1. With mark_safe used as a function:
from django.utils.safestring import SafeString
from django.utils.safestring import mark_safe

def some_func():
    return mark_safe('<script>alert("evil!")</script>')  # oh no

print(type(some_func()) is SafeString)

it works fine:

ruff --select S308 test.py 
test.py:7:12: S308 Use of `mark_safe` may expose cross-site scripting vulnerabilities
Found 1 error.
  1. With mark_safe used as a decorator:
from django.utils.safestring import SafeString
from django.utils.safestring import mark_safe

@mark_safe
def some_func():
    return '<script>alert("evil!")</script>'  # oh no

print(type(some_func()) is SafeString)

it doesn't raise the error:

ruff --select S308 test.py
  1. With function imported from django.utils.html, might be it's wrong way to import it, but it works and we have a lot of such usages in old code.
from django.utils.safestring import SafeString
from django.utils.html import mark_safe

def some_func():
    return mark_safe('<script>alert("evil!")</script>')  # oh no

print(type(some_func()) is SafeString)

there is no errors:

ruff --select S308 test.py
  1. With decorator imported from django.utils.html
from django.utils.safestring import SafeString
from django.utils.html import mark_safe

@mark_safe
def some_func():
    return '<script>alert("evil!")</script>'  # oh no

print(type(some_func()) is SafeString)

also no errors:

ruff --select S308 test.py

So, perhaps this check can be improved, I tried to looks to code, but for the first look I understand nothing ) Rust is only in far away future plan for me.

ruff --version
ruff 0.1.15
@charliermarsh
Copy link
Member

Interesting, thanks. We should be able to add these diagnostics to the existing rule.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Feb 5, 2024
@charliermarsh charliermarsh self-assigned this Feb 8, 2024
charliermarsh added a commit that referenced this issue Feb 8, 2024
## Summary

Django's `mark_safe` can also be used as a decorator, so we should
detect usages of `@mark_safe` for the purpose of the relevant Bandit
rule.

Closes #9780.
@charliermarsh
Copy link
Member

Fixed in #9887 -- thank you!

nkxxll pushed a commit to nkxxll/ruff that referenced this issue Mar 10, 2024
## Summary

Django's `mark_safe` can also be used as a decorator, so we should
detect usages of `@mark_safe` for the purpose of the relevant Bandit
rule.

Closes astral-sh#9780.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants