codeant-ai-for-open-source[bot] commented on code in PR #35098:
URL: https://github.com/apache/superset/pull/35098#discussion_r2584565278
##########
superset-frontend/src/SqlLab/components/QueryTable/index.tsx:
##########
@@ -68,7 +69,7 @@ interface QueryTableProps {
}
const openQuery = (id: number) => {
- const url = `/sqllab?queryId=${id}`;
+ const url = makeUrl(`/sqllab?queryId=${id}`);
window.open(url);
Review Comment:
**Suggestion:** Security (reverse tabnabbing): opening a URL with
window.open(url) without specifying a target and opener protection allows the
opened page to access window.opener and potentially redirect the original page.
Use window.open with target '_blank' and prevent access to window.opener by
passing window features (e.g., 'noopener,noreferrer') so the newly opened page
cannot manipulate the opener. [security]
**Severity Level:** Critical 🚨
```suggestion
window.open(url, '_blank', 'noopener,noreferrer');
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The existing code calls window.open(url) which can expose the opener to the
newly opened page in some browsers. The suggested change (adding '_blank' and
'noopener,noreferrer') prevents reverse tabnabbing by ensuring the opened
window can't access or manipulate window.opener. This is a real security
hardening (not just stylistic) and is safe and self-contained in this PR
context.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/QueryTable/index.tsx
**Line:** 73:73
**Comment:**
*Security: Security (reverse tabnabbing): opening a URL with
window.open(url) without specifying a target and opener protection allows the
opened page to access window.opener and potentially redirect the original page.
Use window.open with target '_blank' and prevent access to window.opener by
passing window features (e.g., 'noopener,noreferrer') so the newly opened page
cannot manipulate the opener.
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.
```
</details>
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -33,7 +33,7 @@ import {
TelemetryPixel,
} from '@superset-ui/core/components';
import type { ItemType, MenuItem } from '@superset-ui/core/components/Menu';
-import { ensureAppRoot } from 'src/utils/pathUtils';
+import { ensureAppRoot, makeUrl } from 'src/utils/pathUtils';
Review Comment:
**Suggestion:** The new import added `makeUrl` but if you revert the
menu.url change (as suggested) `makeUrl` becomes unused. Keep imports minimal
to avoid unused-import lint errors and confusion: only import `ensureAppRoot`
here since other code in this file calls `ensureAppRoot` directly. [code
quality]
**Severity Level:** Minor ⚠️
```suggestion
import { ensureAppRoot } from 'src/utils/pathUtils';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid code-quality suggestion but conditional: as the file
currently stands makeUrl is used (menu.url uses makeUrl). If you apply
suggestion #1 (revert menu.url to the raw route), then makeUrl will become
unused and should be removed to avoid lint errors and confusion. Removing the
unused import is a small, correct cleanup.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/home/RightMenu.tsx
**Line:** 36:36
**Comment:**
*Code Quality: The new import added `makeUrl` but if you revert the
menu.url change (as suggested) `makeUrl` becomes unused. Keep imports minimal
to avoid unused-import lint errors and confusion: only import `ensureAppRoot`
here since other code in this file calls `ensureAppRoot` directly.
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.
```
</details>
##########
superset-frontend/src/features/home/EmptyState.tsx:
##########
@@ -58,7 +59,7 @@ const REDIRECTS = {
create: {
[WelcomeTable.Charts]: '/chart/add',
Review Comment:
**Suggestion:** The create redirect '/chart/add' is a raw path and will not
include the application root for subdirectory deployments; wrap it with
`makeUrl` so the navigation URL is correct when the app is served from a
subdirectory. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
[WelcomeTable.Charts]: makeUrl('/chart/add'),
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Using a raw '/chart/add' path will break when the app is served from a
subdirectory. Converting to makeUrl('/chart/add') ensures the application root
is prepended and is consistent with how SavedQueries.create is handled.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/home/EmptyState.tsx
**Line:** 60:60
**Comment:**
*Logic Error: The create redirect '/chart/add' is a raw path and will
not include the application root for subdirectory deployments; wrap it with
`makeUrl` so the navigation URL is correct when the app is served from a
subdirectory.
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.
```
</details>
##########
superset-frontend/src/SqlLab/components/ResultSet/index.tsx:
##########
@@ -315,7 +315,7 @@ const ResultSet = ({
};
const getExportCsvUrl = (clientId: string) =>
- ensureAppRoot(`/api/v1/sqllab/export/${clientId}/`);
+ makeUrl(`/api/v1/sqllab/export/${clientId}/`);
Review Comment:
**Suggestion:** Path injection / malformed-URL risk: `clientId` is
interpolated directly into the path passed to `makeUrl`. If `clientId` contains
unexpected characters (slashes, spaces, query chars), the resulting URL can be
malformed or allow unintentional path manipulation. Apply URL-encoding to
`clientId` (encodeURIComponent) before building the URL. [security]
**Severity Level:** Critical 🚨
```suggestion
makeUrl(`/api/v1/sqllab/export/${encodeURIComponent(clientId)}/`);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
URL-encoding the clientId is a small, useful hardening: if clientId ever
contains characters like slashes, spaces or query chars it could produce a
malformed path. The current code interpolates raw input into a path;
encodeURIComponent(clientId) is safe and doesn't change expected behavior for
normal IDs (UUIDs/numeric/nanoid) while preventing accidental path
manipulation. This is a real, low-risk security/robustness improvement.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/ResultSet/index.tsx
**Line:** 318:318
**Comment:**
*Security: Path injection / malformed-URL risk: `clientId` is
interpolated directly into the path passed to `makeUrl`. If `clientId` contains
unexpected characters (slashes, spaces, query chars), the resulting URL can be
malformed or allow unintentional path manipulation. Apply URL-encoding to
`clientId` (encodeURIComponent) before building the URL.
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.
```
</details>
##########
superset-frontend/src/explore/components/controls/ViewQuery.tsx:
##########
@@ -137,7 +138,9 @@ const ViewQuery: FC<ViewQueryProps> = props => {
if (domEvent.metaKey || domEvent.ctrlKey) {
domEvent.preventDefault();
window.open(
-
`/sqllab?datasourceKey=${datasource}&sql=${encodeURIComponent(currentSQL)}`,
+ makeUrl(
+
`/sqllab?datasourceKey=${datasource}&sql=${encodeURIComponent(currentSQL)}`,
Review Comment:
**Suggestion:** The URL constructed for opening SQL Lab includes
`datasource` raw; if `datasource` contains characters that must be URL-encoded
the resulting URL can be malformed. Encode `datasource` with
`encodeURIComponent` before interpolating it into the query string passed to
`makeUrl(...)`. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
`/sqllab?datasourceKey=${encodeURIComponent(datasource ??
'')}&sql=${encodeURIComponent(currentSQL)}`,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a straightforward bug: interpolating datasource directly into a
query string can break URLs if it contains characters like '&', '?', or '#'.
Using encodeURIComponent(datasource ?? '') is the correct and minimal fix to
avoid malformed URLs and potential parameter injection.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/ViewQuery.tsx
**Line:** 142:142
**Comment:**
*Possible Bug: The URL constructed for opening SQL Lab includes
`datasource` raw; if `datasource` contains characters that must be URL-encoded
the resulting URL can be malformed. Encode `datasource` with
`encodeURIComponent` before interpolating it into the query string passed to
`makeUrl(...)`.
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.
```
</details>
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -201,7 +201,7 @@ const RightMenu = ({
},
{
label: t('SQL query'),
- url: '/sqllab?new=true',
+ url: makeUrl('/sqllab?new=true'),
icon: <Icons.SearchOutlined data-test={`menu-item-${t('SQL query')}`} />,
Review Comment:
**Suggestion:** Using makeUrl to pre-prefix the SQL query URL stores an
application-root-prefixed path into `menu.url`. Later code expects `menu.url`
to be a raw route and calls `ensureAppRoot(menu.url)` when rendering external
links (or passes it to `isFrontendRoute`). Because `ensureAppRoot` always
prefixes the application root, this causes double-prefixing (e.g.
'/superset/superset/sqllab...') or incorrect frontend-route detection in
subdirectory deployments. Use the raw route here (e.g., '/sqllab?new=true') and
let the existing rendering logic call `ensureAppRoot` where needed. [logic
error]
**Severity Level:** Minor ⚠️
```suggestion
url: '/sqllab?new=true',
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The suggestion is correct and fixes a real logic bug. makeUrl() is just a
thin wrapper that calls ensureAppRoot(path) (see src/utils/pathUtils.ts where
makeUrl returns ensureAppRoot(path)). In RightMenu.buildNewDropdownItems the
code later calls ensureAppRoot(menu.url) when rendering non-frontend links (and
also passes menu.url to isFrontendRoute). If menu.url is already prefixed via
makeUrl, calling ensureAppRoot again double-prefixes the path (e.g.
'/superset/sqllab' -> '/superset/superset/sqllab') and can also break
frontend-route detection. I verified makeUrl usage and implementation
(pathUtils.ts) and the RightMenu rendering sites that wrap menu.url with
ensureAppRoot, so keeping the raw route ('/sqllab?new=true') here is the
correct fix.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/home/RightMenu.tsx
**Line:** 205:205
**Comment:**
*Logic Error: Using makeUrl to pre-prefix the SQL query URL stores an
application-root-prefixed path into `menu.url`. Later code expects `menu.url`
to be a raw route and calls `ensureAppRoot(menu.url)` when rendering external
links (or passes it to `isFrontendRoute`). Because `ensureAppRoot` always
prefixes the application root, this causes double-prefixing (e.g.
'/superset/superset/sqllab...') or incorrect frontend-route detection in
subdirectory deployments. Use the raw route here (e.g., '/sqllab?new=true') and
let the existing rendering logic call `ensureAppRoot` where needed.
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.
```
</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: [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]
