Просмотр исходного кода

Refuse a request field the control socket does not define

An unknown parameter was refused and an unknown request field was
passed over, so a client that misspelled `command` got an empty request
and a baffling answer. The daemon now reads a request strictly and names
the field, as it names a parameter. The client reads a response
leniently, so a field a newer daemon adds is not fatal to an older
client.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Matt Richardson 1 день назад
Родитель
Сommit
3e4f7c5e61
5 измененных файлов с 47 добавлено и 10 удалено
  1. 1 1
      localapi/client.go
  2. 10 2
      localapi/protocol.go
  3. 19 6
      localapi/protocol_test.go
  4. 1 1
      localapi/server.go
  5. 16 0
      localapi/server_test.go

+ 1 - 1
localapi/client.go

@@ -153,7 +153,7 @@ func (c *Client) do(req *Request) (*Response, error) {
 
 func readResponse(conn net.Conn) (*Response, error) {
 	res := &Response{}
-	if err := readFrame(conn, MaxResponseBytes, "response", res); err != nil {
+	if err := readFrame(conn, MaxResponseBytes, "response", res, false); err != nil {
 		if errors.Is(err, io.EOF) {
 			return nil, errors.New("the daemon closed the connection without answering")
 		}

+ 10 - 2
localapi/protocol.go

@@ -105,7 +105,11 @@ func (e *frameError) Unwrap() error { return e.err }
 // single line, a line over max bytes is refused rather than buffered, and a final line
 // with no newline before EOF still counts. what names the frame in errors ("request", "response"); a
 // read failure is returned wrapped, so a caller can still see io.EOF or a timeout in it.
-func readFrame(r io.Reader, max int, what string, v any) error {
+// strict refuses a field v does not declare, which is the daemon's reading of a request:
+// a client that misspells one deserves to hear about it, as it does for a parameter.
+// The client reads a response leniently, so a field a newer daemon adds is passed over
+// rather than fatal.
+func readFrame(r io.Reader, max int, what string, v any, strict bool) error {
 	br := bufio.NewReader(io.LimitReader(r, int64(max)+1))
 
 	line, err := br.ReadBytes('\n')
@@ -126,7 +130,11 @@ func readFrame(r io.Reader, max int, what string, v any) error {
 		return &frameError{fmt.Errorf("%s exceeded %d bytes", what, max)}
 	}
 
-	if err := json.Unmarshal(body, v); err != nil {
+	dec := json.NewDecoder(bytes.NewReader(body))
+	if strict {
+		dec.DisallowUnknownFields()
+	}
+	if err := dec.Decode(v); err != nil {
 		return &frameError{fmt.Errorf("could not parse %s: %w", what, err)}
 	}
 

+ 19 - 6
localapi/protocol_test.go

@@ -24,16 +24,16 @@ func TestReadFrameLimitIsOnTheBody(t *testing.T) {
 
 	// Exactly at the limit, terminated: accepted.
 	v = ""
-	require.NoError(t, readFrame(bytes.NewReader([]byte(body(64)+"\n")), 64, "request", &v))
+	require.NoError(t, readFrame(bytes.NewReader([]byte(body(64)+"\n")), 64, "request", &v, true))
 	assert.Len(t, v, 62)
 
 	// Exactly at the limit, EOF instead of a newline: still accepted.
 	v = ""
-	require.NoError(t, readFrame(bytes.NewReader([]byte(body(64))), 64, "request", &v))
+	require.NoError(t, readFrame(bytes.NewReader([]byte(body(64))), 64, "request", &v, true))
 	assert.Len(t, v, 62)
 
 	// One byte over: refused as oversized, not as a parse failure.
-	err := readFrame(bytes.NewReader([]byte(body(65)+"\n")), 64, "request", &v)
+	err := readFrame(bytes.NewReader([]byte(body(65)+"\n")), 64, "request", &v, true)
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "exceeded 64 bytes")
 }
@@ -68,7 +68,7 @@ func (timeoutError) Temporary() bool { return true }
 func TestReadFramePartialLineThenReadErrorIsTheError(t *testing.T) {
 	var v map[string]any
 
-	err := readFrame(&stallingReader{data: []byte(`{"command": "hel`), err: timeoutError{}}, 1024, "request", &v)
+	err := readFrame(&stallingReader{data: []byte(`{"command": "hel`), err: timeoutError{}}, 1024, "request", &v, true)
 	require.Error(t, err)
 	var ne net.Error
 	require.ErrorAs(t, err, &ne, "the timeout is still visible through the wrapping: %v", err)
@@ -78,12 +78,25 @@ func TestReadFramePartialLineThenReadErrorIsTheError(t *testing.T) {
 	// EOF after an unterminated line is the one exception: a final line with no newline
 	// is still a frame.
 	v = nil
-	require.NoError(t, readFrame(&stallingReader{data: []byte(`{"command": "help"}`), err: io.EOF}, 1024, "request", &v))
+	require.NoError(t, readFrame(&stallingReader{data: []byte(`{"command": "help"}`), err: io.EOF}, 1024, "request", &v, true))
 	assert.Equal(t, "help", v["command"])
 
 	// And an unterminated line that is EOF only because the limit cut it off is still
 	// reported as oversized.
-	err = readFrame(bytes.NewReader([]byte(strings.Repeat("a", 70)+"\n")), 64, "request", &v)
+	err = readFrame(bytes.NewReader([]byte(strings.Repeat("a", 70)+"\n")), 64, "request", &v, true)
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "exceeded 64 bytes")
 }
+
+// TestReadFrameUnknownFieldIsRefusedWhenStrict: the daemon refuses a request field it
+// does not know, as it refuses an unknown parameter, so a misspelling is reported rather
+// than ignored; the client, reading leniently, passes over a field a newer daemon sends.
+func TestReadFrameUnknownFieldIsRefusedWhenStrict(t *testing.T) {
+	var req Request
+	err := readFrame(bytes.NewReader([]byte(`{"version":1,"comand":"help"}`+"\n")), 1024, "request", &req, true)
+	require.ErrorContains(t, err, `unknown field "comand"`)
+
+	var res Response
+	require.NoError(t, readFrame(bytes.NewReader([]byte(`{"version":1,"later":true}`+"\n")), 1024, "response", &res, false))
+	assert.Equal(t, 1, res.Version)
+}

+ 1 - 1
localapi/server.go

@@ -447,7 +447,7 @@ func stringParams(in map[string]any) (map[string]string, error) {
 
 func readRequest(conn net.Conn) (*Request, error) {
 	req := &Request{}
-	if err := readFrame(conn, MaxRequestBytes, "request", req); err != nil {
+	if err := readFrame(conn, MaxRequestBytes, "request", req, true); err != nil {
 		return nil, err
 	}
 

+ 16 - 0
localapi/server_test.go

@@ -940,3 +940,19 @@ func TestRebindAndStopSerialize(t *testing.T) {
 		}
 	}
 }
+
+// TestUnknownRequestFieldIsRefused: a request with a field the protocol does not define
+// is answered with the field named, the way an unknown parameter is, and not run.
+func TestUnknownRequestFieldIsRefused(t *testing.T) {
+	_, path := serve(t, basicRegistry())
+
+	conn, err := dial(path, time.Second)
+	require.NoError(t, err)
+	defer conn.Close()
+	require.NoError(t, writeFrame(conn, map[string]any{"version": ProtocolVersion, "command": "echo", "arguments": []string{"x"}}))
+
+	var res Response
+	require.NoError(t, readFrame(conn, MaxResponseBytes, "response", &res, false))
+	assert.Contains(t, res.Error, `unknown field "arguments"`)
+	assert.Empty(t, res.Result)
+}