Skip to content

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

Merged
jtoar merged 5 commits intoredwoodjs:mainfrom
hawkk9:fix/fastify-server-not-handling-periods-in-query-params
Dec 19, 2023
Merged

fix(fastify): Server not handling periods in query params#9714
jtoar merged 5 commits intoredwoodjs:mainfrom
hawkk9:fix/fastify-server-not-handling-periods-in-query-params

Conversation

@hawkk9
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

hawkk9 commented Dec 16, 2023

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

@Tobbe
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
@hawkk9
Copy link
Copy Markdown
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 <[email protected]>
Co-authored-by: Dominic Saadi <[email protected]>
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.

3 participants