[flake8-bandit] Handle string literal bindings in suspicious-url-open-usage (S310)

[danparizher]

Summary

Fixes false positive in S310 when URL arguments are assigned to variables that resolve to string literals with safe schemes (http:// or https://).

Fixes #21462.

Problem

The rule flagged urllib.request.urlretrieve(path, ...) when path was assigned a string literal like "https://example.com/data.csv" because it only checked direct string literals, f-strings, and concatenation, not variable bindings.

Approach

Added resolve_to_string_literal() helper that uses find_binding_value() to resolve variable bindings to their string literal values, following the pattern in unnecessary_regular_expression.rs. Updated the urlopen/urlretrieve handler to check resolved string literals before falling back to leading_chars().

Test Plan

Added test case to S310.py for the variable binding scenario.

[astral-sh-bot]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+2 -4 violations, +0 -0 fixes in 4 projects; 51 projects unchanged)

apache/airflow (+0 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview --select ALL

- dev/breeze/src/airflow_breeze/utils/constraints_version_check.py:364:14: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected.

ibis-project/ibis (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview

+ ibis/examples/gen_registry.py:175:59: RUF100 Unused `noqa` directive (unused: `S310`)

rotki/rotki (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview

+ package.py:350:52: RUF100 [*] Unused `noqa` directive (unused: `S310`)

zulip/zulip (+0 -3 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview --select ALL

- tools/droplets/create.py:49:20: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected.
- tools/droplets/create.py:63:20: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected.
- tools/droplets/create.py:82:20: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected.

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
S310	4	0	4	0	0
RUF100	2	2	0	0	0

[edgarrmondragon]

It might also be worth similarly resolving the url argument of urllib.request.Request

[amyreese]

The one ecosystem change is from a project where this previously triggered no longer getting a false positive, and therefore getting an "unused suppression" from their noqa.

This might be worth gating behind preview flag.

[ntBre]

Thanks, this looks reasonable to me. But I think we can resolve names more generally, and I agree with the suggestion to apply this to the url argument too.

I also agree that this should be a preview change.

[ntBre]

This version of resolve_to_string_literal actually seems too restrictive in this case since leading_chars just takes any Expr. I guess we should just resolve an Expr::Name to whatever its binding is and pass that directly to leading_chars. Something like:

                        Some(expr) => {
                            let expr = resolve_name(expr);

                            if leading_chars(expr).is_some_and(has_http_prefix) {
                                return;
                            }
                        }

Then we could add tests showing that we handle resolved f-strings and concatenated string literals.

[edgarrmondragon]

@danparizher I opened danparizher#1 against your fork to handle calls to urllib.request.Request outside of urllib.request.urlopen, and also process concatenated string literals recursively.

[ntBre]

Oh, I was waiting to see if the PR against the fork was merged, but I think the URL resolution was also fixed in the last commit. Is that right?

[edgarrmondragon]

Oh, I was waiting to see if the PR against the fork was merged, but I think the URL resolution was also fixed in the last commit. Is that right?

IIUC, the last commit in this PR fixed the urllib.request.urlopen(urllib.request.Request(url)) case, but not urllib.request.Request(url) on its own.

It also didn't fully address this comment (#21469 (comment))

handle resolved f-strings and concatenated string literals.

But my all means, feel free to merge without my updates, which I can open a new PR for.

[MichaReiser]

Thank you