Skip to content

fix: resolve conditional React Hook calls in Link and Script shims #223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
southpolesteve merged 2 commits into from
Mar 3, 2026
+38 −32
Merged

fix: resolve conditional React Hook calls in Link and Script shims #223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
341e76a
fix: resolve conditional React Hook calls in Link and Script shims
Mar 1, 2026
bd82431
fix: skip prefetch setup for dangerous href schemes
southpolesteve Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 21 additions & 14 deletions packages/vinext/src/shims/link.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,19 +280,15 @@ const Link = forwardRef<HTMLAnchorElement, LinkProps>(function Link(
// where href is a route pattern like "/user/[id]" and as is "/user/1")
const resolvedHref = as ?? resolveHref(href);

// Block dangerous URI schemes (javascript:, data:, vbscript:).
// Render an inert <a> without href to prevent XSS while preserving
// styling and attributes like className, id, aria-*.
if (typeof resolvedHref === "string" && isDangerousScheme(resolvedHref)) {
if (process.env.NODE_ENV !== "production") {
console.warn(`<Link> blocked dangerous href: ${resolvedHref}`);
}
const { passHref: _p, ...safeProps } = restWithoutLocale;
return <a {...safeProps}>{children}</a>;
}

// Apply locale prefix if specified
const localizedHref = applyLocaleToHref(resolvedHref, locale);
const isDangerous =
typeof resolvedHref === "string" && isDangerousScheme(resolvedHref);

// Apply locale prefix if specified (safe even for dangerous hrefs since we
// won't use the result when isDangerous is true)
const localizedHref = applyLocaleToHref(
isDangerous ? "/" : resolvedHref,
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The substitution of "/" for the dangerous href is a reasonable way to keep applyLocaleToHref and withBasePath from operating on a javascript: string. However, this means localizedHref, fullHref, handleClick, and the prefetch useEffect all execute with a valid-looking "/" path when the href is actually dangerous.

This is fine today because:

  1. The early return at line 477 prevents the <a> from having an href or onClick/handleClick attached, so the prefetched "/" and the click handler are never used.
  2. The useEffect at line 317 will set up an IntersectionObserver for "/" and attempt to prefetch the root page — this is a harmless but unnecessary side effect.

Worth guarding the prefetch effect to avoid the wasted work:

Suggested change
isDangerous ? "/" : resolvedHref,
const localizedHref = applyLocaleToHref(
isDangerous ? "/" : resolvedHref,
locale,
);

Actually, the substitution itself is fine — but consider also skipping prefetch setup when isDangerous is true. You could add !isDangerous to the shouldPrefetch condition, or add an early return inside the prefetch useEffect. Minor nit though, not a blocker.

locale,
);
// Full href with basePath for browser URLs and fetches
const fullHref = withBasePath(localizedHref);

Expand All @@ -307,7 +303,7 @@ const Link = forwardRef<HTMLAnchorElement, LinkProps>(function Link(
// Prefetching: observe the element when it enters the viewport.
// prefetch={false} disables, prefetch={true} or undefined/null (default) enables.
const internalRef = useRef<HTMLAnchorElement | null>(null);
const shouldPrefetch = prefetchProp !== false;
const shouldPrefetch = prefetchProp !== false && !isDangerous;

const setRefs = useCallback(
(node: HTMLAnchorElement | null) => {
Expand Down Expand Up @@ -474,6 +470,17 @@ const Link = forwardRef<HTMLAnchorElement, LinkProps>(function Link(

const linkStatusValue = React.useMemo(() => ({ pending }), [pending]);

// Block dangerous URI schemes (javascript:, data:, vbscript:).
// Render an inert <a> without href to prevent XSS while preserving
// styling and attributes like className, id, aria-*.
// This check is placed after all hooks to satisfy the Rules of Hooks.
if (isDangerous) {
if (process.env.NODE_ENV !== "production") {
console.warn(`<Link> blocked dangerous href: ${resolvedHref}`);
}
return <a {...anchorProps}>{children}</a>;
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small behavioral difference from the original code: the old early return used restWithoutLocale (minus passHref) for the inert <a>, while this now uses anchorProps which is also restWithoutLocale minus passHref (defined at line 469). So the spread is equivalent — good.

One thing the old code did not pass through was ref and onClick. The new code also omits those (no ref={setRefs}, no onClick={handleClick}) which is correct for an inert element. Looks good.

}

return (
<LinkStatusContext.Provider value={linkStatusValue}>
<a ref={setRefs} href={fullHref} onClick={handleClick} {...anchorProps}>
Expand Down
35 changes: 17 additions & 18 deletions packages/vinext/src/shims/script.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,26 +107,10 @@ function Script(props: ScriptProps): React.ReactElement | null {
} = props;

const hasMounted = useRef(false);

// SSR path: only "beforeInteractive" renders a <script> tag server-side
if (typeof window === "undefined") {
if (strategy === "beforeInteractive") {
const scriptProps: Record<string, unknown> = { ...rest };
if (src) scriptProps.src = src;
if (id) scriptProps.id = id;
if (dangerouslySetInnerHTML) {
scriptProps.dangerouslySetInnerHTML = dangerouslySetInnerHTML;
}
return React.createElement("script", scriptProps, children);
}
// Other strategies don't render during SSR
return null;
}

const key = id ?? src ?? "";

// Client path: load scripts via useEffect based on strategy
// eslint-disable-next-line react-hooks/rules-of-hooks
// Client path: load scripts via useEffect based on strategy.
// useEffect never runs during SSR, so it's safe to call unconditionally.
useEffect(() => {
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the cleanest fix of the two. useEffect never fires during SSR, so moving it before the typeof window === "undefined" check is a no-op in terms of runtime behavior while satisfying the Rules of Hooks. The removed // eslint-disable-next-line react-hooks/rules-of-hooks suppression is also a good sign — the lint rule now passes without it.

Nicely done.

if (hasMounted.current) return;
hasMounted.current = true;
Expand Down Expand Up @@ -203,6 +187,21 @@ function Script(props: ScriptProps): React.ReactElement | null {
}
}, [src, id, strategy, onLoad, onReady, onError, children, dangerouslySetInnerHTML, key, rest]);

// SSR path: only "beforeInteractive" renders a <script> tag server-side
if (typeof window === "undefined") {
if (strategy === "beforeInteractive") {
const scriptProps: Record<string, unknown> = { ...rest };
if (src) scriptProps.src = src;
if (id) scriptProps.id = id;
if (dangerouslySetInnerHTML) {
scriptProps.dangerouslySetInnerHTML = dangerouslySetInnerHTML;
}
return React.createElement("script", scriptProps, children);
}
// Other strategies don't render during SSR
return null;
}

// The component itself renders nothing — scripts are injected imperatively
return null;
}
Expand Down
Loading