Skip to content

Conversation

@ntBre
Copy link
Contributor

@ntBre ntBre commented Mar 11, 2025

Summary

Stabilizes S704, which is also being recoded from RUF035 in 0.10.

Test Plan

Existing tests with PreviewMode removed from the settings.

There was one issue closed on 2024-12-20 calling the rule noisy and asking for a config option, but the option was added and then there were no more issues or PRs.

Summary
--

Stabilizes S704, which is also being recoded from RUF035 in 0.10.

Test Plan
--
Existing tests with `PreviewMode` removed from the settings.

There was one issue closed on 2024-12-20 calling the rule noisy and asking for a
config option, but the option was added and then there were no more issues or PRs.
@ntBre ntBre added the rule Implementing or modifying a lint rule label Mar 11, 2025
@ntBre ntBre added this to the v0.10 milestone Mar 11, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 11, 2025

CodSpeed Performance Report

Merging #16643 will degrade performances by 12.73%

Comparing brent/s704-0.10 (5678e15) with micha/ruff-0.10 (85f7871)

Summary

❌ 1 regressions
✅ 31 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
red_knot_check_file[incremental] 4.8 ms 5.5 ms -12.73%

@github-actions
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

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

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

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

+ providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py:2307:19: S704 Unsafe use of `markupsafe.Markup` detected

apache/superset (+7 -0 violations, +0 -0 fixes)

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

+ superset/connectors/sqla/models.py:1308:16: S704 Unsafe use of `markupsafe.Markup` detected
+ superset/models/dashboard.py:225:16: S704 Unsafe use of `markupsafe.Markup` detected
+ superset/models/helpers.py:538:16: S704 Unsafe use of `markupsafe.Markup` detected
+ superset/models/helpers.py:567:16: S704 Unsafe use of `markupsafe.Markup` detected
+ superset/models/slice.py:338:16: S704 Unsafe use of `markupsafe.Markup` detected
+ superset/models/sql_lab.py:445:16: S704 Unsafe use of `markupsafe.Markup` detected
+ superset/utils/core.py:485:16: S704 Unsafe use of `markupsafe.Markup` detected

freedomofpress/securedrop (+21 -0 violations, +0 -0 fixes)

+ securedrop/journalist_app/account.py:100:20: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/account.py:87:20: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/admin.py:224:32: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/admin.py:279:20: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/admin.py:295:20: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/col.py:103:17: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/col.py:75:13: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/main.py:169:17: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/main.py:192:21: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/main.py:203:21: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/utils.py:267:9: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/utils.py:337:13: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/utils.py:366:13: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/utils.py:380:13: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/utils.py:503:17: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/journalist_app/utils.py:519:13: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/source_app/utils.py:37:16: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/source_app/utils.py:44:11: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/source_app/utils.py:51:22: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/source_app/utils.py:65:11: S704 Unsafe use of `markupsafe.Markup` detected
+ securedrop/template_filters.py:24:21: S704 Unsafe use of `markupsafe.Markup` detected

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

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

+ zerver/views/documentation.py:292:35: S704 Unsafe use of `markupsafe.Markup` detected

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
S704 30 30 0 0 0

Linter (preview)

✅ ecosystem check detected no linter changes.

@ntBre
Copy link
Contributor Author

ntBre commented Mar 11, 2025

There appear to be a few varieties of false positives here:

  • The first case I clicked on is inside of a function called _cli_safe_flash. I guess this would need a noqa because it doesn't seem like the rule offers another way to mark an enclosing function as safe. This one seems pretty fair because the caller is supposed to guarantee the safety, based on the name of the function.
  • There are two f-string expressions in this case, but the second calls markupsafe.escape, which I don't think the rule handles.
  • The argument here is generated by nh3.clean but assigned to a variable. As far as I can tell the rule doesn't do any variable resolution, it only checks for string literals, bytes literals, and calls directly in the Markdown() call, so this also won't work with the allowlist.
  • This case calls .format() with the results of two escape calls
    • securedrop has ~12 other calls like that

These seem pretty tricky to handle in general, so I don't necessarily think they should halt stabilization. The main case that would be nice to handle is probably checking if a variable passed to Markdown has just had escape called on it.

