derhuerst / gemini

Gemini protocol server & client for Node.js.
ISC License
49 stars 8 forks source link

Fix up protocol issues #4

Closed RangerMauve closed 4 years ago

RangerMauve commented 4 years ago

With these changes, it all seems to work. In my tests I set verifyAlpnID to () => true to permit all ALPNs

derhuerst commented 4 years ago
  • Got rid of URI encoding for URL, that doesn't seem to be in the spec

So the assumption is that, because the URL is delimited by (and only by) \r\n, it can be unencoded?

  • tls.connect(options) takes the hostname under the host property. Thus connections were always going to localhost.

Thanks!

In my tests I set verifyAlpnID to () => true to permit all ALPNs

I'd like to keep the ALPN ID check if the server sent one, so should we change the default to alpnId => alpnId ? alpnId === ALPN_ID : true?

RangerMauve commented 4 years ago

Regarding the URL, it seems the specification doesn't say anything about encoding the URL other than it being utf-8 So I think it should be safe to have it as is. Also important to note was that you had an extra space which was leading to not found errors since it changed the value of the path.

Is the ALPN stuff really necessary? It doesn't seem like any of the gemini servers I connected to implemented it, and it doesn't seem to be anywhere in the spec.

derhuerst commented 4 years ago

Is the ALPN stuff really necessary? It doesn't seem like any of the gemini servers I connected to implemented it, and it doesn't seem to be anywhere in the spec.

Back when I read the spec & docs, Iā€˜m pretty sure they mentioned ALPN. Besides, I think ALPN nicely fits the TLS-everything & the Gemini-next-to-HTTPS-on-one-machine stories. What do you think?

RangerMauve commented 4 years ago

Hmm, I personally think ALPN is a great idea, but when you add the check in there, it fails to connect to gemini://gemini.circumlunar.space which I think is the most important gemini site to be able to connect to at the moment. šŸ˜…

Also, doing a search in the current spec for ALPN or negotiation doesn't yield any results: https://gemini.circumlunar.space/docs/specification.html

derhuerst commented 4 years ago

Hmm, I personally think ALPN is a great idea, but when you add the check in there, it fails to connect to gemini://gemini.circumlunar.space which I think is the most important gemini site to be able to connect to at the moment. šŸ˜…

Yeah, that should definitely work! But my proposal alpnId => alpnId ? alpnId === ALPN_ID : true would work then, right?

RangerMauve commented 4 years ago

Oh yeah! That looks great, I think I misunderstood it the first time. šŸ˜ I'll adjust the PR to account for it. šŸ’œ

RangerMauve commented 4 years ago

Change the ALPN logic based on your suggestions. Works perfectly with gemini://gemini.circumlunar.space by default now

derhuerst commented 4 years ago

Do you want to be added the package.json as a contributor? If yes, with which signature? RangerMauve <RangerMauve@hotmail.com>?

RangerMauve commented 4 years ago

Yeah, that would be good and that signature works. šŸ˜