Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix: make port optional when parsing URL #154

Merged
merged 1 commit into from
Sep 24, 2024
Merged

Conversation

johnyb
Copy link
Contributor

@johnyb johnyb commented Sep 23, 2024

URL parsing is broken when port is not specified, as the URL is basically invalid in this case.

Fixes #153

Checklist

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

Would you mind to add a test to cover this use case?

@johnyb
Copy link
Contributor Author

johnyb commented Sep 23, 2024

Currently trying to figure out a way to reproduce this in a test. At least what doesn't work in my case works when trying in this test env. I tried this:

test('should parse path without a port specified', async (t) => {
  t.plan(2)
  const fastify = Fastify()
  fastify
    .register(plugin)

  fastify.get('/', (req, reply) => {
    const path = req.urlData('path')
    reply.send('That worked, path is ' + path)
  })

  const res = await fastify.inject({ url: '/' })
  t.equal(res.statusCode, 200)
  t.equal(res.body, 'That worked, path is /')
})

URL parsing is broken when port is not specified, as the URL is basically invalid in this case.

Fixes fastify#153 

Signed-off-by: Julian <julian@svg4all.de>
@johnyb
Copy link
Contributor Author

johnyb commented Sep 23, 2024

Would you mind to add a test to cover this use case?

Done :D this basically also adds a workaround for my usecase: specify a port in the host header of the inject call. Took me this test to find out ;)

@johnyb johnyb requested a review from Eomm September 23, 2024 15:30
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@dancastillo dancastillo left a comment

Choose a reason for hiding this comment

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

LGTM

@Eomm Eomm merged commit 5cfef3f into fastify:master Sep 24, 2024
11 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

urlData().path starts with "null/..." on Vercel or Railway with Fastify 5
4 participants