fix(#9480): apply proper quote escapes for the datum access expression #9479
Conversation
Deploying vega-lite with
|
| Latest commit: |
29a88bc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c68e1d5d.vega-lite.pages.dev |
| Branch Preview URL: | https://fix-timeunitbandpositionexpr.vega-lite.pages.dev |
|
Can you link to the issue? |
Done! (edited the PR description) |
yhoonkim
changed the title
|
If you say "fixes" or "closes" before it, GitHub will link it and auto close the issue when the pull request is merged. |
yhoonkim
changed the title
src/util.ts
Outdated
| @@ -297,6 +297,10 @@ function escapePathAccess(string: string) { | |||
| return string.replace(/(\[|\]|\.|'|")/g, '\\$1'); | |||
| } | |||
|
|
|||
| export function escapeSingleQuotes(value: string) { | |||
There was a problem hiding this comment.
It seems like a code smell that we need to add a new escapeSingleQuotes utility method.
Shouldn't we already have another escape utility in other places? (Also, shouldn't the escape handle all characters that need escaping, not just "single quotes"?)
There was a problem hiding this comment.
I tried other utility functions to re-escape the field name but could not find a good one. Maybe I will try to find again and ping you if I can't find.
There was a problem hiding this comment.
There may be some similar logic in Vega (utils).
There was a problem hiding this comment.
Ended up with creating more generic function, accessWithDatumToUnescapedPath, to solve this (datum['${field}'], datum["${field}"]) case.
accessWithDatumToUnescapedPath is taking un-escaped string to build the datum-access expression, where it only escapes the single quotes as datum['${}'] using the single quotes.
yhoonkim
force-pushed
the
fix-timeunitBandPositionExpr
branch
2 times, most recently
from
fff4bef to
29a88bc
Compare
yhoonkim
changed the title
| @@ -556,7 +559,7 @@ function errorBarAggregationAndCalculation< | |||
| for (const postAggregateCalculate of postAggregateCalculates) { | |||
| tooltipSummary.push({ | |||
| fieldPrefix: postAggregateCalculate.as.substring(0, 6), | |||
| titlePrefix: replaceAll(replaceAll(postAggregateCalculate.calculate, 'datum["', ''), '"]', '') | |||
| titlePrefix: replaceAll(replaceAll(postAggregateCalculate.calculate, "datum['", ''), "']", '') | |||
There was a problem hiding this comment.
do we use accessWithDatumToUnescapedPath here?
There was a problem hiding this comment.
Yes.
-
The existing logic peel off
datum["and"]from thecalculateto create a prefix.
E.g.,datum["yh"]->yh. -
I replaced the double quote datum expression with
accessWithDatumToUnescapedPathusing the single quote. -
To keep the existing logic, I changed the code to peel off
datum['and']
PR Description
Problem
The expression like
which is vulnerable when
strhas double quotes, such as'hello, "world"'.The expression with single quotes is the same
which is vulnerable when
strhas single quotes, such as"Vega's Favorite".Solution
This PR fixes the existing datum access expression to have a proper quote escape by calling
accessWithDatumToUnescapedPathIt fixes the issue of timeunit band position transforms which don't escape the field name correctly.
#9480