Copilot commented on code in PR #38316:
URL: https://github.com/apache/superset/pull/38316#discussion_r2870558826
##########
superset-frontend/src/utils/pathUtils.ts:
##########
@@ -21,23 +21,33 @@ import { applicationRoot } from
'src/utils/getBootstrapData';
/**
* Takes a string path to a resource and prefixes it with the application root
that is
* defined in the application configuration. The application path is sanitized.
- * @param path A string path to a resource
+ *
+ * Absolute URLs (e.g. https://..., ftp://..., mailto:...) and
protocol-relative
+ * URLs (e.g. //example.com) are returned unchanged — only relative paths
receive
+ * the application root prefix.
Review Comment:
The JSDoc still says "The application path is sanitized", but
`ensureAppRoot()` only normalizes/prefixes and now may also return absolute
URLs unchanged. Consider updating this wording to avoid implying any
sanitization of user-controlled URL content is happening here.
##########
superset-frontend/src/utils/pathUtils.ts:
##########
@@ -21,23 +21,33 @@ import { applicationRoot } from
'src/utils/getBootstrapData';
/**
* Takes a string path to a resource and prefixes it with the application root
that is
* defined in the application configuration. The application path is sanitized.
- * @param path A string path to a resource
+ *
+ * Absolute URLs (e.g. https://..., ftp://..., mailto:...) and
protocol-relative
+ * URLs (e.g. //example.com) are returned unchanged — only relative paths
receive
+ * the application root prefix.
+ *
+ * @param path A string path or URL to a resource
*/
export function ensureAppRoot(path: string): string {
+ if (/^[a-zA-Z][a-zA-Z0-9+\-.]*:/.test(path) || path.startsWith('//')) {
+ return path;
+ }
return `${applicationRoot()}${path.startsWith('/') ? path : `/${path}`}`;
Review Comment:
The regex literal is re-created on every `ensureAppRoot()` call. Since this
helper may be called frequently during render/navigation, consider hoisting the
scheme RegExp to a module-level constant to avoid repeated compilation.
##########
superset-frontend/src/utils/pathUtils.ts:
##########
@@ -21,23 +21,33 @@ import { applicationRoot } from
'src/utils/getBootstrapData';
/**
* Takes a string path to a resource and prefixes it with the application root
that is
* defined in the application configuration. The application path is sanitized.
- * @param path A string path to a resource
+ *
+ * Absolute URLs (e.g. https://..., ftp://..., mailto:...) and
protocol-relative
+ * URLs (e.g. //example.com) are returned unchanged — only relative paths
receive
+ * the application root prefix.
+ *
+ * @param path A string path or URL to a resource
*/
export function ensureAppRoot(path: string): string {
+ if (/^[a-zA-Z][a-zA-Z0-9+\-.]*:/.test(path) || path.startsWith('//')) {
Review Comment:
The new scheme check treats any `<scheme>:` prefix as an absolute URL, which
will also pass through potentially dangerous schemes like `javascript:` and
`data:` unchanged. Since `ensureAppRoot()` is used to build `href`/navigation
targets in the UI, consider explicitly blocking these schemes (or restricting
passthrough to an allowlist such as http/https/mailto/tel) to avoid introducing
an XSS vector if any caller ever passes untrusted input.
```suggestion
* Absolute URLs with safe schemes (e.g. https://..., http://...,
mailto:..., tel:...)
* and protocol-relative URLs (e.g. //example.com) are returned unchanged —
only
* relative paths receive the application root prefix. Potentially dangerous
schemes
* such as javascript: and data: are not treated as absolute and will be
prefixed.
*
* @param path A string path or URL to a resource
*/
export function ensureAppRoot(path: string): string {
// Only allow well-known safe schemes to pass through unchanged. Other
inputs
// (including javascript: and data:) are treated as relative paths.
if (/^(https?|mailto|tel):/i.test(path) || path.startsWith('//')) {
```
--
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]
