-
Notifications
You must be signed in to change notification settings - Fork 892
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
Avoid raising S310 if user explicitly checks for URL scheme #7918
Comments
Could you clarify why |
I think s310 should not be triggered because I check the URL in the first lines. |
Ah I misunderstood. I thought from your description that the rule was not triggering and it should. But if I understand you correctly now, it is being flagged, and you want to change rule S310 to not flag if the URL passed to it has been checked to start with @charliermarsh is that something that is desirable to implement or is this something that should be manually marked as a check skip by the dev in question. Docs just suggest:
|
I think this will be hard to get right with absolute certainty and so I'm somewhat hesitant to invest in it given that it's a security-related rule. If someone wants to try, though, I am happy to review it. |
We now no longer flag this if you use a string literal, which is at least an improvement. |
What about string literal but not in place? This is currently triggering the warning: url = "https://pypi.org/pypi?:action=list_classifiers"
context = ssl.create_default_context()
with urlopen(url, context=context) as response: |
A string literal passed through import urllib
urllib.request.urlopen(urllib.request.Request("https://example.com/")) (obviously in this case I could just pass the string literal directly; in reality, I’m constructing the |
I can fix a few of these. |
…equest` argument (#10964) ## Summary Allows, e.g.: ```python import urllib urllib.request.urlopen(urllib.request.Request("https://example.com/")) ``` ...in [`suspicious-url-open-usage`](https://docs.astral.sh/ruff/rules/suspicious-url-open-usage/). See: #7918 (comment)
It would be cool if stuff like url = f"https://{self.target_url}{self.path}" didn't trigger this. |
Also it would be great if ruff could recognize asserts (like
assert url.startswith(("https://", "http://"))
)ruff /path/to/file.py --fix
), ideally including the--isolated
flag.ruff --isolated --select S t.py
pyproject.toml
).No pyproject.toml
ruff --version
).ruff 0.0.292
The text was updated successfully, but these errors were encountered: