hono icon indicating copy to clipboard operation
hono copied to clipboard

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

Open yusukebe opened this issue 1 year ago • 2 comments

Fixes #3550

In the current behavior, the c.req.param() will throw the error when decoding invalid percent encoding or invalid UTF-8 sequence.

// app
app.get('/static/:path', (c) => c.text(c.req.param('path'))

// throw `URIError: URI malformed` error:
await app.request('/static/%A4%A2') //  %A4%A2 is invalid UTF-8 sequence

// throw `URIError: URI malformed` error:
await app.request('/static/%a') // %a is invalid percent encoding

These behaviors are expected in the current version implementation; they are tested:

https://github.com/honojs/hono/compare/fix/request-dont-throw-error-with-percent-strings?expand=1#diff-8ac13809c9886e994d1db33943de82df4d6c5d88b73fd0270c0189804ff565c2L772-L814

However, according to #3550, throwing the error will stop the application procedure, and it can accept malicious requests. I think it should not throw the error. This should be fixed, though we should rewrite the test code.

The correct behavior

How to handle the invalid path parameter strings. One way is throwing 400 Bad Request, but as I commented, it should follow the decode method of the getPath() to resolve the URL path:

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

So, the result will be like the following:

res = await app.request('/static/%A4%A2')
console.log(await res.text()) // %A4%A2

res = await app.request('/static/%a')
console.log(await res.text()) // %a

In this PR, I used the same logic of tryDecodeURI for tryDecodeURIComponent.

The author should do the following, if applicable

  • [x] Add tests
  • [x] Run tests
  • [x] bun run format:fix && bun run lint:fix to format the code
  • [ ] Add TSDoc/JSDoc to document the code

yusukebe avatar Oct 28 '24 01:10 yusukebe

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.71%. Comparing base (3a59d86) to head (546a188). Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3573      +/-   ##
==========================================
- Coverage   94.71%   94.71%   -0.01%     
==========================================
  Files         157      158       +1     
  Lines        9539     9537       -2     
  Branches     2774     2794      +20     
==========================================
- Hits         9035     9033       -2     
  Misses        504      504              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 28 '24 01:10 codecov[bot]

Hi @usualoma !

Can you review this?

yusukebe avatar Oct 28 '24 01:10 yusukebe

@usualoma Thanks! Agree all. Can you review this again?

yusukebe avatar Oct 29 '24 04:10 yusukebe

@usualoma

Thanks! Merging now.

yusukebe avatar Oct 30 '24 01:10 yusukebe