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

[Bug?]: fastify server not handling periods in query params #9663

Open
1 task
ajluker opened this issue Dec 12, 2023 · 9 comments
Open
1 task

[Bug?]: fastify server not handling periods in query params #9663

ajluker opened this issue Dec 12, 2023 · 9 comments
Assignees
Labels
bug/confirmed We have confirmed this is a bug topic/web

Comments

@ajluker
Copy link

ajluker commented Dec 12, 2023

What's not working?

It doesn't look like the fastify server is returning the index when a query parameter contains a period.

For example, appending this to any routed URL works:
/your-route?this_Should_be_ok

But this doesnt:
/your-route?this_Shoul.d_be_ok

Instead of being served the index html file, I receive a 404

How do we reproduce the bug?

Tack ?this_Shoul.d_be_ok onto any path

What's your environment? (If it applies)

Binaries:
    Node: 18.18.0 - /tmp/xfs-1fd38dbd/node
    Yarn: 3.2.3 - /tmp/xfs-1fd38dbd/yarn
  npmPackages:
    @redwoodjs/auth-custom-setup: 6.4.2 => 6.4.2 
    @redwoodjs/cli-storybook: 6.4.2 => 6.4.2 
    @redwoodjs/core: 6.4.2 => 6.4.2

Are you interested in working on this?

  • I'm interested in working on this
@ajluker ajluker added the bug/needs-info More information is needed for reproduction label Dec 12, 2023
@dthyresson
Copy link
Contributor

dthyresson commented Dec 13, 2023

Hi @ajluker and thanks for the issue write up.

Could you help me understand the issue more by sharing an example repo that reproduces this (so we can quickly run and see in in action)?

  • Are you seeing this in dev or in prod? If in prod, when are you deployed?
  • Are you seeing this via "yarn rw serve" ?

If a repo isn't available, could you provide some steps to recreate from scratch, for example:

  • create rw app
  • configure fastify as "such"
  • yarn rw g page home /home
  • yarn rw dev or yarn rw serve
  • go to /home, ok
  • go to /home?this-ok
  • go to /home?this/not.ok and 404s

So we can be sure we are seeing the asme behavior you are

@dthyresson
Copy link
Contributor

I was able to reproduce using those basic steps above (with no septic fastify config, just oui of of) with yarn rw serve.

However, url encoding the querystring params like http://localhost:8910/home?this_ok%2Enot was ok. If just a plain period, then a not found error occurs,

Need to determine if Fastify expect urls to be encoded or not -- or if there is a setting to configure.

@dthyresson
Copy link
Contributor

@jtoar or @Josh-Walker-GM do you think that the not found handler is invoked because it thinks such a url with a period is an asset/extension (like a .png, .jpg etc)? And thus the not found?

See:

const requestedExtension = path.extname(req.url)

@dthyresson
Copy link
Contributor

According to path docs:

The path.extname() method returns the extension of the path, from the last occurrence of the . (period) character to end of string in the last portion of the path. If there is no . in the last portion of the path, or if there are no . characters other than the first character of the basename of path (see path.basename()) , an empty string is returned.

See: https://nodejs.org/api/path.html#pathextnamepath

@ajluker Is there a specific reason you need have a period in the url that isn't URL encoded? Seems to think that when a period is in the url, it's an extension?

@dthyresson
Copy link
Contributor

Also @jtoar seems this new behavior was introduced in

https://github.com/redwoodjs/redwood/pull/9272/files

@dthyresson dthyresson added bug/confirmed We have confirmed this is a bug topic/web and removed bug/needs-info More information is needed for reproduction labels Dec 13, 2023
@ajluker
Copy link
Author

ajluker commented Dec 13, 2023

I can work around it by encoding for sure, but I shouldn't really have to, it's part of the spec to allow unencoded period characters in urlencoded search params:

https://url.spec.whatwg.org/#application-x-www-form-urlencoded-percent-encode-set

@dthyresson
Copy link
Contributor

I can work around it by encoding for sure, but I shouldn't really have to, it's part of the spec to allow unencoded period characters in urlencoded search params:

https://url.spec.whatwg.org/#application-x-www-form-urlencoded-percent-encode-set

@ajluker Agreed.

I just found the PR that changed the behavior and assigned to @jtoar to explore. Please see: https://github.com/redwoodjs/redwood/pull/9272/files

It was put into to help with serving static assets, but had some downstream effect.

Thanks for reporting and now we have a way to reproduce and found the likely cause.

@ajluker
Copy link
Author

ajluker commented Dec 13, 2023

Thanks for being responsive!

@jtoar
Copy link
Contributor

jtoar commented Dec 19, 2023

Just adding to the reproduction steps here that this only works if the page isn't prerendered

jtoar added a commit that referenced this issue Dec 19, 2023
## What does this PR do?

Fix for this issue.
#9663

If the query parameter contains`. `will result in a 404 error when
routing to the defined route.

Example.)
http://localhost:8910/posts?redirect_url=https%3A%2F%2Fwww.google.com

## The problem occurs

- Version: 6.3.3 or higher
- When the application is running with `yarn rw serve` command.

## The problem not occurs

- Version: 6.3.2 or lower
- When the application is running with `yarn rw dev` command.

---------

Co-authored-by: yuichi.nishitani <hawkk9.public@gmail.com>
Co-authored-by: Dominic Saadi <dominiceliassaadi@gmail.com>
Tobbe pushed a commit that referenced this issue Dec 21, 2023
Fix for this issue.
#9663

If the query parameter contains`. `will result in a 404 error when
routing to the defined route.

Example.)
http://localhost:8910/posts?redirect_url=https%3A%2F%2Fwww.google.com

- Version: 6.3.3 or higher
- When the application is running with `yarn rw serve` command.

- Version: 6.3.2 or lower
- When the application is running with `yarn rw dev` command.

---------

Co-authored-by: yuichi.nishitani <hawkk9.public@gmail.com>
Co-authored-by: Dominic Saadi <dominiceliassaadi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/confirmed We have confirmed this is a bug topic/web
Projects
None yet
Development

No branches or pull requests

3 participants