Open kegsay opened 7 years ago
Looks like the default mux will 301 all the paths with encoded slashes in them.
https://github.com/golang/go/blob/master/src/net/http/server.go#L2187
I'd suggest not using "/" in state keys.
I'd suggest not using "/" in state keys.
That isn't a viable solution. The IRC bridge uses the state key as the irc://
URI to prevent bridging to the same place twice. Given that Synapse actually works fine with encoded slashes, it's bizarre that the bridging protocol has to change because of Dendron.
The problem with https://github.com/golang/go/blob/master/src/net/http/server.go#L2187 is not the fact that it is sanitizing - it's the fact it is sanitizing with .Path
and not .RawPath
:
func main() {
u, _ := url.Parse("https://google.com/path/with%2Fencoded%2Fslashes")
fmt.Println("Path: ", u.Path, " RawPath: ", u.RawPath)
}
Path: /path/with/encoded/slashes RawPath: /path/with%2Fencoded%2Fslashes
golang/go#14815 has a good write up. In my opinion this is something that enough languages get wrong that we should work around it in the spec. Otherwise we are just making unnecessary work for ourselves.
That's a pretty good write-up and does also include a workaround:
The workaround is to pass your own handler to Serve or ListenAndServe and have that handler pick off the %2F paths you care about before handing the rest to ServeMux. One reason the root handler is just an http.Handler instead of a ServeMux is so that you can do this when it is necessary. (Sadly, this unexpected usage of REST has made it more necessary than we anticipated.)
As for:
In my opinion this is something that enough languages get wrong that we should work around it in the spec. Otherwise we are just making unnecessary work for ourselves.
I agree, but not by disallowing /
in state keys. I'd rather see a more RMI-like API rather than REST API, which means the state_key
could fall into the HTTP body and this problem goes away. By having another API which then defers to the same code path, we preserve backwards-compatibility for clients/bots/bridges which may already be relying on /
in state keys.
As for this particular issue with Dendron, like it or not, we need to fix it. You must be well aware that this does not just affect state_keys
: it affects every path with user input, so:
/_matrix/client/r0/rooms/{roomId}/state/{eventType}/{stateKey}
/_matrix/client/r0/rooms/{roomId}/send/{eventType}/{txnId} (if txnId has '//')
/_matrix/client/r0/directory/room/{roomAlias} (if alias has '//')
/_matrix/client/r0/rooms/{roomId}/receipt/{receiptType}/{eventId} (if receipt type has '//')
/_matrix/client/r0/user/{userId}/rooms/{roomId}/tags/{tag} (if tag has '//')
On a development synapse:
Same request on matrix.org:
Note the state_key in the
Location
isirc:/
and notirc://
. Knowing Go, this is probably aPath
vsRawPath
bug.