fix(jinja): apply consistent escaping to url_param values from request args#40633
Conversation
Code Review Agent Run #2d9e16Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40633 +/- ##
=======================================
Coverage 64.05% 64.05%
=======================================
Files 2662 2662
Lines 143254 143254
Branches 32941 32941
=======================================
+ Hits 91764 91766 +2
+ Misses 49903 49902 -1
+ Partials 1587 1586 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes inconsistent escaping in ExtraCache.url_param(): previously values from request query string took an early return that bypassed dialect-specific literal escaping applied to form-data values. Now both sources flow through the same escaping path, honoring escape_result=False.
Changes:
- Removed early-return in
url_param()so request-args values pass through the shared escaping block. - Added two unit tests covering escaped and unescaped request-args cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| superset/jinja_context.py | Refactor url_param so request-args and form-data values share the same escaping logic. |
| tests/unit_tests/jinja_context_test.py | New tests verifying escape behavior for request-args values. |
sha174n
added
the
merge-if-green
…t args `url_param()` escaped values sourced from `form_data["url_params"]` using the dialect's literal processor, but returned values read from the request query string unescaped via an early return. Both sources are interpolated into the rendered SQL, so route them through the same escaping (still honoring `escape_result=False`). Adds tests covering the request-args path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rusackas
force-pushed
the
fix/jinja-url-param-consistent-escaping
branch
from
0af6515 to
754759b
Compare
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rusackas
moved this from Needs Review
to Approved and/or Merged
in Superset Review Help Wanted
Code Review Agent Run #96528cActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
SUMMARY
ExtraCache.url_param()escapes values that come fromform_data["url_params"]using the dialect's literal processor, but values read from the request query string took an early return that skipped that escaping. Both sources are interpolated into the rendered SQL, so this routes them through the same escaping path (still honoringescape_result=Falsefor callers that opt out). It also means request-args values now consistently participate in the cache key.BEFORE / AFTER
url_param('foo')with?foo=O'Brienon a dialect that doubles quotes:O'Brien(raw, from the request-args early return)O''Brien(escaped, same as the form-data path)TESTING INSTRUCTIONS
Adds
test_url_param_escaped_request_argsandtest_url_param_unescaped_request_args(12/12 url_param tests pass).ADDITIONAL INFORMATION
🤖 Generated with Claude Code