Path param decoding failure not handled

[oladayo21]

What version of Hono are you using?

4.4.2

What runtime/platform is your app running on?

Node

What steps can reproduce the bug?

import { Hono } from 'hono';

const app = new Hono();

app.get('/api/:tenant/offers', (c) =>
  c.json({ tenant: c.req.param('tenant') })
);

const response = await app.request('/api/%F0%27%22%3Cbmt>bmt%3D1/offers');

This was a malicious request sent to exploit the param. We have defined a schema to validate the tenant, so our expectation is that the api should respond with a 400 Bad Request, instead it crashes with a 500 Internal server error. This is because attempt to decode the param here fails but the failure is not handled. Is this intended? Wrapping a trycatch around that call should fix the issue I guess.

What is the expected behavior?

No response

What do you see instead?

No response

Additional information

No response

[yusukebe]

@oladayo21 thank you for raising the issue!

Hi @usualoma

I also think this should not throw a 500 error. There are some options for the behavior if it can't decode the param:

• Throw a 400 error.
• Return %F0%27%22%3Cbmt>bmt%3D1 that is not decoded.
• Return undefined.

In my opinion, throwing 400 with HTTPExpception is good. For example:

diff --git a/src/request.ts b/src/request.ts
index ffd40fa6..8381ddf7 100644
--- a/src/request.ts
+++ b/src/request.ts
@@ -1,4 +1,5 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
+import { HTTPException } from './http-exception'
 import type { Result } from './router'
 import type {
   Input,
@@ -95,7 +96,18 @@ export class HonoRequest<P extends string = '/', I extends Input['out'] = {}> {
     const paramKey = this.#matchResult[0][this.routeIndex][1][key]
     const param = this.getParamValue(paramKey)

-    return param ? (/\%/.test(param) ? decodeURIComponent_(param) : param) : undefined
+    if (param && /\%/.test(param)) {
+      try {
+        return decodeURIComponent_(param)
+      } catch (e) {
+        if (e instanceof URIError) {
+          throw new HTTPException(400, {
+            message: `URIError: Failed to decode param ${param}`,
+          })
+        }
+      }
+    }
+    return param
   }

   private getAllDecodedParams(): Record<string, string> {

@usualoma What do you think of it?

[oladayo21]

@yusukebe I agree with your proposed solution of throwing a 400 exception.

[usualoma]

Hi @oladayo21, Thank you!

@yusukebe

At first, I thought throwing 400 exceptions would be appropriate, but if we're conscious of web standards, your second option might be good. This is because the result of parsing the parameters is expected to be the same as the result obtained via the following Web standard objects. Honestly, I think it goes against user expectations a little, but I think it's important that Hono behaves in the same way as web standards, so I think the following result is appropriate.

• URLSearchParams
• Request

URLSearchParams

> new URLSearchParams('k=%h').get('k')
'%h'

Request

> new Request('https://localhost/', { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', }, body: 'k=%h', }).formData().then((d) => console.log(d.get('k')))
%h

[yusukebe]

@usualoma

I don't think we have to decode the value of path params in the same way as URLSearchParams or Request. This issue is how to decode the path string(c.req.param('id') of /posts/:id), not the URL query (c.req.query('page') of /posts?page=123).

How about following the logic in getPath()?

https://github.com/honojs/hono/blob/0a99bd3e74cc444d4b968dab775c99674b89835f/src/utils/url.ts#L112

Thre results are below:

const decodeParamValues = (v: string) => {
  return tryDecodeURI(v.includes('%25') ? v.replace(/%25/g, '%2525') : v)
}

console.log(decodeParamValues('%E7%82%8E')) // 炎
console.log(decodeParamValues('%h')) // %h
console.log(decodeParamValues('%F0%27%22%3Cbmt>bmt%3D1')) // %F0%27%22%3Cbmt>bmt%3D1

However, the result will be the same as the "second" option I said.

[usualoma]

@yusukebe Thanks for your reply.

Yes, I agree with your point. Indeed, path parameters are a matter of application semantics. (They are also related to web standards, but they don't necessarily have to be the same.)

I don't think we have to decode the value of path params in the same way as URLSearchParams or Request.

However, as a user, I think it would be easier to use if the behavior when sending data with invalid percent encoding was the same for both c.req.param() and c.req.query(). And for c.req.query(), I think it would be better if it had the same behavior as URLSearchParams.

Currently, if c.req.query() receives invalid percent encoding, it will result in an internal server error, so I think it would be good if the behavior could be adjusted to include this.

Considering this, I think the method you have shown, decodeParamValues is suitable for decoding the path parameters.

[yusukebe]

@usualoma

Currently, if c.req.query() receives invalid percent encoding, it will result in an internal server error, so I think it would be good if the behavior could be adjusted to include this.

Ah, exactly. This means the following case, right?

const app = new Hono()

app.get('/', (c) => {
  const q = c.req.query('q') ?? 'q'
  return c.text(q)
})

const res = await app.request('/?q=%h') // URIError
console.log(res.status) // 500

I also think this behavior should follow URLSearchParams.