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

feat: decode percent-encoded path in getPath #2714

Merged
merged 10 commits into from
May 22, 2024

Conversation

usualoma
Copy link
Member

@usualoma usualoma commented May 18, 2024

#2672

This PR contains #2688 + #2688 (comment)

Specification Changes

There are no major release-level changes, but there are some minor changes as follows

Symbols and multibyte characters can be included in the static string of the routing definition

When you define a routing as follows, it will now match, whereas before it did not match because the percent encoding for static strings was not taken into account when routing.

app.get('/🔥', (c) => c.text(`I'm hono`)
http://example.com/%F0%9F%94%A5

Requests made with a URL that are encoded more than necessary are now matched with a decoded URL

Suppose we have a routing definition like this,

app.get('/static', (c) => c.text('static)

Even if unreserved characters are extra encoded, they are now decoded and matched.

http://example.com/%73tatic // %73 is 's'

Regular expressions for path parameters are now tested against decoded characters

We do not think that this will break any applications, but applications that use regular expressions in path parameters should check when updating to see if they are affected.

Suppose we have a routing definition like this,

app.get('/about/:product{🔥|Hono}', (c) => c.text(`Hono is a framework`)

The following URLs match

http://example.com/about/%F0%9F%94%A5

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun denoify to generate files for Deno
  • bun run format:fix && bun run lint:fix to format the code

// Although this is a performance disadvantage, it is acceptable since we prefer cases that do not include percent encoding.
const queryIndex = url.indexOf('?', i)
return decodeURI(
url.slice(start, queryIndex === -1 ? undefined : queryIndex).replace(/%25/g, '%2525')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my benchmarks it seems like str.includes('%25') ? str.replaceAll('%25', '%2525') : str is faster than calling .replace with regex. Can you verify?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. You are right!

It's faster to check in .includes first.
s/replace/replaceAll/ didn't change in my benchmark results, so I'll leave replace(//g) as it is, as it is generally used elsewhere in Hono.

Applied in 0eeaa7e

usualoma and others added 3 commits May 19, 2024 05:55
…2525"

Co-authored-by: Szymon Marczak <36894700+szmarczak@users.noreply.github.com>
@usualoma usualoma marked this pull request as ready for review May 19, 2024 11:55
@yusukebe yusukebe added the v4.4 label May 21, 2024
@yusukebe
Copy link
Member

@usualoma

Thanks! I'll include this in the next minor release and merge it into the next branch later.

@yusukebe yusukebe changed the base branch from main to next May 22, 2024 20:38
@yusukebe yusukebe merged commit 778ac10 into honojs:next May 22, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants