prettyJSON throws on invalid query parameters

[lincolnremi]

What version of Hono are you using?

4.7.2

What runtime/platform is your app running on? (with version if possible)

NodeJS v20.16.0

What steps can reproduce the bug?

prettyJSON calls HonoRequest.query() without a try clause, so when an invalid query parameter is passed, it throws the URIError: URI Malformed error.

Here is the code:

import { serve } from '@hono/node-server'
import { Hono } from 'hono'
import { prettyJSON } from "hono/pretty-json";

const app = new Hono()
app.use("*", prettyJSON());

app.get('/', (c) => {
  return c.text('Hello Hono!')
})

serve({
  fetch: app.fetch,
  port: 3000
}, (info) => {
  console.log(`Server is running on http://localhost:${info.port}`)
})

This code was obtained by running npm create hono@latest and adding two lines for importing and using prettyJSON.

What is the expected behavior?

It should not throw an error

What do you see instead?

Making requests to the server throws the error as follows:

lincolnbergeson@Lincolns-MacBook-Pro-2 ~ % curl 'localhost:3000'
Hello Hono!%
lincolnbergeson@Lincolns-MacBook-Pro-2 ~ % curl 'localhost:3000?%E0%A4%A'
Internal Server Error%

And the server logs:

Server is running on http://localhost:3000
URIError: URI malformed
    at decodeURIComponent (<anonymous>)
    at _decodeURI (file:///Users/lincolnbergeson/Documents/GitHub/hono-demo/node_modules/hono/dist/utils/url.js:132:38)
    at _getQueryParam (file:///Users/lincolnbergeson/Documents/GitHub/hono-demo/node_modules/hono/dist/utils/url.js:171:14)
    at HonoRequest.query (file:///Users/lincolnbergeson/Documents/GitHub/hono-demo/node_modules/hono/dist/request.js:41:12)
    at prettyJSON2 (file:///Users/lincolnbergeson/Documents/GitHub/hono-demo/node_modules/hono/dist/middleware/pretty-json/index.js:5:26)
    at dispatch (file:///Users/lincolnbergeson/Documents/GitHub/hono-demo/node_modules/hono/dist/compose.js:22:23)
    at file:///Users/lincolnbergeson/Documents/GitHub/hono-demo/node_modules/hono/dist/compose.js:5:12
    at file:///Users/lincolnbergeson/Documents/GitHub/hono-demo/node_modules/hono/dist/hono-base.js:195:31
    at #dispatch (file:///Users/lincolnbergeson/Documents/GitHub/hono-demo/node_modules/hono/dist/hono-base.js:205:7)
    at fetch (file:///Users/lincolnbergeson/Documents/GitHub/hono-demo/node_modules/hono/dist/hono-base.js:208:26)

Additional information

No response

[lincolnremi]

This patch should work:

diff --git a/src/middleware/pretty-json/index.ts b/src/middleware/pretty-json/index.ts
index bc198e58..a5428f7d 100644
--- a/src/middleware/pretty-json/index.ts
+++ b/src/middleware/pretty-json/index.ts
@@ -40,7 +40,15 @@ interface PrettyOptions {
 export const prettyJSON = (options?: PrettyOptions): MiddlewareHandler => {
   const targetQuery = options?.query ?? 'pretty'
   return async function prettyJSON(c, next) {
-    const pretty = c.req.query(targetQuery) || c.req.query(targetQuery) === ''
+    let pretty;
+    try {
+      pretty = c.req.query(targetQuery) || c.req.query(targetQuery) === ''
+    } catch (e) {
+      // Ignore URIError caused by invalid query parameter
+      if (!(e instanceof URIError)) {
+        throw e;
+      }
+    }
     await next()
     if (pretty && c.res.headers.get('Content-Type')?.startsWith('application/json')) {
       const obj = await c.res.json()

I can put this into a PR & test if the maintainers are open to it

[yusukebe]

Hi @lincolnremi Thank you for the issue! Good catch. But please wait a bit.

Hey @usualoma ! What do you think of the following?

Not only a Pretty JSON middleware case, but I wonder whether it is correct behavior to get a URIError: URI malformed error if you passed an invalid query to c.req.query().

In the case of Express, instead of throwing an error, the invalid query is treated as a string.

The app code:

const express = require('express')
const app = express()

app.get('/', (req, res) => {
  const foo = req.query.foo
  res.send(`foo: ${foo}`)
})

app.listen(3000, () => {
  console.log('Server is running on port 3000')
})

The results:

$ curl 'localhost:3000?foo=%E3%81%82'
foo: あ%
$ curl 'localhost:3000?foo=%E3%81%8'
foo: %E3%81%8%

But the Hono app:

import { Hono } from 'hono'

const app = new Hono()

app.get('/', (c) => {
  const foo = c.req.query('foo')
  return c.text(`foo: ${foo}`)
})

export default app

Perhaps we should follow the same spec as Express? The results:

$ curl 'localhost:3000?foo=%E3%81%82'
foo: あ%
$ curl 'localhost:3000?foo=%E3%81%8'
Internal Server Error%

[EdamAme-x]

Express relies on query parser libraries such as qs. As suggested by someone before, it might be a good idea to add a custom query parser option.

[usualoma]

I also think it would be good if the result here was the same as with the express.

$ curl 'localhost:3000?foo=%E3%81%82'
foo: あ
$ curl 'localhost:3000?foo=%E3%81%8'
foo: %E3%81%8

I think it would be better if users defined and used their own custom query parser. I think it is more likely that users want to use their own parser because they want to define their own type, so I think it would be better if they defined it themselves rather than incorporating it into the hono.

const foo = new myCustomQuery(c).get(‘foo’)

[yusukebe]

@EdamAme-x @usualoma

Thank you for the comments!

Regarding the custom query parser, I prefer to use the syntax @usualoma suggested.

myCustomQuery(c).get(‘foo’)

But we should discuss implementing the same behavior with Express for the default. Of course, the user can change the behavior with the custom query parser, but almost all users do not use it, and the default behavior is important.

We can treat the difference between Hono's and Express's as a bug or unexpected behavior. I think we should make it so that the error is not thrown if c.req.query parses invalid strings since it can cause vulnerability by intentionally sending invalid values. It's unexpected behavior, so I think it would be better that we should fix it.

I want to know what you think.

[usualoma]

Hi @yusukebe. I'm sorry for not getting back to you sooner.

Yes, I also think it would be preferable to make it behave the same as Express. I believe the following changes would achieve this.

diff --git a/src/utils/url.ts b/src/utils/url.ts
index 27604022..5772adae 100644
--- a/src/utils/url.ts
+++ b/src/utils/url.ts
@@ -216,7 +216,7 @@ const _decodeURI = (value: string) => {
   if (value.indexOf('+') !== -1) {
     value = value.replace(/\+/g, ' ')
   }
-  return value.indexOf('%') !== -1 ? decodeURIComponent_(value) : value
+  return value.indexOf('%') !== -1 ? tryDecode(value, decodeURIComponent_) : value
 }
 
 const _getQueryParam = (

[yusukebe]

This issue was fixed with #4110. You can use the latest version 4.8.0 including the fix! Thanks.