Stream: git-wasmtime

Topic: wasmtime / PR #11145 Remove the check for ':' when settin...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 09:40):

rylev opened PR #11145 from rylev:remove-authority-check to bytecodealliance:main:

Authorities can have : in them when they are IPV6 addresses. With the current check it is impossible to use ipv6 addresses as authorities for outgoing http requests.

It's not clear to me why the check was added in the first place, so I might be missing something obvious here why the check is necessary.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 09:40):

rylev requested wasmtime-wasi-reviewers for a review on PR #11145.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 10:31):

rylev requested alexcrichton for a review on PR #11145.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 10:31):

rylev requested wasmtime-core-reviewers for a review on PR #11145.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 10:31):

rylev updated PR #11145.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 13:53):

dicej commented on PR #11145:

It looks like the check for : was an attempt to validate this:

The HTTP and HTTPS schemes always require an authority. Fails if the string given is not a syntactically valid URI authority.

Presumably, that means it should _not_ contain a port number. So if we update the check to ensure it doesn't end with a colon followed by zero or more digits (e.g. the regex :[0-9]*), that should allow IPv6 addresses but still disallow port numbers.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 14:02):

badeend commented on PR #11145:

From the PR that introduced this check, I can't reverse engineer why this check was added.

The URI authority _can_ include the port. As matter of fact, it is the _only_ way in the wasi-http interface the declare the port number.

Taking the code at face value, I _think_ what it tried to check for is that: when a port is provided it shall be a valid u16.
A better way to write this could be:

if auth.port().is_some() && auth.port_u16().is_none() {
   return Ok(Err(()));
}

I agree with the general sentiment of this PR: http::uri::Authority::from_str alone is likely better equipped to validate URI's than ad-hoc validations in wasmtime itself.

Could you add a test that checks a IPv6 address +/- a port number passes the validation? E.g. "[::]:443"

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 14:03):

badeend edited a comment on PR #11145:

From the PR that introduced this check, I can't reverse engineer why this check was added.

The URI authority _can_ include the port. As matter of fact, it is the _only_ way in the wasi-http interface to declare the port number.

Taking the code at face value, I _think_ what it tried to check for is that: when a port is provided it shall be a valid u16.
A better way to write this could be:

if auth.port().is_some() && auth.port_u16().is_none() {
   return Ok(Err(()));
}

Maybe there's still value in keeping such a validation around.

Other than that: I agree with the general sentiment of this PR: http::uri::Authority::from_str alone is likely better equipped to validate URI's than ad-hoc validations in wasmtime itself.

Could you add a test that checks a IPv6 address +/- a port number passes the validation? E.g. "[::]:443"

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 14:04):

dicej edited a comment on PR #11145:

It looks like the check for : was an attempt to validate this:

The HTTP and HTTPS schemes always require an authority. Fails if the string given is not a syntactically valid URI authority.

Presumably, that means it should _not_ contain a port number. So if we update the check to ensure it doesn't end with a colon followed by zero or more digits (e.g. the regex :[0-9]*), that should allow IPv6 addresses but still disallow port numbers. EDIT: Disregard what I wrote. Reading the code more carefully, I see the code was not trying to disallow specifying the host. I don't know what it was trying to do :shrug:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 14:05):

dicej edited a comment on PR #11145:

It looks like the check for : was an attempt to validate this:

The HTTP and HTTPS schemes always require an authority. Fails if the string given is not a syntactically valid URI authority.

Presumably, that means it should _not_ contain a port number. So if we update the check to ensure it doesn't end with a colon followed by zero or more digits (e.g. the regex :[0-9]*), that should allow IPv6 addresses but still disallow port numbers. EDIT: Disregard what I wrote. Reading the code more carefully, I see the code was not trying to disallow specifying the port. I don't know what it was trying to do :shrug:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 14:05):

badeend edited a comment on PR #11145:

From the PR that introduced this check, I can't reverse engineer why this check was added.

The URI authority _can_ include the port. As matter of fact, it is the _only_ way in the wasi-http interface to declare the port number.

Taking the code at face value, I _think_ what it tried to check for is that: when a port is provided it shall be a valid u16. For example: http://bad-port:99999 is syntactically a valid URI authority, but 99999 is not a valid port number.
A better way to write this could be:

if auth.port().is_some() && auth.port_u16().is_none() {
   return Ok(Err(()));
}

Maybe there's still value in keeping such a validation around.

Other than that: I agree with the general sentiment of this PR: http::uri::Authority::from_str alone is likely better equipped to validate URI's than ad-hoc validations in wasmtime itself.

Could you add a test that checks a IPv6 address +/- a port number passes the validation? E.g. "[::]:443"

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 14:08):

dicej edited a comment on PR #11145:

It looks like the check for : was an attempt to validate this:

The HTTP and HTTPS schemes always require an authority. Fails if the string given is not a syntactically valid URI authority.

Presumably, that means it should _not_ contain a port number. So if we update the check to ensure it doesn't end with a colon followed by zero or more digits (e.g. the regex :[0-9]*), that should allow IPv6 addresses but still disallow port numbers. EDIT: Disregard what I wrote. Reading the code more carefully, I see it was not trying to disallow specifying the port. I don't know what it was trying to do :shrug:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 14:16):

rylev updated PR #11145.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 14:16):

rylev commented on PR #11145:

FYI: the check suggested of...

if auth.port().is_some() && auth.port_u16().is_none() {
   return Ok(Err(()));
}

... won't work as auth.port() already parses the port as a u16. So if auth.port().is_some() is true - auth.port_u16() will also be Some.

I have pushed a test for ipv6 with and without a port.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 14:23):

badeend commented on PR #11145:

Hmm. I see. In that case, I don't know why that check was there.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 14:24):

badeend submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2025 at 14:54):

alexcrichton merged PR #11145.

Last updated: Dec 06 2025 at 06:05 UTC