fix(req): correct c.req.param decodes invalid percent strings (#3573)

* fix(req): correct `c.req.param` decodes invalid percent strings * make `tryDecodeURIComponent` as a `const` function * use `/\%/.test()` for a better performance
honojs · Oct 30, 2024 · 2bde76d · 2bde76d
1 parent e9de50d
commit 2bde76d
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 44 deletions.
diff --git a/src/hono.test.ts b/src/hono.test.ts
@@ -771,46 +771,22 @@ describe('Routing', () => {
 
     it('should decode alphabets with invalid UTF-8 sequence', async () => {
       app.get('/static/:path', (c) => {
-        try {
-          return c.text(`by c.req.param: ${c.req.param('path')}`) // this should throw an error
-        } catch (e) {
-          return c.text(`by c.req.url: ${c.req.url.replace(/.*\//, '')}`)
-        }
+        return c.text(`by c.req.param: ${c.req.param('path')}`)
       })
 
       const res = await app.request('http://localhost/%73tatic/%A4%A2') // %73 is 's', %A4%A2 is invalid UTF-8 sequence
       expect(res.status).toBe(200)
-      expect(await res.text()).toBe('by c.req.url: %A4%A2')
+      expect(await res.text()).toBe('by c.req.param: %A4%A2')
     })
 
     it('should decode alphabets with invalid percent encoding', async () => {
       app.get('/static/:path', (c) => {
-        try {
-          return c.text(`by c.req.param: ${c.req.param('path')}`) // this should throw an error
-        } catch (e) {
-          return c.text(`by c.req.url: ${c.req.url.replace(/.*\//, '')}`)
-        }
+        return c.text(`by c.req.param: ${c.req.param('path')}`)
       })
 
       const res = await app.request('http://localhost/%73tatic/%a') // %73 is 's', %a is invalid percent encoding
       expect(res.status).toBe(200)
-      expect(await res.text()).toBe('by c.req.url: %a')
-    })
-
-    it('should be able to catch URIError', async () => {
-      app.onError((err, c) => {
-        if (err instanceof URIError) {
-          return c.text(err.message, 400)
-        }
-        throw err
-      })
-      app.get('/static/:path', (c) => {
-        return c.text(`by c.req.param: ${c.req.param('path')}`) // this should throw an error
-      })
-
-      const res = await app.request('http://localhost/%73tatic/%a') // %73 is 's', %a is invalid percent encoding
-      expect(res.status).toBe(400)
-      expect(await res.text()).toBe('URI malformed')
+      expect(await res.text()).toBe('by c.req.param: %a')
     })
 
     it('should not double decode', async () => {

diff --git a/src/request.ts b/src/request.ts
@@ -13,7 +13,7 @@ import { parseBody } from './utils/body'
 import type { BodyData, ParseBodyOptions } from './utils/body'
 import type { CustomHeader, RequestHeader } from './utils/headers'
 import type { Simplify, UnionToIntersection } from './utils/types'
-import { decodeURIComponent_, getQueryParam, getQueryParams } from './utils/url'
+import { decodeURIComponent_, getQueryParam, getQueryParams, tryDecode } from './utils/url'
 
 type Body = {
   json: any
@@ -24,6 +24,8 @@ type Body = {
 }
 type BodyCache = Partial<Body & { parsedBody: BodyData }>
 
+const tryDecodeURIComponent = (str: string) => tryDecode(str, decodeURIComponent_)
+
 export class HonoRequest<P extends string = '/', I extends Input['out'] = {}> {
   /**
    * `.raw` can get the raw Request object.
@@ -95,8 +97,7 @@ export class HonoRequest<P extends string = '/', I extends Input['out'] = {}> {
   private getDecodedParam(key: string): string | undefined {
     const paramKey = this.#matchResult[0][this.routeIndex][1][key]
     const param = this.getParamValue(paramKey)
-
-    return param ? (/\%/.test(param) ? decodeURIComponent_(param) : param) : undefined
+    return param ? (/\%/.test(param) ? tryDecodeURIComponent(param) : param) : undefined
   }
 
   private getAllDecodedParams(): Record<string, string> {
@@ -106,7 +107,7 @@ export class HonoRequest<P extends string = '/', I extends Input['out'] = {}> {
     for (const key of keys) {
       const value = this.getParamValue(this.#matchResult[0][this.routeIndex][1][key])
       if (value && typeof value === 'string') {
-        decoded[key] = /\%/.test(value) ? decodeURIComponent_(value) : value
+        decoded[key] = /\%/.test(value) ? tryDecodeURIComponent(value) : value
       }
     }
 

diff --git a/src/utils/url.ts b/src/utils/url.ts
@@ -74,29 +74,32 @@ export const getPattern = (label: string): Pattern | null => {
   return null
 }
 
-/**
- * Try to apply decodeURI() to given string.
- * If it fails, skip invalid percent encoding or invalid UTF-8 sequences, and apply decodeURI() to the rest as much as possible.
- * @param str The string to decode.
- * @returns The decoded string that sometimes contains undecodable percent encoding.
- * @example
- * tryDecodeURI('Hello%20World') // 'Hello World'
- * tryDecodeURI('Hello%20World/%A4%A2') // 'Hello World/%A4%A2'
- */
-const tryDecodeURI = (str: string): string => {
+type Decoder = (str: string) => string
+export const tryDecode = (str: string, decoder: Decoder): string => {
   try {
-    return decodeURI(str)
+    return decoder(str)
   } catch {
     return str.replace(/(?:%[0-9A-Fa-f]{2})+/g, (match) => {
       try {
-        return decodeURI(match)
+        return decoder(match)
       } catch {
         return match
       }
     })
   }
 }
 
+/**
+ * Try to apply decodeURI() to given string.
+ * If it fails, skip invalid percent encoding or invalid UTF-8 sequences, and apply decodeURI() to the rest as much as possible.
+ * @param str The string to decode.
+ * @returns The decoded string that sometimes contains undecodable percent encoding.
+ * @example
+ * tryDecodeURI('Hello%20World') // 'Hello World'
+ * tryDecodeURI('Hello%20World/%A4%A2') // 'Hello World/%A4%A2'
+ */
+const tryDecodeURI = (str: string) => tryDecode(str, decodeURI)
+
 export const getPath = (request: Request): string => {
   const url = request.url
   const start = url.indexOf('/', 8)