Fix tsl typo and comment about address validation#238
Conversation
I actually don't know what the right answer is here but I wanted to add some discussion and remove the suggestion of delegating to KJ's `parseAddress()` which doesn't seem like the right answer.
| struct SocketOptions { | ||
| bool tsl; // TODO(later): TCP socket options need to be implemented. | ||
| JSG_STRUCT(tsl); | ||
| jsg::Unimplemented tls; // TODO(later): TCP socket options need to be implemented. |
There was a problem hiding this comment.
Sorry it took me so long to get to this, but I did actually make the same change in https://github.com/cloudflare/workerd/pull/237/files#diff-60b97f1a91fb6495e0c3c870b0f479e52fd448c80b9c445a06795d006f369ca8R14
| // Note that we intentionally leave it up to the connect() implementation to decide what is a | ||
| // valid address. This means that people using `workerd`, for example, can arrange to connect | ||
| // to Unix sockets (if they define a "Network" service that permits local connections, which | ||
| // the default internet service will not). Also, hypothetically, in W2W communications, the | ||
| // address could be an arbitrary string which the receiving Worker can validate however it wants. | ||
| // | ||
| // TODO(soon): This results in an "internal error" in the case that the address couldn't parse, | ||
| // which is not a great experience. Should we attempt to validate the address here to give a | ||
| // better error? But that takes away the backend's flexibility to define its own address | ||
| // format. Maybe that's good though? It's more consistent with fetch(), which requires a | ||
| // valid URL even for W2W. The only other way we can get good errors here is if we use string | ||
| // matching to detect KJ's invalid-address error message, which seems pretty gross but could | ||
| // work. Note that if we do decide to validate addresses, we should not try to use KJ's | ||
| // `parseAddress()` but instead decide for ourselves what format we want to permit here, maybe | ||
| // as a regex. |
There was a problem hiding this comment.
Hm, I didn't consider leaving this to the connect implementation. But that sounds fair enough.
When I wrote this comment my primary concern was something like: connect("blah.org HTTP/1.1\r\nARBITRARY_HTTP_HEADERS_HERE"). Without validation this would mean the Worker can send arbitrary http headers to the proxy. But I suppose we should ensure this cannot happen in HttpClient (maybe HttpClient already does this?).
There was a problem hiding this comment.
Yeah hopefully HttpClient already prevents this. If it doesn't that is a bug we need to fix.
2 participants
This addresses my unresolved comments from #162.