Skip to content
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.

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

Add warning for invalid href being passed to router #8231

Merged
merged 6 commits into from
Aug 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 19 additions & 0 deletions errors/invalid-href-passed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Invalid href passed to router

#### Why This Error Occurred

Next.js provides a router which can be utilized via a component imported via `next/link`, a wrapper `withRouter(Component)`, and now a hook `useRouter()`.
When using any of these, it is expected they are only used for internal navigation, i.e. navigating between pages in the same Next.js application.

Either you passed a non-internal `href` to a `next/link` component or you called `Router#push` or `Router#replace` with one.

Invalid `href`s include external sites (https://google.com) and `mailto:` links. In the past, usage of these invalid `href`s could have gone unnoticed but since they can cause unexpected behavior.
We now show a warning in development for them.

#### Possible Ways to Fix It

Look for any usage of `next/link` or `next/router` that is being passed a non-internal `href` and replace them with either an anchor tag (`<a>`) or `window.location.href = YOUR_HREF`.

### Useful Links

- [Routing section in Documentation](https://nextjs.org/docs#routing)
25 changes: 21 additions & 4 deletions packages/next-server/lib/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,16 @@ export default class Router implements BaseRouter {
return resolve(true)
}

const { pathname, query } = parse(url, true)
const { pathname, query, protocol } = parse(url, true)

if (!pathname || protocol) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this condition looks wrong, IMO should be !pathname || !protocol

Copy link
Contributor

@Kinbaum Kinbaum Sep 30, 2019

Choose a reason for hiding this comment

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

I agree, just stumbled upon this today using Router.push and passing an absolute URL object to both href, and as.

if (process.env.NODE_ENV !== 'production') {
throw new Error(
`Invalid href passed to router: ${url} https://err.sh/zeit/next.js/invalid-href-passed`
)
}
return resolve(false)
}

// If asked to change the current URL we should reload the current page
// (not location.reload() but reload getInitialProps and other Next.js stuffs)
Expand Down Expand Up @@ -326,7 +335,7 @@ export default class Router implements BaseRouter {
const appComp: any = this.components['/_app'].Component
;(window as any).next.isPrerendered =
appComp.getInitialProps === appComp.origGetInitialProps &&
!routeInfo.Component.getInitialProps
!(routeInfo.Component as any).getInitialProps
}

// @ts-ignore pathname is always defined
Expand Down Expand Up @@ -536,10 +545,18 @@ export default class Router implements BaseRouter {
*/
prefetch(url: string): Promise<void> {
return new Promise((resolve, reject) => {
const { pathname, protocol } = parse(url)

if (!pathname || protocol) {
if (process.env.NODE_ENV !== 'production') {
throw new Error(
`Invalid href passed to router: ${url} https://err.sh/zeit/next.js/invalid-href-passed`
)
}
return
}
// Prefetch is not supported in development mode because it would trigger on-demand-entries
if (process.env.NODE_ENV !== 'production') return

const { pathname } = parse(url)
// @ts-ignore pathname is always defined
const route = toRoute(pathname)
this.pageLoader.prefetch(route).then(resolve, reject)
Expand Down
15 changes: 7 additions & 8 deletions packages/next/client/link.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ class Link extends Component<LinkProps> {
let { href, as } = this.formatUrls(this.props.href, this.props.as)

if (!isLocal(href)) {
// ignore click if it's outside our scope
// ignore click if it's outside our scope (e.g. https://google.com)
return
}

Expand All @@ -171,14 +171,13 @@ class Link extends Component<LinkProps> {
// replace state instead of push if prop is present
Router[this.props.replace ? 'replace' : 'push'](href, as, {
shallow: this.props.shallow,
}).then((success: boolean) => {
if (!success) return
if (scroll) {
window.scrollTo(0, 0)
document.body.focus()
}
})
.then((success: boolean) => {
if (!success) return
if (scroll) {
window.scrollTo(0, 0)
document.body.focus()
}
})
}

prefetch() {
Expand Down
27 changes: 27 additions & 0 deletions test/integration/invalid-href/pages/first.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import Link from 'next/link'
import { useRouter } from 'next/router'

const invalidLink = 'mailto:idk@idk.com'

export default () => {
const { query, ...router } = useRouter()
const { method } = query

return method ? (
<a
id='click-me'
onClick={e => {
e.preventDefault()
router[method](invalidLink)
}}
>
invalid link :o
</a>
) : (
// this should throw an error on load since prefetch
// receives the invalid href
<Link href={invalidLink}>
<a id='click-me'>invalid link :o</a>
</Link>
)
}
2 changes: 2 additions & 0 deletions test/integration/invalid-href/pages/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// page used for loading and installing error catcher
export default () => <p>Hi 👋</p>
27 changes: 27 additions & 0 deletions test/integration/invalid-href/pages/second.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import Link from 'next/link'
import { useRouter } from 'next/router'

const invalidLink = 'https://google.com/another'

export default () => {
const { query, ...router } = useRouter()
const { method } = query

return method ? (
<a
id='click-me'
onClick={e => {
e.preventDefault()
router[method](invalidLink)
}}
>
invalid link :o
</a>
) : (
// this should throw an error on load since prefetch
// receives the invalid href
<Link href={invalidLink}>
<a id='click-me'>invalid link :o</a>
</Link>
)
}
119 changes: 119 additions & 0 deletions test/integration/invalid-href/test/index.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/* eslint-env jest */
/* global jasmine */
import { join } from 'path'
import webdriver from 'next-webdriver'
import {
findPort,
launchApp,
killApp,
nextStart,
nextBuild,
getReactErrorOverlayContent,
waitFor
} from 'next-test-utils'

jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 5
let app
let appPort
const appDir = join(__dirname, '..')

const firstErrorRegex = /Invalid href passed to router: mailto:idk@idk.com.*invalid-href-passed/
const secondErrorRegex = /Invalid href passed to router: .*google\.com.*invalid-href-passed/

const showsError = async (pathname, regex, click = false) => {
const browser = await webdriver(appPort, pathname)
if (click) {
await browser.elementByCss('a').click()
}
const errorContent = await getReactErrorOverlayContent(browser)
expect(errorContent).toMatch(regex)
await browser.close()
}

const noError = async (pathname, click = false) => {
const browser = await webdriver(appPort, '/')
await browser.eval(`(function() {
window.caughtErrors = []
window.addEventListener('error', function (error) {
window.caughtErrors.push(error.message || 1)
})
window.addEventListener('unhandledrejection', function (error) {
window.caughtErrors.push(error.message || 1)
})
window.next.router.replace('${pathname}')
})()`)
await waitFor(250)
if (click) {
await browser.elementByCss('a').click()
}
const numErrors = await browser.eval(`window.caughtErrors.length`)
expect(numErrors).toBe(0)
await browser.close()
}

describe('Invalid hrefs', () => {
describe('dev mode', () => {
beforeAll(async () => {
appPort = await findPort()
app = await launchApp(appDir, appPort)
})
afterAll(() => killApp(app))

it('shows error when mailto: is used as href on Link', async () => {
await showsError('/first', firstErrorRegex)
})

it('shows error when mailto: is used as href on router.push', async () => {
await showsError('/first?method=push', firstErrorRegex, true)
})

it('shows error when mailto: is used as href on router.replace', async () => {
await showsError('/first?method=replace', firstErrorRegex, true)
})

it('shows error when https://google.com is used as href on Link', async () => {
await showsError('/second', secondErrorRegex)
})

it('shows error when http://google.com is used as href on router.push', async () => {
await showsError('/second?method=push', secondErrorRegex, true)
})

it('shows error when https://google.com is used as href on router.replace', async () => {
await showsError('/second?method=replace', secondErrorRegex, true)
})
})

describe('production mode', () => {
beforeAll(async () => {
await nextBuild(appDir)
appPort = await findPort()
app = await nextStart(appDir, appPort)
})
afterAll(() => killApp())

it('shows error when mailto: is used as href on Link', async () => {
await noError('/first')
})

it('shows error when mailto: is used as href on router.push', async () => {
await noError('/first?method=push', true)
})

it('shows error when mailto: is used as href on router.replace', async () => {
await noError('/first?method=replace', true)
})

it('shows error when https://google.com is used as href on Link', async () => {
await noError('/second')
})

it('shows error when http://google.com is used as href on router.push', async () => {
await noError('/second?method=push', true)
})

it('shows error when https://google.com is used as href on router.replace', async () => {
await noError('/second?method=replace', true)
})
})
})