You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "dhimasardinata (via GitHub)" <gi...@apache.org> on 2026/05/17 09:26:54 UTC
[PR] perf(csv): avoid regex in CSV value escaping [superset]
dhimasardinata opened a new pull request, #40195:
URL: https://github.com/apache/superset/pull/40195
### SUMMARY
Avoid running two regular expressions for every CSV cell passed through `escape_value()`. The CSV export path can call this for every object-valued cell and header, so replacing the regex checks with direct prefix checks keeps the existing CSV injection behavior while reducing per-cell overhead.
The implementation preserves the currently tested cases for formula prefixes, doubled-quote prefixes, leading whitespace, pipe escaping, and negative numeric values.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable.
### TESTING INSTRUCTIONS
Ran locally:
```bash
python3 -m py_compile superset/utils/csv.py
```
Also ran a local microbenchmark comparing the previous regex logic with this implementation over mixed dashboard/CSV export values:
```text
old: per_call_us min/med/max=2.006/2.746/4.318
patched: per_call_us min/med/max=1.617/2.015/2.696
patched_speedup_vs_old=1.36x
```
`python3 -m pytest tests/unit_tests/utils/csv_tests.py -q` was not runnable in this local checkout because `pytest` is not installed.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(csv): avoid regex in CSV value escaping [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40195:
URL: https://github.com/apache/superset/pull/40195#issuecomment-4942560692
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(csv): avoid regex in CSV value escaping [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40195:
URL: https://github.com/apache/superset/pull/40195#discussion_r3254393712
##########
superset/utils/csv.py:
##########
@@ -28,15 +27,27 @@
logger = logging.getLogger(__name__)
-negative_number_re = re.compile(r"^-[0-9.]+$")
+PROBLEMATIC_CSV_PREFIXES = "-@+|=%"
-# This regex will match if the string starts with:
-#
-# 1. one of -, @, +, |, =, %
-# 2. two double quotes immediately followed by one of -, @, +, |, =, %
-# 3. one or more spaces immediately followed by one of -, @, +, |, =, %
-#
-problematic_chars_re = re.compile(r'^(?:"{2}|\s{1,})(?=[\-@+|=%])|^[\-@+|=%]')
+
+def _starts_like_spreadsheet_formula(value: str) -> bool:
+ first = value[0]
+ if first in PROBLEMATIC_CSV_PREFIXES:
+ return True
+ if first == '"' and len(value) > 2:
+ return value[1] == '"' and value[2] in PROBLEMATIC_CSV_PREFIXES
+ if first.isspace():
+ stripped = value.lstrip()
+ return bool(stripped) and stripped[0] in PROBLEMATIC_CSV_PREFIXES
Review Comment:
**Suggestion:** The leading-whitespace branch only checks `stripped[0]` against prefix characters, so values like ` " "=10+2` (whitespace followed by doubled-quote formula prefix, e.g. ` ""=...`) are not escaped. This creates a CSV-injection bypass for the quoted-formula case whenever attackers prepend whitespace. Reuse the same quoted-prefix logic after trimming whitespace (instead of only checking the first trimmed character) so both direct and whitespace-prefixed quoted formulas are escaped. [security]
<details>
<summary><b>Severity Level:</b> Critical ๐จ</summary>
```mdx
- โ CSV chart exports allow spreadsheet formula injection bypass.
- โ Malicious cell formulas execute when analysts open exported CSVs.
- โ ๏ธ Scheduled email reports with CSV attachments inherit this risk.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Create or identify a chart whose underlying dataset has a string column containing a
value like ` ""=10+2` (two leading spaces, then `""=10+2`), so this value appears in the
chart's result DataFrame.
2. Trigger a CSV export for that chart via the public API `GET
/api/v1/chart/<pk>/data/?format=csv&type=full` or the UI "Download as CSV", which is
handled by `ChartDataRestApi.data` in `superset/charts/data/api.py:65-105`. This builds a
`QueryContext` and later calls `_get_data_response`, which eventually calls
`QueryContext.get_data()` as seen in `superset/common/query_actions.py:4-23` and
`superset/common/query_context.py:7-12`.
3. Inside `QueryContextProcessor.get_data`
(`superset/common/query_context_processor.py:245-260`), when `result_format ==
ChartDataResultFormat.CSV`, the code calls `csv.df_to_escaped_csv(df, index=include_index,
**current_app.config["CSV_EXPORT"])`, which in turn applies `escape_value` to all string
headers and object-typed cell values in `superset/utils/csv.py:78-90`.
4. For the malicious cell value ` ""=10+2`, `escape_value` (`superset/utils/csv.py:53-75`)
calls `_starts_like_spreadsheet_formula` (`superset/utils/csv.py:33-42`). In that helper,
the first character is whitespace so the `if first.isspace()` branch at lines 39-41 runs,
`stripped = value.lstrip()` becomes `'""=10+2'`, and the function returns `bool(stripped)
and stripped[0] in PROBLEMATIC_CSV_PREFIXES`. Because `stripped[0]` is `"`, not one of
`-@+|=%` from `PROBLEMATIC_CSV_PREFIXES` (line 30), `_starts_like_spreadsheet_formula`
incorrectly returns `False`. As a result the guard `if
_starts_like_spreadsheet_formula(value) and not _is_negative_number(value):` at lines
62-63 is not taken, the apostrophe quoting is never added, and the CSV cell is written
containing ` ""=10+2`. When a user opens this exported CSV in a spreadsheet (Excel, Google
Sheets), this whitespace-prefixed, double-quoted formula is still interpreted as a
formula, creating a CSV injection vector. This matches the suggestion's claim that
whitespace-prefixed doubled-quote formulas bypass the current escaping because the
whitespace branch only checks `stripped[0]` against the prefix set and never reuses the
quoted-formula logic from lines 37-38 on the stripped value.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Futils%2Fcsv.py%0A%2A%2ALine%3A%2A%2A%2039%3A41%0A%2A%2AComment%3A%2A%2A%0A%09%2ASecurity%3A%20The%20leading-whitespace%20branch%20only%20checks%20%60stripped%5B0%5D%60%20against%20prefix%20characters%2C%20so%20values%20like%20%60%20%22%20%22%3D10%2B2%60%20%28whitespace%20followed%20by%20doubled-quote%20formula%20prefix%2C%20e.g.%20%60%20%20%22%22%3D...%60%29%20are%20not%20escaped.%20This%20creates%20a%20CSV-injection%20bypass%20for%20the%20quoted-formula%20case%20whenever%20attackers%20prepend%20whitespace.%20Reuse%20the%20same%20quoted-prefix%20logic%20after%20trimming%20whitespace%20%28instead%20of%20only%20checking%20the%20first%20trimmed%20character%29%20so%20both%20direct%20and%20whitespace-prefixed%20quoted%20formulas%20are%20escaped.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20corre
ct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Futils%2Fcsv.py%0A%2A%2ALine%3A%2A%2A%2039%3A41%0A%2A%2AComment%3A%2A%2A%0A%09%2ASecurity%3A%20The%20leading-whitespace%20branch%20only%20checks%20%60stripped%5B0%5D%60%20against%20prefix%20characters%2C%20so%20values%20like%20%60%20%22%20%22%3D10%2B2%60%20%28whitespace%20followed%20by%20doubled-quote%20formula%20prefix%2C%20e.g.%20%60%20%20%22%22%3D
...%60%29%20are%20not%20escaped.%20This%20creates%20a%20CSV-injection%20bypass%20for%20the%20quoted-formula%20case%20whenever%20attackers%20prepend%20whitespace.%20Reuse%20the%20same%20quoted-prefix%20logic%20after%20trimming%20whitespace%20%28instead%20of%20only%20checking%20the%20first%20trimmed%20character%29%20so%20both%20direct%20and%20whitespace-prefixed%20quoted%20formulas%20are%20escaped.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/csv.py
**Line:** 39:41
**Comment:**
*Security: The leading-whitespace branch only checks `stripped[0]` against prefix characters, so values like ` " "=10+2` (whitespace followed by doubled-quote formula prefix, e.g. ` ""=...`) are not escaped. This creates a CSV-injection bypass for the quoted-formula case whenever attackers prepend whitespace. Reuse the same quoted-prefix logic after trimming whitespace (instead of only checking the first trimmed character) so both direct and whitespace-prefixed quoted formulas are escaped.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40195&comment_hash=47726552491dc2ef8ae4c23ddb593a0bf2f1404e3558a685b6d6293ed48fe155&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40195&comment_hash=47726552491dc2ef8ae4c23ddb593a0bf2f1404e3558a685b6d6293ed48fe155&reaction=dislike'>๐</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(csv): avoid regex in CSV value escaping [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40195:
URL: https://github.com/apache/superset/pull/40195#issuecomment-4948774546
**Bito Automatic Review Skipped โ PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(csv): avoid regex in CSV value escaping [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40195:
URL: https://github.com/apache/superset/pull/40195#discussion_r3562888643
##########
superset/utils/csv.py:
##########
@@ -27,18 +26,37 @@
logger = logging.getLogger(__name__)
-negative_number_re = re.compile(r"^-[0-9.]+$")
+PROBLEMATIC_CSV_PREFIXES = "-@+|=%"
Review Comment:
**Suggestion:** Add an explicit type annotation to this new module-level constant so it complies with the type-hint requirement for annotatable variables. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is a newly added module-level constant in modified Python code, and it has no type annotation even though it is trivially annotatable as a string. That matches the custom Python type-hint rule.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6378c0beba3e4b588c2d1dc8f562061a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6378c0beba3e4b588c2d1dc8f562061a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/csv.py
**Line:** 29:29
**Comment:**
*Custom Rule: Add an explicit type annotation to this new module-level constant so it complies with the type-hint requirement for annotatable variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40195&comment_hash=375c0dccd2ffb4a6bb4dd3d15e912ad20c5a2863d6d62eb56242d61a4f410f2d&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40195&comment_hash=375c0dccd2ffb4a6bb4dd3d15e912ad20c5a2863d6d62eb56242d61a4f410f2d&reaction=dislike'>๐</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(csv): avoid regex in CSV value escaping [superset]
Posted by "dhimasardinata (via GitHub)" <gi...@apache.org>.
dhimasardinata commented on PR #40195:
URL: https://github.com/apache/superset/pull/40195#issuecomment-4470184960
/resolve
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(csv): avoid regex in CSV value escaping [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40195:
URL: https://github.com/apache/superset/pull/40195#discussion_r3564765099
##########
superset/utils/csv.py:
##########
@@ -27,18 +26,37 @@
logger = logging.getLogger(__name__)
-negative_number_re = re.compile(r"^-[0-9.]+$")
+PROBLEMATIC_CSV_PREFIXES = "-@+|=%"
Review Comment:
Fair, annotated it as str.
##########
superset/utils/csv.py:
##########
@@ -27,18 +26,37 @@
logger = logging.getLogger(__name__)
-negative_number_re = re.compile(r"^-[0-9.]+$")
+PROBLEMATIC_CSV_PREFIXES = "-@+|=%"
-# This regex will match if the string starts with:
-#
-# 1. one of -, @, +, |, =, %, a tab, or a carriage return
-# 2. two double quotes immediately followed by one of -, @, +, |, =, %
-# 3. one or more spaces immediately followed by one of -, @, +, |, =, %
-#
-# A leading tab or carriage return is treated as dangerous on its own because
-# some spreadsheet software trims that leading whitespace and then evaluates
-# the remaining cell content as a formula.
-problematic_chars_re = re.compile(r'^(?:"{2}|\s{1,})(?=[\-@+|=%])|^[\-@+|=%\t\r]')
+
+def _starts_with_formula_prefix(value: str) -> bool:
+ first = value[0]
+ if first in PROBLEMATIC_CSV_PREFIXES:
+ return True
+ if first == '"' and len(value) > 2:
+ return value[1] == '"' and value[2] in PROBLEMATIC_CSV_PREFIXES
+ return False
+
+
+def _starts_like_spreadsheet_formula(value: str) -> bool:
+ # A leading tab or carriage return is treated as dangerous on its own
+ # because some spreadsheet software trims that leading whitespace and
+ # then evaluates the remaining cell content as a formula.
+ first = value[0]
+ if first in ("\t", "\r"):
+ return True
+ if first.isspace():
+ stripped = value.lstrip()
+ return bool(stripped) and _starts_with_formula_prefix(stripped)
+ return _starts_with_formula_prefix(value)
+
+
+def _is_negative_number(value: str) -> bool:
+ return (
+ len(value) > 1
+ and value[0] == "-"
+ and all("0" <= character <= "9" or character == "." for character in value[1:])
+ )
Review Comment:
`-.`, `-..`, `-1.2.3` all matched the old `^-[0-9.]+$` regex too, so this isn't a new gap. This PR is a perf refactor that preserves the prior escaping semantics exactly. Tightening the numeric validation is a separate behavior change, not something to slip into this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(csv): avoid regex in CSV value escaping [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #40195:
URL: https://github.com/apache/superset/pull/40195
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(csv): avoid regex in CSV value escaping [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40195:
URL: https://github.com/apache/superset/pull/40195#discussion_r3562896102
##########
superset/utils/csv.py:
##########
@@ -27,18 +26,37 @@
logger = logging.getLogger(__name__)
-negative_number_re = re.compile(r"^-[0-9.]+$")
+PROBLEMATIC_CSV_PREFIXES = "-@+|=%"
-# This regex will match if the string starts with:
-#
-# 1. one of -, @, +, |, =, %, a tab, or a carriage return
-# 2. two double quotes immediately followed by one of -, @, +, |, =, %
-# 3. one or more spaces immediately followed by one of -, @, +, |, =, %
-#
-# A leading tab or carriage return is treated as dangerous on its own because
-# some spreadsheet software trims that leading whitespace and then evaluates
-# the remaining cell content as a formula.
-problematic_chars_re = re.compile(r'^(?:"{2}|\s{1,})(?=[\-@+|=%])|^[\-@+|=%\t\r]')
+
+def _starts_with_formula_prefix(value: str) -> bool:
+ first = value[0]
+ if first in PROBLEMATIC_CSV_PREFIXES:
+ return True
+ if first == '"' and len(value) > 2:
+ return value[1] == '"' and value[2] in PROBLEMATIC_CSV_PREFIXES
+ return False
+
+
+def _starts_like_spreadsheet_formula(value: str) -> bool:
+ # A leading tab or carriage return is treated as dangerous on its own
+ # because some spreadsheet software trims that leading whitespace and
+ # then evaluates the remaining cell content as a formula.
+ first = value[0]
+ if first in ("\t", "\r"):
+ return True
+ if first.isspace():
+ stripped = value.lstrip()
+ return bool(stripped) and _starts_with_formula_prefix(stripped)
+ return _starts_with_formula_prefix(value)
+
+
+def _is_negative_number(value: str) -> bool:
+ return (
+ len(value) > 1
+ and value[0] == "-"
+ and all("0" <= character <= "9" or character == "." for character in value[1:])
+ )
Review Comment:
**Suggestion:** The negative-number guard is too permissive: it treats any `-` string made only of digits and dots as numeric, so malformed inputs like `-.`, `-..`, or `-1.2.3` bypass escaping. Because `-` is a dangerous spreadsheet prefix, these malformed values should still be escaped; tighten the numeric check to a valid decimal format (for example, require at least one digit and at most one decimal point). [security]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ SQL Lab CSV export leaves malformed negatives unescaped.
- โ Chart CSV export paths share same misclassification risk.
- โ ๏ธ Viz CSV exports may emit unsafe minus-prefixed values.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. In SQL Lab, run a query whose result includes a text column containing a value like
`-.` (this value will be part of the DataFrame used for CSV export via
`superset/commands/sql_lab/export.py:134` which calls `csv.df_to_escaped_csv(df, ...)`).
2. From SQL Lab, click the "Download to CSV" action so the export command at
`superset/commands/sql_lab/export.py:134` invokes `csv.df_to_escaped_csv()` in
`superset/utils/csv.py:87-107`, which applies `escape_value()` to every string cell and
header.
3. During export, `escape_value("-.")` executes in `superset/utils/csv.py:62-84`;
`_starts_like_spreadsheet_formula("-.")` returns True because `"-"` is in
`PROBLEMATIC_CSV_PREFIXES` checked by `_starts_with_formula_prefix()` at
`superset/utils/csv.py:29-38`, and `_is_negative_number("-.")` returns True due to the
permissive digit-or-dot check at `superset/utils/csv.py:54-59` (accepts any sequence of
digits and dots after the leading `-`).
4. Because the guard `if _starts_like_spreadsheet_formula(value) and not
_is_negative_number(value):` at `superset/utils/csv.py:71` sees
`_is_negative_number("-.")` as True, the condition is False, so the cell `-.` is written
to the CSV without a leading quote or pipe escaping; this minus-prefixed, non-numeric
value bypasses the intended spreadsheet formula escaping in all CSV export paths that use
`df_to_escaped_csv()` (viz exports in `superset/viz.py:733`, chart CSVs in
`superset/common/query_context_processor.py:270` and
`superset/charts/client_processing.py:401`, and SQL Lab exports in
`superset/commands/sql_lab/export.py:134`).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=807968fb2f6747bca085ed3611bb1c44&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=807968fb2f6747bca085ed3611bb1c44&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/csv.py
**Line:** 54:59
**Comment:**
*Security: The negative-number guard is too permissive: it treats any `-` string made only of digits and dots as numeric, so malformed inputs like `-.`, `-..`, or `-1.2.3` bypass escaping. Because `-` is a dangerous spreadsheet prefix, these malformed values should still be escaped; tighten the numeric check to a valid decimal format (for example, require at least one digit and at most one decimal point).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40195&comment_hash=a1ed1784a1cf0ce3a7c9c26daafd5b67d7cf45d7073af613f15b50c6185e5a3c&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40195&comment_hash=a1ed1784a1cf0ce3a7c9c26daafd5b67d7cf45d7073af613f15b50c6185e5a3c&reaction=dislike'>๐</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(csv): avoid regex in CSV value escaping [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40195:
URL: https://github.com/apache/superset/pull/40195#issuecomment-4470704432
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(csv): avoid regex in CSV value escaping [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40195:
URL: https://github.com/apache/superset/pull/40195#issuecomment-4470135496
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(csv): avoid regex in CSV value escaping [superset]
Posted by "dhimasardinata (via GitHub)" <gi...@apache.org>.
dhimasardinata commented on code in PR #40195:
URL: https://github.com/apache/superset/pull/40195#discussion_r3254408779
##########
superset/utils/csv.py:
##########
@@ -28,15 +27,27 @@
logger = logging.getLogger(__name__)
-negative_number_re = re.compile(r"^-[0-9.]+$")
+PROBLEMATIC_CSV_PREFIXES = "-@+|=%"
-# This regex will match if the string starts with:
-#
-# 1. one of -, @, +, |, =, %
-# 2. two double quotes immediately followed by one of -, @, +, |, =, %
-# 3. one or more spaces immediately followed by one of -, @, +, |, =, %
-#
-problematic_chars_re = re.compile(r'^(?:"{2}|\s{1,})(?=[\-@+|=%])|^[\-@+|=%]')
+
+def _starts_like_spreadsheet_formula(value: str) -> bool:
+ first = value[0]
+ if first in PROBLEMATIC_CSV_PREFIXES:
+ return True
+ if first == '"' and len(value) > 2:
+ return value[1] == '"' and value[2] in PROBLEMATIC_CSV_PREFIXES
+ if first.isspace():
+ stripped = value.lstrip()
+ return bool(stripped) and stripped[0] in PROBLEMATIC_CSV_PREFIXES
Review Comment:
Addressed in 20b8442 by applying the same quoted-prefix check after trimming leading whitespace and adding a regression test for ` ""=10+2`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(csv): avoid regex in CSV value escaping [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40195:
URL: https://github.com/apache/superset/pull/40195#issuecomment-4479536374
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40195?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `21.05263%` with `15 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.35%. Comparing base ([`8d2b655`](https://app.codecov.io/gh/apache/superset/commit/8d2b655c22d8e678b25f8be1cea8602523bd6f90?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`20b8442`](https://app.codecov.io/gh/apache/superset/commit/20b8442fb009ffe30050cad5c7653edd5ef96f68?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 36 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/40195?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/utils/csv.py](https://app.codecov.io/gh/apache/superset/pull/40195?src=pr&el=tree&filepath=superset%2Futils%2Fcsv.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvY3N2LnB5) | 21.05% | [15 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (8d2b655) and HEAD (20b8442). Click for more details.
>
> <details><summary>HEAD has 196 uploads less than BASE</summary>
>
>| Flag | BASE (8d2b655) | HEAD (20b8442) |
>|------|------|------|
>|python|168|2|
>|presto|16|1|
>|hive|16|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40195 +/- ##
==========================================
- Coverage 64.16% 55.35% -8.81%
==========================================
Files 2591 2591
Lines 138162 138175 +13
Branches 32048 32052 +4
==========================================
- Hits 88647 76488 -12159
- Misses 47986 61024 +13038
+ Partials 1529 663 -866
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40195/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/40195/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.45% <21.05%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40195/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/40195/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/40195/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.14% <21.05%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/40195/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.21% <21.05%> (-19.47%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40195/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/40195/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/40195?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org