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 #2688

Closed
wants to merge 4 commits into from

Conversation

usualoma
Copy link
Member

#2672

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

@usualoma usualoma changed the title Feat decode uri in get path feat: decode percent-encoded path in getPath May 16, 2024
// If the path contains percent encoding, use `indexOf()` to find '?' and return the result immediately.
// 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 decodeURIComponent(url.slice(start, queryIndex === -1 ? undefined : queryIndex))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return decodeURIComponent(url.slice(start, queryIndex === -1 ? undefined : queryIndex))
return decodeURI(url.slice(start, queryIndex === -1 ? undefined : queryIndex))

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion.

With the current code, if we s/decodeURIComponent/decodeURI/ , the following test fails, are you saying that "id is çawa yî%3F" is the expected result?

- id is çawa yî?
+ id is çawa yî%3F

https://github.com/usualoma/hono/blob/29735fe9cb5038ba5a45245be93344e5c16cfaef/src/hono.test.ts#L742-L748

This can be fixed by reverting 7192497. If we do that, the following test will fail. (double decoded)

- posts of %25
+ posts of %

https://github.com/usualoma/hono/blob/29735fe9cb5038ba5a45245be93344e5c16cfaef/src/hono.test.ts#L768-L774

Copy link
Contributor

@szmarczak szmarczak May 17, 2024

Choose a reason for hiding this comment

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

This can be fixed by reverting 7192497.

Correct.

If we do that, the following test will fail. (double decoded)

Ah, I see now, thanks for explaining! Well, in that case we need to .replaceAll('%25', '%2525') prior to calling decodeURIComponent.

Copy link
Member Author

Choose a reason for hiding this comment

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

@szmarczak Thank you!

Unfortunately,

decodeURI('%2F') === decodeURI('%252F')

And cannot in principle be reverted with replaceAll(), since the original information is lost when the decodeURI() is applied.

For my part, I think that "using decodeURI() on a requested URL" is impossible.

Copy link
Contributor

@szmarczak szmarczak May 18, 2024

Choose a reason for hiding this comment

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

What about doing .replaceAll('%25', '%2525') before calling decodeURI? This means that in order to catch % people will need to use %25 in the path string, e.g. /foo%25bar (not: /foo%bar), but for all other percent-encoded characters it should decode as expected (meaning /foo/bar/ąę should work as expected; also the params should use decodeURIComponent - double decoding should be prevented with the mentioned replaceAll happening just before decodeURI).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see, that is true. That's great! I think it will be the spec you want.

I'm not going to include this change in this PR, since this PR has the same result as Django, but I'll make a separate PR for this.

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!

@usualoma usualoma closed this May 21, 2024
@usualoma usualoma reopened this May 21, 2024
@usualoma usualoma closed this May 21, 2024
@usualoma usualoma deleted the feat-decode-uri-in-get-path branch May 21, 2024 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants