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

fix: normalize routes and decode resolved query #8430

Merged
merged 29 commits into from
Dec 6, 2020
Merged
Show file tree
Hide file tree
Changes from 19 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
7 changes: 5 additions & 2 deletions packages/config/src/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ import pick from 'lodash/pick'
import uniq from 'lodash/uniq'
import consola from 'consola'
import destr from 'destr'
import { TARGETS, MODES, guardDir, isNonEmptyString, isPureObject, isUrl, getMainModule, urlJoin, getPKG } from '@nuxt/utils'
import {
TARGETS, MODES, guardDir, isNonEmptyString,
isPureObject, isUrl, getMainModule, urlJoin, getPKG, safeEncode
} from '@nuxt/utils'
import { defaultNuxtConfigFile, getDefaultNuxtConfig } from './config'

export function getNuxtConfig (_options) {
Expand Down Expand Up @@ -126,7 +129,7 @@ export function getNuxtConfig (_options) {
if (!/\/$/.test(options.router.base)) {
options.router.base += '/'
}
options.router.base = encodeURI(decodeURI(options.router.base))
options.router.base = safeEncode(options.router.base)

// Legacy support for export
if (options.export) {
Expand Down
12 changes: 8 additions & 4 deletions packages/server/src/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,16 @@ export default class Server {
}))

// DX: redirect if router.base in development
if (this.options.dev && this.nuxt.options.router.base !== '/') {
const routerBase = this.nuxt.options.router.base
if (this.options.dev && routerBase !== '/') {
this.useMiddleware({
prefix: false,
handler: (req, res) => {
const to = urlJoin(this.nuxt.options.router.base, req.url)
consola.info(`[Development] Redirecting from \`${decodeURI(req.url)}\` to \`${decodeURI(to)}\` (router.base specified).`)
handler: (req, res, next) => {
if (decodeURI(req.url).startsWith(decodeURI(routerBase))) {
pi0 marked this conversation as resolved.
Show resolved Hide resolved
return next()
}
const to = urlJoin(routerBase, req.url)
consola.info(`[Development] Redirecting from \`${decodeURI(req.url)}\` to \`${decodeURI(to)}\` (router.base specified)`)
res.writeHead(302, {
Location: to
})
Expand Down
11 changes: 9 additions & 2 deletions packages/utils/src/route.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import path from 'path'
import get from 'lodash/get'
import consola from 'consola'

import { r } from './resolve'

const routeChildren = function (route) {
Expand Down Expand Up @@ -202,7 +201,7 @@ export const createRoutes = function createRoutes ({
route.path += i > 0 ? '' : '/'
} else {
const isDynamic = key.startsWith('_')
route.path += '/' + getRoutePathExtension(isDynamic ? key : encodeURIComponent(decodeURIComponent(key)))
route.path += '/' + getRoutePathExtension(isDynamic ? key : safeEncode(key))

if (isDynamic && key.length > 1) {
route.path += '?'
Expand Down Expand Up @@ -280,3 +279,11 @@ export const promisifyRoute = function promisifyRoute (fn, ...args) {
}
return promise
}

export function safeEncodeComponent (str) {
return /%[0-9a-fA-F]{2}/.test(str) ? str : encodeURI(str)
Copy link
Member

Choose a reason for hiding this comment

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

What if url includes %.. as path or parameter ?

Like: /rate/%30?max=%90

Copy link
Member Author

Choose a reason for hiding this comment

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

/%[0-9a-fA-F]{2}/.test('/rate/%30?max=%90') > true

In this case we avoid encoding

Copy link
Member

Choose a reason for hiding this comment

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

So if url also includes special characters, we’ll skip encoding, is that expected?

Like: /rate/%30?max=%90&name= тест

Copy link
Member Author

Choose a reason for hiding this comment

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

Last update: /rate/%30?max=%90&name=%20%D1%82%D0%B5%D1%81%D1%82

Copy link
Member

@danielroe danielroe Dec 4, 2020

Choose a reason for hiding this comment

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

Is there any merit in using something like this as our util - not reinventing url parsing?

function safeEncode (str) {
  const { pathname, search } = new URL(`http://localhost${str}`)
  return `${pathname}${search}`
}

safeEncode('/rate/%30?max=%90&name= тест')
// "/rate/%30?max=%90&name=%20%D1%82%D0%B5%D1%81%D1%82"

Copy link
Member Author

@pi0 pi0 Dec 4, 2020

Choose a reason for hiding this comment

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

Indeed we can but it costs bundle size for rest of the browsers and actually it is not enough to make url utils (for joining or ensure there is no leading slash for example) -- not specific to this PR's case

Copy link
Member

Choose a reason for hiding this comment

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

In fact IE11 works fine for me - just have to wrap in brackets.
Screenshot from 2020-12-04 16-23-20

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough :) Switching to URL

Copy link
Member Author

Choose a reason for hiding this comment

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

There is just one downside that it is implicit it is full url or URI :))

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for bringing it up. Working on util before merge ;)

}

export function safeEncode (str) {
return str.split('/').map(safeEncodeComponent).join('/')
}
21 changes: 16 additions & 5 deletions packages/vue-app/template/router.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import Vue from 'vue'
import Router from 'vue-router'
import { interopDefault } from './utils'<%= isTest ? '// eslint-disable-line no-unused-vars' : '' %>
import { interopDefault, safeEncode } from './utils'<%= isTest ? '// eslint-disable-line no-unused-vars' : '' %>
import scrollBehavior from './router.scrollBehavior.js'

<% function recursiveRoutes(routes, tab, components, indentCount) {
Expand Down Expand Up @@ -105,16 +105,27 @@ export const routerOptions = {
fallback: <%= router.fallback %>
}

function decodeObj(obj) {
for (const key in obj) {
if (typeof obj[key] === 'string') {
obj[key] = decodeURIComponent(obj[key])
}
}
}

export function createRouter () {
const router = new Router(routerOptions)
const resolve = router.resolve.bind(router)

// encodeURI(decodeURI()) ~> support both encoded and non-encoded urls
const resolve = router.resolve.bind(router)
router.resolve = (to, current, append) => {
if (typeof to === 'string') {
to = encodeURI(decodeURI(to))
to = safeEncode(to)
}
const r = resolve(to, current, append)
if (r && r.resolved && r.resolved.query) {
decodeObj(r.resolved.query)
}
return resolve(to, current, append)
return r
}

return router
Expand Down
7 changes: 4 additions & 3 deletions packages/vue-app/template/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import {
<% if (features.middleware) { %>middlewareSeries,<% } %>
<% if (features.middleware && features.layouts) { %>sanitizeComponent,<% } %>
getMatchedComponents,
promisify
promisify,
safeEncode
} from './utils.js'
<% if (features.fetch) { %>import fetchMixin from './mixins/fetch.server'<% } %>
import { createApp<% if (features.layouts) { %>, NuxtError<% } %> } from './index.js'
Expand Down Expand Up @@ -50,12 +51,12 @@ const createNext = ssrContext => (opts) => {
opts.path = urlJoin(routerBase, opts.path)
}
// Avoid loop redirect
if (encodeURI(decodeURI(opts.path)) === ssrContext.url) {
if (decodeURI(opts.path) === decodeURI(ssrContext.url)) {
ssrContext.redirected = false
return
}
ssrContext.res.writeHead(opts.status, {
Location: opts.path
Location: safeEncode(opts.path)
})
ssrContext.res.end()
}
Expand Down
10 changes: 9 additions & 1 deletion packages/vue-app/template/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ export function getLocation (base, mode) {

const fullPath = (path || '/') + window.location.search + window.location.hash

return encodeURI(fullPath)
return safeEncode(fullPath)
}

// Imported from path-to-regexp
Expand Down Expand Up @@ -692,3 +692,11 @@ export function setScrollRestoration (newVal) {
window.history.scrollRestoration = newVal;
} catch(e) {}
}

export function safeEncodeComponent(str) {
return /%[0-9a-fA-F]{2}/.test(str) ? str : encodeURI(str)
}

export function safeEncode(str) {
return str.split('/').map(safeEncodeComponent).join('/')
}
12 changes: 12 additions & 0 deletions test/dev/encoding.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,18 @@ describe('encoding', () => {
expect(response).toContain('Unicode base works!')
})

test('/ö/dynamic?q=food,coffee (encodeURIComponent)', async () => {
const { body: response } = await rp(url('/ö/dynamic?q=food%252Ccoffee'))

expect(response).toContain('food,coffee')
})

test('/ö/@about', async () => {
const { body: response } = await rp(url('/ö/@about'))

expect(response).toContain('About')
})

// Close server and ask nuxt to stop listening to file changes
afterAll(async () => {
await nuxt.close()
Expand Down
43 changes: 30 additions & 13 deletions test/fixtures/encoding/layouts/default.vue
Original file line number Diff line number Diff line change
@@ -1,24 +1,41 @@
<template>
<div>
<div>
<NLink to="/тест">
/тест
</NLink>
<NLink :to="encodeURI('/тест')">
/тест (encoded)
</NLink>
<br>
<NLink to="/тест?spa">
/тест (SPA)
</NLink>
<NLink :to="encodeURI('/тест?spa')">
/тест (SPA encoded)
</NLink>
<ul>
<li v-for="link in links" :key="link">
<NLink :to="link">
{{ link }}
</NLink>
<NLink :to="link.includes('?') ? link.replace('?', '?spa&') : (link + '?spa')">
(spa)
</NLink>
<a :href="encodeURI('/ö') + link">(direct)</a>
</li>
</ul>
</div>
<Nuxt />
</div>
</template>

<script>
export default {
computed: {
links () {
return [
'/redirect',
'/@about',
'/тест',
encodeURI('/тест'),
'/dynamic/سلام چطوری?q=cofee,food,دسر',
encodeURI('/dynamic/سلام چطوری?q=cofee,food,دسر'),
// Using encodeURIComponent on each segment
'/dynamic/%D8%B3%D9%84%D8%A7%D9%85%20%DA%86%D8%B7%D9%88%D8%B1%DB%8C?q=cofee%2Cfood%2C%D8%AF%D8%B3%D8%B1'
]
}
}
}
</script>

<style scoped>
a {
color: grey;
Expand Down
5 changes: 5 additions & 0 deletions test/fixtures/encoding/pages/@about.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<template>
<div>
About
</div>
</template>
17 changes: 17 additions & 0 deletions test/fixtures/encoding/pages/dynamic/_id.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<template>
<div>
<div>Query (SSR): <pre v-text="q" /> </div>
<div>Params (SSR): <pre v-text="p" /> </div>
</div>
</template>

<script>
export default {
asyncData ({ route }) {
return {
q: JSON.stringify(route.query),
p: JSON.stringify(route.params)
}
}
}
</script>
13 changes: 13 additions & 0 deletions test/fixtures/encoding/pages/redirect.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<template>
<div>
Redirecting...
</div>
</template>

<script>
export default {
asyncData ({ redirect }) {
return redirect('/dynamic/重新导向?foo bar')
}
}
</script>