-
Notifications
You must be signed in to change notification settings - Fork 25
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.
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
feat: support query strings #280
feat: support query strings #280
Conversation
Adds support for creating WebTransport session with URLs that include query strings: ```js const wt = new WebTransport('https://example.com/some-endpoint?foo=bar') ``` ...and also accessing the path via the `.url` property which is consistent with Node http/http2 requests ```js const sessionReader = server.sessionStream('/some-endpoint').getReader() const { value: session } = await sessionReader.read() console.info(session.url) // /some-endpoint?foo=bar ``` Fixes fails-components#279
@@ -268,15 +268,19 @@ export class HttpServer { | |||
* @param {HttpWTServerSessionVisitorEvent} args | |||
*/ | |||
onHttpWTSessionVisitor(args) { | |||
// match session streams on path without query string | |||
const path = args.path.split('?')[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is not the header path field, we should not temper here. So please remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The args
passed here during the new should connect to a path with a query in the URL
test are:
{
session: Http3WTSessionVisitor {},
path: '/session_with_query?foo=bar'
}
If we don't remove the query string from the path, the session is never passed to the session stream controller for the endpoint and the test fails.
No, I do not think I will include the change. It is too specific for your application case. Please see the callback mechanism, which is very flexible for many use cases. And I will be happy to answer questions if the mechanism is unclear. (Especially as I assume, that you want to deny requests early, that are not authorized.) |
That's a pity, all it's doing is enabling sending/reading query strings and paths, like any web server. I do not feel this is application-specific.
No, this is not what I'm trying to do. In the scenario I'm trying to implement, a client and a server both have a public/private keypair (unrelated to the server's SSL certificate, which is self signed and ephemeral). They both desire encryption and authentication (e.g. for each to prove to the other that they possess the private key that corresponds to the public key). There are multiple ways to prove possession of the private key, one way is a noise handshake, in the future there may be others, so we need to be able to specify which authentication scheme we want to use.
const wt = new WebTransport('https://example.com/.well-known/libp2p-webtransport?type=noise', {
serverCertificateHashes: [ ... ]
})
I don't think the callback mechanism helps here. All I need from this module is the ability to send a query string from the client (as Chrome and Firefox both do) and to read that query string on the server which is all that's in this PR. |
But you can do it already with the other mechanism.., which is transport independent, as a middleware in express would handle it. I will post to the issue an untested example. |
Adds support for creating WebTransport session with URLs that include query strings:
...and also accessing the path via the
.url
property which is consistent with Node http/http2 requestsFixes #279