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.

Sign up for GitHub

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

Jump to bottom

fix parsing of port #96

Closed
wants to merge 1 commit into from
Closed

fix parsing of port #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,11 @@ function parse (uri, opts) {

// fix port number
if (isNaN(parsed.port)) {
parsed.port = matches[5]
if (matches[5] === undefined) {
parsed.port = parsed.scheme === 'http' ? 80 : 443
Copy link
Member

Choose a reason for hiding this comment

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

This assumes that we only parse http/https uri schemes. We support more than this.

Suggested change
parsed.port = parsed.scheme === 'http' ? 80 : 443
switch (parsed.scheme) {
case 'http':
parsed.port = 80
case 'https':
parsed.port = 443
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not Go. I'd have to add in break statements. In my opinion, this is the exact scenario for a ternary.

Copy link
Member

@zekth zekth Jul 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It was an example, sorry i wasn't explicit. Ternary assumes that we resolves only http or https; which is not true, we support any scheme; like ftp sftp etc.

At the same time i don't feel this is a good thing because we're assuming the port based on the scheme; but the uri doesn't provide any.

Copy link
Member Author

Choose a reason for hiding this comment

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

If this library is going to assert that it fully parses URIs, it must parse the port portion:

https://datatracker.ietf.org/doc/html/rfc3986#section-3.2

authority = [ userinfo "@" ] host [ ":" port ]

URI producers and normalizers should omit the ":" delimiter that
separates host from port if the port component is empty. Some
schemes do not allow the userinfo and/or port subcomponents.

In other words, http://example.com/ translates to:

  • http
  • example.com
  • 80

Copy link
Member

Choose a reason for hiding this comment

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

That's not true. In http://example.com there's no port from the uri point of view. Automatically translating to 80 for http is an extension of the interpretation of the rfc, not what the rfc dictactes.

} else {
parsed.port = matches[5]
}
}
if (parsed.host) {
const ipv4result = normalizeIPv4(parsed.host)
Expand Down
6 changes: 6 additions & 0 deletions test/parse.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,5 +314,11 @@ test('URI parse', (t) => {

components = URI.parse('urn:foo:|\\24fpl')
t.equal(components.error, 'URN can not be parsed.')

components = URI.parse('http://example.com/')
t.equal(components.port, 80)
components = URI.parse('https://example.com/')
t.equal(components.port, 443)

t.end()
})
Loading