PUTing state events 301s

[kegsay]

On a development synapse:

curl -XPUT -d '{}' "http://localhost:8008/_matrix/client/r0/rooms/%21cBrPbzWazCtlkMNQSF:localhost/state/m.room.bridging/irc%3A%2F%2Firc.test.com%2F%23tsomething?access_token=MDA....H1Cg"

HTTP/1.1 200 OK
Content-Length: 53
Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Type, Accept
Server: Synapse/0.18.3 (b=develop,9355a5c)
Date: Mon, 21 Nov 2016 11:43:41 GMT
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET, POST, PUT, DELETE, OPTIONS
Content-Type: application/json
{
    "event_id": "$14797283221088yNiKx:localhost"
}

Same request on matrix.org:

curl -XPUT -D - -d '{}' "https://matrix.org/_matrix/client/r0/rooms/%21wAtthICZcUpPDfXPJp:matrix.org/state/m.room.bridging/irc%3A%2F%2Firc.test.com%2F%23tsomething?access_token=MDA...x"

HTTP/1.1 301 Moved Permanently
Location: /_matrix/client/r0/rooms/%21wAtthICZcUpPDfXPJp:matrix.org/state/m.room.bridging/irc:/irc.test.com/%23tsomething?access_token=MDA...x
Date: Mon, 21 Nov 2016 11:42:58 GMT
Content-Length: 0
Content-Type: text/plain; charset=utf-8

Note the state_key in the Location is irc:/ and not irc://. Knowing Go, this is probably a Path vs RawPath bug.

[NegativeMjark]

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

[NegativeMjark]

I'd suggest not using "/" in state keys.

[kegsay]

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.

[kegsay]

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

[NegativeMjark]

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.

[kegsay]

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.

[kegsay]

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 '//')