Copilot commented on code in PR #40546:
URL: https://github.com/apache/superset/pull/40546#discussion_r3337852730
##########
superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx:
##########
@@ -246,7 +246,7 @@ export const SaveDatasetModal = ({
if (openWindow) {
window.open(url, '_blank', 'noreferrer');
} else {
- window.location.href = url;
+ window.location.href = sanitizeUrl(url);
}
Review Comment:
`createWindow()` still passes the raw `url` into `window.open()`. Since this
helper is the URL navigation sink for this modal, it should sanitize both
branches consistently to fully address the URL-sink finding.
##########
superset-frontend/src/features/databases/DatabaseModal/SqlAlchemyForm.tsx:
##########
@@ -87,7 +88,9 @@ const SqlAlchemyTab = ({
<div className="helper">
{t('Refer to the')}{' '}
<a
- href={fallbackDocsUrl || conf?.SQLALCHEMY_DOCS_URL || ''}
+ href={sanitizeUrl(
+ fallbackDocsUrl || conf?.SQLALCHEMY_DOCS_URL || '',
+ )}
target="_blank"
Review Comment:
The `fallbackDocsUrl`/`fallbackDisplayText` values are always truthy (they
already include hard-coded defaults), so `conf.SQLALCHEMY_DOCS_URL` and
`conf.SQLALCHEMY_DISPLAY_TEXT` are effectively ignored. If `conf` is intended
to override the defaults, it should be prioritized before the fallback values
(and then sanitized).
##########
superset-frontend/packages/superset-ui-core/src/components/ListViewCard/index.tsx:
##########
@@ -140,7 +141,7 @@ const ThinSkeleton = styled(Skeleton)`
const paragraphConfig = { rows: 1, width: 150 };
const AnchorLink: FC<LinkProps> = ({ to, children }) => (
- <a href={to}>{children}</a>
+ <a href={to !== undefined ? sanitizeUrl(to) : undefined}>{children}</a>
);
Review Comment:
`LinkProps.to` is required to be a string, so the `to !== undefined ? ... :
undefined` branch is dead code and makes the intent harder to read. Sanitizing
the required string directly is sufficient.
##########
superset-frontend/package-lock.json:
##########
@@ -49373,7 +49380,7 @@
"react-js-cron": "^5.2.0",
"react-markdown": "^8.0.7",
"react-resize-detector": "^7.1.2",
- "react-syntax-highlighter": "^16.1.0",
+ "react-syntax-highlighter": "^16.1.1",
"react-ultimate-pagination": "^1.3.2",
Review Comment:
`package-lock.json` shows semver bumps (e.g. `react-syntax-highlighter`)
unrelated to adding `@braintree/sanitize-url`, and the workspace entry for
`packages/superset-ui-core` does not list `@braintree/sanitize-url` even though
it was added to that package's `package.json`. This can cause `npm
ci`/workspace installs to fail due to lockfile/package.json mismatch; please
regenerate the lockfile so it only contains the intended dependency changes and
is consistent with all workspace package.json files.
##########
superset-frontend/package-lock.json:
##########
@@ -48900,7 +48907,7 @@
"dependencies": {
"chalk": "^5.6.2",
"lodash-es": "^4.18.1",
- "yeoman-generator": "^8.1.2",
+ "yeoman-generator": "^8.2.2",
"yosay": "^3.0.0"
},
Review Comment:
`package-lock.json` includes an unrelated dependency resolution bump
(`yeoman-generator` ^8.1.2 -> ^8.2.2) as part of this change. If the goal is
only to add `@braintree/sanitize-url`, it’s better to keep the lockfile diff
minimal to reduce supply-chain review surface and make future bisects easier.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
