Răsfoiți Sursa

Keep Run's line on a text refusal, and refuse bytes after a request's object

Two things the control socket got wrong at the edges of a reply and a
request. A reload that failed lost its "Reloading config" line in text,
since a refusal took only the outcome; ssh printed both. And a request
line was read as its first JSON object with whatever followed ignored,
so a second object or stray bytes passed unremarked where an unknown
field was refused.

- render puts the preface ahead of a refusal's text too
- readFrame, when strict, refuses anything after the object on the line

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Matt Richardson 15 ore în urmă
părinte
comite
2e049e9ab7
3 a modificat fișierele cu 48 adăugiri și 1 ștergeri
  1. 9 0
      localapi/protocol.go
  2. 1 1
      localapi/server.go
  3. 38 0
      localapi/server_test.go

+ 9 - 0
localapi/protocol.go

@@ -144,6 +144,15 @@ func readFrame(r io.Reader, max int, what string, v any, strict bool) error {
 		return &frameError{fmt.Errorf("could not parse %s: %w", what, err)}
 	}
 
+	// Strict is one object and nothing after it: a second object, or stray bytes, on
+	// the line would be read as part of a request that happened to decode, and is
+	// refused the way an unknown field is.
+	if strict {
+		if _, err := dec.Token(); err != io.EOF {
+			return fmt.Errorf("could not parse %s: unexpected data after the object", what)
+		}
+	}
+
 	return nil
 }
 

+ 1 - 1
localapi/server.go

@@ -335,7 +335,7 @@ func render(res *Response, cmd *commands.Command, r commands.Result, format comm
 	// A command that ran and refused reads as an error to whoever asked for text: they
 	// are a person or a script, and both expect a refusal on the error stream.
 	if !r.OK {
-		res.Error = buf.String()
+		res.Error = string(append(preface, buf.Bytes()...))
 		return res
 	}
 

+ 38 - 0
localapi/server_test.go

@@ -792,6 +792,23 @@ func TestThenIsAnsweredInTheOneReply(t *testing.T) {
 	require.NoError(t, err)
 	assert.JSONEq(t, `{"ok":true,"reason":"acted"}`, string(res.Result))
 
+	// A refusal from Then still carries Run's line ahead of it in text, as ssh prints it.
+	acted = false
+	reg.Register(&commands.Command{
+		Name:             "fail",
+		ShortDescription: "replies, then fails to act",
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return commands.Did("about to act"), nil
+		},
+		Then: func() commands.Result {
+			return commands.Failed("could not act")
+		},
+	})
+	res, err = (&Client{Path: path}).Invoke("fail", nil, nil, "text")
+	require.NoError(t, err)
+	assert.Equal(t, "about to act\ncould not act\n", res.Error)
+	assert.Empty(t, res.Text)
+
 	// A Run that refused is the outcome, and Then does not run.
 	acted = false
 	reg.Register(&commands.Command{
@@ -887,6 +904,27 @@ func TestUnknownRequestFieldIsRefused(t *testing.T) {
 	assert.Empty(t, res.Result)
 }
 
+// TestTrailingBytesOnARequestAreRefused: a request line is one object and nothing
+// more; a second object or stray bytes after it are refused rather than ignored.
+func TestTrailingBytesOnARequestAreRefused(t *testing.T) {
+	_, path := serve(t, basicRegistry())
+
+	for _, line := range []string{
+		`{"version":1,"command":"echo"} junk` + "\n",
+		`{"version":1,"command":"echo"}{"version":1,"command":"boom"}` + "\n",
+	} {
+		conn, err := dial(path, time.Second)
+		require.NoError(t, err)
+		_, err = conn.Write([]byte(line))
+		require.NoError(t, err)
+
+		var res Response
+		require.NoError(t, readFrame(conn, MaxResponseBytes, "response", &res, false))
+		assert.Contains(t, res.Error, "unexpected data after the object", line)
+		conn.Close()
+	}
+}
+
 // TestTextOutputKeepsTheBytes: a text rendering travels as the bytes the command wrote.
 // A JSON string would have had to be UTF-8, and a byte that is not would have come out
 // as U+FFFD, where the ssh server passes it through.