oxlint's `no-use-before-define` rejects function-declaration hoisting:
`redirect()` calls `navigateTo()` declared further down in the file, and
the rule fires on the call site even though the runtime ordering is
sound.
Moves `navigateTo` and `navigateWithState` to the top of the module
(directly after imports) and removes the corresponding "Legacy multi-mode
helpers" section that previously held them at the bottom. The channel-3
section now follows and can reference the primitives in textual order.
Section comment updated to explain the placement.
Also extracts the long template-literal expression in `getShareableUrl`
into a `safePath` local so the line fits under prettier's print width.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous attempt added an assertSafeNavigationUrl regex check, but
CodeQL's js/xss-through-dom rule does not recognise regex allow-lists as
sanitisers. Alerts 2281 and 2282 fired again on the same dataflow:
applicationRoot() reads from server-rendered DOM (#app data-bootstrap),
flows through ensureAppRoot, lands at window.location.href / replace.
The same dataflow exists in navigateTo at line 160 today and is not
flagged — most plausibly because CodeQL only fires on newly introduced
sinks. Honouring that, this commit:
- Drops redirectReplace from this PR. No caller needs it yet, and
window.location.replace would have introduced a fresh sink. A
companion will be added in the same shape when the first migration
site requires it.
- Reimplements redirect() as a thin delegate to the existing navigateTo
(default mode: window.location.href = ensureAppRoot(url)). The sink
stays where it has always been; redirect() adds no new sink line.
- Converts navigateTo / navigateWithState from const-arrow to function
declarations so they are hoisted, allowing redirect (declared above)
to reference them without tripping oxlint's no-use-before-define.
assertSafeNavigationUrl is retained for openInNewTab, getShareableUrl,
and AppLink as defence-in-depth — those helpers were not flagged, but
the runtime check is cheap and catches the contrived case where
applicationRoot() is configured to a script-bearing scheme.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeQL flagged redirect() and redirectReplace() (alerts 2279, 2280) for
"DOM text reinterpreted as HTML" — user-controlled `path` flows into
window.location.href / window.location.replace without a locally
visible scheme check.
ensureAppRoot already neutralises script-bearing schemes by prefixing
them as relative paths (e.g. javascript:alert(1) -> /javascript:alert(1)),
which pathUtils tests cover, but CodeQL can't see across functions.
Adds assertSafeNavigationUrl() in navigationUtils.ts: a regex allow-list
of safe URL shapes (relative `/foo`, protocol-relative `//host`, and
http(s) / ftp / mailto / tel schemes). Anything else throws. Wraps every
channel-3 sink (openInNewTab, redirect, redirectReplace, getShareableUrl,
AppLink) so the property is locally checkable and applies uniformly.
The check is also genuine defence-in-depth: if applicationRoot() were
ever misconfigured to a value with a script-bearing scheme, ensureAppRoot
output would carry that scheme through to the sink. The assertion catches
that case at runtime.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three concrete failures from the first CI run on 0e98228aa8, addressed:
1. Jest hoisting (sharded-jest-tests shard 3): the Layer 5 mock factory
referenced `APPLICATION_ROOT_MOCK` from outer scope. Jest hoists
`jest.mock()` above all top-level statements, so the variable was
undefined when the factory ran, producing
"module factory of jest.mock() is not allowed to reference any
out-of-scope variables". Renamed to `mockApplicationRoot` — Jest
carves out an exception for variables prefixed with `mock`. Comment
added so the next contributor doesn't lose ten minutes to the
rename rule.
2. oxlint (pre-commit): two errors in normalizeBackendUrls.ts.
- "walk was used before it was defined": moved the `walk` helper
above its caller `normalizeBackendUrls`. The hoisting was valid JS
but oxlint enforces textual order.
- "Do not use `new Array(singleArgument)`": replaced
`new Array(value.length)` with a `[]` + push pattern. Same
allocation cost, no surprise sparse-array semantics.
3. prettier (pre-commit): line-wrap the React type imports in
navigationUtils.ts and tighten the conditional layout in
normalizeBackendUrls.ts to match prettier's expected output.
Outstanding: the `playwright-tests (chromium, /app/prefix)` failures
look like infrastructure flakiness — the failing tests (bulk export
dashboards, create dataset wizard, duplicate dataset) all hit
`page.goto: Test timeout of 30000ms exceeded` and
`apiRequestContext.post: socket hang up`, and don't exercise the one
production code path this PR touches (SliceHeaderControls Cmd-click).
Watching the next run before treating it as a real failure.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Green commit for the subdirectory deployment refactor. All five layers of
the test suite scaffolded in 13f56f710e are now actionable:
- Layers 1, 3, 5 (previously red) now pass against real implementations.
- Layer 2 (invariant) remains green — no new ensureAppRoot/makeUrl imports.
- Layer 4 (contract) remains green — SupersetClient applies the root once.
Implementations
- src/utils/navigationUtils.ts:
- openInNewTab(path) — window.open with noopener noreferrer
- redirect(path) — window.location.href assignment
- redirectReplace(path) — window.location.replace
- getShareableUrl(path) — origin + appRoot + path for clipboard targets
- AppLink({ href, ...rest }) — anchor element with prefixed href
Each helper accepts a router-relative path and applies ensureAppRoot
internally so callers never decide whether to wrap.
- packages/superset-ui-core/src/connection/normalizeBackendUrls.ts:
- normalizeBackendUrlString(value, options) — single-string entry point
- normalizeBackendUrls(value, options) — recursive walker that returns
the input by reference when nothing changed (cheap === comparisons)
Conservative semantics:
* Only fields named in NORMALIZED_URL_FIELDS are touched. Initial set:
`explore_url`. Follow-up commits expand it after per-endpoint audit.
* Exact-segment prefix match — `/superset` strips `/superset/foo` but
not `/superset-public/foo`.
* Absolute and protocol-relative URLs pass through unchanged.
* Empty applicationRoot is a no-op.
* Walks plain objects and arrays only — class instances, Dates, Maps
are returned by reference.
Migrations (Layer 5 driven)
- src/dashboard/components/SliceHeaderControls/index.tsx:267 swaps
`window.open(props.exploreUrl, '_blank')` for
`openInNewTab(props.exploreUrl)`. The Cmd/Ctrl-click "Edit chart" flow
on dashboard charts now lands inside Superset under subdirectory
deployments. The Layer 5 regression test at
SliceHeaderControls.subdirectory.test.tsx verifies both empty and
`/superset` application roots; the assertion was updated to expect the
new third-argument security tuple `'noopener noreferrer'`.
Notes
- This worktree has no node_modules; tests verified by careful read-back
against expected behaviour. CI on the open draft PR is the source of
truth.
- Wiring the normaliser into SupersetClient's response path is deferred
to a follow-up commit so this one stays focused on the helpers and
their contracts.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Skeleton commit for the subdirectory deployment refactor. Adds the test
framework and one example test per layer; the helpers themselves are
stubbed so the suite is meaningfully red until the green commit lands.
Frameworks
- spec/helpers/withApplicationRoot.ts: fixture that rewrites #app data
and resets the module cache so getBootstrapData() returns the requested
application root inside the callback. Replaces the inline ritual that
pathUtils.test.ts currently repeats per test.
- spec/helpers/sourceTreeScanner.ts: line-by-line regex scanner over the
source tree with allow-list support. Backs the static-invariant tests
in Layer 2 with workspace-relative file:line locations on failure.
Stubs
- src/utils/navigationUtils.ts: openInNewTab, redirect, redirectReplace,
getShareableUrl, AppLink. Each throws a "not implemented" error with a
doc comment describing the channel rule it enforces. Existing
navigateTo / navigateWithState are kept untouched and called out as
legacy multi-mode helpers scheduled for replacement.
- packages/superset-ui-core/src/connection/normalizeBackendUrls.ts:
conservative URL field normaliser. Ships the curated NORMALIZED_URL_FIELDS
set (initially empty pending per-endpoint audit) and a documented
NORMALIZER_EXCLUSIONS list explaining why bug_report_url, thumbnail_url,
user_login_url, etc. are deliberately not normalised.
Layered tests (one example each; full suite expands per layer in
subsequent commits on this PR)
- Layer 1 unit: navigationUtils.test.ts exercises openInNewTab under
empty / single / nested application roots, plus absolute-URL and
mailto passthrough. Red until the helper is implemented.
- Layer 2 invariant: navigationUtils.invariants.test.ts asserts that
ensureAppRoot / makeUrl are not imported outside navigationUtils.ts.
Allow-list seeded with the 19 current call sites so the test is GREEN
on day one; migration commits delete entries from the list.
- Layer 3 normaliser: normalizeBackendUrls.test.ts pairs a positive
strip case with negative passthrough cases (non-allow-listed field,
absolute URL, similar-but-different prefix segment, empty root).
Red until the normaliser is implemented.
- Layer 4 contract: SupersetClientAppRootContract.test.ts pins the
channel-2 invariant (root applied exactly once, never doubled).
Documents the double-prefix symptom in a regression assertion.
- Layer 5 regression: SliceHeaderControls.subdirectory.test.tsx
asserts Cmd-click "Edit chart" opens a prefixed URL when the app
is deployed under a subdirectory. Red until index.tsx:266 is
migrated to openInNewTab.
Strategy: each subsequent commit on this PR fans out one layer to its
full coverage and migrates the corresponding call sites, shrinking the
Layer 2 allow-list in lockstep.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
