sadpandajoe commented on code in PR #39925:
URL: https://github.com/apache/superset/pull/39925#discussion_r3331508216
##########
superset-frontend/src/utils/navigationUtils.ts:
##########
@@ -16,29 +16,204 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ensureAppRoot } from './pathUtils';
+import {
+ createElement,
+ type AnchorHTMLAttributes,
+ type ReactElement,
+} from 'react';
+import { ensureAppRoot, makeUrl, stripAppRoot } from './pathUtils';
-export const navigateTo = (
+// Re-export so callers that legitimately need a raw prefixed path (native
+// fetch, navigator.sendBeacon, image src, third-party `href` props) have a
+// single sanctioned import location. The static-invariant scan disallows
+// importing from `pathUtils` directly outside this module.
+export { ensureAppRoot, makeUrl, stripAppRoot };
+
+// The guard helpers are declared before `navigateTo` / `navigateWithState`
+// so oxlint's no-use-before-define lint (which does not honour function-
+// declaration hoisting) does not fire on the wired-up imperative-nav
+// path. The focused helpers below (`openInNewTab`, `getShareableUrl`,
+// `AppLink`) also reach for `assertSafeNavigationUrl` directly.
+
+const NEW_TAB_FEATURES = 'noopener noreferrer';
+
+// Allow-list of safe URL shapes for navigation: router-relative paths and a
+// small set of known-safe schemes. `ensureAppRoot` already neutralises
+// `javascript:` / `data:` by prefixing them as relative paths; protocol-
+// relative `//host` is intentionally excluded here because it is a cross-
+// origin navigation primitive that previously enabled open redirects.
+//
+// nit-3 / AF-1 hardening: the leading-slash branch also rejects any URL
+// containing a backslash anywhere — browsers normalise `/\evil.com` →
+// `//evil.com` in the special-scheme authority, so backslashes in any
+// position let an attacker craft a path that looks router-relative until
+// the browser parses it. http(s)/ftp URLs with userinfo (`@` before the
+// first `/` after `//`) are rejected by the post-regex authority check
+// below, since `https://[email protected]` resolves to the host `evil.com`
+// despite presenting as a same-origin-looking URL to the eye.
+const SAFE_NAVIGATION_URL_RE = /^(?:\/(?!\/)|https?:|ftp:|mailto:|tel:)/i;
+const USERINFO_BEARING_SCHEME_RE = /^(?:https?|ftp):/i;
+
+function assertSafeNavigationUrl(url: string): string {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: only relative paths and ' +
+ 'http(s):, ftp:, mailto:, tel: schemes are allowed.',
+ );
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ let parsed: URL;
+ try {
+ parsed = new URL(url);
+ } catch {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: unparseable authority.',
+ );
+ }
+ if (parsed.username || parsed.password) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: ' +
+ 'http(s)/ftp URLs with userinfo are not allowed.',
+ );
+ }
+ }
+ return url;
+}
+
+// Inline regex predicate. Mirrors `assertSafeNavigationUrl` exactly but as
+// a boolean test so CodeQL's data-flow analysis recognises it as a
+// sanitiser barrier at each `window.*` sink in `navigateTo` /
+// `navigateWithState`. Through-function sanitisers (`assertSafeNavigationUrl`)
+// are not in CodeQL's default model and tripped `js/client-side-*` even
+// though the throw path guaranteed safety.
+function isSafeNavigationUrl(url: string): boolean {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ return false;
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ try {
+ const parsed = new URL(url);
+ if (parsed.username || parsed.password) {
+ return false;
+ }
+ } catch {
+ return false;
+ }
+ }
+ return true;
+}
+
+/**
+ * Imperative full-page navigation. Internal entry point for `redirect()`
+ * and a handful of legacy callers; new code should prefer `<AppLink>` or
+ * `redirect()`. Unsafe URLs (protocol-relative, backslash-laden, userinfo-
+ * carrying http(s)) fall back to `ensureAppRoot('/')` with a `console.error`
+ * — never a silent navigation to the rejected target.
+ *
+ * @internal
+ */
+export function navigateTo(
url: string,
options?: { newWindow?: boolean; assign?: boolean },
-) => {
- if (options?.newWindow) {
- window.open(ensureAppRoot(url), '_blank', 'noopener noreferrer');
- } else if (options?.assign) {
- window.location.assign(ensureAppRoot(url));
- } else {
- window.location.href = ensureAppRoot(url);
+): void {
+ const target = ensureAppRoot(url);
+ // Inline regex check (not a function call) so CodeQL's data-flow
+ // analysis recognises the sanitiser barrier and propagates `target` as
+ // safe at each sink below. Mirrors `isSafeNavigationUrl` precisely.
+ if (
+ SAFE_NAVIGATION_URL_RE.test(target) &&
+ !target.includes('\\') &&
+ (!USERINFO_BEARING_SCHEME_RE.test(target) || isSafeNavigationUrl(target))
+ ) {
+ if (options?.newWindow) {
+ window.open(target, '_blank', NEW_TAB_FEATURES);
+ } else if (options?.assign) {
+ window.location.assign(target);
Review Comment:
Addressed in 54085f4de3: refactored `navigateTo` / `navigateWithState` so
each `window.*` sink lives inside an if-block gated by pure inline barriers
(`String.startsWith`, `String.includes`, constant equality on `URL.protocol`,
property reads on `URL.username` / `URL.password`).
The prior shape composed the guard as `SAFE_NAVIGATION_URL_RE.test(target)
&& !target.includes('\\') && (!USERINFO_BEARING_SCHEME_RE.test(target) ||
isSafeNavigationUrl(target))`. CodeQL's data-flow model didn't propagate the
trailing disjunction (the `isSafeNavigationUrl(...)` call) as a sanitiser, so
the sink still tracked the source-tainted `target`.
The new shape splits the work into a router-relative fast-path
(`startsWith('/') && !startsWith('//') && !includes('\\')` — the canonical
barrier triple) and an external-URL path (`new URL(target)` + literal-equality
scheme check + `!parsed.username && !parsed.password`, feeding `parsed.href` to
the sink). Backslash-laden authorities (AF-1) and userinfo-bearing URLs (nit-3)
are still rejected; the fallback to `ensureAppRoot('/')` is preserved with the
same inline barrier triple. Behaviour is unchanged. Resolving.
##########
superset-frontend/src/utils/navigationUtils.ts:
##########
@@ -16,29 +16,204 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ensureAppRoot } from './pathUtils';
+import {
+ createElement,
+ type AnchorHTMLAttributes,
+ type ReactElement,
+} from 'react';
+import { ensureAppRoot, makeUrl, stripAppRoot } from './pathUtils';
-export const navigateTo = (
+// Re-export so callers that legitimately need a raw prefixed path (native
+// fetch, navigator.sendBeacon, image src, third-party `href` props) have a
+// single sanctioned import location. The static-invariant scan disallows
+// importing from `pathUtils` directly outside this module.
+export { ensureAppRoot, makeUrl, stripAppRoot };
+
+// The guard helpers are declared before `navigateTo` / `navigateWithState`
+// so oxlint's no-use-before-define lint (which does not honour function-
+// declaration hoisting) does not fire on the wired-up imperative-nav
+// path. The focused helpers below (`openInNewTab`, `getShareableUrl`,
+// `AppLink`) also reach for `assertSafeNavigationUrl` directly.
+
+const NEW_TAB_FEATURES = 'noopener noreferrer';
+
+// Allow-list of safe URL shapes for navigation: router-relative paths and a
+// small set of known-safe schemes. `ensureAppRoot` already neutralises
+// `javascript:` / `data:` by prefixing them as relative paths; protocol-
+// relative `//host` is intentionally excluded here because it is a cross-
+// origin navigation primitive that previously enabled open redirects.
+//
+// nit-3 / AF-1 hardening: the leading-slash branch also rejects any URL
+// containing a backslash anywhere — browsers normalise `/\evil.com` →
+// `//evil.com` in the special-scheme authority, so backslashes in any
+// position let an attacker craft a path that looks router-relative until
+// the browser parses it. http(s)/ftp URLs with userinfo (`@` before the
+// first `/` after `//`) are rejected by the post-regex authority check
+// below, since `https://[email protected]` resolves to the host `evil.com`
+// despite presenting as a same-origin-looking URL to the eye.
+const SAFE_NAVIGATION_URL_RE = /^(?:\/(?!\/)|https?:|ftp:|mailto:|tel:)/i;
+const USERINFO_BEARING_SCHEME_RE = /^(?:https?|ftp):/i;
+
+function assertSafeNavigationUrl(url: string): string {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: only relative paths and ' +
+ 'http(s):, ftp:, mailto:, tel: schemes are allowed.',
+ );
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ let parsed: URL;
+ try {
+ parsed = new URL(url);
+ } catch {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: unparseable authority.',
+ );
+ }
+ if (parsed.username || parsed.password) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: ' +
+ 'http(s)/ftp URLs with userinfo are not allowed.',
+ );
+ }
+ }
+ return url;
+}
+
+// Inline regex predicate. Mirrors `assertSafeNavigationUrl` exactly but as
+// a boolean test so CodeQL's data-flow analysis recognises it as a
+// sanitiser barrier at each `window.*` sink in `navigateTo` /
+// `navigateWithState`. Through-function sanitisers (`assertSafeNavigationUrl`)
+// are not in CodeQL's default model and tripped `js/client-side-*` even
+// though the throw path guaranteed safety.
+function isSafeNavigationUrl(url: string): boolean {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ return false;
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ try {
+ const parsed = new URL(url);
+ if (parsed.username || parsed.password) {
+ return false;
+ }
+ } catch {
+ return false;
+ }
+ }
+ return true;
+}
+
+/**
+ * Imperative full-page navigation. Internal entry point for `redirect()`
+ * and a handful of legacy callers; new code should prefer `<AppLink>` or
+ * `redirect()`. Unsafe URLs (protocol-relative, backslash-laden, userinfo-
+ * carrying http(s)) fall back to `ensureAppRoot('/')` with a `console.error`
+ * — never a silent navigation to the rejected target.
+ *
+ * @internal
+ */
+export function navigateTo(
url: string,
options?: { newWindow?: boolean; assign?: boolean },
-) => {
- if (options?.newWindow) {
- window.open(ensureAppRoot(url), '_blank', 'noopener noreferrer');
- } else if (options?.assign) {
- window.location.assign(ensureAppRoot(url));
- } else {
- window.location.href = ensureAppRoot(url);
+): void {
+ const target = ensureAppRoot(url);
+ // Inline regex check (not a function call) so CodeQL's data-flow
+ // analysis recognises the sanitiser barrier and propagates `target` as
+ // safe at each sink below. Mirrors `isSafeNavigationUrl` precisely.
+ if (
+ SAFE_NAVIGATION_URL_RE.test(target) &&
+ !target.includes('\\') &&
+ (!USERINFO_BEARING_SCHEME_RE.test(target) || isSafeNavigationUrl(target))
+ ) {
+ if (options?.newWindow) {
+ window.open(target, '_blank', NEW_TAB_FEATURES);
+ } else if (options?.assign) {
+ window.location.assign(target);
Review Comment:
Addressed in 54085f4de3: refactored `navigateTo` / `navigateWithState` so
each `window.*` sink lives inside an if-block gated by pure inline barriers
(`String.startsWith`, `String.includes`, constant equality on `URL.protocol`,
property reads on `URL.username` / `URL.password`).
The prior shape composed the guard as `SAFE_NAVIGATION_URL_RE.test(target)
&& !target.includes('\\') && (!USERINFO_BEARING_SCHEME_RE.test(target) ||
isSafeNavigationUrl(target))`. CodeQL's data-flow model didn't propagate the
trailing disjunction (the `isSafeNavigationUrl(...)` call) as a sanitiser, so
the sink still tracked the source-tainted `target`.
The new shape splits the work into a router-relative fast-path
(`startsWith('/') && !startsWith('//') && !includes('\\')` — the canonical
barrier triple) and an external-URL path (`new URL(target)` + literal-equality
scheme check + `!parsed.username && !parsed.password`, feeding `parsed.href` to
the sink). Backslash-laden authorities (AF-1) and userinfo-bearing URLs (nit-3)
are still rejected; the fallback to `ensureAppRoot('/')` is preserved with the
same inline barrier triple. Behaviour is unchanged. Resolving.
##########
superset-frontend/src/utils/navigationUtils.ts:
##########
@@ -16,29 +16,204 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ensureAppRoot } from './pathUtils';
+import {
+ createElement,
+ type AnchorHTMLAttributes,
+ type ReactElement,
+} from 'react';
+import { ensureAppRoot, makeUrl, stripAppRoot } from './pathUtils';
-export const navigateTo = (
+// Re-export so callers that legitimately need a raw prefixed path (native
+// fetch, navigator.sendBeacon, image src, third-party `href` props) have a
+// single sanctioned import location. The static-invariant scan disallows
+// importing from `pathUtils` directly outside this module.
+export { ensureAppRoot, makeUrl, stripAppRoot };
+
+// The guard helpers are declared before `navigateTo` / `navigateWithState`
+// so oxlint's no-use-before-define lint (which does not honour function-
+// declaration hoisting) does not fire on the wired-up imperative-nav
+// path. The focused helpers below (`openInNewTab`, `getShareableUrl`,
+// `AppLink`) also reach for `assertSafeNavigationUrl` directly.
+
+const NEW_TAB_FEATURES = 'noopener noreferrer';
+
+// Allow-list of safe URL shapes for navigation: router-relative paths and a
+// small set of known-safe schemes. `ensureAppRoot` already neutralises
+// `javascript:` / `data:` by prefixing them as relative paths; protocol-
+// relative `//host` is intentionally excluded here because it is a cross-
+// origin navigation primitive that previously enabled open redirects.
+//
+// nit-3 / AF-1 hardening: the leading-slash branch also rejects any URL
+// containing a backslash anywhere — browsers normalise `/\evil.com` →
+// `//evil.com` in the special-scheme authority, so backslashes in any
+// position let an attacker craft a path that looks router-relative until
+// the browser parses it. http(s)/ftp URLs with userinfo (`@` before the
+// first `/` after `//`) are rejected by the post-regex authority check
+// below, since `https://[email protected]` resolves to the host `evil.com`
+// despite presenting as a same-origin-looking URL to the eye.
+const SAFE_NAVIGATION_URL_RE = /^(?:\/(?!\/)|https?:|ftp:|mailto:|tel:)/i;
+const USERINFO_BEARING_SCHEME_RE = /^(?:https?|ftp):/i;
+
+function assertSafeNavigationUrl(url: string): string {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: only relative paths and ' +
+ 'http(s):, ftp:, mailto:, tel: schemes are allowed.',
+ );
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ let parsed: URL;
+ try {
+ parsed = new URL(url);
+ } catch {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: unparseable authority.',
+ );
+ }
+ if (parsed.username || parsed.password) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: ' +
+ 'http(s)/ftp URLs with userinfo are not allowed.',
+ );
+ }
+ }
+ return url;
+}
+
+// Inline regex predicate. Mirrors `assertSafeNavigationUrl` exactly but as
+// a boolean test so CodeQL's data-flow analysis recognises it as a
+// sanitiser barrier at each `window.*` sink in `navigateTo` /
+// `navigateWithState`. Through-function sanitisers (`assertSafeNavigationUrl`)
+// are not in CodeQL's default model and tripped `js/client-side-*` even
+// though the throw path guaranteed safety.
+function isSafeNavigationUrl(url: string): boolean {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ return false;
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ try {
+ const parsed = new URL(url);
+ if (parsed.username || parsed.password) {
+ return false;
+ }
+ } catch {
+ return false;
+ }
+ }
+ return true;
+}
+
+/**
+ * Imperative full-page navigation. Internal entry point for `redirect()`
+ * and a handful of legacy callers; new code should prefer `<AppLink>` or
+ * `redirect()`. Unsafe URLs (protocol-relative, backslash-laden, userinfo-
+ * carrying http(s)) fall back to `ensureAppRoot('/')` with a `console.error`
+ * — never a silent navigation to the rejected target.
+ *
+ * @internal
+ */
+export function navigateTo(
url: string,
options?: { newWindow?: boolean; assign?: boolean },
-) => {
- if (options?.newWindow) {
- window.open(ensureAppRoot(url), '_blank', 'noopener noreferrer');
- } else if (options?.assign) {
- window.location.assign(ensureAppRoot(url));
- } else {
- window.location.href = ensureAppRoot(url);
+): void {
+ const target = ensureAppRoot(url);
+ // Inline regex check (not a function call) so CodeQL's data-flow
+ // analysis recognises the sanitiser barrier and propagates `target` as
+ // safe at each sink below. Mirrors `isSafeNavigationUrl` precisely.
+ if (
+ SAFE_NAVIGATION_URL_RE.test(target) &&
+ !target.includes('\\') &&
+ (!USERINFO_BEARING_SCHEME_RE.test(target) || isSafeNavigationUrl(target))
+ ) {
+ if (options?.newWindow) {
+ window.open(target, '_blank', NEW_TAB_FEATURES);
+ } else if (options?.assign) {
+ window.location.assign(target);
+ } else {
+ window.location.href = target;
Review Comment:
Addressed in 54085f4de3: refactored `navigateTo` / `navigateWithState` so
each `window.*` sink lives inside an if-block gated by pure inline barriers
(`String.startsWith`, `String.includes`, constant equality on `URL.protocol`,
property reads on `URL.username` / `URL.password`).
The prior shape composed the guard as `SAFE_NAVIGATION_URL_RE.test(target)
&& !target.includes('\\') && (!USERINFO_BEARING_SCHEME_RE.test(target) ||
isSafeNavigationUrl(target))`. CodeQL's data-flow model didn't propagate the
trailing disjunction (the `isSafeNavigationUrl(...)` call) as a sanitiser, so
the sink still tracked the source-tainted `target`.
The new shape splits the work into a router-relative fast-path
(`startsWith('/') && !startsWith('//') && !includes('\\')` — the canonical
barrier triple) and an external-URL path (`new URL(target)` + literal-equality
scheme check + `!parsed.username && !parsed.password`, feeding `parsed.href` to
the sink). Backslash-laden authorities (AF-1) and userinfo-bearing URLs (nit-3)
are still rejected; the fallback to `ensureAppRoot('/')` is preserved with the
same inline barrier triple. Behaviour is unchanged. Resolving.
##########
superset-frontend/src/utils/navigationUtils.ts:
##########
@@ -16,29 +16,204 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ensureAppRoot } from './pathUtils';
+import {
+ createElement,
+ type AnchorHTMLAttributes,
+ type ReactElement,
+} from 'react';
+import { ensureAppRoot, makeUrl, stripAppRoot } from './pathUtils';
-export const navigateTo = (
+// Re-export so callers that legitimately need a raw prefixed path (native
+// fetch, navigator.sendBeacon, image src, third-party `href` props) have a
+// single sanctioned import location. The static-invariant scan disallows
+// importing from `pathUtils` directly outside this module.
+export { ensureAppRoot, makeUrl, stripAppRoot };
+
+// The guard helpers are declared before `navigateTo` / `navigateWithState`
+// so oxlint's no-use-before-define lint (which does not honour function-
+// declaration hoisting) does not fire on the wired-up imperative-nav
+// path. The focused helpers below (`openInNewTab`, `getShareableUrl`,
+// `AppLink`) also reach for `assertSafeNavigationUrl` directly.
+
+const NEW_TAB_FEATURES = 'noopener noreferrer';
+
+// Allow-list of safe URL shapes for navigation: router-relative paths and a
+// small set of known-safe schemes. `ensureAppRoot` already neutralises
+// `javascript:` / `data:` by prefixing them as relative paths; protocol-
+// relative `//host` is intentionally excluded here because it is a cross-
+// origin navigation primitive that previously enabled open redirects.
+//
+// nit-3 / AF-1 hardening: the leading-slash branch also rejects any URL
+// containing a backslash anywhere — browsers normalise `/\evil.com` →
+// `//evil.com` in the special-scheme authority, so backslashes in any
+// position let an attacker craft a path that looks router-relative until
+// the browser parses it. http(s)/ftp URLs with userinfo (`@` before the
+// first `/` after `//`) are rejected by the post-regex authority check
+// below, since `https://[email protected]` resolves to the host `evil.com`
+// despite presenting as a same-origin-looking URL to the eye.
+const SAFE_NAVIGATION_URL_RE = /^(?:\/(?!\/)|https?:|ftp:|mailto:|tel:)/i;
+const USERINFO_BEARING_SCHEME_RE = /^(?:https?|ftp):/i;
+
+function assertSafeNavigationUrl(url: string): string {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: only relative paths and ' +
+ 'http(s):, ftp:, mailto:, tel: schemes are allowed.',
+ );
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ let parsed: URL;
+ try {
+ parsed = new URL(url);
+ } catch {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: unparseable authority.',
+ );
+ }
+ if (parsed.username || parsed.password) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: ' +
+ 'http(s)/ftp URLs with userinfo are not allowed.',
+ );
+ }
+ }
+ return url;
+}
+
+// Inline regex predicate. Mirrors `assertSafeNavigationUrl` exactly but as
+// a boolean test so CodeQL's data-flow analysis recognises it as a
+// sanitiser barrier at each `window.*` sink in `navigateTo` /
+// `navigateWithState`. Through-function sanitisers (`assertSafeNavigationUrl`)
+// are not in CodeQL's default model and tripped `js/client-side-*` even
+// though the throw path guaranteed safety.
+function isSafeNavigationUrl(url: string): boolean {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ return false;
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ try {
+ const parsed = new URL(url);
+ if (parsed.username || parsed.password) {
+ return false;
+ }
+ } catch {
+ return false;
+ }
+ }
+ return true;
+}
+
+/**
+ * Imperative full-page navigation. Internal entry point for `redirect()`
+ * and a handful of legacy callers; new code should prefer `<AppLink>` or
+ * `redirect()`. Unsafe URLs (protocol-relative, backslash-laden, userinfo-
+ * carrying http(s)) fall back to `ensureAppRoot('/')` with a `console.error`
+ * — never a silent navigation to the rejected target.
+ *
+ * @internal
+ */
+export function navigateTo(
url: string,
options?: { newWindow?: boolean; assign?: boolean },
-) => {
- if (options?.newWindow) {
- window.open(ensureAppRoot(url), '_blank', 'noopener noreferrer');
- } else if (options?.assign) {
- window.location.assign(ensureAppRoot(url));
- } else {
- window.location.href = ensureAppRoot(url);
+): void {
+ const target = ensureAppRoot(url);
+ // Inline regex check (not a function call) so CodeQL's data-flow
+ // analysis recognises the sanitiser barrier and propagates `target` as
+ // safe at each sink below. Mirrors `isSafeNavigationUrl` precisely.
+ if (
+ SAFE_NAVIGATION_URL_RE.test(target) &&
+ !target.includes('\\') &&
+ (!USERINFO_BEARING_SCHEME_RE.test(target) || isSafeNavigationUrl(target))
+ ) {
+ if (options?.newWindow) {
+ window.open(target, '_blank', NEW_TAB_FEATURES);
+ } else if (options?.assign) {
+ window.location.assign(target);
+ } else {
+ window.location.href = target;
Review Comment:
Addressed in 54085f4de3: refactored `navigateTo` / `navigateWithState` so
each `window.*` sink lives inside an if-block gated by pure inline barriers
(`String.startsWith`, `String.includes`, constant equality on `URL.protocol`,
property reads on `URL.username` / `URL.password`).
The prior shape composed the guard as `SAFE_NAVIGATION_URL_RE.test(target)
&& !target.includes('\\') && (!USERINFO_BEARING_SCHEME_RE.test(target) ||
isSafeNavigationUrl(target))`. CodeQL's data-flow model didn't propagate the
trailing disjunction (the `isSafeNavigationUrl(...)` call) as a sanitiser, so
the sink still tracked the source-tainted `target`.
The new shape splits the work into a router-relative fast-path
(`startsWith('/') && !startsWith('//') && !includes('\\')` — the canonical
barrier triple) and an external-URL path (`new URL(target)` + literal-equality
scheme check + `!parsed.username && !parsed.password`, feeding `parsed.href` to
the sink). Backslash-laden authorities (AF-1) and userinfo-bearing URLs (nit-3)
are still rejected; the fallback to `ensureAppRoot('/')` is preserved with the
same inline barrier triple. Behaviour is unchanged. Resolving.
##########
superset-frontend/src/utils/navigationUtils.ts:
##########
@@ -16,29 +16,204 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ensureAppRoot } from './pathUtils';
+import {
+ createElement,
+ type AnchorHTMLAttributes,
+ type ReactElement,
+} from 'react';
+import { ensureAppRoot, makeUrl, stripAppRoot } from './pathUtils';
-export const navigateTo = (
+// Re-export so callers that legitimately need a raw prefixed path (native
+// fetch, navigator.sendBeacon, image src, third-party `href` props) have a
+// single sanctioned import location. The static-invariant scan disallows
+// importing from `pathUtils` directly outside this module.
+export { ensureAppRoot, makeUrl, stripAppRoot };
+
+// The guard helpers are declared before `navigateTo` / `navigateWithState`
+// so oxlint's no-use-before-define lint (which does not honour function-
+// declaration hoisting) does not fire on the wired-up imperative-nav
+// path. The focused helpers below (`openInNewTab`, `getShareableUrl`,
+// `AppLink`) also reach for `assertSafeNavigationUrl` directly.
+
+const NEW_TAB_FEATURES = 'noopener noreferrer';
+
+// Allow-list of safe URL shapes for navigation: router-relative paths and a
+// small set of known-safe schemes. `ensureAppRoot` already neutralises
+// `javascript:` / `data:` by prefixing them as relative paths; protocol-
+// relative `//host` is intentionally excluded here because it is a cross-
+// origin navigation primitive that previously enabled open redirects.
+//
+// nit-3 / AF-1 hardening: the leading-slash branch also rejects any URL
+// containing a backslash anywhere — browsers normalise `/\evil.com` →
+// `//evil.com` in the special-scheme authority, so backslashes in any
+// position let an attacker craft a path that looks router-relative until
+// the browser parses it. http(s)/ftp URLs with userinfo (`@` before the
+// first `/` after `//`) are rejected by the post-regex authority check
+// below, since `https://[email protected]` resolves to the host `evil.com`
+// despite presenting as a same-origin-looking URL to the eye.
+const SAFE_NAVIGATION_URL_RE = /^(?:\/(?!\/)|https?:|ftp:|mailto:|tel:)/i;
+const USERINFO_BEARING_SCHEME_RE = /^(?:https?|ftp):/i;
+
+function assertSafeNavigationUrl(url: string): string {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: only relative paths and ' +
+ 'http(s):, ftp:, mailto:, tel: schemes are allowed.',
+ );
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ let parsed: URL;
+ try {
+ parsed = new URL(url);
+ } catch {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: unparseable authority.',
+ );
+ }
+ if (parsed.username || parsed.password) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: ' +
+ 'http(s)/ftp URLs with userinfo are not allowed.',
+ );
+ }
+ }
+ return url;
+}
+
+// Inline regex predicate. Mirrors `assertSafeNavigationUrl` exactly but as
+// a boolean test so CodeQL's data-flow analysis recognises it as a
+// sanitiser barrier at each `window.*` sink in `navigateTo` /
+// `navigateWithState`. Through-function sanitisers (`assertSafeNavigationUrl`)
+// are not in CodeQL's default model and tripped `js/client-side-*` even
+// though the throw path guaranteed safety.
+function isSafeNavigationUrl(url: string): boolean {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ return false;
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ try {
+ const parsed = new URL(url);
+ if (parsed.username || parsed.password) {
+ return false;
+ }
+ } catch {
+ return false;
+ }
+ }
+ return true;
+}
+
+/**
+ * Imperative full-page navigation. Internal entry point for `redirect()`
+ * and a handful of legacy callers; new code should prefer `<AppLink>` or
+ * `redirect()`. Unsafe URLs (protocol-relative, backslash-laden, userinfo-
+ * carrying http(s)) fall back to `ensureAppRoot('/')` with a `console.error`
+ * — never a silent navigation to the rejected target.
+ *
+ * @internal
+ */
+export function navigateTo(
url: string,
options?: { newWindow?: boolean; assign?: boolean },
-) => {
- if (options?.newWindow) {
- window.open(ensureAppRoot(url), '_blank', 'noopener noreferrer');
- } else if (options?.assign) {
- window.location.assign(ensureAppRoot(url));
- } else {
- window.location.href = ensureAppRoot(url);
+): void {
+ const target = ensureAppRoot(url);
+ // Inline regex check (not a function call) so CodeQL's data-flow
+ // analysis recognises the sanitiser barrier and propagates `target` as
+ // safe at each sink below. Mirrors `isSafeNavigationUrl` precisely.
+ if (
+ SAFE_NAVIGATION_URL_RE.test(target) &&
+ !target.includes('\\') &&
+ (!USERINFO_BEARING_SCHEME_RE.test(target) || isSafeNavigationUrl(target))
+ ) {
+ if (options?.newWindow) {
+ window.open(target, '_blank', NEW_TAB_FEATURES);
+ } else if (options?.assign) {
+ window.location.assign(target);
+ } else {
+ window.location.href = target;
+ }
+ return;
}
-};
+ // eslint-disable-next-line no-console -- guarded surface, observable in dev.
+ console.error('navigationUtils.navigateTo refused unsafe URL:', url);
+ // Fallback navigation: `ensureAppRoot('/')` resolves to the deployment
+ // home. Re-validated through the same inline gate so CodeQL sees the
+ // sanitiser at every `window.location.*` write in this function.
+ const home = ensureAppRoot('/');
+ if (
+ SAFE_NAVIGATION_URL_RE.test(home) &&
+ !home.includes('\\') &&
+ (!USERINFO_BEARING_SCHEME_RE.test(home) || isSafeNavigationUrl(home))
+ ) {
+ window.location.href = home;
Review Comment:
Addressed in 54085f4de3: refactored `navigateTo` / `navigateWithState` so
each `window.*` sink lives inside an if-block gated by pure inline barriers
(`String.startsWith`, `String.includes`, constant equality on `URL.protocol`,
property reads on `URL.username` / `URL.password`).
The prior shape composed the guard as `SAFE_NAVIGATION_URL_RE.test(target)
&& !target.includes('\\') && (!USERINFO_BEARING_SCHEME_RE.test(target) ||
isSafeNavigationUrl(target))`. CodeQL's data-flow model didn't propagate the
trailing disjunction (the `isSafeNavigationUrl(...)` call) as a sanitiser, so
the sink still tracked the source-tainted `target`.
The new shape splits the work into a router-relative fast-path
(`startsWith('/') && !startsWith('//') && !includes('\\')` — the canonical
barrier triple) and an external-URL path (`new URL(target)` + literal-equality
scheme check + `!parsed.username && !parsed.password`, feeding `parsed.href` to
the sink). Backslash-laden authorities (AF-1) and userinfo-bearing URLs (nit-3)
are still rejected; the fallback to `ensureAppRoot('/')` is preserved with the
same inline barrier triple. Behaviour is unchanged. Resolving.
##########
superset-frontend/src/utils/navigationUtils.ts:
##########
@@ -16,29 +16,204 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ensureAppRoot } from './pathUtils';
+import {
+ createElement,
+ type AnchorHTMLAttributes,
+ type ReactElement,
+} from 'react';
+import { ensureAppRoot, makeUrl, stripAppRoot } from './pathUtils';
-export const navigateTo = (
+// Re-export so callers that legitimately need a raw prefixed path (native
+// fetch, navigator.sendBeacon, image src, third-party `href` props) have a
+// single sanctioned import location. The static-invariant scan disallows
+// importing from `pathUtils` directly outside this module.
+export { ensureAppRoot, makeUrl, stripAppRoot };
+
+// The guard helpers are declared before `navigateTo` / `navigateWithState`
+// so oxlint's no-use-before-define lint (which does not honour function-
+// declaration hoisting) does not fire on the wired-up imperative-nav
+// path. The focused helpers below (`openInNewTab`, `getShareableUrl`,
+// `AppLink`) also reach for `assertSafeNavigationUrl` directly.
+
+const NEW_TAB_FEATURES = 'noopener noreferrer';
+
+// Allow-list of safe URL shapes for navigation: router-relative paths and a
+// small set of known-safe schemes. `ensureAppRoot` already neutralises
+// `javascript:` / `data:` by prefixing them as relative paths; protocol-
+// relative `//host` is intentionally excluded here because it is a cross-
+// origin navigation primitive that previously enabled open redirects.
+//
+// nit-3 / AF-1 hardening: the leading-slash branch also rejects any URL
+// containing a backslash anywhere — browsers normalise `/\evil.com` →
+// `//evil.com` in the special-scheme authority, so backslashes in any
+// position let an attacker craft a path that looks router-relative until
+// the browser parses it. http(s)/ftp URLs with userinfo (`@` before the
+// first `/` after `//`) are rejected by the post-regex authority check
+// below, since `https://[email protected]` resolves to the host `evil.com`
+// despite presenting as a same-origin-looking URL to the eye.
+const SAFE_NAVIGATION_URL_RE = /^(?:\/(?!\/)|https?:|ftp:|mailto:|tel:)/i;
+const USERINFO_BEARING_SCHEME_RE = /^(?:https?|ftp):/i;
+
+function assertSafeNavigationUrl(url: string): string {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: only relative paths and ' +
+ 'http(s):, ftp:, mailto:, tel: schemes are allowed.',
+ );
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ let parsed: URL;
+ try {
+ parsed = new URL(url);
+ } catch {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: unparseable authority.',
+ );
+ }
+ if (parsed.username || parsed.password) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: ' +
+ 'http(s)/ftp URLs with userinfo are not allowed.',
+ );
+ }
+ }
+ return url;
+}
+
+// Inline regex predicate. Mirrors `assertSafeNavigationUrl` exactly but as
+// a boolean test so CodeQL's data-flow analysis recognises it as a
+// sanitiser barrier at each `window.*` sink in `navigateTo` /
+// `navigateWithState`. Through-function sanitisers (`assertSafeNavigationUrl`)
+// are not in CodeQL's default model and tripped `js/client-side-*` even
+// though the throw path guaranteed safety.
+function isSafeNavigationUrl(url: string): boolean {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ return false;
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ try {
+ const parsed = new URL(url);
+ if (parsed.username || parsed.password) {
+ return false;
+ }
+ } catch {
+ return false;
+ }
+ }
+ return true;
+}
+
+/**
+ * Imperative full-page navigation. Internal entry point for `redirect()`
+ * and a handful of legacy callers; new code should prefer `<AppLink>` or
+ * `redirect()`. Unsafe URLs (protocol-relative, backslash-laden, userinfo-
+ * carrying http(s)) fall back to `ensureAppRoot('/')` with a `console.error`
+ * — never a silent navigation to the rejected target.
+ *
+ * @internal
+ */
+export function navigateTo(
url: string,
options?: { newWindow?: boolean; assign?: boolean },
-) => {
- if (options?.newWindow) {
- window.open(ensureAppRoot(url), '_blank', 'noopener noreferrer');
- } else if (options?.assign) {
- window.location.assign(ensureAppRoot(url));
- } else {
- window.location.href = ensureAppRoot(url);
+): void {
+ const target = ensureAppRoot(url);
+ // Inline regex check (not a function call) so CodeQL's data-flow
+ // analysis recognises the sanitiser barrier and propagates `target` as
+ // safe at each sink below. Mirrors `isSafeNavigationUrl` precisely.
+ if (
+ SAFE_NAVIGATION_URL_RE.test(target) &&
+ !target.includes('\\') &&
+ (!USERINFO_BEARING_SCHEME_RE.test(target) || isSafeNavigationUrl(target))
+ ) {
+ if (options?.newWindow) {
+ window.open(target, '_blank', NEW_TAB_FEATURES);
Review Comment:
Addressed in 54085f4de3: refactored `navigateTo` / `navigateWithState` so
each `window.*` sink lives inside an if-block gated by pure inline barriers
(`String.startsWith`, `String.includes`, constant equality on `URL.protocol`,
property reads on `URL.username` / `URL.password`).
The prior shape composed the guard as `SAFE_NAVIGATION_URL_RE.test(target)
&& !target.includes('\\') && (!USERINFO_BEARING_SCHEME_RE.test(target) ||
isSafeNavigationUrl(target))`. CodeQL's data-flow model didn't propagate the
trailing disjunction (the `isSafeNavigationUrl(...)` call) as a sanitiser, so
the sink still tracked the source-tainted `target`.
The new shape splits the work into a router-relative fast-path
(`startsWith('/') && !startsWith('//') && !includes('\\')` — the canonical
barrier triple) and an external-URL path (`new URL(target)` + literal-equality
scheme check + `!parsed.username && !parsed.password`, feeding `parsed.href` to
the sink). Backslash-laden authorities (AF-1) and userinfo-bearing URLs (nit-3)
are still rejected; the fallback to `ensureAppRoot('/')` is preserved with the
same inline barrier triple. Behaviour is unchanged. Resolving.
##########
superset-frontend/src/utils/navigationUtils.ts:
##########
@@ -16,29 +16,204 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ensureAppRoot } from './pathUtils';
+import {
+ createElement,
+ type AnchorHTMLAttributes,
+ type ReactElement,
+} from 'react';
+import { ensureAppRoot, makeUrl, stripAppRoot } from './pathUtils';
-export const navigateTo = (
+// Re-export so callers that legitimately need a raw prefixed path (native
+// fetch, navigator.sendBeacon, image src, third-party `href` props) have a
+// single sanctioned import location. The static-invariant scan disallows
+// importing from `pathUtils` directly outside this module.
+export { ensureAppRoot, makeUrl, stripAppRoot };
+
+// The guard helpers are declared before `navigateTo` / `navigateWithState`
+// so oxlint's no-use-before-define lint (which does not honour function-
+// declaration hoisting) does not fire on the wired-up imperative-nav
+// path. The focused helpers below (`openInNewTab`, `getShareableUrl`,
+// `AppLink`) also reach for `assertSafeNavigationUrl` directly.
+
+const NEW_TAB_FEATURES = 'noopener noreferrer';
+
+// Allow-list of safe URL shapes for navigation: router-relative paths and a
+// small set of known-safe schemes. `ensureAppRoot` already neutralises
+// `javascript:` / `data:` by prefixing them as relative paths; protocol-
+// relative `//host` is intentionally excluded here because it is a cross-
+// origin navigation primitive that previously enabled open redirects.
+//
+// nit-3 / AF-1 hardening: the leading-slash branch also rejects any URL
+// containing a backslash anywhere — browsers normalise `/\evil.com` →
+// `//evil.com` in the special-scheme authority, so backslashes in any
+// position let an attacker craft a path that looks router-relative until
+// the browser parses it. http(s)/ftp URLs with userinfo (`@` before the
+// first `/` after `//`) are rejected by the post-regex authority check
+// below, since `https://[email protected]` resolves to the host `evil.com`
+// despite presenting as a same-origin-looking URL to the eye.
+const SAFE_NAVIGATION_URL_RE = /^(?:\/(?!\/)|https?:|ftp:|mailto:|tel:)/i;
+const USERINFO_BEARING_SCHEME_RE = /^(?:https?|ftp):/i;
+
+function assertSafeNavigationUrl(url: string): string {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: only relative paths and ' +
+ 'http(s):, ftp:, mailto:, tel: schemes are allowed.',
+ );
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ let parsed: URL;
+ try {
+ parsed = new URL(url);
+ } catch {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: unparseable authority.',
+ );
+ }
+ if (parsed.username || parsed.password) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: ' +
+ 'http(s)/ftp URLs with userinfo are not allowed.',
+ );
+ }
+ }
+ return url;
+}
+
+// Inline regex predicate. Mirrors `assertSafeNavigationUrl` exactly but as
+// a boolean test so CodeQL's data-flow analysis recognises it as a
+// sanitiser barrier at each `window.*` sink in `navigateTo` /
+// `navigateWithState`. Through-function sanitisers (`assertSafeNavigationUrl`)
+// are not in CodeQL's default model and tripped `js/client-side-*` even
+// though the throw path guaranteed safety.
+function isSafeNavigationUrl(url: string): boolean {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ return false;
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ try {
+ const parsed = new URL(url);
+ if (parsed.username || parsed.password) {
+ return false;
+ }
+ } catch {
+ return false;
+ }
+ }
+ return true;
+}
+
+/**
+ * Imperative full-page navigation. Internal entry point for `redirect()`
+ * and a handful of legacy callers; new code should prefer `<AppLink>` or
+ * `redirect()`. Unsafe URLs (protocol-relative, backslash-laden, userinfo-
+ * carrying http(s)) fall back to `ensureAppRoot('/')` with a `console.error`
+ * — never a silent navigation to the rejected target.
+ *
+ * @internal
+ */
+export function navigateTo(
url: string,
options?: { newWindow?: boolean; assign?: boolean },
-) => {
- if (options?.newWindow) {
- window.open(ensureAppRoot(url), '_blank', 'noopener noreferrer');
- } else if (options?.assign) {
- window.location.assign(ensureAppRoot(url));
- } else {
- window.location.href = ensureAppRoot(url);
+): void {
+ const target = ensureAppRoot(url);
+ // Inline regex check (not a function call) so CodeQL's data-flow
+ // analysis recognises the sanitiser barrier and propagates `target` as
+ // safe at each sink below. Mirrors `isSafeNavigationUrl` precisely.
+ if (
+ SAFE_NAVIGATION_URL_RE.test(target) &&
+ !target.includes('\\') &&
+ (!USERINFO_BEARING_SCHEME_RE.test(target) || isSafeNavigationUrl(target))
+ ) {
+ if (options?.newWindow) {
+ window.open(target, '_blank', NEW_TAB_FEATURES);
+ } else if (options?.assign) {
+ window.location.assign(target);
Review Comment:
Addressed in 54085f4de3: refactored `navigateTo` / `navigateWithState` so
each `window.*` sink lives inside an if-block gated by pure inline barriers
(`String.startsWith`, `String.includes`, constant equality on `URL.protocol`,
property reads on `URL.username` / `URL.password`).
The prior shape composed the guard as `SAFE_NAVIGATION_URL_RE.test(target)
&& !target.includes('\\') && (!USERINFO_BEARING_SCHEME_RE.test(target) ||
isSafeNavigationUrl(target))`. CodeQL's data-flow model didn't propagate the
trailing disjunction (the `isSafeNavigationUrl(...)` call) as a sanitiser, so
the sink still tracked the source-tainted `target`.
The new shape splits the work into a router-relative fast-path
(`startsWith('/') && !startsWith('//') && !includes('\\')` — the canonical
barrier triple) and an external-URL path (`new URL(target)` + literal-equality
scheme check + `!parsed.username && !parsed.password`, feeding `parsed.href` to
the sink). Backslash-laden authorities (AF-1) and userinfo-bearing URLs (nit-3)
are still rejected; the fallback to `ensureAppRoot('/')` is preserved with the
same inline barrier triple. Behaviour is unchanged. Resolving.
##########
superset-frontend/src/utils/navigationUtils.ts:
##########
@@ -16,29 +16,204 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ensureAppRoot } from './pathUtils';
+import {
+ createElement,
+ type AnchorHTMLAttributes,
+ type ReactElement,
+} from 'react';
+import { ensureAppRoot, makeUrl, stripAppRoot } from './pathUtils';
-export const navigateTo = (
+// Re-export so callers that legitimately need a raw prefixed path (native
+// fetch, navigator.sendBeacon, image src, third-party `href` props) have a
+// single sanctioned import location. The static-invariant scan disallows
+// importing from `pathUtils` directly outside this module.
+export { ensureAppRoot, makeUrl, stripAppRoot };
+
+// The guard helpers are declared before `navigateTo` / `navigateWithState`
+// so oxlint's no-use-before-define lint (which does not honour function-
+// declaration hoisting) does not fire on the wired-up imperative-nav
+// path. The focused helpers below (`openInNewTab`, `getShareableUrl`,
+// `AppLink`) also reach for `assertSafeNavigationUrl` directly.
+
+const NEW_TAB_FEATURES = 'noopener noreferrer';
+
+// Allow-list of safe URL shapes for navigation: router-relative paths and a
+// small set of known-safe schemes. `ensureAppRoot` already neutralises
+// `javascript:` / `data:` by prefixing them as relative paths; protocol-
+// relative `//host` is intentionally excluded here because it is a cross-
+// origin navigation primitive that previously enabled open redirects.
+//
+// nit-3 / AF-1 hardening: the leading-slash branch also rejects any URL
+// containing a backslash anywhere — browsers normalise `/\evil.com` →
+// `//evil.com` in the special-scheme authority, so backslashes in any
+// position let an attacker craft a path that looks router-relative until
+// the browser parses it. http(s)/ftp URLs with userinfo (`@` before the
+// first `/` after `//`) are rejected by the post-regex authority check
+// below, since `https://[email protected]` resolves to the host `evil.com`
+// despite presenting as a same-origin-looking URL to the eye.
+const SAFE_NAVIGATION_URL_RE = /^(?:\/(?!\/)|https?:|ftp:|mailto:|tel:)/i;
+const USERINFO_BEARING_SCHEME_RE = /^(?:https?|ftp):/i;
+
+function assertSafeNavigationUrl(url: string): string {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: only relative paths and ' +
+ 'http(s):, ftp:, mailto:, tel: schemes are allowed.',
+ );
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ let parsed: URL;
+ try {
+ parsed = new URL(url);
+ } catch {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: unparseable authority.',
+ );
+ }
+ if (parsed.username || parsed.password) {
+ throw new Error(
+ 'navigationUtils refused unsafe URL: ' +
+ 'http(s)/ftp URLs with userinfo are not allowed.',
+ );
+ }
+ }
+ return url;
+}
+
+// Inline regex predicate. Mirrors `assertSafeNavigationUrl` exactly but as
+// a boolean test so CodeQL's data-flow analysis recognises it as a
+// sanitiser barrier at each `window.*` sink in `navigateTo` /
+// `navigateWithState`. Through-function sanitisers (`assertSafeNavigationUrl`)
+// are not in CodeQL's default model and tripped `js/client-side-*` even
+// though the throw path guaranteed safety.
+function isSafeNavigationUrl(url: string): boolean {
+ if (!SAFE_NAVIGATION_URL_RE.test(url) || url.includes('\\')) {
+ return false;
+ }
+ if (USERINFO_BEARING_SCHEME_RE.test(url)) {
+ try {
+ const parsed = new URL(url);
+ if (parsed.username || parsed.password) {
+ return false;
+ }
+ } catch {
+ return false;
+ }
+ }
+ return true;
+}
+
+/**
+ * Imperative full-page navigation. Internal entry point for `redirect()`
+ * and a handful of legacy callers; new code should prefer `<AppLink>` or
+ * `redirect()`. Unsafe URLs (protocol-relative, backslash-laden, userinfo-
+ * carrying http(s)) fall back to `ensureAppRoot('/')` with a `console.error`
+ * — never a silent navigation to the rejected target.
+ *
+ * @internal
+ */
+export function navigateTo(
url: string,
options?: { newWindow?: boolean; assign?: boolean },
-) => {
- if (options?.newWindow) {
- window.open(ensureAppRoot(url), '_blank', 'noopener noreferrer');
- } else if (options?.assign) {
- window.location.assign(ensureAppRoot(url));
- } else {
- window.location.href = ensureAppRoot(url);
+): void {
+ const target = ensureAppRoot(url);
+ // Inline regex check (not a function call) so CodeQL's data-flow
+ // analysis recognises the sanitiser barrier and propagates `target` as
+ // safe at each sink below. Mirrors `isSafeNavigationUrl` precisely.
+ if (
+ SAFE_NAVIGATION_URL_RE.test(target) &&
+ !target.includes('\\') &&
+ (!USERINFO_BEARING_SCHEME_RE.test(target) || isSafeNavigationUrl(target))
+ ) {
+ if (options?.newWindow) {
+ window.open(target, '_blank', NEW_TAB_FEATURES);
+ } else if (options?.assign) {
+ window.location.assign(target);
+ } else {
+ window.location.href = target;
Review Comment:
Addressed in 54085f4de3: refactored `navigateTo` / `navigateWithState` so
each `window.*` sink lives inside an if-block gated by pure inline barriers
(`String.startsWith`, `String.includes`, constant equality on `URL.protocol`,
property reads on `URL.username` / `URL.password`).
The prior shape composed the guard as `SAFE_NAVIGATION_URL_RE.test(target)
&& !target.includes('\\') && (!USERINFO_BEARING_SCHEME_RE.test(target) ||
isSafeNavigationUrl(target))`. CodeQL's data-flow model didn't propagate the
trailing disjunction (the `isSafeNavigationUrl(...)` call) as a sanitiser, so
the sink still tracked the source-tainted `target`.
The new shape splits the work into a router-relative fast-path
(`startsWith('/') && !startsWith('//') && !includes('\\')` — the canonical
barrier triple) and an external-URL path (`new URL(target)` + literal-equality
scheme check + `!parsed.username && !parsed.password`, feeding `parsed.href` to
the sink). Backslash-laden authorities (AF-1) and userinfo-bearing URLs (nit-3)
are still rejected; the fallback to `ensureAppRoot('/')` is preserved with the
same inline barrier triple. Behaviour is unchanged. Resolving.
--
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]