@ntBre ntBre mentioned this pull request Mar 12, 2025
2 tasks
@MichaReiser
Copy link
Member

MichaReiser commented Mar 12, 2025

These seem pretty tricky to handle in general, so I don't necessarily think they should halt stabilization. The main case that would be nice to handle is probably checking if a variable passed to Markdown has just had escape called on it.

I agree; supporting this would be nice. I'm less concerned about keeping nh3 clean because it's a third-party library. I also think it's fine to stabilize the rule regardless and creating a follow up issue to support tracing the escape call because the implementation matches upstream PyCQA/bandit#1225. It's also a security rule where we optimize for fewer false negatives over avoiding false-positives.

@Daverball
Copy link
Contributor

Daverball commented Mar 12, 2025

The first case I clicked on is inside of a function called _cli_safe_flash. I guess this would need a noqa because it doesn't seem like the rule offers another way to mark an enclosing function as safe. This one seems pretty fair because the caller is supposed to guarantee the safety, based on the name of the function.

I would consider this a true positive, considering you can inject markup with your username there. They pass f-strings into that function. It might be possible for them to add airflow.providers.fab.auth_manager.security_manager.override.FabAirflowSecurityManagerOverride._cli_safe_flash to extend-markup-names to detect that. Which would turn it into an actual false positive.

There are two f-string expressions in this case, but the second calls markupsafe.escape, which I don't think the rule handles.

Indeed, people should be directed towards Markup.format for safe interpolation even when they have remembered to manually use escape, since it's less error-prone. It might be slightly slower at the moment though, since the C-Extension currently only accelerates the inner-most escape. It doesn't currently accelerate the custom formatter logic. Although if/once it does, it might be slightly faster, so it doesn't feel like a strong enough argument to me, to let people live dangerously.

The argument here is generated by nh3.clean but assigned to a variable. As far as I can tell the rule doesn't do any variable resolution, it only checks for string literals, bytes literals, and calls directly in the Markdown() call, so this also won't work with the allowlist.

Correct, the rule does not support indirect assignment yet, it's part of and documented in the test cases.

securedrop has ~12 other calls like that
These seem pretty tricky to handle in general, so I don't necessarily think they should halt stabilization. The main case that would be nice to handle is probably checking if a variable passed to Markdown has just had escape called on it.

Yeah, I've seen those. Once again I think it's good to direct them towards Markup.format, so they can build safer habits. But I realize it's a balancing act, since causing too much churn might motivate people to just disable the rule entirely.

Maybe we could add a strictness setting as a compromise, if there's sufficient demand for allowing f-strings and str.format as long as all the interpolations are using either markupsafe.escape or a function from the whitelist.

@Daverball
Copy link
Contributor

Alternatively we could consider adding an unsafe fix for converting simple interpolations inside Markup calls via str.format or f-strings, into a safe Markup.format call. Since that is the most common source of errors.

@ntBre
Copy link
Contributor Author

ntBre commented Mar 12, 2025

Thanks again for the context, I think it does make sense to push people toward Markup.format and to err on the side of security as you both said.

Let's go ahead and stabilize this, and I'll open an issue to track the escape and autofix ideas!

@ntBre ntBre merged commit 5f05012 into micha/ruff-0.10 Mar 12, 2025
20 of 21 checks passed
@ntBre ntBre deleted the brent/s704-0.10 branch March 12, 2025 18:48
MichaReiser pushed a commit that referenced this pull request Mar 13, 2025
Summary
--

Stabilizes S704, which is also being recoded from RUF035 in 0.10.

Test Plan
--
Existing tests with `PreviewMode` removed from the settings.

There was one issue closed on 2024-12-20 calling the rule noisy and
asking for a config option, but the option was added and then there were
no more issues or PRs.
MichaReiser pushed a commit that referenced this pull request Mar 13, 2025
Summary
--

Stabilizes S704, which is also being recoded from RUF035 in 0.10.

Test Plan
--
Existing tests with `PreviewMode` removed from the settings.

There was one issue closed on 2024-12-20 calling the rule noisy and
asking for a config option, but the option was added and then there were
no more issues or PRs.
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 this pull request may close these issues.

4 participants