fix(nano): refuse hasJoined requests that declare a body
A GET to hasJoined with a Content-Length and no body held its connection indefinitely. The handler returned, but net/http tries to drain an unread body before it writes the answer, and nothing bounds that wait: ReadHeaderTimeout ends with the headers. One such request per socket pins a goroutine and a descriptor on nano or on felis-api's internal face. Velocity never sends a body, so any request that declares one, including a chunked one, now gets a 400 with Connection: close, which skips the drain and releases the connection once the answer is written. The new subtest writes that request over a raw socket and waits three seconds for an answer. Before the change it times out with no response at all; now it reads a 400 marked close.
This commit is contained in:
2 files changed
+36
No files matched your search
@@ -106,6 +106,15 @@ func HasJoinedHandler(sources []AuthSource, repo Repo) http.Handler {
|
|||||||
// service token. A rejected login is 204 No Content — exactly what Mojang returns for an
|
// service token. A rejected login is 204 No Content — exactly what Mojang returns for an
|
||||||
// invalid session, which authlib maps to "failed to verify username".
|
// invalid session, which authlib maps to "failed to verify username".
|
||||||
func (a *API) handleHasJoined(w http.ResponseWriter, r *http.Request) {
|
func (a *API) handleHasJoined(w http.ResponseWriter, r *http.Request) {
|
||||||
|
// Velocity sends no body. When a request declares one anyway, net/http tries to drain it
|
||||||
|
// before writing any answer, so one that never arrives holds the connection with no
|
||||||
|
// timeout: ReadHeaderTimeout stopped at the headers. Marking the reply as the last one on
|
||||||
|
// this connection skips the drain.
|
||||||
|
if r.ContentLength != 0 {
|
||||||
|
w.Header().Set("Connection", "close")
|
||||||
|
w.WriteHeader(http.StatusBadRequest)
|
||||||
|
return
|
||||||
|
}
|
||||||
q := r.URL.Query()
|
q := r.URL.Query()
|
||||||
username, serverID := q.Get("username"), q.Get("serverId")
|
username, serverID := q.Get("username"), q.Get("serverId")
|
||||||
if username == "" || serverID == "" {
|
if username == "" || serverID == "" {
|
||||||
|
|||||||
@@ -1,14 +1,18 @@
|
|||||||
package api
|
package api
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"bufio"
|
||||||
"encoding/hex"
|
"encoding/hex"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
|
"io"
|
||||||
|
"net"
|
||||||
"net/http"
|
"net/http"
|
||||||
"net/http/httptest"
|
"net/http/httptest"
|
||||||
"path"
|
"path"
|
||||||
"strings"
|
"strings"
|
||||||
"sync/atomic"
|
"sync/atomic"
|
||||||
"testing"
|
"testing"
|
||||||
|
"time"
|
||||||
|
|
||||||
"github.com/google/uuid"
|
"github.com/google/uuid"
|
||||||
)
|
)
|
||||||
@@ -311,6 +315,29 @@ func TestHasJoined(t *testing.T) {
|
|||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
|
// A GET that declares a body it never sends must still be answered and lose its
|
||||||
|
// connection; otherwise each such socket stays open for as long as the client likes.
|
||||||
|
t.Run("request declaring a body is refused and closed", func(t *testing.T) {
|
||||||
|
api := newTestAPI(newFakeRepo(), newFakeCluster())
|
||||||
|
srv := httptest.NewServer(api.InternalHandler())
|
||||||
|
t.Cleanup(srv.Close)
|
||||||
|
conn, err := net.Dial("tcp", srv.Listener.Addr().String())
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
defer conn.Close()
|
||||||
|
_, _ = io.WriteString(conn, "GET /session/minecraft/hasJoined?username=a&serverId=b HTTP/1.1\r\nHost: x\r\nContent-Length: 1000\r\n\r\n")
|
||||||
|
_ = conn.SetReadDeadline(time.Now().Add(3 * time.Second))
|
||||||
|
resp, err := http.ReadResponse(bufio.NewReader(conn), nil)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("no answer while the declared body never arrives: %v", err)
|
||||||
|
}
|
||||||
|
resp.Body.Close()
|
||||||
|
if resp.StatusCode != http.StatusBadRequest || !resp.Close {
|
||||||
|
t.Fatalf("code = %d close = %v, want 400 with Connection: close", resp.StatusCode, resp.Close)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
// Missing query fields → 204 without touching any source.
|
// Missing query fields → 204 without touching any source.
|
||||||
t.Run("missing username -> 204", func(t *testing.T) {
|
t.Run("missing username -> 204", func(t *testing.T) {
|
||||||
api := newTestAPI(newFakeRepo(), newFakeCluster())
|
api := newTestAPI(newFakeRepo(), newFakeCluster())
|
||||||
|
|||||||
Reference in new issue
Block a user