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 public/ file name encoding #10022

Merged
merged 9 commits into from
Jan 10, 2020
47 changes: 26 additions & 21 deletions packages/next/next-server/server/next-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -647,30 +647,35 @@ export default class Server {
}

protected generatePublicRoutes(): Route[] {
const routes: Route[] = []
const publicFiles = recursiveReadDirSync(this.publicDir)

publicFiles.forEach(path => {
const unixPath = path.replace(/\\/g, '/')
// Only include public files that will not replace a page path
// this should not occur now that we check this during build
if (!this.pagesManifest![unixPath]) {
routes.push({
match: route(unixPath),
type: 'route',
name: 'public catchall',
fn: async (req, res, _params, parsedUrl) => {
const p = join(this.publicDir, unixPath)
await this.serveStatic(req, res, p, parsedUrl)
const publicFiles = new Set(
recursiveReadDirSync(this.publicDir).map(p => p.replace(/\\/g, '/'))
)

return [
{
match: route('/:path*'),
name: 'public folder catchall',
fn: async (req, res, params, parsedUrl) => {
const path = `/${(params.path || []).join('/')}`

if (publicFiles.has(path)) {
await this.serveStatic(
req,
res,
// we need to re-encode it since send decodes it
join(this.dir, 'public', encodeURIComponent(path)),
parsedUrl
)
return {
finished: true,
}
},
})
}
})

return routes
}
return {
finished: false,
}
},
} as Route,
]
}

protected getDynamicRoutes() {
Expand Down
11 changes: 6 additions & 5 deletions packages/next/server/next-dev-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,14 +258,14 @@ export default class DevServer extends Server {
protected async _beforeCatchAllRender(
req: IncomingMessage,
res: ServerResponse,
_params: Params,
params: Params,
parsedUrl: UrlWithParsedQuery
) {
const { pathname } = parsedUrl

const path = `/${(params.path || []).join('/')}`
// check for a public file, throwing error if there's a
// conflicting page
if (await this.hasPublicFile(pathname!)) {
if (await this.hasPublicFile(path)) {
if (await this.hasPage(pathname!)) {
const err = new Error(
`A conflicting public file and page file was found for path ${pathname} https://err.sh/zeit/next.js/conflicting-public-file-page`
Expand All @@ -274,7 +274,7 @@ export default class DevServer extends Server {
await this.renderError(err, req, res, pathname!, {})
return true
}
await this.servePublic(req, res, pathname!)
await this.servePublic(req, res, path)
return true
}

Expand Down Expand Up @@ -484,7 +484,8 @@ export default class DevServer extends Server {
}

servePublic(req: IncomingMessage, res: ServerResponse, path: string) {
const p = join(this.publicDir, path)
const p = join(this.publicDir, encodeURIComponent(path))
// we need to re-encode it since send decodes it
return this.serveStatic(req, res, p)
}

Expand Down
1 change: 1 addition & 0 deletions test/integration/dynamic-routing/public/hello copy.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hello world copy
1 change: 1 addition & 0 deletions test/integration/dynamic-routing/public/hello%20copy.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hello world %20
1 change: 1 addition & 0 deletions test/integration/dynamic-routing/public/hello+copy.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hello world +
32 changes: 32 additions & 0 deletions test/integration/dynamic-routing/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,34 @@ function runTests(dev) {
expect(data).toMatch(/hello world/)
})

it('should serve file with space from public folder', async () => {
const res = await fetchViaHTTP(appPort, '/hello copy.txt')
const text = (await res.text()).trim()
expect(text).toBe('hello world copy')
expect(res.status).toBe(200)
})

it('should serve file with plus from public folder', async () => {
const res = await fetchViaHTTP(appPort, '/hello+copy.txt')
const text = (await res.text()).trim()
expect(text).toBe('hello world +')
expect(res.status).toBe(200)
})

it('should serve file from public folder encoded', async () => {
const res = await fetchViaHTTP(appPort, '/hello%20copy.txt')
const text = (await res.text()).trim()
expect(text).toBe('hello world copy')
expect(res.status).toBe(200)
})

it('should serve file with %20 from public folder', async () => {
const res = await fetchViaHTTP(appPort, '/hello%2520copy.txt')
const text = (await res.text()).trim()
expect(text).toBe('hello world %20')
expect(res.status).toBe(200)
})

if (dev) {
it('should work with HMR correctly', async () => {
const browser = await webdriver(appPort, '/post-1/comments')
Expand Down Expand Up @@ -389,6 +417,10 @@ function runTests(dev) {
join(appDir, '.next/routes-manifest.json')
)

for (const route of manifest.dynamicRoutes) {
route.regex = normalizeRegEx(route.regex)
}

expect(manifest).toEqual({
version: 1,
basePath: '',
Expand Down