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

fix(fastify): Server not handling periods in query params #9714

Conversation

hawkk9
Copy link
Contributor

@hawkk9 hawkk9 commented Dec 15, 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.

@hawkk9 hawkk9 marked this pull request as ready for review December 15, 2023 09:07
@Tobbe
Copy link
Member

Tobbe commented Dec 16, 2023

Thanks a lot for your PR @hawkk9 🙂

Could you please take a look at the automatic review comments you got and try to fix that warning? Thanks!

@hawkk9
Copy link
Contributor Author

hawkk9 commented Dec 16, 2023

@Tobbe
Thanks for the comment🙂
Lint warning has been resolved.

@Tobbe
Copy link
Member

Tobbe commented Dec 17, 2023

Thanks @hawkk9

I should have asked this too in my original comment, but would it be possible to add some tests for this? I'm not super familiar with this code. Some tests would make me feel more comfortable merging this.
Sorry for the extra back-and-forth

@jtoar jtoar added the release:fix This PR is a fix label Dec 18, 2023
@jtoar jtoar added this to the next-release-patch milestone Dec 18, 2023
Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @hawkk9! I took the liberty of getting this one over the finish line since I was familiar with the tests for these packages and knew that the code here was duplicated in a few places.

@jtoar jtoar enabled auto-merge (squash) December 19, 2023 00:32
@jtoar jtoar merged commit beb7a0a into redwoodjs:main Dec 19, 2023
32 checks passed
@hawkk9
Copy link
Contributor Author

hawkk9 commented Dec 19, 2023

@Tobbe @jtoar
Sorry for the delay in responding. Thanks for your comments and support!

Tobbe pushed a commit that referenced this pull request 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
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants