From 4bf774bcfdb99bae315d11d1e791b0295a785830 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 09:20:32 +0200 Subject: [PATCH 01/19] Limit packet length --- server.go | 2 +- server_test.go | 2 ++ session.go | 73 +++++++++++++++++++++++++++++++++++-------------- session_test.go | 57 ++++++++++++++++++++++++++++++++++++-- 4 files changed, 110 insertions(+), 24 deletions(-) diff --git a/server.go b/server.go index c7ad60b..1790984 100644 --- a/server.go +++ b/server.go @@ -303,7 +303,7 @@ func (s *Server) handshake(conn net.Conn) { _ = conn.SetReadDeadline(time.Now().Add(s.cfg.InitTimeout)) reader := bufio.NewReader(conn) - data, err := readPacket(reader) + data, err := readPacket(reader, maxInitPacketBytes) if err != nil { _ = conn.Close() return diff --git a/server_test.go b/server_test.go index 4839d64..0a3f6ff 100644 --- a/server_test.go +++ b/server_test.go @@ -66,6 +66,8 @@ func TestServerDropsBadInit(t *testing.T) { "missing nul": "7\x00X", "wrong root": "11\x00\x00", "closed too soon": "100\x00 0: + return int(length), nil + case b < '0' || b > '9': + return 0, fmt.Errorf("invalid packet length: unexpected byte %q", b) + case digits == maxDigits: + return 0, fmt.Errorf("invalid packet length: more than %d digits", maxDigits) + } + length = length*10 + int64(b-'0') + if length > int64(limit) { + return 0, fmt.Errorf("packet length exceeds the limit of %d bytes", limit) + } + } } // readLoop reads packets until the connection ends, then marks the session @@ -305,7 +336,7 @@ func readPacket(reader *bufio.Reader) ([]byte, error) { func (s *Session) readLoop(reader *bufio.Reader) { defer s.finish() for { - data, err := readPacket(reader) + data, err := readPacket(reader, maxPacketBytes) if err != nil { return } diff --git a/session_test.go b/session_test.go index 5229638..5d4cbc5 100644 --- a/session_test.go +++ b/session_test.go @@ -1,9 +1,11 @@ package dbgp import ( + "bufio" "context" "encoding/base64" "errors" + "io" "reflect" "strconv" "strings" @@ -489,10 +491,61 @@ func TestCommandAfterDisconnectFailsFast(t *testing.T) { } } +func TestReadPacket(t *testing.T) { + const limit = 100 + tests := []struct { + name, raw string + want string // the data; empty when an error is expected + }{ + {"packet", "5\x00hello\x00", "hello"}, + {"at the limit", "100\x00" + strings.Repeat("x", 100) + "\x00", strings.Repeat("x", 100)}, + {"leading zero", "05\x00hello\x00", "hello"}, + {"over the limit", "101\x00" + strings.Repeat("x", 101) + "\x00", ""}, + {"huge length", "9223372036854775807\x00", ""}, + {"overflowing length", "99999999999999999999999\x00", ""}, + {"endless zeros", strings.Repeat("0", 1000), ""}, + {"no digits", "\x00hello\x00", ""}, + {"negative", "-5\x00hello\x00", ""}, + {"plus sign", "+5\x00hello\x00", ""}, + {"space", " 5\x00hello\x00", ""}, + {"short data", "10\x00hello", ""}, + {"missing nul", "5\x00helloX", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + data, err := readPacket(bufio.NewReader(strings.NewReader(tt.raw)), limit) + if tt.want == "" { + if err == nil { + t.Errorf("readPacket = %q, want an error", data) + } + return + } + if err != nil || string(data) != tt.want { + t.Errorf("readPacket = %q, %v; want %q", data, err, tt.want) + } + }) + } +} + +// A packet that claims a huge length is rejected at once, without reading +// on: what follows its length is never read. +func TestReadPacketStopsAtBadLength(t *testing.T) { + r := bufio.NewReader(strings.NewReader("9999999999999\x00rest")) + if _, err := readPacket(r, 100); err == nil { + t.Fatal("readPacket accepted a huge length") + } + if rest, _ := io.ReadAll(r); !strings.HasSuffix(string(rest), "\x00rest") { + t.Errorf("readPacket read on past the length: left %q", rest) + } +} + func TestMalformedPacketClosesSession(t *testing.T) { for name, raw := range map[string]string{ - "bad length": "xyz\x00\x00", - "missing nul": "10\x00X", + "bad length": "xyz\x00\x00", + "missing nul": "10\x00X", + "huge length": "9223372036854775807\x00", + "over the limit": strconv.Itoa(maxPacketBytes+1) + "\x00", + "endless length": strings.Repeat("0", 100), } { t.Run(name, func(t *testing.T) { s, _ := startFakeSession(t, map[string]fakeHandler{ From 10d31f3dd10c13529443c4cb782e2f5150d0614b Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 09:27:07 +0200 Subject: [PATCH 02/19] MCP Authentication --- README.md | 2 +- cmd/dbgp/mcp.go | 2 +- cmd/dbgp/mcp_daemon.go | 80 +++++++++++++++++-- cmd/dbgp/mcp_daemon_test.go | 155 +++++++++++++++++++++++++++++++++--- 4 files changed, 219 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 133676e..90d497d 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,7 @@ dbgp mcp status # is it running? dbgp mcp stop # stop it ``` -While it runs, agents that start `dbgp mcp` use it instead, with the options it was started with. After updating dbgp, restart it to use the new version. +While it runs, agents that start `dbgp mcp` use it instead, with the options it was started with. Only you can use it: `dbgp mcp` connects with a token kept in a file that other users on the machine cannot read. After updating dbgp, restart it to use the new version. `dbgp mcp -h` lists all the options. diff --git a/cmd/dbgp/mcp.go b/cmd/dbgp/mcp.go index 50e28c3..06ce03f 100644 --- a/cmd/dbgp/mcp.go +++ b/cmd/dbgp/mcp.go @@ -130,7 +130,7 @@ func runMCP(args []string) int { fmt.Fprintf(os.Stderr, "dbgp: using the background server at %s, with its own options: %s ignored\n", st.URL, strings.Join(args, " ")) } - if err := bridge(ctx, st.URL); err != nil && ctx.Err() == nil { + if err := bridge(ctx, st); err != nil && ctx.Err() == nil { fmt.Fprintln(os.Stderr, err) return 1 } diff --git a/cmd/dbgp/mcp_daemon.go b/cmd/dbgp/mcp_daemon.go index 2aa1a9a..1955c4c 100644 --- a/cmd/dbgp/mcp_daemon.go +++ b/cmd/dbgp/mcp_daemon.go @@ -2,6 +2,8 @@ package main import ( "context" + "crypto/rand" + "crypto/subtle" "encoding/json" "errors" "fmt" @@ -20,7 +22,9 @@ import ( // The background MCP server, over HTTP: dbgp mcp start starts it and // returns, dbgp mcp stop stops it, and dbgp mcp started by an agent passes // the agent on to it. Its state file records its process and URL, so they -// find it; there is one per user. +// find it; there is one per user. The state file also holds a token that +// every request must carry: only the user can read the file, so other users +// on the machine cannot use the server, which runs PHP code for its agents. // serveCommand runs the background server in this process. dbgp mcp start // runs it; it is not meant to be typed. @@ -40,6 +44,8 @@ const ( type daemonState struct { PID int `json:"pid"` URL string `json:"url"` + // Token authenticates agents: requests send it as a bearer token. + Token string `json:"token"` } // stateDir is where the state file and the background server's log are. @@ -86,14 +92,31 @@ func writeState(st daemonState) error { if err != nil { return err } - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o700); err != nil { return err } data, err := json.Marshal(st) if err != nil { return err } - return os.WriteFile(path, data, 0o600) + // A new file, renamed into place: it is readable by the user only even + // if an old state file was not, and readers never see half of it. + f, err := os.CreateTemp(dir, "mcp-*.json") + if err != nil { + return err + } + _, err = f.Write(data) + if cerr := f.Close(); err == nil { + err = cerr + } + if err == nil { + err = os.Rename(f.Name(), path) + } + if err != nil { + _ = os.Remove(f.Name()) + } + return err } // removeState removes the state file if it still records process pid. @@ -256,14 +279,18 @@ func daemonStatus() int { } // bridge passes MCP messages between stdin and stdout and the background -// server at url, until either side closes. -func bridge(ctx context.Context, url string) error { +// server st, until either side closes. +func bridge(ctx context.Context, st *daemonState) error { local, err := (&mcp.StdioTransport{}).Connect(ctx) if err != nil { return err } defer local.Close() - remote, err := (&mcp.StreamableClientTransport{Endpoint: url}).Connect(ctx) + client, err := tokenClient(st.URL, st.Token) + if err != nil { + return err + } + remote, err := (&mcp.StreamableClientTransport{Endpoint: st.URL, HTTPClient: client}).Connect(ctx) if err != nil { return err } @@ -291,6 +318,43 @@ func bridge(ctx context.Context, url string) error { return nil } +// tokenClient returns an HTTP client that sends token to the server at +// rawURL, and to no other. A state file from before tokens has none: that +// server takes requests without one. +func tokenClient(rawURL, token string) (*http.Client, error) { + u, err := url.Parse(rawURL) + if err != nil { + return nil, err + } + return &http.Client{Transport: &bearerTransport{host: u.Host, token: token}}, nil +} + +// bearerTransport adds the token to requests for host. +type bearerTransport struct { + host, token string +} + +func (t *bearerTransport) RoundTrip(req *http.Request) (*http.Response, error) { + if t.token != "" && req.URL.Host == t.host { + req = req.Clone(req.Context()) + req.Header.Set("Authorization", "Bearer "+t.token) + } + return http.DefaultTransport.RoundTrip(req) +} + +// requireToken passes on only requests that carry token as a bearer token. +func requireToken(token string, next http.Handler) http.Handler { + want := []byte("Bearer " + token) + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if subtle.ConstantTimeCompare([]byte(req.Header.Get("Authorization")), want) != 1 { + w.Header().Set("WWW-Authenticate", `Bearer realm="dbgp"`) + http.Error(w, "unauthorized: connect agents with dbgp mcp, which sends the token", http.StatusUnauthorized) + return + } + next.ServeHTTP(w, req) + }) +} + // serveHTTP serves MCP over HTTP at addr until ctx is done, recording the // server in the state file meanwhile. func serveHTTP(ctx context.Context, server *mcp.Server, addr string) error { @@ -298,7 +362,7 @@ func serveHTTP(ctx context.Context, server *mcp.Server, addr string) error { if err != nil { return err } - st := daemonState{PID: os.Getpid(), URL: "http://" + ln.Addr().String() + "/mcp"} + st := daemonState{PID: os.Getpid(), URL: "http://" + ln.Addr().String() + "/mcp", Token: rand.Text()} if err := writeState(st); err != nil { _ = ln.Close() return err @@ -308,7 +372,7 @@ func serveHTTP(ctx context.Context, server *mcp.Server, addr string) error { handler := mcp.NewStreamableHTTPHandler(func(*http.Request) *mcp.Server { return server }, &mcp.StreamableHTTPOptions{CrossOriginProtection: http.NewCrossOriginProtection()}) mux := http.NewServeMux() - mux.Handle("/mcp", handler) + mux.Handle("/mcp", requireToken(st.Token, handler)) hs := &http.Server{Handler: mux, ReadHeaderTimeout: 10 * time.Second} go func() { <-ctx.Done() diff --git a/cmd/dbgp/mcp_daemon_test.go b/cmd/dbgp/mcp_daemon_test.go index a938ce5..63114f2 100644 --- a/cmd/dbgp/mcp_daemon_test.go +++ b/cmd/dbgp/mcp_daemon_test.go @@ -3,9 +3,14 @@ package main import ( "context" "net" + "net/http" + "net/http/httptest" + "os" "os/exec" "path/filepath" "regexp" + "runtime" + "slices" "strings" "testing" "time" @@ -66,11 +71,7 @@ func TestMCPDaemon(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() for range 2 { - cs, err := mcp.NewClient(&mcp.Implementation{Name: "test", Version: "0"}, nil). - Connect(ctx, &mcp.StreamableClientTransport{Endpoint: url}, nil) - if err != nil { - t.Fatal(err) - } + cs := connectDirect(ctx, t, url) var status statusOutput callTool(t, cs, "status", nil, &status) if status.Listening { @@ -165,11 +166,7 @@ func TestMCPBridge(t *testing.T) { if err != nil { t.Fatal(err) } - direct, err := mcp.NewClient(&mcp.Implementation{Name: "test", Version: "0"}, nil). - Connect(ctx, &mcp.StreamableClientTransport{Endpoint: url}, nil) - if err != nil { - t.Fatal(err) - } + direct := connectDirect(ctx, t, url) defer direct.Close() list, err := bridged.ListTools(ctx, nil) @@ -198,3 +195,141 @@ func TestMCPBridge(t *testing.T) { // The background server carries on after the agent leaves. callTool(t, direct, "status", nil, &status) } + +// connectDirect connects to the background server over HTTP, with the token +// from the state file, as dbgp mcp does. +func connectDirect(ctx context.Context, t *testing.T, url string) *mcp.ClientSession { + t.Helper() + st, err := readState() + if err != nil || st == nil { + t.Fatalf("state = %+v, %v", st, err) + } + client, err := tokenClient(url, st.Token) + if err != nil { + t.Fatal(err) + } + cs, err := mcp.NewClient(&mcp.Implementation{Name: "test", Version: "0"}, nil). + Connect(ctx, &mcp.StreamableClientTransport{Endpoint: url, HTTPClient: client}, nil) + if err != nil { + t.Fatal(err) + } + return cs +} + +// Only requests with the token from the state file reach the server: other +// users on the machine cannot read the file, so they cannot use it. +func TestMCPDaemonRequiresToken(t *testing.T) { + bin := buildBinary(t) + daemonEnv(t) + url := startTestDaemon(t, bin) + st, err := readState() + if err != nil || st == nil || st.Token == "" { + t.Fatalf("state = %+v, %v; want a token", st, err) + } + + initialize := `{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-06-18","capabilities":{},"clientInfo":{"name":"test","version":"0"}}}` + for name, auth := range map[string]string{ + "no token": "", + "wrong token": "Bearer " + strings.Repeat("A", len(st.Token)), + "bare token": st.Token, + "other scheme": "Basic " + st.Token, + } { + req, err := http.NewRequest("POST", url, strings.NewReader(initialize)) + if err != nil { + t.Fatal(err) + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + if auth != "" { + req.Header.Set("Authorization", auth) + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusUnauthorized { + t.Errorf("%s: status %d, want 401", name, resp.StatusCode) + } + } + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + _, err = mcp.NewClient(&mcp.Implementation{Name: "test", Version: "0"}, nil). + Connect(ctx, &mcp.StreamableClientTransport{Endpoint: url, MaxRetries: -1}, nil) + if err == nil { + t.Error("an agent without the token connected") + } + cs := connectDirect(ctx, t, url) + _ = cs.Close() + + if strings.Contains(readLog(), st.Token) { + t.Error("the log shows the token") + } + if runtime.GOOS != "windows" { + path, _ := statePath() + if info, err := os.Stat(path); err != nil || info.Mode().Perm() != 0o600 { + t.Errorf("state file: %v, %v; want mode 0600", info.Mode().Perm(), err) + } + } +} + +// A state file left readable by others is replaced by one only the user +// can read, since it holds the token. +func TestWriteStateIsPrivate(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows has no permission bits; LocalAppData is private to the user") + } + daemonEnv(t) + path, err := statePath() + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + want := daemonState{PID: 1, URL: "http://127.0.0.1:1/mcp", Token: "secret"} + if err := writeState(want); err != nil { + t.Fatal(err) + } + if info, err := os.Stat(path); err != nil || info.Mode().Perm() != 0o600 { + t.Errorf("state file: %v, %v; want mode 0600", info.Mode().Perm(), err) + } + if got, err := readState(); err != nil || got == nil || *got != want { + t.Errorf("readState = %+v, %v; want %+v", got, err, want) + } + if entries, _ := os.ReadDir(filepath.Dir(path)); len(entries) != 1 { + t.Errorf("state directory has %d entries, want only the state file", len(entries)) + } +} + +// The token goes to the background server only, not to other hosts, e.g. +// after a redirect. +func TestTokenClientSendsTokenOnlyToItsServer(t *testing.T) { + var got []string + record := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + got = append(got, req.Header.Get("Authorization")) + }) + server := httptest.NewServer(record) + defer server.Close() + other := httptest.NewServer(record) + defer other.Close() + + client, err := tokenClient(server.URL+"/mcp", "secret") + if err != nil { + t.Fatal(err) + } + for _, u := range []string{server.URL + "/mcp", other.URL + "/mcp"} { + resp, err := client.Get(u) + if err != nil { + t.Fatal(err) + } + _ = resp.Body.Close() + } + if want := []string{"Bearer secret", ""}; !slices.Equal(got, want) { + t.Errorf("Authorization headers = %q, want %q", got, want) + } +} From 554db8e92bf909bdb5d9dfb47848b9bfa3ade32a Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 10:06:03 +0200 Subject: [PATCH 03/19] Port options --- README.md | 4 ++- cmd/dbgp/main.go | 35 ++++++++++++++++++++---- cmd/dbgp/mcp.go | 34 ++++++++++++++--------- cmd/dbgp/mcp_daemon.go | 18 ++++++++----- cmd/dbgp/mcp_daemon_test.go | 22 ++++++++++++--- cmd/dbgp/mcp_e2e_test.go | 8 +++--- cmd/dbgp/mcp_test.go | 20 +++++++------- cmd/dbgp/repl.go | 6 ++--- cmd/dbgp/repl_test.go | 54 ++++++++++++++++++++++++++++++++----- 9 files changed, 149 insertions(+), 52 deletions(-) diff --git a/README.md b/README.md index 90d497d..0b196c7 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,7 @@ You can optionally add some options after `dbgp mcp`: - `-no-eval` leaves out the tools that run PHP code in the debugged script (evaluating expressions and changing variables), so the agent can only look. - `-map LOCAL=REMOTE` maps a directory on this machine to where PHP sees it, for PHP in a container or on a server. The agent can also add mappings itself. -- `-addr` changes the address dbgp listens on for PHP, `localhost:9003` by default. +- `-php-port` changes the port dbgp listens on for PHP, 9003 by default. dbgp listens on every address of this machine, so PHP in a container or on another machine can connect too. By default each agent session starts its own dbgp. To keep breakpoints and sessions between agent sessions, and share them between agents, run dbgp in the background: @@ -61,6 +61,8 @@ dbgp mcp status # is it running? dbgp mcp stop # stop it ``` +`dbgp mcp start` also takes `-mcp-port`, the port agents reach it on, 9004 by default. It listens on this machine only (127.0.0.1). + While it runs, agents that start `dbgp mcp` use it instead, with the options it was started with. Only you can use it: `dbgp mcp` connects with a token kept in a file that other users on the machine cannot read. After updating dbgp, restart it to use the new version. `dbgp mcp -h` lists all the options. diff --git a/cmd/dbgp/main.go b/cmd/dbgp/main.go index 935cbc9..d4f9595 100644 --- a/cmd/dbgp/main.go +++ b/cmd/dbgp/main.go @@ -10,9 +10,11 @@ import ( "flag" "fmt" "io" + "net" "os" "os/signal" "path/filepath" + "strconv" "strings" dbgp "github.com/php-debugger/dbgp-client" @@ -33,7 +35,7 @@ func main() { os.Exit(runMCP(os.Args[2:])) } - addr := flag.String("addr", dbgp.DefaultAddr, "address to listen on for PHP connections") + phpPort := flag.Int("php-port", defaultPHPPort, "port to listen on for PHP connections, on every address of this machine") idekey := flag.String("idekey", "", "accept only sessions with this IDE key") history := flag.String("history", defaultHistoryFile(), "`file` to keep command history in; empty disables it") showVersion := flag.Bool("version", false, "print the version and exit") @@ -43,7 +45,7 @@ func main() { flag.Usage = func() { fmt.Fprintf(flag.CommandLine.Output(), "Usage: dbgp [options]\n dbgp mcp [options]\n\n"+ "Interactive debugger for PHP Debugger and Xdebug. Start PHP with debugging\n"+ - "enabled and pointed at -addr; type help at the prompt for commands.\n"+ + "enabled and pointed at -php-port; type help at the prompt for commands.\n"+ "dbgp mcp runs it as an MCP server for AI agents instead (dbgp mcp -h).\n\nOptions:\n") flag.PrintDefaults() } @@ -58,7 +60,11 @@ func main() { fmt.Fprintln(os.Stderr, err) os.Exit(2) } - srv, err := dbgp.Listen(dbgp.Config{Addr: *addr, IDEKey: *idekey, PathMap: pathMap}) + if err := checkPort("-php-port", *phpPort); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } + srv, err := dbgp.Listen(dbgp.Config{Addr: phpAddr(*phpPort), IDEKey: *idekey, PathMap: pathMap}) if err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(1) @@ -88,8 +94,8 @@ func main() { for _, b := range breaks { r.execLine("break " + b) } - r.printf("Listening on %s. Sessions are announced when PHP connects, and commands\n"+ - "that need one wait for it. Type help for commands.\n", srv.Addr()) + r.printf("Listening for PHP on port %d. Sessions are announced when PHP connects,\n"+ + "and commands that need one wait for it. Type help for commands.\n", srv.Port()) r.printListenWarnings() interrupts := make(chan os.Signal, 1) @@ -99,6 +105,25 @@ func main() { r.run(lines) } +// defaultPHPPort is where PHP Debugger and Xdebug connect by default. +const defaultPHPPort = 9003 + +// phpAddr is the address to listen on for PHP: every address of this +// machine, so that PHP in a container, a VM or on another machine can +// connect as well as PHP on this one. +func phpAddr(port int) string { + return net.JoinHostPort("", strconv.Itoa(port)) +} + +// checkPort reports an error unless port, the value of the named flag, is +// a TCP port or 0 for any free one. +func checkPort(name string, port int) error { + if port < 0 || port > 65535 { + return fmt.Errorf("invalid %s %d: want 1 to 65535, or 0 for any free port", name, port) + } + return nil +} + // parsePathMap parses LOCAL=REMOTE flags. func parsePathMap(specs []string) ([]dbgp.PathMapping, error) { var m []dbgp.PathMapping diff --git a/cmd/dbgp/mcp.go b/cmd/dbgp/mcp.go index 06ce03f..7a9923a 100644 --- a/cmd/dbgp/mcp.go +++ b/cmd/dbgp/mcp.go @@ -45,10 +45,12 @@ xdebug.start_with_request for it. Xdebug needs xdebug.mode=debug and xdebug.start_with_request=yes. PHP finds dbgp through xdebug.client_host and xdebug.client_port, which -default to localhost and 9003 in both debuggers. When PHP runs on this machine -and the listen tool's address is localhost:9003, dbgp's default, those -defaults already reach dbgp and neither needs to be set. Otherwise set them to -the port from that address and a host PHP can reach this machine on.` +default to localhost and 9003 in both debuggers. dbgp listens on every address +of this machine, on the port the listen tool returns, 9003 by default. When PHP +runs on this machine and the port is 9003, those defaults already reach dbgp +and neither needs to be set. Otherwise set client_port to that port, and +client_host to an address PHP can reach this machine on, such as +host.docker.internal from a Docker container.` // noCodeInstructions are added to mcpInstructions when dbgp mcp runs with // -no-eval, so agents know the code tools were left out on purpose. @@ -86,11 +88,11 @@ func runMCP(args []string) int { } fs := flag.NewFlagSet("dbgp mcp", flag.ContinueOnError) - addr := fs.String("addr", dbgp.DefaultAddr, "address to listen on for PHP connections") + phpPort := fs.Int("php-port", defaultPHPPort, "port to listen on for PHP connections, on every address of this machine") idekey := fs.String("idekey", "", "accept only sessions with this IDE key") listen := fs.Bool("listen", false, "listen for PHP connections from the start, instead of waiting for the listen tool") noEval := fs.Bool("no-eval", false, "leave out the eval and set_variable tools, which run PHP code in the debugged script") - httpAddr := fs.String("http", defaultHTTPAddr, "with start: the address agents connect to over HTTP") + mcpPort := fs.Int("mcp-port", defaultHTTPPort, "with start: the port agents connect to over HTTP, on "+httpHost+" only") var maps listFlag fs.Var(&maps, "map", "path mapping `LOCAL=REMOTE` for PHP in a container or on a server (repeatable)") fs.Usage = func() { @@ -118,6 +120,12 @@ func runMCP(args []string) int { fmt.Fprintln(os.Stderr, err) return 2 } + for name, port := range map[string]int{"-php-port": *phpPort, "-mcp-port": *mcpPort} { + if err := checkPort(name, port); err != nil { + fmt.Fprintln(os.Stderr, err) + return 2 + } + } ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stop() @@ -138,7 +146,7 @@ func runMCP(args []string) int { } } - srv, err := dbgp.NewServer(dbgp.Config{Addr: *addr, IDEKey: *idekey, PathMap: pathMap}) + srv, err := dbgp.NewServer(dbgp.Config{Addr: phpAddr(*phpPort), IDEKey: *idekey, PathMap: pathMap}) if err != nil { fmt.Fprintln(os.Stderr, err) return 2 @@ -152,7 +160,7 @@ func runMCP(args []string) int { } server := newMCPServer(srv, !*noEval) if command == serveCommand { - err = serveHTTP(ctx, server, *httpAddr) + err = serveHTTP(ctx, server, *mcpPort) } else { err = server.Run(ctx, &mcp.StdioTransport{}) } @@ -184,7 +192,7 @@ func newMCPServer(srv *dbgp.Server, allowCode bool) *mcp.Server { mcp.AddTool(server, &mcp.Tool{ Name: "listen", Description: "Start listening for PHP connections, so the next PHP script or request with " + - "debugging enabled connects and becomes a session. The result gives the address PHP must connect to.", + "debugging enabled connects and becomes a session. The result gives the port PHP must connect to.", Annotations: &mcp.ToolAnnotations{IdempotentHint: true}, OutputSchema: outputSchema[listenOutput]("warnings"), }, t.listen) @@ -298,7 +306,7 @@ type noInput struct{} type statusOutput struct { Listening bool `json:"listening" jsonschema:"whether PHP connections are accepted"` - Address string `json:"address" jsonschema:"the address PHP connects to when listening"` + Port int `json:"port" jsonschema:"the port PHP connects to when listening, on any address of this machine"` Sessions []sessionInfo `json:"sessions" jsonschema:"debugging sessions, oldest first, including ended ones"` Breakpoints []breakpointInfo `json:"breakpoints" jsonschema:"breakpoints, applied to every session"` PathMappings []pathMapping `json:"pathMappings" jsonschema:"local directories mapped to the directories PHP sees"` @@ -330,7 +338,7 @@ type pathMapping struct { func (t *tools) status(context.Context, *mcp.CallToolRequest, noInput) (*mcp.CallToolResult, statusOutput, error) { out := statusOutput{ Listening: t.srv.Listening(), - Address: t.srv.Addr(), + Port: t.srv.Port(), Sessions: t.sessionList(), } out.Breakpoints = t.breakpointList() @@ -365,7 +373,7 @@ func (t *tools) pathMappingList() []pathMapping { type listenOutput struct { Listening bool `json:"listening" jsonschema:"whether PHP connections are accepted"` - Address string `json:"address" jsonschema:"the address PHP must connect to (host and port)"` + Port int `json:"port" jsonschema:"the port PHP must connect to (xdebug.client_port), on any address of this machine"` Warnings []string `json:"warnings,omitempty" jsonschema:"addresses dbgp could not listen on, although it listens on others; PHP connecting there may reach another program instead"` } @@ -373,7 +381,7 @@ func (t *tools) listen(context.Context, *mcp.CallToolRequest, noInput) (*mcp.Cal if err := t.srv.StartListening(); err != nil { return nil, listenOutput{}, err } - return nil, listenOutput{Listening: true, Address: t.srv.Addr(), Warnings: t.srv.ListenWarnings()}, nil + return nil, listenOutput{Listening: true, Port: t.srv.Port(), Warnings: t.srv.ListenWarnings()}, nil } type unlistenOutput struct { diff --git a/cmd/dbgp/mcp_daemon.go b/cmd/dbgp/mcp_daemon.go index 1955c4c..ff4a737 100644 --- a/cmd/dbgp/mcp_daemon.go +++ b/cmd/dbgp/mcp_daemon.go @@ -14,6 +14,7 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "time" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -30,9 +31,14 @@ import ( // runs it; it is not meant to be typed. const serveCommand = "serve" -// defaultHTTPAddr is where agents connect: on this machine only, next to -// DBGp's 9003. -const defaultHTTPAddr = "127.0.0.1:9004" +// The background server listens on httpHost, on this machine only: agents +// on it read the server's token from the state file, and the token is sent +// unencrypted. An IP address, not localhost, so no lookup can send it +// elsewhere. Its port defaults to defaultHTTPPort, next to PHP's 9003. +const ( + httpHost = "127.0.0.1" + defaultHTTPPort = 9004 +) // How long the commands wait for the server to start or stop. const ( @@ -355,10 +361,10 @@ func requireToken(token string, next http.Handler) http.Handler { }) } -// serveHTTP serves MCP over HTTP at addr until ctx is done, recording the +// serveHTTP serves MCP over HTTP on port until ctx is done, recording the // server in the state file meanwhile. -func serveHTTP(ctx context.Context, server *mcp.Server, addr string) error { - ln, err := net.Listen("tcp", addr) +func serveHTTP(ctx context.Context, server *mcp.Server, port int) error { + ln, err := net.Listen("tcp", net.JoinHostPort(httpHost, strconv.Itoa(port))) if err != nil { return err } diff --git a/cmd/dbgp/mcp_daemon_test.go b/cmd/dbgp/mcp_daemon_test.go index 63114f2..d3dc54b 100644 --- a/cmd/dbgp/mcp_daemon_test.go +++ b/cmd/dbgp/mcp_daemon_test.go @@ -45,7 +45,7 @@ var runningAt = regexp.MustCompile(`running at (\S+) \(pid (\d+)\)`) // returns its URL. It is stopped at cleanup. func startTestDaemon(t *testing.T, bin string, args ...string) string { t.Helper() - code, out := runDbgp(t, bin, append([]string{"mcp", "start", "-http", "127.0.0.1:0"}, args...)...) + code, out := runDbgp(t, bin, append([]string{"mcp", "start", "-mcp-port", "0"}, args...)...) m := runningAt.FindStringSubmatch(out) if code != 0 || m == nil { t.Fatalf("dbgp mcp: exit %d\n%s", code, out) @@ -59,6 +59,9 @@ func TestMCPDaemon(t *testing.T) { daemonEnv(t) url := startTestDaemon(t, bin) + if !strings.HasPrefix(url, "http://127.0.0.1:") { + t.Errorf("URL %s: want the server on 127.0.0.1", url) + } if !strings.Contains(runDbgpOut(t, bin, "mcp", "status"), url) { t.Error("dbgp mcp status does not show the URL") } @@ -117,7 +120,8 @@ func TestMCPDaemonDoesNotStart(t *testing.T) { } defer ln.Close() - code, out := runDbgp(t, bin, "mcp", "start", "-http", ln.Addr().String()) + _, port, _ := net.SplitHostPort(ln.Addr().String()) + code, out := runDbgp(t, bin, "mcp", "start", "-mcp-port", port) if code != 1 || !strings.Contains(out, "did not start") || !strings.Contains(out, ln.Addr().String()) { t.Errorf("start on a taken port: exit %d\n%s", code, out) } @@ -138,15 +142,27 @@ func TestMCPDaemonStaleState(t *testing.T) { } func TestMCPArguments(t *testing.T) { + daemonEnv(t) for _, args := range [][]string{ {"restart"}, - {"stop", "-http", "127.0.0.1:0"}, + {"stop", "-mcp-port", "0"}, {"start", "extra"}, + {"start", "-mcp-port", "-1"}, + {"start", "-mcp-port", "65536"}, + {"start", "-mcp-port", "http"}, + {"start", "-php-port", "65536"}, + // Agents connect on 127.0.0.1 only, and PHP on every address. + {"start", "-http", "0.0.0.0:9004"}, + {"start", "-addr", "127.0.0.1:9003"}, + {serveCommand, "-mcp-port", "-1"}, } { if code := runMCP(args); code != 2 { t.Errorf("%v: exit %d, want 2", args, code) } } + if st, err := readState(); st != nil || err != nil { + t.Errorf("state = %+v, %v; want no server started", st, err) + } } // An agent that starts dbgp mcp while the background server runs uses it: diff --git a/cmd/dbgp/mcp_e2e_test.go b/cmd/dbgp/mcp_e2e_test.go index 3345644..3fa66a4 100644 --- a/cmd/dbgp/mcp_e2e_test.go +++ b/cmd/dbgp/mcp_e2e_test.go @@ -5,6 +5,7 @@ import ( "net" "os/exec" "path/filepath" + "strconv" "testing" "time" @@ -24,7 +25,7 @@ func TestMCPEndToEnd(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) defer cancel() daemonEnv(t) - startTestDaemon(t, bin, "-addr", "127.0.0.1:0") + startTestDaemon(t, bin, "-php-port", "0") transport := &mcp.CommandTransport{Command: exec.CommandContext(ctx, bin, "mcp")} cs, err := mcp.NewClient(&mcp.Implementation{Name: "test", Version: "0"}, nil).Connect(ctx, transport, nil) if err != nil { @@ -36,10 +37,7 @@ func TestMCPEndToEnd(t *testing.T) { callTool(t, cs, "add_breakpoint", map[string]any{"file": script, "line": 24}, &bp) var listening listenOutput callTool(t, cs, "listen", nil, &listening) - _, port, err := net.SplitHostPort(listening.Address) - if err != nil { - t.Fatal(err) - } + port := strconv.Itoa(listening.Port) // The script runs in the background while it is debugged. php := exec.Command("php", "-dxdebug.mode=debug", "-dxdebug.start_with_request=yes", diff --git a/cmd/dbgp/mcp_test.go b/cmd/dbgp/mcp_test.go index 5555558..48835bf 100644 --- a/cmd/dbgp/mcp_test.go +++ b/cmd/dbgp/mcp_test.go @@ -8,6 +8,7 @@ import ( "os/exec" "path/filepath" "reflect" + "strconv" "strings" "testing" "time" @@ -134,7 +135,7 @@ func TestMCPStatusBeforeListening(t *testing.T) { } // Empty lists are [], not null, so agents need not special-case them. text := resultText(res) - for _, want := range []string{`"listening":false`, `"address":"127.0.0.1:0"`, `"sessions":[]`, `"breakpoints":[]`, `"pathMappings":[]`} { + for _, want := range []string{`"listening":false`, `"port":0`, `"sessions":[]`, `"breakpoints":[]`, `"pathMappings":[]`} { if !strings.Contains(text, want) { t.Errorf("status text lacks %s: %s", want, text) } @@ -153,7 +154,8 @@ func TestMCPListenAndUnlisten(t *testing.T) { var listened listenOutput callTool(t, cs, "listen", nil, &listened) - if !listened.Listening || listened.Address != srv.Addr() || refused(listened.Address) { + addr := net.JoinHostPort("127.0.0.1", strconv.Itoa(listened.Port)) + if !listened.Listening || listened.Port == 0 || listened.Port != srv.Port() || refused(addr) { t.Fatalf("listen = %+v; server listening %v", listened, srv.Listening()) } callTool(t, cs, "listen", nil, &listened) @@ -163,13 +165,13 @@ func TestMCPListenAndUnlisten(t *testing.T) { var status statusOutput callTool(t, cs, "status", nil, &status) - if !status.Listening || status.Address != listened.Address { + if !status.Listening || status.Port != listened.Port { t.Errorf("status after listen = %+v", status) } var unlistened unlistenOutput callTool(t, cs, "unlisten", nil, &unlistened) - if unlistened.Listening || srv.Listening() || !refused(listened.Address) { + if unlistened.Listening || srv.Listening() || !refused(addr) { t.Errorf("after unlisten: %+v, server listening %v", unlistened, srv.Listening()) } callTool(t, cs, "status", nil, &status) @@ -283,9 +285,9 @@ func TestMCPBinary(t *testing.T) { args []string listening bool }{ - {[]string{"mcp", "-addr", "127.0.0.1:0"}, false}, - {[]string{"mcp", "-addr", "127.0.0.1:0", "-listen"}, true}, - {[]string{"mcp", "-addr", "127.0.0.1:0", "-no-eval"}, false}, + {[]string{"mcp", "-php-port", "0"}, false}, + {[]string{"mcp", "-php-port", "0", "-listen"}, true}, + {[]string{"mcp", "-php-port", "0", "-no-eval"}, false}, } { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) transport := &mcp.CommandTransport{Command: exec.CommandContext(ctx, bin, tc.args...)} @@ -528,7 +530,7 @@ func TestMCPListenWarnings(t *testing.T) { cs, _ := newTestMCPOn(t, "localhost:"+port, true) var out listenOutput callTool(t, cs, "listen", nil, &out) - if !out.Listening || out.Address != "localhost:"+port || len(out.Warnings) != 1 || !strings.Contains(out.Warnings[0], "[::1]:"+port) { - t.Errorf("listen = %+v, want localhost:%s with a warning about [::1]", out, port) + if !out.Listening || strconv.Itoa(out.Port) != port || len(out.Warnings) != 1 || !strings.Contains(out.Warnings[0], "[::1]:"+port) { + t.Errorf("listen = %+v, want port %s with a warning about [::1]", out, port) } } diff --git a/cmd/dbgp/repl.go b/cmd/dbgp/repl.go index b00031d..947d525 100644 --- a/cmd/dbgp/repl.go +++ b/cmd/dbgp/repl.go @@ -335,13 +335,13 @@ func (r *repl) cmdMaps(context.Context, string) error { func (r *repl) cmdListen(context.Context, string) error { if r.srv.Listening() { - r.printf("Already listening on %s.\n", r.srv.Addr()) + r.printf("Already listening for PHP on port %d.\n", r.srv.Port()) return nil } if err := r.srv.StartListening(); err != nil { return err } - r.printf("Listening on %s.\n", r.srv.Addr()) + r.printf("Listening for PHP on port %d.\n", r.srv.Port()) r.printListenWarnings() return nil } @@ -367,7 +367,7 @@ func (r *repl) cmdUnlisten(context.Context, string) error { func (r *repl) cmdSessions(context.Context, string) error { if r.srv.Listening() { - r.printf("Listening on %s.\n", r.srv.Addr()) + r.printf("Listening for PHP on port %d.\n", r.srv.Port()) } else { r.printf("Not listening: PHP connections are refused (use listen).\n") } diff --git a/cmd/dbgp/repl_test.go b/cmd/dbgp/repl_test.go index 7ae8a3b..e232790 100644 --- a/cmd/dbgp/repl_test.go +++ b/cmd/dbgp/repl_test.go @@ -10,6 +10,7 @@ import ( "reflect" "slices" "sort" + "strconv" "strings" "testing" "time" @@ -429,21 +430,26 @@ func TestBinary(t *testing.T) { t.Errorf("-map nonsense: %v\n%s", err, out) } - srv, err := dbgp.Listen(dbgp.Config{Addr: "127.0.0.1:0"}) + srv, err := dbgp.Listen(dbgp.Config{Addr: phpAddr(0)}) if err != nil { t.Fatal(err) } - busy := srv.Addr() - cmd = exec.Command(bin, "-addr", busy) + cmd = exec.Command(bin, "-php-port", strconv.Itoa(srv.Port())) if out, err := cmd.CombinedOutput(); cmd.ProcessState.ExitCode() != 1 { - t.Errorf("busy address: %v\n%s", err, out) + t.Errorf("busy port: %v\n%s", err, out) } _ = srv.Close() + for _, port := range []string{"-1", "65536"} { + cmd = exec.Command(bin, "-php-port", port) + if out, err := cmd.CombinedOutput(); cmd.ProcessState.ExitCode() != 2 { + t.Errorf("-php-port %s: %v\n%s", port, err, out) + } + } - cmd = exec.Command(bin, "-addr", "127.0.0.1:0", "-break", "/app/a.php:3", "-break", "/app/b.php:5") + cmd = exec.Command(bin, "-php-port", "0", "-break", "/app/a.php:3", "-break", "/app/b.php:5") cmd.Stdin = strings.NewReader("breaks\nquit\n") out, err = cmd.CombinedOutput() - if err != nil || !strings.Contains(string(out), "Listening on 127.0.0.1:") || + if err != nil || !strings.Contains(string(out), "Listening for PHP on port ") || !strings.Contains(string(out), "1 /app/a.php:3\n") || !strings.Contains(string(out), "2 /app/b.php:5\n") { t.Errorf("session: %v\n%s", err, out) } @@ -479,6 +485,7 @@ func TestFormatVariableUnset(t *testing.T) { func TestListenAndUnlisten(t *testing.T) { r, srv, output := newTestREPL(t) addr := srv.Addr() + port := strconv.Itoa(srv.Port()) refused := func() bool { conn, err := net.DialTimeout("tcp", addr, time.Second) if err == nil { @@ -514,7 +521,7 @@ func TestListenAndUnlisten(t *testing.T) { "Stopped listening: new PHP connections are refused. Connected sessions carry on.\nNot listening.\n", "Not listening: PHP connections are refused (use listen).\nNo sessions.\n", "Error: no session, and not listening for PHP connections (use listen)\n", - "Listening on " + addr + ".\nAlready listening on " + addr + ".\nListening on " + addr + ".\nNo sessions.\n", + "Listening for PHP on port " + port + ".\nAlready listening for PHP on port " + port + ".\nListening for PHP on port " + port + ".\nNo sessions.\n", } { if !strings.Contains(out, want) { t.Errorf("output lacks %q", want) @@ -555,3 +562,36 @@ func TestMapAndMaps(t *testing.T) { t.Logf("output:\n%s", out) } } + +// dbgp listens for PHP on every address of this machine, so PHP in a +// container or on another machine reaches it as well as PHP on this one. +func TestPHPAddrIsEveryAddress(t *testing.T) { + srv, err := dbgp.Listen(dbgp.Config{Addr: phpAddr(0)}) + if err != nil { + t.Fatal(err) + } + defer srv.Close() + var hosts []string + addrs, _ := net.InterfaceAddrs() + for _, a := range addrs { + if ipnet, ok := a.(*net.IPNet); ok && ipnet.IP.To4() != nil && !ipnet.IP.IsLoopback() && !ipnet.IP.IsLinkLocalUnicast() { + hosts = append(hosts, ipnet.IP.String()) + break + } + } + if len(hosts) == 0 { + t.Log("no network address besides loopback: checking loopback only") + } + for _, host := range append(hosts, "127.0.0.1", "::1") { + conn, err := net.DialTimeout("tcp", net.JoinHostPort(host, strconv.Itoa(srv.Port())), time.Second) + if err != nil { + if host == "::1" { + t.Logf("no IPv6 loopback: %v", err) + continue + } + t.Errorf("PHP cannot connect on %s: %v", host, err) + continue + } + _ = conn.Close() + } +} From 44ea609ddb11b1d6345879dbee218ffc56447c1f Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 10:17:24 +0200 Subject: [PATCH 04/19] Do not allow conditions in no-eval mode --- README.md | 2 +- cmd/dbgp/mcp.go | 41 ++++++++++++++++------- cmd/dbgp/mcp_code_test.go | 69 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 0b196c7..10aa6b3 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,7 @@ The agent starts the PHP code itself, as a script or a web request, and debugs i You can optionally add some options after `dbgp mcp`: -- `-no-eval` leaves out the tools that run PHP code in the debugged script (evaluating expressions and changing variables), so the agent can only look. +- `-no-eval` turns off everything that runs PHP code in the debugged script: evaluating expressions, changing variables and breakpoint conditions, so the agent can only look. - `-map LOCAL=REMOTE` maps a directory on this machine to where PHP sees it, for PHP in a container or on a server. The agent can also add mappings itself. - `-php-port` changes the port dbgp listens on for PHP, 9003 by default. dbgp listens on every address of this machine, so PHP in a container or on another machine can connect too. diff --git a/cmd/dbgp/mcp.go b/cmd/dbgp/mcp.go index 7a9923a..5da359c 100644 --- a/cmd/dbgp/mcp.go +++ b/cmd/dbgp/mcp.go @@ -8,6 +8,7 @@ import ( "os" "os/signal" "path/filepath" + "slices" "strings" "sync" "syscall" @@ -56,11 +57,13 @@ host.docker.internal from a Docker container.` // -no-eval, so agents know the code tools were left out on purpose. const noCodeInstructions = ` -The user started dbgp with -no-eval, which turns off the eval and set_variable -tools, so you cannot evaluate PHP or change variables in the debugged script. -This is the user's choice, not a missing feature or an outdated build: do not -suggest rebuilding or restarting dbgp to get them. Read values with variables, -variable and variable_value instead, and work out results from them.` +The user started dbgp with -no-eval, which turns off everything that runs PHP +code in the debugged script: the eval and set_variable tools, and conditions on +breakpoints. So you cannot evaluate PHP, change variables or set conditional +breakpoints. This is the user's choice, not a missing feature or an outdated +build: do not suggest rebuilding or restarting dbgp to get them. Read values +with variables, variable and variable_value instead, and work out results from +them. Instead of a condition, stop at the line and check the values each time.` // runMCP runs dbgp as an MCP server. Agents start it as dbgp mcp and talk // to it on stdin and stdout: it serves them itself, or passes them on to the @@ -91,7 +94,7 @@ func runMCP(args []string) int { phpPort := fs.Int("php-port", defaultPHPPort, "port to listen on for PHP connections, on every address of this machine") idekey := fs.String("idekey", "", "accept only sessions with this IDE key") listen := fs.Bool("listen", false, "listen for PHP connections from the start, instead of waiting for the listen tool") - noEval := fs.Bool("no-eval", false, "leave out the eval and set_variable tools, which run PHP code in the debugged script") + noEval := fs.Bool("no-eval", false, "turn off everything that runs PHP code in the debugged script: the eval and set_variable tools, and breakpoint conditions") mcpPort := fs.Int("mcp-port", defaultHTTPPort, "with start: the port agents connect to over HTTP, on "+httpHost+" only") var maps listFlag fs.Var(&maps, "map", "path mapping `LOCAL=REMOTE` for PHP in a container or on a server (repeatable)") @@ -172,7 +175,8 @@ func runMCP(args []string) int { } // newMCPServer returns an MCP server whose tools drive srv. allowCode adds -// the tools that run PHP code in the debugged script. +// what runs PHP code in the debugged script: the code tools, and conditions +// on breakpoints. func newMCPServer(srv *dbgp.Server, allowCode bool) *mcp.Server { instructions := mcpInstructions if !allowCode { @@ -180,7 +184,7 @@ func newMCPServer(srv *dbgp.Server, allowCode bool) *mcp.Server { } server := mcp.NewServer(&mcp.Implementation{Name: "dbgp", Title: "PHP debugger", Version: version}, &mcp.ServerOptions{Instructions: instructions}) - t := &tools{srv: srv} + t := &tools{srv: srv, allowCode: allowCode} mcp.AddTool(server, &mcp.Tool{ Name: "status", @@ -204,13 +208,17 @@ func newMCPServer(srv *dbgp.Server, allowCode bool) *mcp.Server { }, t.unlisten) notDestructive := false + conditions := "A line breakpoint with a condition stops only when the condition is true. " + if !allowCode { + conditions = "Conditions are turned off (-no-eval), since they run PHP code. " + } mcp.AddTool(server, &mcp.Tool{ Name: "add_breakpoint", Description: "Add a breakpoint. It applies to every debugging session, including ones that connect later. " + - "A line breakpoint with a condition stops only when the condition is true. " + + conditions + "Remove it with remove_breakpoint once you no longer need it: breakpoints slow down later sessions.", Annotations: &mcp.ToolAnnotations{DestructiveHint: ¬Destructive}, - InputSchema: addBreakpointSchema(), + InputSchema: addBreakpointSchema(allowCode), OutputSchema: outputSchema[addBreakpointOutput](), }, t.addBreakpoint) mcp.AddTool(server, &mcp.Tool{ @@ -294,6 +302,8 @@ func pageOf[T any](items []T, n int) ([]T, int, error) { // tools implements the MCP tools on a dbgp server. type tools struct { srv *dbgp.Server + // allowCode allows what runs PHP code in the debugged script. + allowCode bool // mu guards the read positions of output and warnings, per session id: // those tools return only what is new since their last call. @@ -410,12 +420,17 @@ type addBreakpointInput struct { } // addBreakpointSchema is addBreakpointInput's schema, listing the types. -func addBreakpointSchema() *jsonschema.Schema { +// Without allowCode it has no condition, which is PHP code. +func addBreakpointSchema(allowCode bool) *jsonschema.Schema { schema, err := jsonschema.For[addBreakpointInput](nil) if err != nil { panic(fmt.Sprintf("add_breakpoint schema: %v", err)) } schema.Properties["type"].Enum = breakpointTypes + if !allowCode { + delete(schema.Properties, "condition") + schema.PropertyOrder = slices.DeleteFunc(schema.PropertyOrder, func(name string) bool { return name == "condition" }) + } return schema } @@ -425,6 +440,10 @@ type addBreakpointOutput struct { } func (t *tools) addBreakpoint(_ context.Context, _ *mcp.CallToolRequest, in addBreakpointInput) (*mcp.CallToolResult, addBreakpointOutput, error) { + if in.Condition != "" && !t.allowCode { + return nil, addBreakpointOutput{}, errors.New("conditions are turned off (-no-eval), since they run PHP code: " + + "add the breakpoint without one and check the values each time it stops") + } bp := dbgp.Breakpoint{Type: in.Type, Line: in.Line, Function: in.Function, Exception: in.Exception} if bp.Type == "" { bp.Type = dbgp.BreakpointLine diff --git a/cmd/dbgp/mcp_code_test.go b/cmd/dbgp/mcp_code_test.go index 1a3a2ac..e9d430c 100644 --- a/cmd/dbgp/mcp_code_test.go +++ b/cmd/dbgp/mcp_code_test.go @@ -2,8 +2,11 @@ package main import ( "context" + "encoding/json" "strings" "testing" + + dbgp "github.com/php-debugger/dbgp-client" ) func TestMCPCodeToolsCanBeLeftOut(t *testing.T) { @@ -128,3 +131,69 @@ func TestMCPEvalDescriptionExplainsFrames(t *testing.T) { } } } + +// Breakpoint conditions are PHP code the engine runs, so -no-eval turns +// them off along with the code tools. +func TestMCPNoEvalTurnsOffConditions(t *testing.T) { + for _, allowCode := range []bool{true, false} { + cs, srv := newTestMCPOn(t, "127.0.0.1:0", allowCode) + res, err := cs.ListTools(context.Background(), nil) + if err != nil { + t.Fatal(err) + } + for _, tool := range res.Tools { + if tool.Name != "add_breakpoint" { + continue + } + data, _ := json.Marshal(tool.InputSchema) + var schema struct { + Properties map[string]any `json:"properties"` + } + if err := json.Unmarshal(data, &schema); err != nil { + t.Fatal(err) + } + if _, ok := schema.Properties["condition"]; ok != allowCode { + t.Errorf("allowCode %v: condition in the schema is %v", allowCode, ok) + } + if mentions := strings.Contains(tool.Description, "-no-eval"); mentions == allowCode { + t.Errorf("allowCode %v: description %q", allowCode, tool.Description) + } + } + if allowCode { + continue + } + + // An agent that sends one anyway is refused, and nothing is added. + args := map[string]any{"file": "/app/a.php", "line": 3, "condition": "file_put_contents('/tmp/x', 'x')"} + callToolError(t, cs, "add_breakpoint", args) + if bps := srv.Breakpoints(); len(bps) != 0 { + t.Errorf("breakpoints after a refused condition = %+v", bps) + } + if instructions := cs.InitializeResult().Instructions; !strings.Contains(instructions, "conditions on\nbreakpoints") { + t.Errorf("instructions do not say conditions are off: %s", instructions) + } + // Breakpoints without one still work. + var added addBreakpointOutput + callTool(t, cs, "add_breakpoint", map[string]any{"file": "/app/a.php", "line": 3}, &added) + if added.Breakpoint.Type != "line" { + t.Errorf("add_breakpoint = %+v", added) + } + } +} + +// The tool refuses a condition itself too, not only through its schema. +func TestAddBreakpointRefusesConditionWithoutCode(t *testing.T) { + srv, err := dbgp.NewServer(dbgp.Config{Addr: "127.0.0.1:0"}) + if err != nil { + t.Fatal(err) + } + defer srv.Close() + tl := &tools{srv: srv} + _, _, err = tl.addBreakpoint(context.Background(), nil, addBreakpointInput{File: "/app/a.php", Line: 3, Condition: "true"}) + if err == nil || !strings.Contains(err.Error(), "-no-eval") { + t.Errorf("addBreakpoint with a condition = %v, want refused for -no-eval", err) + } + if bps := srv.Breakpoints(); len(bps) != 0 { + t.Errorf("breakpoints = %+v, want none", bps) + } +} From 61cf1160bc7b504d05933a0bd600f69440534b49 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 10:32:02 +0200 Subject: [PATCH 05/19] Connection limits --- cmd/dbgp/mcp.go | 15 +++++- cmd/dbgp/mcp_code.go | 4 +- cmd/dbgp/mcp_inspect.go | 8 ++-- cmd/dbgp/mcp_output.go | 21 ++------ cmd/dbgp/mcp_sessions.go | 59 +++++++++++++++-------- cmd/dbgp/mcp_sessions_test.go | 75 ++++++++++++++++++++++++++++- cmd/dbgp/repl.go | 16 ++++++- cmd/dbgp/repl_test.go | 23 +++++---- server.go | 54 +++++++++++++++++---- server_test.go | 90 +++++++++++++++++++++++++++++++++++ session.go | 3 ++ 11 files changed, 304 insertions(+), 64 deletions(-) diff --git a/cmd/dbgp/mcp.go b/cmd/dbgp/mcp.go index 5da359c..ef7ecf0 100644 --- a/cmd/dbgp/mcp.go +++ b/cmd/dbgp/mcp.go @@ -36,6 +36,12 @@ continue. output and warnings show what the script printed and the PHP warnings it raised. End every session with stop or detach when you are done with it: until then its PHP process or web server worker is held. +The session tools act on the session wait_for_session last returned, or the +last one you named by id. Each session shows the address PHP connected from: +127.0.0.1 or ::1 for PHP on this machine. dbgp accepts PHP connections from +every network this machine is on, so stop any session that is not from where +your PHP runs, and do not trust what it shows. + Long results come in parts: lists in pages, see page and pages, and long text when more or next is set. Ask only for the parts you need. @@ -305,11 +311,15 @@ type tools struct { // allowCode allows what runs PHP code in the debugged script. allowCode bool - // mu guards the read positions of output and warnings, per session id: - // those tools return only what is new since their last call. + // mu guards the fields below. output and warnings return only what is + // new since their last call, so they keep read positions per session id. mu sync.Mutex outputFrom map[int]int warningsFrom map[int]int + // selected is the session the tools act on by default: the one + // wait_for_session last returned or a tool was last given by id. Not + // simply the newest, which anything that connects would become. + selected int } type noInput struct{} @@ -324,6 +334,7 @@ type statusOutput struct { type sessionInfo struct { ID int `json:"id" jsonschema:"session id, used by the tools that act on a session"` + From string `json:"from" jsonschema:"the address PHP connected from: 127.0.0.1 or ::1 for PHP on this machine"` Script string `json:"script" jsonschema:"local path of the script being debugged"` Status string `json:"status" jsonschema:"starting, running, break (stopped), stopping (script finished) or ended"` File string `json:"file,omitempty" jsonschema:"local path of the file where execution is stopped, when status is break"` diff --git a/cmd/dbgp/mcp_code.go b/cmd/dbgp/mcp_code.go index 440eeb6..2d78efb 100644 --- a/cmd/dbgp/mcp_code.go +++ b/cmd/dbgp/mcp_code.go @@ -31,7 +31,7 @@ func (t *tools) addCodeTools(server *mcp.Server) { } type evalInput struct { - Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the newest session that has not ended"` + Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the one wait_for_session last returned or you last named"` Code string `json:"code" jsonschema:"a PHP expression, e.g. count($items) or $user->getName(); it sees the variables of the frame where execution stopped"` Page int `json:"page,omitempty" jsonschema:"the page of the result's elements or properties, from 0 (the default); see pages"` } @@ -84,7 +84,7 @@ func (t *tools) eval(_ context.Context, _ *mcp.CallToolRequest, in evalInput) (* } type setVariableInput struct { - Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the newest session that has not ended"` + Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the one wait_for_session last returned or you last named"` Name string `json:"name" jsonschema:"the variable, or a path into one, e.g. $count or $user[\"name\"]"` Value string `json:"value" jsonschema:"a PHP expression for the new value, e.g. 42, 'text' or $a * 2"` Depth int `json:"depth,omitempty" jsonschema:"the stack frame: 0 (the default) is where execution stopped, 1 its caller, and so on; see stack"` diff --git a/cmd/dbgp/mcp_inspect.go b/cmd/dbgp/mcp_inspect.go index b9a1928..44bd9f6 100644 --- a/cmd/dbgp/mcp_inspect.go +++ b/cmd/dbgp/mcp_inspect.go @@ -48,7 +48,7 @@ type frameInfo struct { } type stackInput struct { - Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the newest session that has not ended"` + Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the one wait_for_session last returned or you last named"` Page int `json:"page,omitempty" jsonschema:"the page of frames, from 0 (the default); see pages"` } @@ -81,7 +81,7 @@ func (t *tools) stack(_ context.Context, _ *mcp.CallToolRequest, in stackInput) // frameInput picks where to look for variables. type frameInput struct { - Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the newest session that has not ended"` + Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the one wait_for_session last returned or you last named"` Depth int `json:"depth,omitempty" jsonschema:"the stack frame: 0 (the default) is where execution stopped, 1 its caller, and so on; see stack"` Context int `json:"context,omitempty" jsonschema:"0 (the default) local variables, 1 superglobals such as $_SERVER and $_GET, 2 user-defined constants"` Page int `json:"page,omitempty" jsonschema:"the page of variables, from 0 (the default); see pages"` @@ -164,7 +164,7 @@ func (t *tools) variables(_ context.Context, _ *mcp.CallToolRequest, in frameInp } type variableInput struct { - Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the newest session that has not ended"` + Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the one wait_for_session last returned or you last named"` Name string `json:"name" jsonschema:"the variable, or a path into one, e.g. $user, $user[\"name\"] or $obj->cache"` Depth int `json:"depth,omitempty" jsonschema:"the stack frame: 0 (the default) is where execution stopped, 1 its caller, and so on; see stack"` Context int `json:"context,omitempty" jsonschema:"0 (the default) local variables, 1 superglobals such as $_SERVER and $_GET, 2 user-defined constants"` @@ -204,7 +204,7 @@ func (t *tools) variable(_ context.Context, _ *mcp.CallToolRequest, in variableI } type variableValueInput struct { - Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the newest session that has not ended"` + Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the one wait_for_session last returned or you last named"` Name string `json:"name" jsonschema:"the variable, or a path into one, e.g. $html or $page[\"body\"]"` Depth int `json:"depth,omitempty" jsonschema:"the stack frame: 0 (the default) is where execution stopped, 1 its caller, and so on; see stack"` Context int `json:"context,omitempty" jsonschema:"0 (the default) local variables, 1 superglobals such as $_SERVER and $_GET, 2 user-defined constants"` diff --git a/cmd/dbgp/mcp_output.go b/cmd/dbgp/mcp_output.go index 94c77c9..567f4eb 100644 --- a/cmd/dbgp/mcp_output.go +++ b/cmd/dbgp/mcp_output.go @@ -44,21 +44,8 @@ func (t *tools) addOutputTools(server *mcp.Server) { }, t.warnings) } -// latestSession returns the session with the given id, or by default the -// newest session, even one that has ended: what it printed is still there. -func (t *tools) latestSession(id int) (*dbgp.Session, error) { - if id != 0 { - return t.session(id) - } - sessions := t.srv.Sessions() - if len(sessions) == 0 { - return nil, errors.New("no session: call listen, start the PHP code, then wait_for_session") - } - return sessions[len(sessions)-1], nil -} - type sourceInput struct { - Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the newest session that has not ended"` + Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the one wait_for_session last returned or you last named"` File string `json:"file,omitempty" jsonschema:"the file, as a path on this machine; defaults to where the session stopped, or its script"` Line int `json:"line,omitempty" jsonschema:"the line to show the code around; defaults to where the session stopped, or 1 for another file"` Around int `json:"around,omitempty" jsonschema:"how many lines to show before and after the line: 5 by default, at most 100"` @@ -131,7 +118,7 @@ func (t *tools) source(_ context.Context, _ *mcp.CallToolRequest, in sourceInput } type readInput struct { - Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the newest session, even one that has ended"` + Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the one wait_for_session last returned or you last named, even if it has ended"` All bool `json:"all,omitempty" jsonschema:"return everything kept so far, not only what is new since the last call"` } @@ -158,7 +145,7 @@ type outputOutput struct { } func (t *tools) output(_ context.Context, _ *mcp.CallToolRequest, in readInput) (*mcp.CallToolResult, outputOutput, error) { - s, err := t.latestSession(in.Session) + s, err := t.session(in.Session) if err != nil { return nil, outputOutput{}, err } @@ -187,7 +174,7 @@ type warningsOutput struct { } func (t *tools) warnings(_ context.Context, _ *mcp.CallToolRequest, in readInput) (*mcp.CallToolResult, warningsOutput, error) { - s, err := t.latestSession(in.Session) + s, err := t.session(in.Session) if err != nil { return nil, warningsOutput{}, err } diff --git a/cmd/dbgp/mcp_sessions.go b/cmd/dbgp/mcp_sessions.go index 711006a..f467c3a 100644 --- a/cmd/dbgp/mcp_sessions.go +++ b/cmd/dbgp/mcp_sessions.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net" "time" "github.com/google/jsonschema-go/jsonschema" @@ -34,8 +35,8 @@ func (t *tools) addSessionTools(server *mcp.Server) { mcp.AddTool(server, &mcp.Tool{ Name: "wait_for_session", Description: "Wait for PHP to connect and return the session, stopped at the start of the script. " + - "Returns the oldest session this tool has not returned yet, so one that connected earlier is not missed. " + - "Call listen first, then start the PHP code.", + "Returns the oldest session this tool has not returned yet, so one that connected earlier is not missed, " + + "and makes it the session the other tools act on by default. Call listen first, then start the PHP code.", Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true}, }, t.waitForSession) mcp.AddTool(server, &mcp.Tool{ @@ -71,7 +72,7 @@ func (t *tools) sessionList() []sessionInfo { func toSessionInfo(s *dbgp.Session) sessionInfo { st := s.State() - info := sessionInfo{ID: s.ID(), Script: s.Script(), Status: st.Status} + info := sessionInfo{ID: s.ID(), From: remoteHost(s), Script: s.Script(), Status: st.Status} if ended(st) { info.Status = "ended" } else if st.Status == dbgp.StatusBreak { @@ -80,22 +81,40 @@ func toSessionInfo(s *dbgp.Session) sessionInfo { return info } -// session returns the session with the given id, or by default the newest -// session that has not ended. +// remoteHost is the address a session's engine connected from, without +// the port. +func remoteHost(s *dbgp.Session) string { + addr := s.RemoteAddr().String() + if host, _, err := net.SplitHostPort(addr); err == nil { + return host + } + return addr +} + +// session returns the session with the given id, which it selects, or by +// default the selected session, even one that has ended. func (t *tools) session(id int) (*dbgp.Session, error) { - if id != 0 { - if s := t.srv.Session(id); s != nil { - return s, nil + t.mu.Lock() + defer t.mu.Unlock() + if id == 0 { + if t.selected == 0 { + return nil, errors.New("no session: call listen, start the PHP code, then wait_for_session") } - return nil, fmt.Errorf("no session %d", id) + id = t.selected } - sessions := t.srv.Sessions() - for i := len(sessions) - 1; i >= 0; i-- { - if !ended(sessions[i].State()) { - return sessions[i], nil - } + s := t.srv.Session(id) + if s == nil { + return nil, fmt.Errorf("no session %d", id) } - return nil, errors.New("no session: call listen, start the PHP code, then wait_for_session") + t.selected = id + return s, nil +} + +// selectSession makes s the session the tools act on by default. +func (t *tools) selectSession(s *dbgp.Session) { + t.mu.Lock() + defer t.mu.Unlock() + t.selected = s.ID() } // waitDuration converts a wait in seconds, 0 meaning the default. @@ -138,6 +157,7 @@ func (t *tools) waitForSession(ctx context.Context, _ *mcp.CallToolRequest, in w done, cancel := context.WithCancel(ctx) cancel() if s, err := t.srv.WaitForSession(done); err == nil { + t.selectSession(s) info := toSessionInfo(s) return nil, waitForSessionOutput{Connected: true, Session: &info}, nil } @@ -152,16 +172,17 @@ func (t *tools) waitForSession(ctx context.Context, _ *mcp.CallToolRequest, in w if err != nil { return nil, waitForSessionOutput{}, err } + t.selectSession(s) info := toSessionInfo(s) return nil, waitForSessionOutput{Connected: true, Session: &info}, nil } type sessionInput struct { - Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the newest session that has not ended"` + Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the one wait_for_session last returned or you last named"` } type continueInput struct { - Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the newest session that has not ended"` + Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the one wait_for_session last returned or you last named"` Command string `json:"command,omitempty" jsonschema:"run (the default) runs to the next breakpoint or the end of the script; step_into, step_over and step_out step one statement, into, over or out of a function call"` Wait float64 `json:"wait,omitempty" jsonschema:"seconds to wait for the script to stop, 30 by default and at most 600"` } @@ -203,7 +224,7 @@ func sessionError(s *dbgp.Session, err error) error { case errors.Is(err, dbgp.ErrRunning): return fmt.Errorf("session %d is still running: call wait_for_stop first", s.ID()) case ended(s.State()): - return fmt.Errorf("session %d has ended", s.ID()) + return fmt.Errorf("session %d has ended: wait_for_session returns the next one", s.ID()) } return err } @@ -229,7 +250,7 @@ func (t *tools) continueSession(ctx context.Context, _ *mcp.CallToolRequest, in } type waitForStopInput struct { - Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the newest session that has not ended"` + Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the one wait_for_session last returned or you last named"` Wait float64 `json:"wait,omitempty" jsonschema:"seconds to wait for the script to stop, 30 by default and at most 600"` } diff --git a/cmd/dbgp/mcp_sessions_test.go b/cmd/dbgp/mcp_sessions_test.go index 27b62f7..1a6a9df 100644 --- a/cmd/dbgp/mcp_sessions_test.go +++ b/cmd/dbgp/mcp_sessions_test.go @@ -7,6 +7,8 @@ import ( "strings" "testing" "time" + + dbgp "github.com/php-debugger/dbgp-client" ) // startPHP runs php with the debugger pointed at port, killed at cleanup. @@ -110,8 +112,9 @@ func TestMCPDebuggingRun(t *testing.T) { if msg := callToolError(t, cs, "continue", map[string]any{"session": 1}); !strings.Contains(msg, "session 1 has ended") { t.Errorf("continue after stop: %q", msg) } - if msg := callToolError(t, cs, "continue", nil); !strings.Contains(msg, "no session") { - t.Errorf("continue with no live session: %q", msg) + // The ended session stays selected, rather than whatever connects next. + if msg := callToolError(t, cs, "continue", nil); !strings.Contains(msg, "session 1 has ended: wait_for_session returns the next one") { + t.Errorf("continue after the selected session ended: %q", msg) } } @@ -148,3 +151,71 @@ func TestMCPRunningSession(t *testing.T) { t.Errorf("detach = %+v", st) } } + +// waitSessions waits up to 10s for srv to have n sessions. +func waitSessions(t *testing.T, srv *dbgp.Server, n int) { + t.Helper() + for deadline := time.Now().Add(10 * time.Second); len(srv.Sessions()) < n; { + if time.Now().After(deadline) { + t.Fatalf("%d sessions, want %d", len(srv.Sessions()), n) + } + time.Sleep(20 * time.Millisecond) + } +} + +// The session tools act on the session the agent picked, not the newest: +// anything that can reach dbgp could connect and become that. Sessions show +// where they connected from. +func TestMCPSelectedSession(t *testing.T) { + requireEngine(t) + cs, srv := newTestMCPOn(t, phpAddr(0), true) + callTool(t, cs, "listen", nil, &listenOutput{}) + const script = `echo "hi\n";` + startPHP(t, srv.Port(), "-r", script) + waitSessions(t, srv, 1) + + // Connected, but not picked yet. + if msg := callToolError(t, cs, "continue", nil); !strings.Contains(msg, "no session: call listen") { + t.Errorf("continue before wait_for_session: %q", msg) + } + var waited waitForSessionOutput + callTool(t, cs, "wait_for_session", map[string]any{"wait": 10}, &waited) + if !waited.Connected || waited.Session.ID != 1 || waited.Session.From != "127.0.0.1" { + t.Fatalf("wait_for_session = %+v, session %+v", waited, waited.Session) + } + + // Another script connects, from this machine's network address if it + // has one, as PHP elsewhere would. + host := lanAddr() + if host == "" { + host = "127.0.0.1" + } + startPHP(t, srv.Port(), "-dxdebug.client_host="+host, "-r", script) + waitSessions(t, srv, 2) + var status statusOutput + callTool(t, cs, "status", nil, &status) + if len(status.Sessions) != 2 || status.Sessions[1].From != host { + t.Errorf("sessions = %+v, want the second from %s", status.Sessions, host) + } + if got := fromElsewhere(srv.Session(1)); got != "" { + t.Errorf("fromElsewhere(session 1) = %q, want nothing for this machine", got) + } + if want := " from " + host; host != "127.0.0.1" && fromElsewhere(srv.Session(2)) != want { + t.Errorf("fromElsewhere(session 2) = %q, want %q", fromElsewhere(srv.Session(2)), want) + } + + // The newer session does not take over; naming it selects it. + var st stateOutput + callTool(t, cs, "continue", nil, &st) + if st.Session != 1 { + t.Errorf("continue acted on session %d, want 1", st.Session) + } + callTool(t, cs, "continue", map[string]any{"session": 2}, &st) + if st.Session != 2 { + t.Errorf("continue with session 2 acted on session %d", st.Session) + } + callTool(t, cs, "stop", nil, &st) + if st.Session != 2 || st.Status != "ended" { + t.Errorf("stop = %+v, want session 2 ended", st) + } +} diff --git a/cmd/dbgp/repl.go b/cmd/dbgp/repl.go index 947d525..4ed803e 100644 --- a/cmd/dbgp/repl.go +++ b/cmd/dbgp/repl.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "math" + "net" "os" "path/filepath" "slices" @@ -107,13 +108,24 @@ func (r *repl) announceSessions() { r.onPrompt(r.prompt()) } } - r.printf("\n[session %d connected: %s%s]\n", s.ID(), s.Script(), note) + r.printf("\n[session %d connected%s: %s%s]\n", s.ID(), fromElsewhere(s), s.Script(), note) for _, w := range s.SetupWarnings() { r.printf("[session %d warning: %s]\n", s.ID(), w) } } } +// fromElsewhere names the address a session connected from, unless it is +// this machine: dbgp accepts PHP from every network the machine is on, so a +// session from elsewhere may not be the user's. +func fromElsewhere(s *dbgp.Session) string { + host := remoteHost(s) + if ip := net.ParseIP(host); ip != nil && ip.IsLoopback() { + return "" + } + return " from " + host +} + // cancelOnInterrupt cancels the running command on Ctrl-C. func (r *repl) cancelOnInterrupt(interrupts <-chan os.Signal) { for range interrupts { @@ -382,7 +394,7 @@ func (r *repl) cmdSessions(context.Context, string) error { if s == cur { mark = "*" } - r.printf("%s %d %-9s %s\n", mark, s.ID(), describeStatus(s.State()), s.Script()) + r.printf("%s %d %-9s %s%s\n", mark, s.ID(), describeStatus(s.State()), s.Script(), fromElsewhere(s)) } return nil } diff --git a/cmd/dbgp/repl_test.go b/cmd/dbgp/repl_test.go index e232790..7afb36d 100644 --- a/cmd/dbgp/repl_test.go +++ b/cmd/dbgp/repl_test.go @@ -572,14 +572,9 @@ func TestPHPAddrIsEveryAddress(t *testing.T) { } defer srv.Close() var hosts []string - addrs, _ := net.InterfaceAddrs() - for _, a := range addrs { - if ipnet, ok := a.(*net.IPNet); ok && ipnet.IP.To4() != nil && !ipnet.IP.IsLoopback() && !ipnet.IP.IsLinkLocalUnicast() { - hosts = append(hosts, ipnet.IP.String()) - break - } - } - if len(hosts) == 0 { + if lan := lanAddr(); lan != "" { + hosts = append(hosts, lan) + } else { t.Log("no network address besides loopback: checking loopback only") } for _, host := range append(hosts, "127.0.0.1", "::1") { @@ -595,3 +590,15 @@ func TestPHPAddrIsEveryAddress(t *testing.T) { _ = conn.Close() } } + +// lanAddr returns an IPv4 address of this machine other than loopback, or +// "" if it has none. +func lanAddr() string { + addrs, _ := net.InterfaceAddrs() + for _, a := range addrs { + if ipnet, ok := a.(*net.IPNet); ok && ipnet.IP.To4() != nil && !ipnet.IP.IsLoopback() && !ipnet.IP.IsLinkLocalUnicast() { + return ipnet.IP.String() + } + } + return "" +} diff --git a/server.go b/server.go index 1790984..65672af 100644 --- a/server.go +++ b/server.go @@ -56,6 +56,18 @@ type Config struct { PathMap []PathMapping } +// Limits on connections, so that a flood of them, from anything that can +// reach the port, cannot exhaust memory. PHP connects once per script run +// or request, and holds its connection while it is debugged. +const ( + // maxHandshakes bounds connections that have not sent their init + // packet yet; each may take Config.InitTimeout to do so. + maxHandshakes = 8 + // maxLiveSessions bounds sessions that have not ended. Beyond it, new + // scripts are detached, so they run on without the debugger. + maxLiveSessions = 16 +) + // Breakpoint is a breakpoint the server applies to every session, including // sessions that connect later. type Breakpoint struct { @@ -98,6 +110,9 @@ type Server struct { arrived chan struct{} nextBPID int breakpoints []Breakpoint + + // The connection limits; tests lower them. + maxHandshakes, maxLiveSessions int } // Listen creates a server and starts listening for engine connections. @@ -125,11 +140,13 @@ func NewServer(cfg Config) (*Server, error) { return nil, err } return &Server{ - cfg: cfg, - paths: paths, - addr: cfg.Addr, - handshakes: map[net.Conn]struct{}{}, - arrived: make(chan struct{}), + cfg: cfg, + paths: paths, + addr: cfg.Addr, + handshakes: map[net.Conn]struct{}{}, + arrived: make(chan struct{}), + maxHandshakes: maxHandshakes, + maxLiveSessions: maxLiveSessions, }, nil } @@ -280,7 +297,7 @@ func (s *Server) acceptLoop(listener net.Listener, done chan struct{}) { return } s.mu.Lock() - if s.closed { + if s.closed || len(s.handshakes) >= s.maxHandshakes { s.mu.Unlock() _ = conn.Close() continue @@ -317,12 +334,16 @@ func (s *Server) handshake(conn net.Conn) { if s.cfg.IDEKey != "" && init.IDEKey != s.cfg.IDEKey { // Not ours: let the script run on without the debugger. - _, _ = conn.Write([]byte("detach -i 1\x00")) - _ = conn.Close() + detachConn(conn) return } s.mu.Lock() + if s.liveSessionsLocked() >= s.maxLiveSessions { + s.mu.Unlock() + detachConn(conn) + return + } s.nextID++ id := s.nextID s.mu.Unlock() @@ -345,6 +366,23 @@ func (s *Server) handshake(conn net.Conn) { s.arrived = make(chan struct{}) } +// detachConn lets the script on conn run on without the debugger. +func detachConn(conn net.Conn) { + _, _ = conn.Write([]byte("detach -i 1\x00")) + _ = conn.Close() +} + +// liveSessionsLocked counts the sessions that have not ended. +func (s *Server) liveSessionsLocked() int { + live := 0 + for _, sess := range s.sessions { + if !sess.ended() { + live++ + } + } + return live +} + // setup enables notifications and output capture, then applies cfg.Features. // Only commands every engine implements are sent here: Xdebug and PHP // Debugger resume the script after an unimplemented command. diff --git a/server_test.go b/server_test.go index 0a3f6ff..11fe6ef 100644 --- a/server_test.go +++ b/server_test.go @@ -655,3 +655,93 @@ func TestBreakpointChangesSkipFinishedSessions(t *testing.T) { t.Errorf("removing the first breakpoint: %v", err) } } + +// setLimits lowers srv's connection limits for a test. +func setLimits(srv *Server, handshakes, live int) { + srv.mu.Lock() + defer srv.mu.Unlock() + srv.maxHandshakes, srv.maxLiveSessions = handshakes, live +} + +// Connections that have not sent their init packet are limited: beyond +// the limit, new ones are closed at once. +func TestServerLimitsHandshakes(t *testing.T) { + srv := newTestServer(t, Config{InitTimeout: time.Minute}) + setLimits(srv, 2, maxLiveSessions) + silent := []net.Conn{dialServer(t, srv), dialServer(t, srv)} + deadline := time.Now().Add(3 * time.Second) + for { + srv.mu.Lock() + n := len(srv.handshakes) + srv.mu.Unlock() + if n == 2 { + break + } + if time.Now().After(deadline) { + t.Fatalf("%d handshakes, want 2", n) + } + time.Sleep(10 * time.Millisecond) + } + + extra := dialServer(t, srv) + _ = extra.SetReadDeadline(time.Now().Add(3 * time.Second)) + if _, err := extra.Read(make([]byte, 1)); err != io.EOF { + t.Errorf("connection beyond the limit: read %v, want it closed", err) + } + + // Once they go, PHP can connect again. + for _, c := range silent { + _ = c.Close() + } + deadline = time.Now().Add(3 * time.Second) + for { + srv.mu.Lock() + n := len(srv.handshakes) + srv.mu.Unlock() + if n == 0 { + break + } + if time.Now().After(deadline) { + t.Fatalf("%d handshakes left after closing them", n) + } + time.Sleep(10 * time.Millisecond) + } + dialFakeEngine(t, srv, fixture(t, "init"), standardHandlers()) + waitSession(t, srv) +} + +// Live sessions are limited: beyond the limit, new scripts are detached +// so they run on without the debugger. +func TestServerLimitsLiveSessions(t *testing.T) { + srv := newTestServer(t, Config{}) + setLimits(srv, maxHandshakes, 1) + first := dialFakeEngine(t, srv, fixture(t, "init"), standardHandlers()) + waitSession(t, srv) + + extra := dialFakeEngine(t, srv, fixture(t, "init"), standardHandlers()) + if !extra.WaitClosed(3 * time.Second) { + t.Fatal("server kept a session beyond the limit") + } + if cmds := extra.Received(); len(cmds) != 1 || cmds[0].Raw != "detach -i 1" { + t.Errorf("session beyond the limit got %+v, want a single detach", cmds) + } + + // An ended session no longer counts. + first.Close() + select { + case <-srv.Session(1).Done(): + case <-time.After(3 * time.Second): + t.Fatal("session 1 did not end") + } + dialFakeEngine(t, srv, fixture(t, "init"), standardHandlers()) + if sess := waitSession(t, srv); sess.ID() != 2 { + t.Errorf("session %d, want 2", sess.ID()) + } +} + +func TestSessionRemoteAddr(t *testing.T) { + sess, _ := startFakeSession(t, standardHandlers()) + if host, _, _ := net.SplitHostPort(sess.RemoteAddr().String()); host != "127.0.0.1" { + t.Errorf("RemoteAddr() = %s, want 127.0.0.1", sess.RemoteAddr()) + } +} diff --git a/session.go b/session.go index 833ec4b..894afd1 100644 --- a/session.go +++ b/session.go @@ -195,6 +195,9 @@ func (s *Session) ID() int { return s.id } // Init returns the session's init packet. func (s *Session) Init() *InitPacket { return s.init } +// RemoteAddr returns the address the engine connected from. +func (s *Session) RemoteAddr() net.Addr { return s.conn.RemoteAddr() } + // pathMap returns the server's current path map. func (s *Session) pathMap() PathMap { if s.server == nil { From c7f282ad93a8c723356879e5899c366771b99f43 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 11:02:24 +0200 Subject: [PATCH 06/19] Check MCP server --- cmd/dbgp/mcp.go | 7 +- cmd/dbgp/mcp_daemon.go | 84 +++++++++++++++- cmd/dbgp/mcp_daemon_test.go | 189 ++++++++++++++++++++++++++++++++++++ 3 files changed, 276 insertions(+), 4 deletions(-) diff --git a/cmd/dbgp/mcp.go b/cmd/dbgp/mcp.go index ef7ecf0..679e6fa 100644 --- a/cmd/dbgp/mcp.go +++ b/cmd/dbgp/mcp.go @@ -142,7 +142,12 @@ func runMCP(args []string) int { case "start": return startDaemon(args) case "": - if st, err := running(); err == nil && st != nil { + st, err := running() + if err != nil { + // Serve the agent here instead. + fmt.Fprintln(os.Stderr, "dbgp:", err) + } + if st != nil { if len(args) > 0 { fmt.Fprintf(os.Stderr, "dbgp: using the background server at %s, with its own options: %s ignored\n", st.URL, strings.Join(args, " ")) diff --git a/cmd/dbgp/mcp_daemon.go b/cmd/dbgp/mcp_daemon.go index ff4a737..1ac2546 100644 --- a/cmd/dbgp/mcp_daemon.go +++ b/cmd/dbgp/mcp_daemon.go @@ -2,8 +2,11 @@ package main import ( "context" + "crypto/hmac" "crypto/rand" + "crypto/sha256" "crypto/subtle" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -26,6 +29,8 @@ import ( // find it; there is one per user. The state file also holds a token that // every request must carry: only the user can read the file, so other users // on the machine cannot use the server, which runs PHP code for its agents. +// The token also proves the server is the one the file records, before +// anything is sent to it: see verify. // serveCommand runs the background server in this process. dbgp mcp start // runs it; it is not meant to be typed. @@ -134,8 +139,14 @@ func removeState(pid int) { } } +// errNotOurs is returned by running when something other than the +// recorded server answers at its URL: the server died, and another +// program, perhaps another user's, took its port. +var errNotOurs = errors.New("something other than the dbgp MCP server answers") + // running returns the recorded server if it is answering, removing a state -// file left behind by one that is not. +// file left behind by one that is not. It fails with errNotOurs when +// something else answers in its place. func running() (*daemonState, error) { st, err := readState() if err != nil || st == nil { @@ -145,9 +156,72 @@ func running() (*daemonState, error) { removeState(st.PID) return nil, nil } + if !verify(st) { + removeState(st.PID) + return nil, fmt.Errorf("%w at %s, where it ran: ignoring it", errNotOurs, st.URL) + } return st, nil } +// identifyPath is where the server proves who it is; see verify. +const identifyPath = "/identify" + +// identity is the server's answer to a challenge. +type identity struct { + PID int `json:"pid"` + Proof string `json:"proof"` +} + +// identityProof answers challenge nonce for the server with pid: an HMAC +// keyed with the token, which only the user can read. +func identityProof(token, nonce string, pid int) string { + mac := hmac.New(sha256.New, []byte(token)) + fmt.Fprintf(mac, "dbgp identify\x00%s\x00%d", nonce, pid) + return hex.EncodeToString(mac.Sum(nil)) +} + +// verify reports whether the server at st.URL is the one st records, with +// its pid: it must answer a fresh challenge with a proof only a holder of +// st's token can give. The token itself is not sent, so a program that took +// the port of a server that died learns nothing from being asked. +func verify(st *daemonState) bool { + u, err := url.Parse(st.URL) + if err != nil || st.Token == "" { + return false + } + nonce := rand.Text() + u.Path, u.RawQuery = identifyPath, url.Values{"nonce": {nonce}}.Encode() + client := &http.Client{ + Timeout: 2 * time.Second, + // Only the recorded server may answer. + CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, + } + resp, err := client.Get(u.String()) + if err != nil { + return false + } + defer resp.Body.Close() + var id identity + if resp.StatusCode != http.StatusOK || json.NewDecoder(io.LimitReader(resp.Body, 4<<10)).Decode(&id) != nil { + return false + } + want := identityProof(st.Token, nonce, st.PID) + return id.PID == st.PID && hmac.Equal([]byte(id.Proof), []byte(want)) +} + +// identify answers verify's challenges for the server st. +func identify(st daemonState) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + nonce := req.URL.Query().Get("nonce") + if len(nonce) < 16 || len(nonce) > 256 { + http.Error(w, "nonce must be 16 to 256 characters", http.StatusBadRequest) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(identity{PID: st.PID, Proof: identityProof(st.Token, nonce, st.PID)}) + }) +} + // answering reports whether something accepts connections at rawURL. func answering(rawURL string) bool { u, err := url.Parse(rawURL) @@ -165,7 +239,10 @@ func answering(rawURL string) bool { // startDaemon starts the server in the background, running this program // with serveCommand and args, and waits for it to answer. func startDaemon(args []string) int { - if st, err := running(); err != nil { + if st, err := running(); errors.Is(err, errNotOurs) { + // Its state is gone: start a new one. + fmt.Fprintln(os.Stderr, err) + } else if err != nil { fmt.Fprintln(os.Stderr, err) return 1 } else if st != nil { @@ -198,7 +275,7 @@ func startDaemon(args []string) int { deadline := time.After(startTimeout) for { - if st, _ := readState(); st != nil && st.PID == cmd.Process.Pid && answering(st.URL) { + if st, _ := readState(); st != nil && st.PID == cmd.Process.Pid && verify(st) { fmt.Printf("dbgp MCP server running at %s (pid %d)\n"+ "Agents that start dbgp mcp now use it. Stop it with: dbgp mcp stop\n", st.URL, st.PID) return 0 @@ -379,6 +456,7 @@ func serveHTTP(ctx context.Context, server *mcp.Server, port int) error { &mcp.StreamableHTTPOptions{CrossOriginProtection: http.NewCrossOriginProtection()}) mux := http.NewServeMux() mux.Handle("/mcp", requireToken(st.Token, handler)) + mux.Handle(identifyPath, identify(st)) hs := &http.Server{Handler: mux, ReadHeaderTimeout: 10 * time.Second} go func() { <-ctx.Done() diff --git a/cmd/dbgp/mcp_daemon_test.go b/cmd/dbgp/mcp_daemon_test.go index d3dc54b..0d278f2 100644 --- a/cmd/dbgp/mcp_daemon_test.go +++ b/cmd/dbgp/mcp_daemon_test.go @@ -2,6 +2,9 @@ package main import ( "context" + "encoding/json" + "errors" + "io" "net" "net/http" "net/http/httptest" @@ -12,6 +15,8 @@ import ( "runtime" "slices" "strings" + "sync" + "syscall" "testing" "time" @@ -349,3 +354,187 @@ func TestTokenClientSendsTokenOnlyToItsServer(t *testing.T) { t.Errorf("Authorization headers = %q, want %q", got, want) } } + +func TestIdentityProof(t *testing.T) { + base := identityProof("token", "nonce-0123456789", 42) + if identityProof("token", "nonce-0123456789", 42) != base { + t.Error("the same challenge gave another proof") + } + for name, other := range map[string]string{ + "token": identityProof("other", "nonce-0123456789", 42), + "nonce": identityProof("token", "nonce-9876543210", 42), + "pid": identityProof("token", "nonce-0123456789", 43), + } { + if other == base { + t.Errorf("another %s gave the same proof", name) + } + } +} + +// impostor is a server that took the port of a background server that +// died. It records every Authorization header it is sent. +type impostor struct { + *httptest.Server + mu sync.Mutex + auths []string +} + +func newImpostor(t *testing.T, handler http.HandlerFunc) *impostor { + t.Helper() + imp := &impostor{} + imp.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + imp.mu.Lock() + imp.auths = append(imp.auths, req.Header.Get("Authorization")) + imp.mu.Unlock() + handler(w, req) + })) + t.Cleanup(imp.Close) + return imp +} + +// sawToken reports whether the impostor was sent token. +func (imp *impostor) sawToken(token string) bool { + imp.mu.Lock() + defer imp.mu.Unlock() + for _, a := range imp.auths { + if strings.Contains(a, token) { + return true + } + } + return false +} + +// A server that answers at the recorded URL is used only if it proves it is +// the one recorded; otherwise its state file is stale. +func TestMCPDaemonImpostor(t *testing.T) { + const token = "secret-token" + pid := os.Getpid() + for name, handler := range map[string]http.HandlerFunc{ + "no identify": http.NotFound, + "wrong proof": func(w http.ResponseWriter, req *http.Request) { + _ = json.NewEncoder(w).Encode(identity{PID: pid, Proof: identityProof("guess", req.URL.Query().Get("nonce"), pid)}) + }, + "other pid": func(w http.ResponseWriter, req *http.Request) { + // Right for its own pid, which is not the recorded one. + _ = json.NewEncoder(w).Encode(identity{PID: pid + 1, Proof: identityProof(token, req.URL.Query().Get("nonce"), pid+1)}) + }, + "redirect": func(w http.ResponseWriter, req *http.Request) { + http.Redirect(w, req, "http://127.0.0.1:1/identify", http.StatusFound) + }, + } { + t.Run(name, func(t *testing.T) { + daemonEnv(t) + imp := newImpostor(t, handler) + if err := writeState(daemonState{PID: pid, URL: imp.URL + "/mcp", Token: token}); err != nil { + t.Fatal(err) + } + if st, err := running(); st != nil || !errors.Is(err, errNotOurs) { + t.Errorf("running() = %+v, %v; want errNotOurs", st, err) + } + if st, _ := readState(); st != nil { + t.Errorf("stale state kept: %+v", st) + } + if imp.sawToken(token) { + t.Error("the impostor was sent the token") + } + }) + } +} + +// The commands do not act on an impostor: stop does not signal the recorded +// process, and agents are served by dbgp mcp itself. +func TestMCPDaemonImpostorCommands(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("needs sleep and signal 0") + } + sleepPath, err := exec.LookPath("sleep") + if err != nil { + t.Skip("no sleep command") + } + bin := buildBinary(t) + const token = "secret-token" + // The recorded process: stop must leave it alone. + victim := exec.Command(sleepPath, "60") + if err := victim.Start(); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = victim.Process.Kill(); _ = victim.Wait() }) + imp := newImpostor(t, http.NotFound) + record := func() { + t.Helper() + if err := writeState(daemonState{PID: victim.Process.Pid, URL: imp.URL + "/mcp", Token: token}); err != nil { + t.Fatal(err) + } + } + daemonEnv(t) + + record() + if code, out := runDbgp(t, bin, "mcp", "stop"); code != 1 || !strings.Contains(out, "something other than the dbgp MCP server") { + t.Errorf("stop: exit %d\n%s", code, out) + } + if err := victim.Process.Signal(syscall.Signal(0)); err != nil { + t.Errorf("stop signalled the recorded process: %v", err) + } + record() + if code, out := runDbgp(t, bin, "mcp", "status"); code != 1 || !strings.Contains(out, "something other than the dbgp MCP server") { + t.Errorf("status: exit %d\n%s", code, out) + } + + record() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + var stderr strings.Builder + cmd := exec.CommandContext(ctx, bin, "mcp") + cmd.Stderr = &stderr + cs, err := mcp.NewClient(&mcp.Implementation{Name: "test", Version: "0"}, nil). + Connect(ctx, &mcp.CommandTransport{Command: cmd}, nil) + if err != nil { + t.Fatal(err) + } + var status statusOutput + callTool(t, cs, "status", nil, &status) + _ = cs.Close() + if !strings.Contains(stderr.String(), "something other than the dbgp MCP server") { + t.Errorf("stderr: %q", stderr.String()) + } + if imp.sawToken(token) { + t.Error("the impostor was sent the token") + } +} + +// The real server proves who it is without giving away its token. +func TestMCPDaemonIdentify(t *testing.T) { + bin := buildBinary(t) + daemonEnv(t) + url := startTestDaemon(t, bin) + st, err := readState() + if err != nil || st == nil { + t.Fatalf("state = %+v, %v", st, err) + } + if !verify(st) { + t.Fatal("the real server failed verify") + } + identifyURL := strings.TrimSuffix(url, "/mcp") + identifyPath + resp, err := http.Get(identifyURL + "?nonce=short") + if err != nil { + t.Fatal(err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("short nonce: status %d, want 400", resp.StatusCode) + } + const nonce = "0123456789abcdef0123" + resp, err = http.Get(identifyURL + "?nonce=" + nonce) + if err != nil { + t.Fatal(err) + } + body, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() + var id identity + if err := json.Unmarshal(body, &id); err != nil || id.PID != st.PID || id.Proof != identityProof(st.Token, nonce, st.PID) { + t.Errorf("identify = %s (%v)", body, err) + } + if strings.Contains(string(body), st.Token) { + t.Error("identify gave away the token") + } +} From 0fd9b284cd3fe2cf6fa414146a418972c706c88f Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 11:20:54 +0200 Subject: [PATCH 07/19] Update toolchain --- .github/workflows/govulncheck.yml | 26 ++++++++++++++++++++++++++ .github/workflows/release.yml | 29 +++++++++++++++++++---------- go.mod | 6 +++--- go.sum | 12 ++++++------ 4 files changed, 54 insertions(+), 19 deletions(-) create mode 100644 .github/workflows/govulncheck.yml diff --git a/.github/workflows/govulncheck.yml b/.github/workflows/govulncheck.yml new file mode 100644 index 0000000..69e24fb --- /dev/null +++ b/.github/workflows/govulncheck.yml @@ -0,0 +1,26 @@ +name: Vulnerabilities + +# Known vulnerabilities in the code dbgp calls, in its dependencies and in +# the standard library of the newest Go, which release builds use. Weekly +# too, since new ones are found in code that has not changed. +on: + pull_request: + push: + branches: [main] + schedule: + - cron: "0 6 * * 1" + +permissions: + contents: read + +jobs: + govulncheck: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v7 + with: + go-version: stable + check-latest: true + - name: govulncheck + run: go run golang.org/x/vuln/cmd/govulncheck@v1.9.0 ./... diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b3999bf..7c95851 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -15,15 +15,24 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v7 + # The newest Go, not go.mod's minimum: the binaries carry its standard + # library, so each release gets every security fix made so far. - uses: actions/setup-go@v7 with: - go-version-file: go.mod + go-version: stable + check-latest: true + + - name: Check for known vulnerabilities + run: go run golang.org/x/vuln/cmd/govulncheck@v1.9.0 ./... - name: Build and package all targets env: TAG: ${{ github.event.release.tag_name }} run: | - mkdir -p dist build + # Outside the checkout, so the binaries are not stamped as built + # from modified sources. + dist="$RUNNER_TEMP/dist" build="$RUNNER_TEMP/build" + mkdir -p "$dist" "$build" ver="${TAG#v}" # strip a leading "v" if present ldflags="-s -w -X main.version=${ver}" # Windows-on-ARM runs x64 under emulation, so one Windows build suffices. @@ -35,26 +44,26 @@ jobs: echo "building $osname/$arch" CGO_ENABLED=0 GOOS="$goos" GOARCH="$arch" \ - go build -trimpath -ldflags "$ldflags" -o "build/$bin" ./cmd/dbgp - chmod +x "build/$bin" + go build -trimpath -ldflags "$ldflags" -o "$build/$bin" ./cmd/dbgp + chmod +x "$build/$bin" # Package into an archive so the executable bit survives download # (raw GitHub release assets lose Unix permissions). base="dbgp-${osname}-${arch}" if [ "$goos" = "windows" ]; then - (cd build && zip -q "../dist/${base}.zip" "$bin") + (cd "$build" && zip -q "$dist/${base}.zip" "$bin") else - tar -C build -czf "dist/${base}.tar.gz" "$bin" + tar -C "$build" -czf "$dist/${base}.tar.gz" "$bin" fi - rm -f "build/$bin" + rm -f "$build/$bin" done # Attach the install scripts too, so releases/latest/download/install.sh # is a stable URL that serves the released version of the script. - cp scripts/install.sh scripts/install.ps1 dist/ - ls -la dist + cp scripts/install.sh scripts/install.ps1 "$dist/" + ls -la "$dist" - name: Attach binaries to the release env: GH_TOKEN: ${{ github.token }} TAG: ${{ github.event.release.tag_name }} - run: gh release upload "$TAG" dist/* --clobber + run: gh release upload "$TAG" "$RUNNER_TEMP"/dist/* --clobber diff --git a/go.mod b/go.mod index 1633fe0..eb0bdb7 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/php-debugger/dbgp-client -go 1.25.0 +go 1.26.0 require ( github.com/ergochat/readline v0.1.3 @@ -13,8 +13,8 @@ require ( github.com/segmentio/encoding v0.5.4 // indirect github.com/yosida95/uritemplate/v3 v3.0.2 // indirect golang.org/x/oauth2 v0.35.0 // indirect - golang.org/x/sync v0.21.0 // indirect + golang.org/x/sync v0.24.0 // indirect golang.org/x/sys v0.44.0 // indirect - golang.org/x/text v0.39.0 // indirect + golang.org/x/text v0.43.0 // indirect golang.org/x/time v0.15.0 // indirect ) diff --git a/go.sum b/go.sum index 87783b9..4acaa5c 100644 --- a/go.sum +++ b/go.sum @@ -16,13 +16,13 @@ github.com/yosida95/uritemplate/v3 v3.0.2 h1:Ed3Oyj9yrmi9087+NczuL5BwkIc4wvTb5zI github.com/yosida95/uritemplate/v3 v3.0.2/go.mod h1:ILOh0sOhIJR3+L/8afwt/kE++YT040gmv5BQTMR2HP4= golang.org/x/oauth2 v0.35.0 h1:Mv2mzuHuZuY2+bkyWXIHMfhNdJAdwW3FuWeCPYN5GVQ= golang.org/x/oauth2 v0.35.0/go.mod h1:lzm5WQJQwKZ3nwavOZ3IS5Aulzxi68dUSgRHujetwEA= -golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= -golang.org/x/sync v0.21.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA= +golang.org/x/sync v0.24.0/go.mod h1:sUUOizhqBxiL6pEWpqNLUiaJn1ShEbZ6BBqskPbjZm0= golang.org/x/sys v0.44.0 h1:ildZl3J4uzeKP07r2F++Op7E9B29JRUy+a27EibtBTQ= golang.org/x/sys v0.44.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= -golang.org/x/text v0.39.0 h1:UbZz4pLOvn600D6Oh6GGEI6VAmndrEBLv8/6BEXzyus= -golang.org/x/text v0.39.0/go.mod h1:3UwRclnC2g0TU9x8PZiyfOajCd1zaUNHF9cvqcQZ+ZM= +golang.org/x/text v0.43.0 h1:1QivrlhwAsnMOcqgOdl9my7s9OyZl8O5/UGUTJVXoWc= +golang.org/x/text v0.43.0/go.mod h1:hbSIYA/amXcRXTFSZqh8tWHEoqRvvbKPJ2uz/KU+a+s= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= -golang.org/x/tools v0.47.0 h1:7Kn5x/d1svx/PzryTsqeoZN4TZwqeH5pGWjefhLi/1Q= -golang.org/x/tools v0.47.0/go.mod h1:dFHnyTvFWY212G+h7ZY4Vsp/K3U4/7W9TyVaAul8uCA= +golang.org/x/tools v0.51.0 h1:k4Xc/1Om9jwkBJBo4NVLMSARBoWtK10mx+W5BnXCeAI= +golang.org/x/tools v0.51.0/go.mod h1:9eEncMayCV6zRMGhR5eZEC2iBx98qWcF1HZ9Z7wJOoA= From 7a8672467ca59110cc65314a474c9d3df83a9ba4 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 11:34:35 +0200 Subject: [PATCH 08/19] Terminal escape injection --- cmd/dbgp/repl.go | 9 ++++- cmd/dbgp/terminal.go | 60 ++++++++++++++++++++++++++++ cmd/dbgp/terminal_test.go | 84 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 cmd/dbgp/terminal.go create mode 100644 cmd/dbgp/terminal_test.go diff --git a/cmd/dbgp/repl.go b/cmd/dbgp/repl.go index 4ed803e..17cc876 100644 --- a/cmd/dbgp/repl.go +++ b/cmd/dbgp/repl.go @@ -42,8 +42,10 @@ type repl struct { notesFrom map[int]int // per session: notifications already shown } +// newREPL returns a REPL that writes to out, through safeText: what it +// prints comes largely from the debugged script. func newREPL(srv *dbgp.Server, out io.Writer) *repl { - return &repl{srv: srv, out: out, outputFrom: map[int]int{}, notesFrom: map[int]int{}} + return &repl{srv: srv, out: safeWriter{out}, outputFrom: map[int]int{}, notesFrom: map[int]int{}} } func (r *repl) printf(format string, args ...any) { @@ -70,6 +72,9 @@ func (r *repl) run(lines lineReader) { } } +// prompt returns the prompt, which shows the selected session's status. +// The terminal reader prints it itself, so it is made safe here: the status +// comes from the engine. func (r *repl) prompt() string { s := r.session() if s == nil { @@ -79,7 +84,7 @@ func (r *repl) prompt() string { if ended(st) { return fmt.Sprintf("(dbgp %d:ended) ", s.ID()) } - return fmt.Sprintf("(dbgp %d:%s) ", s.ID(), st.Status) + return safeText(fmt.Sprintf("(dbgp %d:%s) ", s.ID(), st.Status)) } func (r *repl) session() *dbgp.Session { diff --git a/cmd/dbgp/terminal.go b/cmd/dbgp/terminal.go new file mode 100644 index 0000000..75c0ed5 --- /dev/null +++ b/cmd/dbgp/terminal.go @@ -0,0 +1,60 @@ +package main + +import ( + "fmt" + "io" + "strings" + "unicode/utf8" +) + +// Most of what the command-line debugger prints comes from the debugged +// script and the engine: program output, variable values, file names and +// messages, often from a web request anyone could send. A terminal acts on +// control characters in text, e.g. escape sequences that rewrite the +// screen, set the window title or write to the clipboard, so those are +// shown as escapes instead. + +// safeWriter writes to w with safeText. +type safeWriter struct { + w io.Writer +} + +func (s safeWriter) Write(p []byte) (int, error) { + if _, err := io.WriteString(s.w, safeText(string(p))); err != nil { + return 0, err + } + return len(p), nil +} + +// safeText returns s with what a terminal would act on rather than show +// written as escapes such as \x1b: control characters other than newline +// and tab, invalid UTF-8, and the characters that reorder text. Line +// endings \r\n become \n. +func safeText(s string) string { + s = strings.ReplaceAll(s, "\r\n", "\n") + var b strings.Builder + b.Grow(len(s)) + for i := 0; i < len(s); { + r, size := utf8.DecodeRuneInString(s[i:]) + switch { + case r == utf8.RuneError && size == 1: + fmt.Fprintf(&b, `\x%02x`, s[i]) + case r == '\n' || r == '\t': + b.WriteRune(r) + case r < 0x20 || r == 0x7f: + fmt.Fprintf(&b, `\x%02x`, r) + case r >= 0x80 && r <= 0x9f, reordersText(r): + fmt.Fprintf(&b, `\u%04x`, r) + default: + b.WriteString(s[i : i+size]) + } + i += size + } + return b.String() +} + +// reordersText reports whether r is a bidirectional embedding, override or +// isolate, which can make text display in another order than it reads. +func reordersText(r rune) bool { + return (r >= '‪' && r <= '‮') || (r >= '⁦' && r <= '⁩') +} diff --git a/cmd/dbgp/terminal_test.go b/cmd/dbgp/terminal_test.go new file mode 100644 index 0000000..2be27a6 --- /dev/null +++ b/cmd/dbgp/terminal_test.go @@ -0,0 +1,84 @@ +package main + +import ( + "bytes" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func TestSafeText(t *testing.T) { + for in, want := range map[string]string{ + "plain text": "plain text", + "tabs\tand\nnewlines\n": "tabs\tand\nnewlines\n", + "windows\r\nlines\r\n": "windows\nlines\n", + "a lone\rreturn": `a lone\x0dreturn`, + "\x1b[2Jcleared": `\x1b[2Jcleared`, + "\x1b]52;c;cGF5bG9hZA==\x07": `\x1b]52;c;cGF5bG9hZA==\x07`, + "bell\a and nul\x00": `bell\x07 and nul\x00`, + "del\x7f": `del\x7f`, + "c1 \u009b2J": `c1 \u009b2J`, + "invalid \xff\xfe utf-8": `invalid \xff\xfe utf-8`, + "evil\u202egnp.php": `evil\u202egnp.php`, + "isolate \u2066x\u2069": `isolate \u2066x\u2069`, + "café 日本語 🐘 \ufffd": "café 日本語 🐘 \ufffd", + "$user[\"name\"] = 'Ana'": "$user[\"name\"] = 'Ana'", + "/var/www/html/index.php:12": "/var/www/html/index.php:12", + } { + if got := safeText(in); got != want { + t.Errorf("safeText(%q) = %q, want %q", in, got, want) + } + } +} + +// Everything the REPL prints goes through safeText, including the prompt. +func TestREPLPrintsSafely(t *testing.T) { + var out bytes.Buffer + r := newREPL(nil, &out) + r.printf("%s\n", "\x1b]0;title\x07") + if got := out.String(); got != `\x1b]0;title\x07`+"\n" { + t.Errorf("printed %q", got) + } +} + +// Escape sequences from a script, in what it prints and in its variables, +// reach the terminal as text, not as escapes it acts on. +func TestREPLShowsScriptEscapesAsText(t *testing.T) { + requireEngine(t) + script := filepath.Join(t.TempDir(), "escapes.php") + code := " Date: Sun, 18 Oct 2026 11:54:34 +0200 Subject: [PATCH 09/19] Supply chain --- .github/dependabot.yml | 11 +++++++++++ .github/workflows/govulncheck.yml | 6 ++++-- .github/workflows/release.yml | 23 +++++++++++++++++++---- .github/workflows/tests.yml | 16 +++++++++++----- README.md | 2 ++ scripts/install.ps1 | 22 +++++++++++++++++----- scripts/install.sh | 25 +++++++++++++++++++++---- 7 files changed, 85 insertions(+), 20 deletions(-) create mode 100644 .github/dependabot.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..8277240 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,11 @@ +# Keeps the actions, pinned by commit, and the Go modules up to date. +version: 2 +updates: + - package-ecosystem: github-actions + directory: / + schedule: + interval: weekly + - package-ecosystem: gomod + directory: / + schedule: + interval: weekly diff --git a/.github/workflows/govulncheck.yml b/.github/workflows/govulncheck.yml index 69e24fb..c6445b1 100644 --- a/.github/workflows/govulncheck.yml +++ b/.github/workflows/govulncheck.yml @@ -17,8 +17,10 @@ jobs: govulncheck: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 - - uses: actions/setup-go@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version: stable check-latest: true diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 7c95851..fd20800 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -7,17 +7,22 @@ on: types: [published] permissions: - contents: write # needed to upload release assets + contents: write # to upload release assets + id-token: write # to sign the provenance attestations + attestations: write # to store them jobs: release: name: Build & attach binaries runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + # Actions are pinned by commit, which a tag is not; Dependabot updates them. + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false # The newest Go, not go.mod's minimum: the binaries carry its standard # library, so each release gets every security fix made so far. - - uses: actions/setup-go@v7 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version: stable check-latest: true @@ -60,7 +65,17 @@ jobs: # Attach the install scripts too, so releases/latest/download/install.sh # is a stable URL that serves the released version of the script. cp scripts/install.sh scripts/install.ps1 "$dist/" - ls -la "$dist" + # The install scripts check downloads against these. + (cd "$dist" && sha256sum -- * > "$RUNNER_TEMP/SHA256SUMS") + mv "$RUNNER_TEMP/SHA256SUMS" "$dist/" + cat "$dist/SHA256SUMS" + + # Records that each file was built here, from this commit: verify one + # with gh attestation verify FILE --repo php-debugger/dbgp-client. + - name: Attest build provenance + uses: actions/attest-build-provenance@4d101475d8b20a2381f78447822ac1eab6504dd8 # v4.2.2 + with: + subject-checksums: ${{ runner.temp }}/dist/SHA256SUMS - name: Attach binaries to the release env: diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index d8362b1..3ca25ab 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -16,8 +16,10 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 - - uses: actions/setup-go@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: go.mod - name: gofmt @@ -39,8 +41,10 @@ jobs: matrix: php: ['8.2', '8.3', '8.4', '8.5'] steps: - - uses: actions/checkout@v7 - - uses: actions/setup-go@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: go.mod # Self-contained PHP interpreter with PHP Debugger compiled in, from the @@ -74,7 +78,9 @@ jobs: matrix: os: [ubuntu-latest, macos-latest, windows-latest] steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - name: install.sh if: runner.os != 'Windows' run: | diff --git a/README.md b/README.md index 10aa6b3..4f6dbd8 100644 --- a/README.md +++ b/README.md @@ -31,6 +31,8 @@ go install github.com/php-debugger/dbgp-client/cmd/dbgp@latest `dbgp -version` shows the installed version. +The install scripts check the download against the release's `SHA256SUMS` before installing it. Each release file also has a build provenance attestation, which shows it was built by this repository's release workflow; to check one you downloaded yourself, run `gh attestation verify FILE --repo php-debugger/dbgp-client`. + To be debugged, PHP needs [PHP Debugger](https://php-debugger.dev) or Xdebug: `php -v` shows which one it has. ## Debugging with AI agents diff --git a/scripts/install.ps1 b/scripts/install.ps1 index 6b460f9..b611cfe 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -3,8 +3,9 @@ dbgp installer (Windows). .DESCRIPTION - Downloads the latest dbgp release for Windows, installs it system-wide and - adds it to the system PATH. Run it from PowerShell as administrator. + Downloads the latest dbgp release for Windows, checks it against the + release's SHA256SUMS, installs it system-wide and adds it to the system + PATH. Run it from PowerShell as administrator. Usage: powershell -c "irm https://github.com/php-debugger/dbgp-client/releases/latest/download/install.ps1 | iex" @@ -31,11 +32,11 @@ if (-not $env:INSTALL_DIR -and -not $IsAdmin) { $Asset = "dbgp-windows-amd64.zip" if ($env:VERSION) { - $Url = "https://github.com/$Repo/releases/download/$($env:VERSION)/$Asset" + $Base = "https://github.com/$Repo/releases/download/$($env:VERSION)" Write-Host "Installing dbgp $($env:VERSION) (windows/amd64)" } else { # The releases/latest/download URL always resolves to the newest release. - $Url = "https://github.com/$Repo/releases/latest/download/$Asset" + $Base = "https://github.com/$Repo/releases/latest/download" Write-Host "Installing dbgp (latest, windows/amd64)" } @@ -43,7 +44,18 @@ $tmp = Join-Path $env:TEMP ("dbgp-" + [System.Guid]::NewGuid().ToString()) New-Item -ItemType Directory -Path $tmp | Out-Null try { $zip = Join-Path $tmp $Asset - Invoke-WebRequest -UseBasicParsing -Uri $Url -OutFile $zip + Invoke-WebRequest -UseBasicParsing -Uri "$Base/$Asset" -OutFile $zip + $sums = Join-Path $tmp "SHA256SUMS" + Invoke-WebRequest -UseBasicParsing -Uri "$Base/SHA256SUMS" -OutFile $sums + # Lines are "checksum file", as sha256sum writes them. + $want = Get-Content $sums | ForEach-Object { + $sum, $name = $_ -split '\s+', 2 + if ($name -and $name.TrimStart('*') -eq $Asset) { $sum } + } | Select-Object -First 1 + if (-not $want) { throw "SHA256SUMS has no checksum for $Asset" } + $got = (Get-FileHash -Algorithm SHA256 -Path $zip).Hash + # -ne ignores case: Get-FileHash gives upper case, sha256sum lower. + if ($got -ne $want) { throw "$Asset does not match its checksum (got $got, want $want): not installing it" } Expand-Archive -Path $zip -DestinationPath $tmp -Force $src = Join-Path $tmp $Bin diff --git a/scripts/install.sh b/scripts/install.sh index 0c5234e..347d90c 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -11,6 +11,9 @@ # # sudo is used only when INSTALL_DIR is not writable. # +# The download is checked against the release's SHA256SUMS before it is +# installed. +# # Because the archive is fetched with curl/wget (not a browser), the binary is # not quarantined, so macOS Gatekeeper does not block it. As a safety net the # script also clears the quarantine attribute if present. @@ -49,18 +52,32 @@ esac ASSET="dbgp-${OS}-${ARCH}.tar.gz" if [ -n "$VERSION" ]; then - URL="https://github.com/$REPO/releases/download/$VERSION/$ASSET" + BASE="https://github.com/$REPO/releases/download/$VERSION" info "Installing $BIN $VERSION ($OS/$ARCH)" else # The releases/latest/download URL always resolves to the newest release. - URL="https://github.com/$REPO/releases/latest/download/$ASSET" + BASE="https://github.com/$REPO/releases/latest/download" info "Installing $BIN (latest, $OS/$ARCH)" fi -# --- download + extract --- +# --- checksum tool --- +if have sha256sum; then + sha256() { sha256sum "$1" | awk '{ print $1 }'; } +elif have shasum; then + sha256() { shasum -a 256 "$1" | awk '{ print $1 }'; } +else + fail "need sha256sum or shasum to check the download" +fi + +# --- download, check + extract --- tmp="$(mktemp -d)" trap 'rm -rf "$tmp"' EXIT INT TERM -download "$URL" "$tmp/$ASSET" || fail "failed to download $URL" +download "$BASE/$ASSET" "$tmp/$ASSET" || fail "failed to download $BASE/$ASSET" +download "$BASE/SHA256SUMS" "$tmp/SHA256SUMS" || fail "failed to download $BASE/SHA256SUMS" +want="$(awk -v f="$ASSET" '$2 == f || $2 == "*" f { print $1; exit }' "$tmp/SHA256SUMS")" +[ -n "$want" ] || fail "SHA256SUMS has no checksum for $ASSET" +got="$(sha256 "$tmp/$ASSET")" +[ "$got" = "$want" ] || fail "$ASSET does not match its checksum (got $got, want $want): not installing it" tar -xzf "$tmp/$ASSET" -C "$tmp" || fail "failed to extract $ASSET" [ -f "$tmp/$BIN" ] || fail "archive did not contain $BIN" chmod +x "$tmp/$BIN" From ceb03558d7612dc56e926398abf7879994271092 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 16:49:31 +0200 Subject: [PATCH 10/19] install.ps1 PATH --- .github/workflows/tests.yml | 14 +++++++++++++- README.md | 2 +- scripts/install.ps1 | 17 ++++++++++++++--- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 3ca25ab..1d47cc3 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -99,9 +99,21 @@ jobs: if (-not (([Environment]::GetEnvironmentVariable("Path", "Machine") -split ';') -contains $dir)) { throw "$dir is not on the system PATH" } - # A chosen release, to a directory of its own. + # A chosen release, to a directory of its own: it goes on the + # user PATH, not the system one, although this runs as admin. $env:INSTALL_DIR = Join-Path $env:RUNNER_TEMP "dbgp" $env:VERSION = "0.1.0" ./scripts/install.ps1 $got = & (Join-Path $env:INSTALL_DIR "dbgp.exe") -version if ($got -ne "dbgp 0.1.0") { throw "installed $got, want dbgp 0.1.0" } + function OnPath($scope, $dir) { ([Environment]::GetEnvironmentVariable("Path", $scope) -split ';') -contains $dir } + if (-not (OnPath "User" $env:INSTALL_DIR)) { throw "$env:INSTALL_DIR is not on the user PATH" } + if (OnPath "Machine" $env:INSTALL_DIR) { throw "$env:INSTALL_DIR is on the system PATH" } + # A relative directory goes on the PATH as a full path. + Push-Location $env:RUNNER_TEMP + $env:INSTALL_DIR = "relative-dbgp" + & (Join-Path $env:GITHUB_WORKSPACE "scripts/install.ps1") + Pop-Location + $full = Join-Path $env:RUNNER_TEMP "relative-dbgp" + if (-not (OnPath "User" $full)) { throw "$full is not on the user PATH" } + if (OnPath "User" "relative-dbgp") { throw "the relative directory is on the user PATH" } diff --git a/README.md b/README.md index 4f6dbd8..6cd6c22 100644 --- a/README.md +++ b/README.md @@ -23,7 +23,7 @@ Windows (PowerShell as administrator; installs to `%ProgramFiles%\dbgp` and adds powershell -c "irm https://github.com/php-debugger/dbgp-client/releases/latest/download/install.ps1 | iex" ``` -Set `INSTALL_DIR` to install elsewhere, and `VERSION` (e.g. `0.1.1`) to install a release other than the latest. Run the script again to update. Builds are published for Linux and macOS (amd64 and arm64) and Windows (amd64); elsewhere, install with Go: +Set `INSTALL_DIR` to install elsewhere (on Windows, it is then added to your own PATH rather than the system one), and `VERSION` (e.g. `0.1.1`) to install a release other than the latest. Run the script again to update. Builds are published for Linux and macOS (amd64 and arm64) and Windows (amd64); elsewhere, install with Go: ```sh go install github.com/php-debugger/dbgp-client/cmd/dbgp@latest diff --git a/scripts/install.ps1 b/scripts/install.ps1 index b611cfe..0ab12cf 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -7,6 +7,11 @@ release's SHA256SUMS, installs it system-wide and adds it to the system PATH. Run it from PowerShell as administrator. + With $env:INSTALL_DIR set, it installs there instead and adds that + directory to your own PATH, not the system one: every user, and programs + run as administrator, use the system PATH, so only a directory that just + administrators can change belongs on it. + Usage: powershell -c "irm https://github.com/php-debugger/dbgp-client/releases/latest/download/install.ps1 | iex" @@ -19,7 +24,12 @@ $ErrorActionPreference = "Stop" $Repo = "php-debugger/dbgp-client" $Bin = "dbgp.exe" -$InstallDir = if ($env:INSTALL_DIR) { $env:INSTALL_DIR } else { Join-Path $env:ProgramFiles "dbgp" } +$DefaultDir = Join-Path $env:ProgramFiles "dbgp" +# A full path: a relative one on the PATH would be looked up from whatever +# directory a command runs in. +$InstallDir = if ($env:INSTALL_DIR) { + $ExecutionContext.SessionState.Path.GetUnresolvedProviderPathFromPSPath($env:INSTALL_DIR).TrimEnd('\') +} else { $DefaultDir } $IsAdmin = ([Security.Principal.WindowsPrincipal][Security.Principal.WindowsIdentity]::GetCurrent()).IsInRole( [Security.Principal.WindowsBuiltInRole]::Administrator) @@ -75,8 +85,9 @@ try { Write-Host "" Write-Host "Installed $(& $dest -version) to $dest" -# Add the directory to the PATH: the system one when installing system-wide. -$scope = if ($IsAdmin) { "Machine" } else { "User" } +# Add the directory to the PATH: the system one only for the default +# directory, which only administrators can change; see the description. +$scope = if ($IsAdmin -and $InstallDir -eq $DefaultDir) { "Machine" } else { "User" } $path = [Environment]::GetEnvironmentVariable("Path", $scope) if (-not $path) { $path = "" } if (-not (($path -split ';') -contains $InstallDir)) { From 12ec995e1268183eb1b9f89c30ef31056b0b7960 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 17:07:44 +0200 Subject: [PATCH 11/19] History file --- cmd/dbgp/input.go | 26 +++++++++++++++++ cmd/dbgp/input_unix.go | 11 +++++++ cmd/dbgp/input_unix_test.go | 57 +++++++++++++++++++++++++++++++++++++ cmd/dbgp/input_windows.go | 5 ++++ 4 files changed, 99 insertions(+) create mode 100644 cmd/dbgp/input_unix.go create mode 100644 cmd/dbgp/input_unix_test.go create mode 100644 cmd/dbgp/input_windows.go diff --git a/cmd/dbgp/input.go b/cmd/dbgp/input.go index c4f6f10..0fa2e50 100644 --- a/cmd/dbgp/input.go +++ b/cmd/dbgp/input.go @@ -6,6 +6,7 @@ import ( "io" "os" "path/filepath" + "runtime" "sort" "github.com/ergochat/readline" @@ -53,6 +54,11 @@ func (t *terminalReader) ReadLine(prompt string) (string, error) { // the returned reader's Stdout redraws the prompt, so messages that arrive // while a line is being typed do not break it. historyFile may be empty. func newTerminalReader(historyFile string) (*terminalReader, error) { + if historyFile != "" { + if err := privateHistory(historyFile); err != nil { + return nil, err + } + } rl, err := readline.NewFromConfig(readlineConfig(historyFile)) if err != nil { return nil, err @@ -93,6 +99,26 @@ func completer() *readline.PrefixCompleter { return readline.NewPrefixCompleter(items...) } +// privateHistory keeps the history file readable by the user only: it +// holds what was typed, such as code to evaluate, which may include +// secrets. readline creates the file, and replaces it when it trims it, +// asking for mode 0666, so the files this process creates are made private +// (it creates no others), and an existing file is made private too. +func privateHistory(path string) error { + privateFiles() + info, err := os.Lstat(path) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + return err + } + if info.Mode().IsRegular() && runtime.GOOS != "windows" && info.Mode().Perm()&0o077 != 0 { + return os.Chmod(path, 0o600) + } + return nil +} + // defaultHistoryFile is ~/.dbgp_history, or "" if there is no home. func defaultHistoryFile() string { home, err := os.UserHomeDir() diff --git a/cmd/dbgp/input_unix.go b/cmd/dbgp/input_unix.go new file mode 100644 index 0000000..7761a03 --- /dev/null +++ b/cmd/dbgp/input_unix.go @@ -0,0 +1,11 @@ +//go:build !windows + +package main + +import "syscall" + +// privateFiles makes the files this process creates from now on readable +// by the user only, whatever mode their creator asks for. +func privateFiles() { + syscall.Umask(0o077) +} diff --git a/cmd/dbgp/input_unix_test.go b/cmd/dbgp/input_unix_test.go new file mode 100644 index 0000000..35f0891 --- /dev/null +++ b/cmd/dbgp/input_unix_test.go @@ -0,0 +1,57 @@ +//go:build !windows + +package main + +import ( + "io" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + + "github.com/ergochat/readline" +) + +// The history file stays readable by the user only: when readline creates +// it, when it was readable by others before, and when readline replaces it +// to trim it. +func TestHistoryFileIsPrivate(t *testing.T) { + defer syscall.Umask(syscall.Umask(0o022)) // a typical umask, restored after + long := strings.Repeat("eval $secret\n", 1001) + for name, existing := range map[string]string{"created": "", "existing": "eval $secret\n", "trimmed": long} { + t.Run(name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "history") + if existing != "" { + if err := os.WriteFile(path, []byte(existing), 0o644); err != nil { + t.Fatal(err) + } + } + if err := privateHistory(path); err != nil { + t.Fatal(err) + } + cfg := readlineConfig(path) + cfg.Stdin, cfg.Stdout, cfg.Stderr = io.NopCloser(strings.NewReader("")), io.Discard, io.Discard + rl, err := readline.NewFromConfig(cfg) + if err != nil { + t.Fatal(err) + } + if err := rl.SaveToHistory("eval $token"); err != nil { + t.Fatal(err) + } + _ = rl.Close() + + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("history file mode %o, want 600", perm) + } + data, _ := os.ReadFile(path) + if lines := strings.Count(string(data), "\n"); name == "trimmed" && lines > cfg.HistoryLimit+1 { + t.Errorf("history has %d lines: readline did not trim it", lines) + } + }) + } +} diff --git a/cmd/dbgp/input_windows.go b/cmd/dbgp/input_windows.go new file mode 100644 index 0000000..5d0d042 --- /dev/null +++ b/cmd/dbgp/input_windows.go @@ -0,0 +1,5 @@ +package main + +// privateFiles does nothing: files in the user's profile are private to +// the user by default. +func privateFiles() {} From 388439860281b331369d4dbeb168fb529cbc5082 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 17:51:37 +0200 Subject: [PATCH 12/19] variable_value memory --- cmd/dbgp/mcp_inspect.go | 19 ++++++++++++++----- cmd/dbgp/repl.go | 2 +- integration_test.go | 10 ++++++++-- session.go | 17 ++++++++++++----- session_test.go | 17 +++++++++++++---- 5 files changed, 48 insertions(+), 17 deletions(-) diff --git a/cmd/dbgp/mcp_inspect.go b/cmd/dbgp/mcp_inspect.go index 44bd9f6..b81329d 100644 --- a/cmd/dbgp/mcp_inspect.go +++ b/cmd/dbgp/mcp_inspect.go @@ -4,6 +4,8 @@ import ( "context" "errors" "fmt" + "math" + "unicode/utf8" "github.com/modelcontextprotocol/go-sdk/mcp" dbgp "github.com/php-debugger/dbgp-client" @@ -226,15 +228,22 @@ func (t *tools) variableValue(_ context.Context, _ *mcp.CallToolRequest, in vari if in.Name == "" { return nil, variableValueOutput{}, errors.New("name is required, e.g. $html") } - value, err := s.GetPropertyValue(in.Name, dbgp.PropertyOptions{Depth: in.Depth, Context: in.Context}) + if in.Offset < 0 || in.Offset > math.MaxInt32 { + return nil, variableValueOutput{}, fmt.Errorf("offset must be from 0 to the size of %s", in.Name) + } + // The engine sends a value's first bytes, not a range: ask for those up + // to the end of this part, not the whole value each time, and a few more + // so that the part can end on a whole character. + opts := dbgp.PropertyOptions{Depth: in.Depth, Context: in.Context, MaxData: in.Offset + maxTextBytes + utf8.UTFMax} + value, size, err := s.GetPropertyValue(in.Name, opts) if err != nil { return nil, variableValueOutput{}, inspectError(s, in.Name, in.Depth, err) } - if in.Offset < 0 || in.Offset > len(value) { - return nil, variableValueOutput{}, fmt.Errorf("offset must be from 0 to %d, the size of %s", len(value), in.Name) + if in.Offset > size || in.Offset > len(value) { + return nil, variableValueOutput{}, fmt.Errorf("offset must be from 0 to %d, the size of %s", size, in.Name) } - out := variableValueOutput{Name: in.Name, Value: cutText(value[in.Offset:], maxTextBytes), Size: len(value)} - if end := in.Offset + len(out.Value); end < len(value) { + out := variableValueOutput{Name: in.Name, Value: cutText(value[in.Offset:], maxTextBytes), Size: size} + if end := in.Offset + len(out.Value); end < size { out.Next = end } return nil, out, nil diff --git a/cmd/dbgp/repl.go b/cmd/dbgp/repl.go index 17cc876..1cfdfe7 100644 --- a/cmd/dbgp/repl.go +++ b/cmd/dbgp/repl.go @@ -791,7 +791,7 @@ func (r *repl) cmdValue(ctx context.Context, args string) error { if err != nil { return err } - v, err := s.GetPropertyValue(name, opts) + v, _, err := s.GetPropertyValue(name, opts) if err != nil { return err } diff --git a/integration_test.go b/integration_test.go index c998ea4..8f6bdbb 100644 --- a/integration_test.go +++ b/integration_test.go @@ -303,8 +303,14 @@ func TestEngineInspection(t *testing.T) { if err != nil || !p.Truncated() || p.Size != 200 { t.Errorf("$long = %+v, %v; want truncated", p, err) } - if v, err := sess.GetPropertyValue("$long", main); err != nil || len(v) != 200 { - t.Errorf("GetPropertyValue($long) = %d bytes, %v; want 200", len(v), err) + if v, size, err := sess.GetPropertyValue("$long", main); err != nil || len(v) != 200 || size != 200 { + t.Errorf("GetPropertyValue($long) = %d bytes, size %d, %v; want 200", len(v), size, err) + } + // With MaxData, only the start is sent, with the whole size. + start := main + start.MaxData = 15 + if v, size, err := sess.GetPropertyValue("$long", start); err != nil || len(v) != 15 || size != 200 { + t.Errorf("GetPropertyValue($long, MaxData 15) = %d bytes, size %d, %v; want 15 of 200", len(v), size, err) } // property_set evaluates its value as PHP. diff --git a/session.go b/session.go index 894afd1..9f02b19 100644 --- a/session.go +++ b/session.go @@ -896,16 +896,23 @@ func (s *Session) GetProperty(fullname string, opts PropertyOptions) (*Property, return &resp.Properties[0], nil } -// GetPropertyValue returns a property's value. With opts.MaxData 0 it -// returns the whole value, however long; Page is ignored. -func (s *Session) GetPropertyValue(fullname string, opts PropertyOptions) (string, error) { +// GetPropertyValue returns a property's value and the length of all of +// it. With opts.MaxData 0 it returns the whole value, however long; +// otherwise only its first MaxData bytes, so a part of a long value can be +// read without sending all of it. Page is ignored. +func (s *Session) GetPropertyValue(fullname string, opts PropertyOptions) (value string, size int, err error) { args := []string{"-n", fullname, "-d", strconv.Itoa(opts.Depth), "-c", strconv.Itoa(opts.Context), "-m", strconv.Itoa(opts.MaxData)} resp, err := s.sendCommand("property_value", args, nil) if err != nil { - return "", err + return "", 0, err + } + if value, err = decodeEncoded(resp.Encoding, resp.Value); err != nil { + return "", 0, err } - return decodeEncoded(resp.Encoding, resp.Value) + // The engine sends the whole length; one that does not at least sent + // this much. + return value, max(resp.Size, len(value)), nil } // SetProperty assigns value, a PHP expression, to a variable or a path diff --git a/session_test.go b/session_test.go index 5d4cbc5..5bb31e3 100644 --- a/session_test.go +++ b/session_test.go @@ -175,8 +175,17 @@ func TestCommandWireFormat(t *testing.T) { }, args: map[string]string{"n": `$user["first name"]`, "d": "1", "c": "1", "p": "2", "m": "100"}}, {name: "GetPropertyValue", command: "property_value", - call: func(s *Session) error { _, err := s.GetPropertyValue("$long", PropertyOptions{Depth: 1}); return err }, + call: func(s *Session) error { + _, _, err := s.GetPropertyValue("$long", PropertyOptions{Depth: 1}) + return err + }, args: map[string]string{"n": "$long", "d": "1", "c": "0", "m": "0"}}, + {name: "GetPropertyValue part", command: "property_value", + call: func(s *Session) error { + _, _, err := s.GetPropertyValue("$long", PropertyOptions{Depth: 1, MaxData: 32772}) + return err + }, + args: map[string]string{"n": "$long", "d": "1", "c": "0", "m": "32772"}}, {name: "SetProperty", command: "property_set", call: func(s *Session) error { return s.SetProperty("$b", "7 * 2", PropertyOptions{}) }, args: map[string]string{"n": "$b", "d": "0", "c": "0"}, @@ -973,9 +982,9 @@ func TestInspectionDecoding(t *testing.T) { } }) t.Run("GetPropertyValue", func(t *testing.T) { - v, err := s.GetPropertyValue("$long", PropertyOptions{Depth: 1}) - if err != nil || v != strings.Repeat("abcdefghij", 20) { - t.Errorf("GetPropertyValue = %q, %v", v, err) + v, size, err := s.GetPropertyValue("$long", PropertyOptions{Depth: 1}) + if err != nil || v != strings.Repeat("abcdefghij", 20) || size != 200 { + t.Errorf("GetPropertyValue = %q, size %d, %v", v, size, err) } }) t.Run("SetProperty", func(t *testing.T) { From d666e250028d896d5bc6aed008623e2f3882c8e9 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 18:00:24 +0200 Subject: [PATCH 13/19] Rejected breakpoints --- server.go | 7 +-- server_test.go | 118 +++++++++++++++++++++++++++++++++++++++++++++---- session.go | 69 ++++++++++++++++++++++------- 3 files changed, 166 insertions(+), 28 deletions(-) diff --git a/server.go b/server.go index 65672af..bb66900 100644 --- a/server.go +++ b/server.go @@ -350,8 +350,9 @@ func (s *Server) handshake(conn net.Conn) { sess := newSession(id, s, conn, reader, init) sess.setup(s.cfg, s.pathMap()) - // Failed breakpoints are retried, and reported, by the first Continue. - _ = sess.syncBreakpoints() + // Nothing reports errors here: the first Continue reports rejected + // breakpoints, and retries the others. + _ = sess.syncBreakpoints(false) s.mu.Lock() defer s.mu.Unlock() @@ -575,7 +576,7 @@ func (s *Server) syncStoppedSessions() error { if sess.State().Status == StatusRunning { continue } - if err := sess.syncBreakpoints(); err != nil { + if err := sess.syncBreakpoints(true); err != nil { errs = append(errs, fmt.Errorf("session %d: %w", sess.id, err)) } } diff --git a/server_test.go b/server_test.go index 11fe6ef..a1a67fa 100644 --- a/server_test.go +++ b/server_test.go @@ -9,6 +9,7 @@ import ( "sort" "strconv" "strings" + "sync" "testing" "time" ) @@ -387,19 +388,120 @@ func TestServerBreakpointErrors(t *testing.T) { t.Errorf("invalid breakpoints were kept: %+v", srv.Breakpoints()) } - // A breakpoint the engine rejects is reported, and retried on Continue. - dialFakeEngine(t, srv, fixture(t, "init"), map[string]fakeHandler{ - "breakpoint_set": func(e *fakeEngine, cmd fakeCommand) []string { - return []string{errorPacket(cmd, 200, "breakpoint could not be set")} - }, - "run": reply("run_break"), + // A breakpoint the engine rejects is reported once, here by + // AddBreakpoint, and not sent again: Continue runs without it. + e := dialFakeEngine(t, srv, fixture(t, "init"), map[string]fakeHandler{ + "breakpoint_set": rejectBreakpoints, + "run": reply("run_break"), }) sess := waitSession(t, srv) if _, err := srv.AddBreakpoint(Breakpoint{File: "/app/basic.php", Line: 3}); err == nil || !strings.Contains(err.Error(), "session 1") { t.Errorf("AddBreakpoint err = %v, want session 1 error", err) } - if _, err := sess.Continue(context.Background(), ContinueRun, time.Second); err == nil || !strings.Contains(err.Error(), "200") { - t.Errorf("Continue err = %v, want breakpoint error 200", err) + e.ClearReceived() + if st, err := sess.Continue(context.Background(), ContinueRun, time.Second); err != nil || st.Status != StatusBreak { + t.Errorf("Continue after the rejection was reported = %+v, %v; want it to run", st, err) + } + if got := breakpointCommands(e); len(got) != 0 { + t.Errorf("rejected breakpoint sent again: %q", got) + } +} + +// rejectBreakpoints answers breakpoint_set as an engine that cannot set it. +func rejectBreakpoints(e *fakeEngine, cmd fakeCommand) []string { + return []string{errorPacket(cmd, 200, "breakpoint could not be set")} +} + +// commandNames returns the names of the commands e received. +func commandNames(e *fakeEngine) string { + var names []string + for _, cmd := range e.Received() { + names = append(names, cmd.Name) + } + return strings.Join(names, " ") +} + +// A breakpoint rejected while the session was set up is reported by the +// first Continue, which does not resume the script; the next one runs +// without it. +func TestRejectedBreakpointReportedByFirstContinue(t *testing.T) { + srv := newTestServer(t, Config{}) + if _, err := srv.AddBreakpoint(Breakpoint{File: "/app/basic.php", Line: 3}); err != nil { + t.Fatal(err) + } + e := dialFakeEngine(t, srv, fixture(t, "init"), map[string]fakeHandler{ + "breakpoint_set": rejectBreakpoints, + "run": reply("run_break"), + }) + sess := waitSession(t, srv) + e.ClearReceived() + + _, err := sess.Continue(context.Background(), ContinueRun, time.Second) + if err == nil || !strings.Contains(err.Error(), "200") || !strings.Contains(err.Error(), "not resumed") { + t.Errorf("first Continue err = %v, want the rejection, not resumed", err) + } + if got := commandNames(e); got != "" { + t.Errorf("first Continue sent %q, want nothing", got) + } + st, err := sess.Continue(context.Background(), ContinueRun, time.Second) + if err != nil || st.Status != StatusBreak { + t.Errorf("second Continue = %+v, %v; want it to run", st, err) + } + if got := commandNames(e); got != "run" { + t.Errorf("second Continue sent %q, want only run", got) + } +} + +// A path mapping change retries rejected breakpoints, as the new path may +// be accepted; removing a rejected breakpoint forgets it. +func TestRejectedBreakpointRetriedAfterPathMapping(t *testing.T) { + srv := newTestServer(t, Config{}) + accept := false + var mu sync.Mutex + e := dialFakeEngine(t, srv, fixture(t, "init"), map[string]fakeHandler{ + "breakpoint_set": func(e *fakeEngine, cmd fakeCommand) []string { + mu.Lock() + defer mu.Unlock() + if !accept { + return rejectBreakpoints(e, cmd) + } + return reply("breakpoint_set_line")(e, cmd) + }, + "breakpoint_remove": reply("breakpoint_remove"), + }) + sess := waitSession(t, srv) + rejected, err := srv.AddBreakpoint(Breakpoint{File: "/home/me/app/basic.php", Line: 3}) + if err == nil { + t.Fatal("AddBreakpoint: the engine's rejection was not reported") + } + + mu.Lock() + accept = true + mu.Unlock() + e.ClearReceived() + if err := srv.AddPathMapping(PathMapping{Local: "/home/me/app", Remote: "/app"}); err != nil { + t.Fatalf("AddPathMapping: %v", err) + } + if got := breakpointCommands(e); len(got) != 1 || got[0] != "breakpoint_set -t line -f file:///app/basic.php -n 3" { + t.Errorf("after the mapping: %q, want the breakpoint set with its new path", got) + } + + // A breakpoint that stays rejected is forgotten once removed. + mu.Lock() + accept = false + mu.Unlock() + second, err := srv.AddBreakpoint(Breakpoint{File: "/home/me/app/other.php", Line: 5}) + if err == nil { + t.Fatal("AddBreakpoint: the second rejection was not reported") + } + if err := srv.RemoveBreakpoint(second.ID); err != nil { + t.Fatal(err) + } + sess.syncMu.Lock() + n, wasSet := len(sess.rejected), sess.bpIDs[rejected.ID] != 0 + sess.syncMu.Unlock() + if n != 0 || !wasSet { + t.Errorf("rejected = %d entries, first breakpoint set %v; want none, and set", n, wasSet) } } diff --git a/session.go b/session.go index 9f02b19..b7f023d 100644 --- a/session.go +++ b/session.go @@ -76,9 +76,12 @@ type Session struct { cmdSlot chan struct{} // syncMu serializes breakpoint syncs; it guards bpIDs, which maps - // server breakpoint ids to this engine's breakpoint ids. + // server breakpoint ids to this engine's breakpoint ids, and rejected. syncMu sync.Mutex bpIDs map[int]int + // rejected holds the server breakpoints the engine refused, by server + // id, so that syncs do not send them again, and so fail only once. + rejected map[int]*rejection // resync asks the next sync to set every breakpoint again, because a // path mapping changed. resync atomic.Bool @@ -165,6 +168,13 @@ func (s *Session) SetupWarnings() []string { return append([]string(nil), s.warnings...) } +// rejection is a breakpoint the engine refused, and whether that has been +// reported. +type rejection struct { + err error + reported bool +} + // pendingCommand is a command waiting for its response. type pendingCommand struct { id int @@ -175,15 +185,16 @@ type pendingCommand struct { // packet has been read. func newSession(id int, server *Server, conn net.Conn, reader *bufio.Reader, init *InitPacket) *Session { s := &Session{ - id: id, - server: server, - conn: conn, - init: init, - closed: make(chan struct{}), - cmdSlot: make(chan struct{}, 1), - bpIDs: map[int]int{}, - state: State{Status: StatusStarting}, - changed: make(chan struct{}), + id: id, + server: server, + conn: conn, + init: init, + closed: make(chan struct{}), + cmdSlot: make(chan struct{}, 1), + bpIDs: map[int]int{}, + rejected: map[int]*rejection{}, + state: State{Status: StatusStarting}, + changed: make(chan struct{}), } go s.readLoop(reader) return s @@ -600,15 +611,18 @@ func responseResult(resp *Response) (*Response, error) { // ContinueStepInto, ContinueStepOver or ContinueStepOut) and waits up to // wait for it to stop. If it is still running then, the returned state has // Status running: call Wait to keep waiting. Breakpoints added to the server -// are applied first. +// are applied first. If that fails, the script is not resumed: a breakpoint +// the engine rejected is reported once, and the next Continue runs without +// it. func (s *Session) Continue(ctx context.Context, command string, wait time.Duration) (State, error) { switch command { case ContinueRun, ContinueStepInto, ContinueStepOver, ContinueStepOut: default: return s.State(), fmt.Errorf("%q is not a continuation command", command) } - if err := s.syncBreakpoints(); err != nil { - return s.State(), err + if err := s.syncBreakpoints(true); err != nil { + return s.State(), fmt.Errorf("%w; the script was not resumed: "+ + "continue again to run it without the breakpoints the engine rejected", err) } if err := s.startContinuation(command); err != nil { return s.State(), err @@ -691,8 +705,11 @@ func (s *Session) finished() bool { } // syncBreakpoints makes the engine's breakpoints match the server's: it -// sets breakpoints added since the last sync and removes deleted ones. -func (s *Session) syncBreakpoints() error { +// sets breakpoints added since the last sync and removes deleted ones. A +// breakpoint the engine rejects is not sent again until a path mapping +// changes; its error is returned once, by the first sync that reports. +// Other errors, such as a timeout, are returned until the breakpoint is set. +func (s *Session) syncBreakpoints(report bool) error { if s.server == nil || s.finished() { return nil } @@ -701,13 +718,15 @@ func (s *Session) syncBreakpoints() error { var errs []error if s.resync.Swap(false) { - // A path mapping changed: set every breakpoint again. + // A path mapping changed: set every breakpoint again, including + // rejected ones, which the new paths may fix. for serverID, engineID := range s.bpIDs { delete(s.bpIDs, serverID) if err := s.RemoveBreakpoint(engineID); err != nil && !errors.Is(err, errConnectionClosed) { errs = append(errs, fmt.Errorf("remove breakpoint %d: %w", serverID, err)) } } + clear(s.rejected) } want := map[int]bool{} for _, bp := range s.server.Breakpoints() { @@ -715,13 +734,29 @@ func (s *Session) syncBreakpoints() error { if _, ok := s.bpIDs[bp.ID]; ok { continue } + if r, ok := s.rejected[bp.ID]; ok { + if report && !r.reported { + r.reported = true + errs = append(errs, r.err) + } + continue + } id, err := s.SetBreakpointSpec(bp) if err != nil { - errs = append(errs, fmt.Errorf("breakpoint %d: %w", bp.ID, err)) + err = fmt.Errorf("breakpoint %d: %w", bp.ID, err) + if engineErr := (*EngineError)(nil); errors.As(err, &engineErr) { + s.rejected[bp.ID] = &rejection{err: err, reported: report} + } + errs = append(errs, err) continue } s.bpIDs[bp.ID] = id } + for serverID := range s.rejected { + if !want[serverID] { + delete(s.rejected, serverID) + } + } for serverID, engineID := range s.bpIDs { if want[serverID] { continue From 74d2ff1bd51b345dc6170a5c335db23a58269a33 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 21:25:13 +0200 Subject: [PATCH 14/19] Shared background server --- cmd/dbgp/mcp.go | 148 ++++++++++++++++++++++++++----- cmd/dbgp/mcp_code.go | 8 +- cmd/dbgp/mcp_daemon.go | 106 +++++++++++++++++++---- cmd/dbgp/mcp_daemon_test.go | 57 ++++++++++-- cmd/dbgp/mcp_inspect.go | 16 ++-- cmd/dbgp/mcp_output.go | 38 ++++---- cmd/dbgp/mcp_output_test.go | 5 +- cmd/dbgp/mcp_sessions.go | 43 ++++----- cmd/dbgp/mcp_sessions_test.go | 159 ++++++++++++++++++++++++++++++++++ cmd/dbgp/mcp_test.go | 17 ++-- 10 files changed, 492 insertions(+), 105 deletions(-) diff --git a/cmd/dbgp/mcp.go b/cmd/dbgp/mcp.go index 679e6fa..5eef996 100644 --- a/cmd/dbgp/mcp.go +++ b/cmd/dbgp/mcp.go @@ -71,6 +71,20 @@ build: do not suggest rebuilding or restarting dbgp to get them. Read values with variables, variable and variable_value instead, and work out results from them. Instead of a condition, stop at the line and check the values each time.` +// sharedInstructions are added to mcpInstructions on the background server, +// which agents share. +const sharedInstructions = ` + +This is the user's background dbgp, which other agents may be using at the +same time. They share its breakpoints, path mappings and sessions; each has its +own selected session, and its own place in output and warnings. Remove only the +breakpoints you added. dbgp keeps listening until every agent that called +listen has called unlisten, so unlisten when you are done. +wait_for_session returns the oldest session no agent has taken yet, which may +be another agent's: if its script is not yours, leave it alone and wait again. +If the wait ends although your PHP started, another agent may have taken your +session: find it with sessions and name it by id.` + // runMCP runs dbgp as an MCP server. Agents start it as dbgp mcp and talk // to it on stdin and stdout: it serves them itself, or passes them on to the // background server when one is running, so they share it. dbgp mcp start, @@ -172,9 +186,9 @@ func runMCP(args []string) int { return 1 } } - server := newMCPServer(srv, !*noEval) + server, t := newMCPServer(srv, !*noEval, command == serveCommand) if command == serveCommand { - err = serveHTTP(ctx, server, *mcpPort) + err = serveHTTP(ctx, server, t, *mcpPort) } else { err = server.Run(ctx, &mcp.StdioTransport{}) } @@ -187,12 +201,16 @@ func runMCP(args []string) int { // newMCPServer returns an MCP server whose tools drive srv. allowCode adds // what runs PHP code in the debugged script: the code tools, and conditions -// on breakpoints. -func newMCPServer(srv *dbgp.Server, allowCode bool) *mcp.Server { +// on breakpoints. shared is for the background server, which agents share. +// The tools are returned too, for what they keep about agents. +func newMCPServer(srv *dbgp.Server, allowCode, shared bool) (*mcp.Server, *tools) { instructions := mcpInstructions if !allowCode { instructions += noCodeInstructions } + if shared { + instructions += sharedInstructions + } server := mcp.NewServer(&mcp.Implementation{Name: "dbgp", Title: "PHP debugger", Version: version}, &mcp.ServerOptions{Instructions: instructions}) t := &tools{srv: srv, allowCode: allowCode} @@ -214,7 +232,8 @@ func newMCPServer(srv *dbgp.Server, allowCode bool) *mcp.Server { mcp.AddTool(server, &mcp.Tool{ Name: "unlisten", Description: "Stop listening: new PHP connections are refused, so PHP runs at full speed. " + - "Sessions already connected are not affected.", + "Sessions already connected are not affected. On the background server, dbgp keeps listening " + + "while other agents that called listen have not called unlisten.", Annotations: &mcp.ToolAnnotations{IdempotentHint: true}, }, t.unlisten) @@ -263,7 +282,7 @@ func newMCPServer(srv *dbgp.Server, allowCode bool) *mcp.Server { if allowCode { t.addCodeTools(server) } - return server + return server, t } // outputSchema returns the schema for a tool's output type, with the named @@ -316,15 +335,78 @@ type tools struct { // allowCode allows what runs PHP code in the debugged script. allowCode bool - // mu guards the fields below. output and warnings return only what is - // new since their last call, so they keep read positions per session id. - mu sync.Mutex - outputFrom map[int]int - warningsFrom map[int]int + // mu guards agents, by agent id: see agentID. + mu sync.Mutex + agents map[string]*agentState +} + +// agentState is what the tools keep for one agent. Agents sharing the +// background server each have their own, so one does not change what the +// tools do for another. +type agentState struct { // selected is the session the tools act on by default: the one // wait_for_session last returned or a tool was last given by id. Not // simply the newest, which anything that connects would become. selected int + // output and warnings return only what is new since their last call, + // so they keep read positions per session id. + outputFrom, warningsFrom map[int]int + // listening is set from the agent's listen to its unlisten. dbgp keeps + // listening while any agent has it set, so one agent does not stop + // PHP connections another is waiting for. + listening bool +} + +// agent returns the state of the agent that sent req. t.mu must be held. +func (t *tools) agent(req *mcp.CallToolRequest) *agentState { + id := agentID(req) + a := t.agents[id] + if a == nil { + if t.agents == nil { + t.agents = map[string]*agentState{} + } + a = &agentState{outputFrom: map[int]int{}, warningsFrom: map[int]int{}} + t.agents[id] = a + } + return a +} + +// leave forgets the agent with the given id, which has gone: it no longer +// keeps dbgp listening. +func (t *tools) leave(id string) error { + t.mu.Lock() + defer t.mu.Unlock() + a := t.agents[id] + if a == nil { + return nil + } + delete(t.agents, id) + if !a.listening || t.listeners() > 0 { + return nil + } + return t.srv.StopListening() +} + +// listeners returns how many agents have called listen and not unlisten. +// t.mu must be held. +func (t *tools) listeners() int { + n := 0 + for _, a := range t.agents { + if a.listening { + n++ + } + } + return n +} + +// agentID identifies the agent that sent req: on the background server, by +// the agentHeader that its dbgp mcp sends. An agent on stdin and stdout is +// the only one, with no id. +func agentID(req *mcp.CallToolRequest) string { + if req == nil || req.Extra == nil { + return "" + } + return req.Extra.Header.Get(agentHeader) } type noInput struct{} @@ -403,18 +485,28 @@ type listenOutput struct { Warnings []string `json:"warnings,omitempty" jsonschema:"addresses dbgp could not listen on, although it listens on others; PHP connecting there may reach another program instead"` } -func (t *tools) listen(context.Context, *mcp.CallToolRequest, noInput) (*mcp.CallToolResult, listenOutput, error) { +func (t *tools) listen(_ context.Context, req *mcp.CallToolRequest, _ noInput) (*mcp.CallToolResult, listenOutput, error) { + t.mu.Lock() + defer t.mu.Unlock() if err := t.srv.StartListening(); err != nil { return nil, listenOutput{}, err } + t.agent(req).listening = true return nil, listenOutput{Listening: true, Port: t.srv.Port(), Warnings: t.srv.ListenWarnings()}, nil } type unlistenOutput struct { - Listening bool `json:"listening" jsonschema:"whether PHP connections are accepted"` + Listening bool `json:"listening" jsonschema:"whether PHP connections are accepted"` + OtherAgents int `json:"otherAgents,omitempty" jsonschema:"how many other agents called listen and not unlisten yet: dbgp keeps listening for them"` } -func (t *tools) unlisten(context.Context, *mcp.CallToolRequest, noInput) (*mcp.CallToolResult, unlistenOutput, error) { +func (t *tools) unlisten(_ context.Context, req *mcp.CallToolRequest, _ noInput) (*mcp.CallToolResult, unlistenOutput, error) { + t.mu.Lock() + defer t.mu.Unlock() + t.agent(req).listening = false + if others := t.listeners(); others > 0 { + return nil, unlistenOutput{Listening: t.srv.Listening(), OtherAgents: others}, nil + } if err := t.srv.StopListening(); err != nil { return nil, unlistenOutput{Listening: t.srv.Listening()}, err } @@ -427,7 +519,7 @@ var breakpointTypes = []any{dbgp.BreakpointLine, dbgp.BreakpointCall, dbgp.Break type addBreakpointInput struct { Type string `json:"type,omitempty" jsonschema:"line (the default) stops at a file and line; call and return stop when a function is called or returns; exception stops when an exception, or a subclass of it, is thrown"` - File string `json:"file,omitempty" jsonschema:"for line: the PHP file, as a path on this machine; a relative path is relative to dbgp's working directory"` + File string `json:"file,omitempty" jsonschema:"for line: the PHP file, as an absolute path on this machine"` Line int `json:"line,omitempty" jsonschema:"for line: the line number"` // A condition makes a line breakpoint a conditional one. Condition string `json:"condition,omitempty" jsonschema:"for line, optional: a PHP expression; execution stops only when it is true"` @@ -468,7 +560,7 @@ func (t *tools) addBreakpoint(_ context.Context, _ *mcp.CallToolRequest, in addB bp.Type, bp.Condition = dbgp.BreakpointConditional, in.Condition } if in.File != "" { - file, err := absolutePath(in.File) + file, err := filePath("file", in.File) if err != nil { return nil, addBreakpointOutput{}, err } @@ -485,11 +577,25 @@ func (t *tools) addBreakpoint(_ context.Context, _ *mcp.CallToolRequest, in addB return nil, out, nil } -// absolutePath makes a local path absolute; file:// URIs are kept as they are. -func absolutePath(path string) (string, error) { - if filepath.IsAbs(path) || strings.HasPrefix(path, "file://") { +// filePath is absolutePath for a file, which can also be given as a +// file:// URI, kept as it is. +func filePath(name, path string) (string, error) { + if strings.HasPrefix(path, "file://") { return path, nil } + return absolutePath(name, path) +} + +// absolutePath checks that the path given as the named argument is +// absolute, and cleans it. Relative paths are refused rather than taken as +// relative to dbgp's working directory, which is not the agent's: the +// background server's is wherever dbgp mcp start ran. +func absolutePath(name, path string) (string, error) { + // On Windows, a path from the root such as \app\index.php is not + // absolute, but means the current drive; Abs adds it. + if !filepath.IsAbs(path) && (path == "" || !os.IsPathSeparator(path[0])) { + return "", fmt.Errorf("%s must be an absolute path, not %q", name, path) + } return filepath.Abs(path) } @@ -528,7 +634,7 @@ func (t *tools) breakpoints(context.Context, *mcp.CallToolRequest, noInput) (*mc } type addPathMappingInput struct { - Local string `json:"local" jsonschema:"the directory on this machine; a relative path is relative to dbgp's working directory"` + Local string `json:"local" jsonschema:"the directory on this machine, as an absolute path"` Remote string `json:"remote" jsonschema:"the same directory as PHP sees it, e.g. /var/www/html in a container"` } @@ -541,7 +647,7 @@ func (t *tools) addPathMapping(_ context.Context, _ *mcp.CallToolRequest, in add if in.Local == "" || in.Remote == "" { return nil, pathMappingsOutput{}, errors.New("local and remote must both be set") } - local, err := filepath.Abs(in.Local) + local, err := absolutePath("local", in.Local) if err != nil { return nil, pathMappingsOutput{}, err } diff --git a/cmd/dbgp/mcp_code.go b/cmd/dbgp/mcp_code.go index 2d78efb..bedf8dd 100644 --- a/cmd/dbgp/mcp_code.go +++ b/cmd/dbgp/mcp_code.go @@ -52,8 +52,8 @@ func codeError(s *dbgp.Session, err error) error { return sessionError(s, err) } -func (t *tools) eval(_ context.Context, _ *mcp.CallToolRequest, in evalInput) (*mcp.CallToolResult, evalOutput, error) { - s, err := t.session(in.Session) +func (t *tools) eval(_ context.Context, req *mcp.CallToolRequest, in evalInput) (*mcp.CallToolResult, evalOutput, error) { + s, err := t.session(req, in.Session) if err != nil { return nil, evalOutput{}, err } @@ -95,8 +95,8 @@ type setVariableOutput struct { Variable variableInfo `json:"variable" jsonschema:"the variable after it was set"` } -func (t *tools) setVariable(_ context.Context, _ *mcp.CallToolRequest, in setVariableInput) (*mcp.CallToolResult, setVariableOutput, error) { - s, err := t.session(in.Session) +func (t *tools) setVariable(_ context.Context, req *mcp.CallToolRequest, in setVariableInput) (*mcp.CallToolResult, setVariableOutput, error) { + s, err := t.session(req, in.Session) if err != nil { return nil, setVariableOutput{}, err } diff --git a/cmd/dbgp/mcp_daemon.go b/cmd/dbgp/mcp_daemon.go index 1ac2546..6dd137c 100644 --- a/cmd/dbgp/mcp_daemon.go +++ b/cmd/dbgp/mcp_daemon.go @@ -369,10 +369,13 @@ func bridge(ctx context.Context, st *daemonState) error { return err } defer local.Close() - client, err := tokenClient(st.URL, st.Token) + // Agents share the server, but each has its own selected session and + // read positions: the id tells the server which agent a request is from. + client, err := tokenClient(st.URL, st.Token, rand.Text()) if err != nil { return err } + defer leaveServer(client, st.URL) remote, err := (&mcp.StreamableClientTransport{Endpoint: st.URL, HTTPClient: client}).Connect(ctx) if err != nil { return err @@ -401,26 +404,84 @@ func bridge(ctx context.Context, st *daemonState) error { return nil } -// tokenClient returns an HTTP client that sends token to the server at -// rawURL, and to no other. A state file from before tokens has none: that -// server takes requests without one. -func tokenClient(rawURL, token string) (*http.Client, error) { +// leavePath is where dbgp mcp tells the server its agent has gone, so it +// no longer keeps dbgp listening. +const leavePath = "/leave" + +// leaveTimeout is how long dbgp mcp waits for the server to hear it has +// gone. +const leaveTimeout = 2 * time.Second + +// leaveServer tells the server at rawURL, through client, that the agent +// client sends the id of has gone. It is best effort: the agent has +// already gone, and so has its dbgp mcp when the server cannot be reached. +func leaveServer(client *http.Client, rawURL string) { + u, err := url.Parse(rawURL) + if err != nil { + return + } + u.Path = leavePath + ctx, cancel := context.WithTimeout(context.Background(), leaveTimeout) + defer cancel() + req, err := http.NewRequestWithContext(ctx, http.MethodPost, u.String(), nil) + if err != nil { + return + } + if resp, err := client.Do(req); err == nil { + _ = resp.Body.Close() + } +} + +// leave forgets the agent each request is from, by its agentHeader. +func leave(t *tools) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if req.Method != http.MethodPost { + w.Header().Set("Allow", http.MethodPost) + http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + return + } + id := req.Header.Get(agentHeader) + if id == "" { + http.Error(w, "no "+agentHeader+" header", http.StatusBadRequest) + return + } + if err := t.leave(id); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusNoContent) + }) +} + +// agentHeader carries the id of the agent a request is from, which its +// dbgp mcp chooses: see agentID. +const agentHeader = "Dbgp-Agent" + +// tokenClient returns an HTTP client that sends token and the agent id to +// the server at rawURL, and to no other. A state file from before tokens +// has none: that server takes requests without one. +func tokenClient(rawURL, token, agent string) (*http.Client, error) { u, err := url.Parse(rawURL) if err != nil { return nil, err } - return &http.Client{Transport: &bearerTransport{host: u.Host, token: token}}, nil + return &http.Client{Transport: &bearerTransport{host: u.Host, token: token, agent: agent}}, nil } -// bearerTransport adds the token to requests for host. +// bearerTransport adds the token and the agent id to requests for host. type bearerTransport struct { - host, token string + host, token, agent string } func (t *bearerTransport) RoundTrip(req *http.Request) (*http.Response, error) { - if t.token != "" && req.URL.Host == t.host { + if req.URL.Host == t.host { req = req.Clone(req.Context()) - req.Header.Set("Authorization", "Bearer "+t.token) + if t.token != "" { + req.Header.Set("Authorization", "Bearer "+t.token) + } + if t.agent != "" { + req.Header.Set(agentHeader, t.agent) + } } return http.DefaultTransport.RoundTrip(req) } @@ -438,9 +499,9 @@ func requireToken(token string, next http.Handler) http.Handler { }) } -// serveHTTP serves MCP over HTTP on port until ctx is done, recording the -// server in the state file meanwhile. -func serveHTTP(ctx context.Context, server *mcp.Server, port int) error { +// serveHTTP serves MCP with server, whose tools are t, over HTTP on port +// until ctx is done, recording the server in the state file meanwhile. +func serveHTTP(ctx context.Context, server *mcp.Server, t *tools, port int) error { ln, err := net.Listen("tcp", net.JoinHostPort(httpHost, strconv.Itoa(port))) if err != nil { return err @@ -452,12 +513,7 @@ func serveHTTP(ctx context.Context, server *mcp.Server, port int) error { } defer removeState(st.PID) - handler := mcp.NewStreamableHTTPHandler(func(*http.Request) *mcp.Server { return server }, - &mcp.StreamableHTTPOptions{CrossOriginProtection: http.NewCrossOriginProtection()}) - mux := http.NewServeMux() - mux.Handle("/mcp", requireToken(st.Token, handler)) - mux.Handle(identifyPath, identify(st)) - hs := &http.Server{Handler: mux, ReadHeaderTimeout: 10 * time.Second} + hs := &http.Server{Handler: httpHandler(server, t, st), ReadHeaderTimeout: 10 * time.Second} go func() { <-ctx.Done() // Tools may be waiting for PHP for minutes: do not wait for them. @@ -469,3 +525,15 @@ func serveHTTP(ctx context.Context, server *mcp.Server, port int) error { } return nil } + +// httpHandler serves MCP at /mcp and agents leaving, to requests with st's +// token, and the identity check. t are server's tools. +func httpHandler(server *mcp.Server, t *tools, st daemonState) http.Handler { + handler := mcp.NewStreamableHTTPHandler(func(*http.Request) *mcp.Server { return server }, + &mcp.StreamableHTTPOptions{CrossOriginProtection: http.NewCrossOriginProtection()}) + mux := http.NewServeMux() + mux.Handle("/mcp", requireToken(st.Token, handler)) + mux.Handle(leavePath, requireToken(st.Token, leave(t))) + mux.Handle(identifyPath, identify(st)) + return mux +} diff --git a/cmd/dbgp/mcp_daemon_test.go b/cmd/dbgp/mcp_daemon_test.go index 0d278f2..680c714 100644 --- a/cmd/dbgp/mcp_daemon_test.go +++ b/cmd/dbgp/mcp_daemon_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "crypto/rand" "encoding/json" "errors" "io" @@ -206,10 +207,16 @@ func TestMCPBridge(t *testing.T) { if len(status.Breakpoints) != 1 || status.Listening { t.Errorf("status seen by another agent = %+v; want the breakpoint, and -listen ignored", status) } + callTool(t, bridged, "listen", nil, &listenOutput{}) if err := bridged.Close(); err != nil { t.Errorf("close: %v", err) } + // Its dbgp mcp told the server the agent left, which counts as unlisten. + callTool(t, direct, "status", nil, &status) + if status.Listening { + t.Error("still listening after the only agent that called listen left") + } if !strings.Contains(stderr.String(), "using the background server at "+url) || !strings.Contains(stderr.String(), "-listen ignored") { t.Errorf("stderr: %q", stderr.String()) } @@ -225,7 +232,7 @@ func connectDirect(ctx context.Context, t *testing.T, url string) *mcp.ClientSes if err != nil || st == nil { t.Fatalf("state = %+v, %v", st, err) } - client, err := tokenClient(url, st.Token) + client, err := tokenClient(url, st.Token, rand.Text()) if err != nil { t.Fatal(err) } @@ -327,19 +334,19 @@ func TestWriteStateIsPrivate(t *testing.T) { } } -// The token goes to the background server only, not to other hosts, e.g. -// after a redirect. +// The token and agent id go to the background server only, not to other +// hosts, e.g. after a redirect. func TestTokenClientSendsTokenOnlyToItsServer(t *testing.T) { var got []string record := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - got = append(got, req.Header.Get("Authorization")) + got = append(got, req.Header.Get("Authorization")+"|"+req.Header.Get(agentHeader)) }) server := httptest.NewServer(record) defer server.Close() other := httptest.NewServer(record) defer other.Close() - client, err := tokenClient(server.URL+"/mcp", "secret") + client, err := tokenClient(server.URL+"/mcp", "secret", "agent-1") if err != nil { t.Fatal(err) } @@ -350,8 +357,8 @@ func TestTokenClientSendsTokenOnlyToItsServer(t *testing.T) { } _ = resp.Body.Close() } - if want := []string{"Bearer secret", ""}; !slices.Equal(got, want) { - t.Errorf("Authorization headers = %q, want %q", got, want) + if want := []string{"Bearer secret|agent-1", "|"}; !slices.Equal(got, want) { + t.Errorf("Authorization|Dbgp-Agent headers = %q, want %q", got, want) } } @@ -538,3 +545,39 @@ func TestMCPDaemonIdentify(t *testing.T) { t.Error("identify gave away the token") } } + +// Each dbgp mcp passing an agent on to the background server identifies +// it as an agent of its own. +func TestMCPBridgesAreSeparateAgents(t *testing.T) { + requireEngine(t) + bin := buildBinary(t) + daemonEnv(t) + startTestDaemon(t, bin, "-php-port", "0") + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + var agents []*mcp.ClientSession + for range 2 { + cs, err := mcp.NewClient(&mcp.Implementation{Name: "test", Version: "0"}, nil). + Connect(ctx, &mcp.CommandTransport{Command: exec.CommandContext(ctx, bin, "mcp")}, nil) + if err != nil { + t.Fatal(err) + } + defer cs.Close() + agents = append(agents, cs) + } + + var listening listenOutput + callTool(t, agents[0], "listen", nil, &listening) + startPHP(t, listening.Port, "-r", `echo "hi\n";`) + var waited waitForSessionOutput + callTool(t, agents[0], "wait_for_session", map[string]any{"wait": 10}, &waited) + if !waited.Connected { + t.Fatal("PHP did not connect") + } + if msg := callToolError(t, agents[1], "continue", nil); !strings.Contains(msg, "no session") { + t.Errorf("the second agent's continue acted on the first agent's session: %q", msg) + } + var st stateOutput + callTool(t, agents[0], "stop", nil, &st) +} diff --git a/cmd/dbgp/mcp_inspect.go b/cmd/dbgp/mcp_inspect.go index b81329d..40996da 100644 --- a/cmd/dbgp/mcp_inspect.go +++ b/cmd/dbgp/mcp_inspect.go @@ -61,8 +61,8 @@ type stackOutput struct { Pages int `json:"pages" jsonschema:"how many pages of frames there are; call stack with page for the others"` } -func (t *tools) stack(_ context.Context, _ *mcp.CallToolRequest, in stackInput) (*mcp.CallToolResult, stackOutput, error) { - s, err := t.session(in.Session) +func (t *tools) stack(_ context.Context, req *mcp.CallToolRequest, in stackInput) (*mcp.CallToolResult, stackOutput, error) { + s, err := t.session(req, in.Session) if err != nil { return nil, stackOutput{}, err } @@ -145,8 +145,8 @@ type variablesOutput struct { Pages int `json:"pages" jsonschema:"how many pages of variables there are; call variables with page for the others"` } -func (t *tools) variables(_ context.Context, _ *mcp.CallToolRequest, in frameInput) (*mcp.CallToolResult, variablesOutput, error) { - s, err := t.session(in.Session) +func (t *tools) variables(_ context.Context, req *mcp.CallToolRequest, in frameInput) (*mcp.CallToolResult, variablesOutput, error) { + s, err := t.session(req, in.Session) if err != nil { return nil, variablesOutput{}, err } @@ -180,8 +180,8 @@ type variableOutput struct { Pages int `json:"pages" jsonschema:"how many pages of elements or properties there are; call variable with page for the others"` } -func (t *tools) variable(_ context.Context, _ *mcp.CallToolRequest, in variableInput) (*mcp.CallToolResult, variableOutput, error) { - s, err := t.session(in.Session) +func (t *tools) variable(_ context.Context, req *mcp.CallToolRequest, in variableInput) (*mcp.CallToolResult, variableOutput, error) { + s, err := t.session(req, in.Session) if err != nil { return nil, variableOutput{}, err } @@ -220,8 +220,8 @@ type variableValueOutput struct { Next int `json:"next,omitempty" jsonschema:"where the rest of the value starts: call variable_value again with this offset; absent when the value is complete"` } -func (t *tools) variableValue(_ context.Context, _ *mcp.CallToolRequest, in variableValueInput) (*mcp.CallToolResult, variableValueOutput, error) { - s, err := t.session(in.Session) +func (t *tools) variableValue(_ context.Context, req *mcp.CallToolRequest, in variableValueInput) (*mcp.CallToolResult, variableValueOutput, error) { + s, err := t.session(req, in.Session) if err != nil { return nil, variableValueOutput{}, err } diff --git a/cmd/dbgp/mcp_output.go b/cmd/dbgp/mcp_output.go index 567f4eb..9e2e9ad 100644 --- a/cmd/dbgp/mcp_output.go +++ b/cmd/dbgp/mcp_output.go @@ -46,7 +46,7 @@ func (t *tools) addOutputTools(server *mcp.Server) { type sourceInput struct { Session int `json:"session,omitempty" jsonschema:"the session id; defaults to the one wait_for_session last returned or you last named"` - File string `json:"file,omitempty" jsonschema:"the file, as a path on this machine; defaults to where the session stopped, or its script"` + File string `json:"file,omitempty" jsonschema:"the file, as an absolute path on this machine; defaults to where the session stopped, or its script"` Line int `json:"line,omitempty" jsonschema:"the line to show the code around; defaults to where the session stopped, or 1 for another file"` Around int `json:"around,omitempty" jsonschema:"how many lines to show before and after the line: 5 by default, at most 100"` } @@ -63,8 +63,8 @@ type sourceOutput struct { Lines []sourceLine `json:"lines" jsonschema:"the source lines, in order"` } -func (t *tools) source(_ context.Context, _ *mcp.CallToolRequest, in sourceInput) (*mcp.CallToolResult, sourceOutput, error) { - s, err := t.session(in.Session) +func (t *tools) source(_ context.Context, req *mcp.CallToolRequest, in sourceInput) (*mcp.CallToolResult, sourceOutput, error) { + s, err := t.session(req, in.Session) if err != nil { return nil, sourceOutput{}, err } @@ -86,7 +86,7 @@ func (t *tools) source(_ context.Context, _ *mcp.CallToolRequest, in sourceInput if file == "" { file = s.Script() } - } else if file, err = absolutePath(file); err != nil { + } else if file, err = filePath("file", file); err != nil { return nil, sourceOutput{}, err } if line <= 0 { @@ -122,21 +122,23 @@ type readInput struct { All bool `json:"all,omitempty" jsonschema:"return everything kept so far, not only what is new since the last call"` } -// readFrom runs read from where the last read of a session's output or -// warnings ended, or from the start for all, and records where it ends. -func (t *tools) readFrom(positions *map[int]int, id int, all bool, read func(from int) int) { +// readFrom runs read from where the agent that sent req last read session +// id's output or warnings, as positions picks, or from the start for all, +// and records where it ends. +func (t *tools) readFrom(req *mcp.CallToolRequest, positions func(*agentState) map[int]int, id int, all bool, read func(from int) int) { t.mu.Lock() defer t.mu.Unlock() - if *positions == nil { - *positions = map[int]int{} - } - from := (*positions)[id] + pos := positions(t.agent(req)) + from := pos[id] if all { from = 0 } - (*positions)[id] = read(from) + pos[id] = read(from) } +func outputPositions(a *agentState) map[int]int { return a.outputFrom } +func warningsPositions(a *agentState) map[int]int { return a.warningsFrom } + type outputOutput struct { Session int `json:"session" jsonschema:"the session id"` Output string `json:"output" jsonschema:"what the script printed; empty if nothing new"` @@ -144,13 +146,13 @@ type outputOutput struct { More bool `json:"more,omitempty" jsonschema:"there is more output after this: call output again for it"` } -func (t *tools) output(_ context.Context, _ *mcp.CallToolRequest, in readInput) (*mcp.CallToolResult, outputOutput, error) { - s, err := t.session(in.Session) +func (t *tools) output(_ context.Context, req *mcp.CallToolRequest, in readInput) (*mcp.CallToolResult, outputOutput, error) { + s, err := t.session(req, in.Session) if err != nil { return nil, outputOutput{}, err } out := outputOutput{Session: s.ID()} - t.readFrom(&t.outputFrom, s.ID(), in.All, func(from int) int { + t.readFrom(req, outputPositions, s.ID(), in.All, func(from int) int { text, next, truncated := s.Output(from) out.Output, out.Truncated = cutText(text, maxTextBytes), truncated out.More = len(out.Output) < len(text) @@ -173,13 +175,13 @@ type warningsOutput struct { More bool `json:"more,omitempty" jsonschema:"there are more after these: call warnings again for them"` } -func (t *tools) warnings(_ context.Context, _ *mcp.CallToolRequest, in readInput) (*mcp.CallToolResult, warningsOutput, error) { - s, err := t.session(in.Session) +func (t *tools) warnings(_ context.Context, req *mcp.CallToolRequest, in readInput) (*mcp.CallToolResult, warningsOutput, error) { + s, err := t.session(req, in.Session) if err != nil { return nil, warningsOutput{}, err } out := warningsOutput{Session: s.ID(), Warnings: []warningInfo{}} - t.readFrom(&t.warningsFrom, s.ID(), in.All, func(from int) int { + t.readFrom(req, warningsPositions, s.ID(), in.All, func(from int) int { notes, next := s.Notifications(from) seen := map[warningInfo]int{} // index in out.Warnings, by warning with no count for i, n := range notes { diff --git a/cmd/dbgp/mcp_output_test.go b/cmd/dbgp/mcp_output_test.go index d91ca37..5874658 100644 --- a/cmd/dbgp/mcp_output_test.go +++ b/cmd/dbgp/mcp_output_test.go @@ -36,9 +36,9 @@ func TestMCPSource(t *testing.T) { if len(out.Lines) != 3 || out.Lines[1].Text != "$x = add(2, 3);" || out.Lines[1].Current { t.Errorf("source around line 36 = %+v", out.Lines) } - callTool(t, cs, "source", map[string]any{"file": "../../testdata/php/basic.php", "line": 1, "around": 2}, &out) + callTool(t, cs, "source", map[string]any{"file": script, "line": 1, "around": 2}, &out) if out.File != script || len(out.Lines) != 3 || out.Lines[0].Line != 1 || out.Lines[0].Text != " 2"}, breakpointInfo{ID: 3, Type: "conditional", File: "/app/a.php", Line: 5, Condition: "$i > 2"}}, {map[string]any{"type": "call", "function": "add"}, breakpointInfo{ID: 4, Type: "call", Function: "add"}}, {map[string]any{"type": "return", "function": "add"}, breakpointInfo{ID: 5, Type: "return", Function: "add"}}, @@ -403,6 +403,8 @@ func TestMCPAddBreakpoint(t *testing.T) { {map[string]any{"type": "exception"}, "needs an exception class"}, {map[string]any{"type": "conditional", "file": "/app/a.php", "line": 3}, "type"}, {map[string]any{"type": "watch", "file": "/app/a.php", "line": 3}, "type"}, + // dbgp's working directory is not the agent's. + {map[string]any{"file": "app/a.php", "line": 3}, `file must be an absolute path, not "app/a.php"`}, } { if msg := callToolError(t, cs, "add_breakpoint", tt.args); !strings.Contains(msg, tt.want) { t.Errorf("add_breakpoint(%v) error = %q, want it to mention %q", tt.args, msg, tt.want) @@ -437,7 +439,6 @@ func TestMCPRemoveBreakpoint(t *testing.T) { func TestMCPPathMappings(t *testing.T) { cs, _ := newTestMCP(t) - abs, _ := filepath.Abs("testdata/php") var listed pathMappingListOutput callTool(t, cs, "path_mappings", nil, &listed) if listed.PathMappings == nil || len(listed.PathMappings) != 0 { @@ -445,9 +446,9 @@ func TestMCPPathMappings(t *testing.T) { } var out pathMappingsOutput callTool(t, cs, "add_path_mapping", map[string]any{"local": "/home/me/app", "remote": "/var/www/html"}, &out) - callTool(t, cs, "add_path_mapping", map[string]any{"local": "testdata/php", "remote": "/srv/php"}, &out) + callTool(t, cs, "add_path_mapping", map[string]any{"local": "/srv/../srv/php", "remote": "/srv/php"}, &out) callTool(t, cs, "add_path_mapping", map[string]any{"local": "/home/me/app/", "remote": "/app"}, &out) - want := []pathMapping{{Local: abs, Remote: "/srv/php"}, {Local: "/home/me/app", Remote: "/app"}} + want := []pathMapping{{Local: "/srv/php", Remote: "/srv/php"}, {Local: "/home/me/app", Remote: "/app"}} if fmt.Sprint(out.PathMappings) != fmt.Sprint(want) || out.Warning != "" { t.Errorf("add_path_mapping = %+v, want %+v", out, want) } @@ -458,6 +459,10 @@ func TestMCPPathMappings(t *testing.T) { for _, args := range []map[string]any{{"local": "/a"}, {"remote": "/b"}, {"local": "", "remote": "/b"}} { callToolError(t, cs, "add_path_mapping", args) } + // dbgp's working directory is not the agent's. + if msg := callToolError(t, cs, "add_path_mapping", map[string]any{"local": "app", "remote": "/app"}); !strings.Contains(msg, `local must be an absolute path, not "app"`) { + t.Errorf("relative local directory: %q", msg) + } } // Breakpoints added and removed over MCP reach a connected PHP session. From f7d6c42dda1b7aba72b6f5ff5762711f2ecf2fc6 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 22:30:07 +0200 Subject: [PATCH 15/19] Halt runway scripts --- cmd/dbgp/mcp_sessions.go | 17 +++++++++++----- cmd/dbgp/mcp_sessions_test.go | 38 ++++++++++++++++++++++++++++++++++- cmd/dbgp/repl.go | 14 +++++++++---- cmd/dbgp/repl_test.go | 35 ++++++++++++++++++++++++++++++++ session.go | 14 +++++++++++-- session_test.go | 28 ++++++++++++++++++++++++++ 6 files changed, 134 insertions(+), 12 deletions(-) diff --git a/cmd/dbgp/mcp_sessions.go b/cmd/dbgp/mcp_sessions.go index 48f1062..e697477 100644 --- a/cmd/dbgp/mcp_sessions.go +++ b/cmd/dbgp/mcp_sessions.go @@ -52,12 +52,15 @@ func (t *tools) addSessionTools(server *mcp.Server) { Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true}, }, t.waitForStop) mcp.AddTool(server, &mcp.Tool{ - Name: "stop", - Description: "End a session: the script stops where it is.", + Name: "stop", + Description: "End a session: the script stops where it is. Only while it is stopped: PHP takes no commands " + + "while the script runs, so a running script, e.g. one stuck in a loop, cannot be stopped; detach it instead.", }, t.stopSession) mcp.AddTool(server, &mcp.Tool{ - Name: "detach", - Description: "Stop debugging a session and let the script run on to its end without the debugger.", + Name: "detach", + Description: "Stop debugging a session and let the script run on to its end without the debugger. " + + "Also works while the script is running, e.g. stuck in a loop: dbgp disconnects from it, " + + "which frees the session, but the script keeps running until it ends or you end its PHP process.", }, t.detachSession) } @@ -278,7 +281,11 @@ func (t *tools) stopSession(_ context.Context, req *mcp.CallToolRequest, in sess if err != nil { return nil, stateOutput{}, err } - if err := s.Stop(); err != nil { + if err := s.Stop(); errors.Is(err, dbgp.ErrRunning) { + return nil, stateOutput{}, fmt.Errorf("session %d is running, and PHP takes no commands while the script runs, "+ + "so dbgp cannot stop it: call wait_for_stop to wait for it to stop, or detach to let it run on "+ + "without the debugger, then end its PHP process if it does not finish", s.ID()) + } else if err != nil { return nil, stateOutput{}, sessionError(s, err) } return nil, toState(s, s.State()), nil diff --git a/cmd/dbgp/mcp_sessions_test.go b/cmd/dbgp/mcp_sessions_test.go index ea7443a..6b768fb 100644 --- a/cmd/dbgp/mcp_sessions_test.go +++ b/cmd/dbgp/mcp_sessions_test.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" "os/exec" "path/filepath" "strings" @@ -143,7 +144,7 @@ func TestMCPRunningSession(t *testing.T) { if msg := callToolError(t, cs, "continue", nil); !strings.Contains(msg, "session 1 is still running: call wait_for_stop first") { t.Errorf("continue while running: %q", msg) } - if msg := callToolError(t, cs, "stop", nil); !strings.Contains(msg, "still running") { + if msg := callToolError(t, cs, "stop", nil); !strings.Contains(msg, "dbgp cannot stop it: call wait_for_stop") { t.Errorf("stop while running: %q", msg) } callTool(t, cs, "wait_for_stop", map[string]any{"wait": 10}, &st) @@ -156,6 +157,41 @@ func TestMCPRunningSession(t *testing.T) { } } +// A script stuck in a loop never stops, so it cannot be stopped, but +// detach lets it go: the session ends, and the script runs on without the +// debugger. +func TestMCPDetachRunawayScript(t *testing.T) { + requireEngine(t) + cs, srv := newTestMCP(t) + callTool(t, cs, "listen", nil, &listenOutput{}) + counter := filepath.Join(t.TempDir(), "counter") + startPHP(t, srv.Port(), "-r", fmt.Sprintf(`for ($i = 0; ; $i++) { file_put_contents(%q, $i); usleep(10000); }`, counter)) + var waited waitForSessionOutput + callTool(t, cs, "wait_for_session", map[string]any{"wait": 10}, &waited) + if !waited.Connected { + t.Fatal("PHP did not connect") + } + var st stateOutput + callTool(t, cs, "continue", map[string]any{"wait": 0.2}, &st) + if st.Status != "running" { + t.Fatalf("continue = %+v, want running", st) + } + if msg := callToolError(t, cs, "stop", nil); !strings.Contains(msg, "detach to let it run on") { + t.Errorf("stop while running: %q", msg) + } + + callTool(t, cs, "detach", nil, &st) + if st.Session != 1 || st.Status != "ended" { + t.Errorf("detach while running = %+v, want session 1 ended", st) + } + count := func() string { data, _ := os.ReadFile(counter); return string(data) } + before := count() + time.Sleep(300 * time.Millisecond) + if after := count(); after == before { + t.Errorf("the script stopped counting at %q after detach, want it to run on", before) + } +} + // waitSessions waits up to 10s for srv to have n sessions. func waitSessions(t *testing.T, srv *dbgp.Server, n int) { t.Helper() diff --git a/cmd/dbgp/repl.go b/cmd/dbgp/repl.go index 1cfdfe7..d4fc9b4 100644 --- a/cmd/dbgp/repl.go +++ b/cmd/dbgp/repl.go @@ -444,7 +444,8 @@ func continueWith(command string) func(r *repl, ctx context.Context, args string } st, err := s.Continue(ctx, command, forever) if errors.Is(err, context.Canceled) { - r.printf("Interrupted; the script is still running. The next command waits for it to stop.\n") + r.printf("Interrupted; the script is still running. The next command waits for it to stop,\n" + + "except detach, which lets it run on without the debugger.\n") return nil } if err != nil { @@ -969,9 +970,14 @@ func (r *repl) cmdStop(ctx context.Context, _ string) error { } func (r *repl) cmdDetach(ctx context.Context, _ string) error { - s, err := r.live(ctx) - if err != nil { - return err + // A running script is detached at once, not waited for: it may never + // stop, e.g. stuck in a loop. + s := r.session() + if s == nil || s.State().Status != dbgp.StatusRunning { + var err error + if s, err = r.live(ctx); err != nil { + return err + } } if err := s.Detach(); err != nil { return err diff --git a/cmd/dbgp/repl_test.go b/cmd/dbgp/repl_test.go index 7afb36d..98bc9c9 100644 --- a/cmd/dbgp/repl_test.go +++ b/cmd/dbgp/repl_test.go @@ -217,6 +217,41 @@ func TestCommandsWaitForSessionUntilInterrupted(t *testing.T) { } } +// A script stuck in a loop: after Ctrl-C, detach lets it go at once +// rather than waiting for it to stop, as other commands do. +func TestDetachRunawayScript(t *testing.T) { + requireEngine(t) + r, srv, output := newTestREPL(t) + go r.announceSessions() + startPHP(t, srv.Port(), "-r", `for (;;) { usleep(10000); }`) + interrupts := make(chan os.Signal, 1) + go r.cancelOnInterrupt(interrupts) + go func() { + time.Sleep(500 * time.Millisecond) + interrupts <- os.Interrupt + }() + r.execLine("run") + if out := output(); !strings.Contains(out, "except detach, which lets it run on without the debugger") { + t.Fatalf("run, then Ctrl-C:\n%s", out) + } + + done := make(chan struct{}) + go func() { r.execLine("detach"); close(done) }() + select { + case <-done: + case <-time.After(5 * time.Second): + interrupts <- os.Interrupt + <-done + t.Fatalf("detach waited for the script to stop:\n%s", output()) + } + if out := output(); !strings.Contains(out, "Detached session 1; the script runs on.") { + t.Errorf("detach:\n%s", out) + } + if st := srv.Session(1).State(); !st.Closed { + t.Errorf("session 1 after detach = %+v, want closed", st) + } +} + // requireEngine skips unless php has PHP Debugger or Xdebug, like the // library's integration tests. func requireEngine(t *testing.T) { diff --git a/session.go b/session.go index b7f023d..8ab4564 100644 --- a/session.go +++ b/session.go @@ -830,7 +830,9 @@ func (s *Session) ListBreakpoints() ([]BreakpointInfo, error) { // Stop ends the script and the session. Stopped mid-script, the engine // waits for the IDE to close the connection before PHP exits, so Stop closes -// it, and returns once the session has ended. +// it, and returns once the session has ended. A running script reads no +// commands, so Stop cannot end it and returns ErrRunning; Detach can let it +// go. func (s *Session) Stop() error { if _, err := s.sendCommand("stop", nil, nil); err != nil { return err @@ -840,9 +842,17 @@ func (s *Session) Stop() error { return nil } -// Detach stops debugging and lets the script run to completion. +// Detach stops debugging and lets the script run to completion. The engine +// reads commands only while the script is stopped, so a running script, e.g. +// one stuck in a loop, is detached by closing the connection instead: the +// engine then carries on without the debugger. func (s *Session) Detach() error { _, err := s.sendCommand("detach", nil, nil) + if errors.Is(err, ErrRunning) { + _ = s.conn.Close() + <-s.closed + return nil + } return err } diff --git a/session_test.go b/session_test.go index 5bb31e3..857436e 100644 --- a/session_test.go +++ b/session_test.go @@ -873,6 +873,34 @@ func TestStopEndsTheSession(t *testing.T) { } } +// A running script reads no commands, e.g. one stuck in a loop: Stop +// cannot end it, but Detach lets it go by closing the connection, so the +// engine carries on without the debugger. +func TestDetachRunningScript(t *testing.T) { + handlers := standardHandlers() + handlers["run"] = func(*fakeEngine, fakeCommand) []string { return nil } // never stops + s, e := startFakeSession(t, handlers) + if st, err := s.Continue(context.Background(), ContinueRun, 50*time.Millisecond); err != nil || st.Status != StatusRunning { + t.Fatalf("Continue = %+v, %v; want running", st, err) + } + if err := s.Stop(); !errors.Is(err, ErrRunning) { + t.Errorf("Stop while running = %v, want ErrRunning", err) + } + e.ClearReceived() + if err := s.Detach(); err != nil { + t.Fatalf("Detach while running: %v", err) + } + if st := s.State(); !st.Closed { + t.Errorf("State after Detach = %+v, want closed", st) + } + if !e.WaitClosed(3 * time.Second) { + t.Error("the engine's connection is still open after Detach") + } + if got := e.Received(); len(got) != 0 { + t.Errorf("Detach sent %v to an engine that reads no commands", got) + } +} + func TestOutputIsCaptured(t *testing.T) { s, e := startFakeSession(t, standardHandlers()) stream := func(text string) string { From a157a8812d08beb70f66df730c9811acffd3dc69 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 18 Oct 2026 22:35:10 +0200 Subject: [PATCH 16/19] Unclaimed sessions --- cmd/dbgp/mcp.go | 15 ++++++-- cmd/dbgp/mcp_sessions.go | 6 +++- cmd/dbgp/mcp_sessions_test.go | 43 +++++++++++++++++++++++ cmd/dbgp/mcp_test.go | 8 ++++- server.go | 40 +++++++++++++++++++++ server_test.go | 65 +++++++++++++++++++++++++++++++++++ 6 files changed, 173 insertions(+), 4 deletions(-) diff --git a/cmd/dbgp/mcp.go b/cmd/dbgp/mcp.go index 5eef996..c3656da 100644 --- a/cmd/dbgp/mcp.go +++ b/cmd/dbgp/mcp.go @@ -12,6 +12,7 @@ import ( "strings" "sync" "syscall" + "time" "unicode/utf8" "github.com/google/jsonschema-go/jsonschema" @@ -19,6 +20,11 @@ import ( dbgp "github.com/php-debugger/dbgp-client" ) +// claimTimeout is how long a session waits for an agent to take it before +// it is detached: PHP Debugger debugs every request, so while dbgp listens, +// requests nobody debugs would otherwise hold their PHP workers. +const claimTimeout = time.Minute + // mcpInstructions are sent to MCP clients when they connect. const mcpInstructions = `dbgp debugs PHP scripts and requests through PHP Debugger or Xdebug. It does not listen for PHP connections until the listen tool is called, so @@ -34,7 +40,10 @@ stop, so it does not finish while you are debugging it. Once stopped, look around with stack, variables, variable and source, and step or run on with continue. output and warnings show what the script printed and the PHP warnings it raised. End every session with stop or detach when you are done -with it: until then its PHP process or web server worker is held. +with it: until then its PHP process or web server worker is held. A session +that neither wait_for_session nor a tool naming it by id takes within a minute +of connecting is detached, so its script runs on without the debugger: call +wait_for_session right after starting the PHP code. The session tools act on the session wait_for_session last returned, or the last one you named by id. Each session shows the address PHP connected from: @@ -174,7 +183,9 @@ func runMCP(args []string) int { } } - srv, err := dbgp.NewServer(dbgp.Config{Addr: phpAddr(*phpPort), IDEKey: *idekey, PathMap: pathMap}) + srv, err := dbgp.NewServer(dbgp.Config{ + Addr: phpAddr(*phpPort), IDEKey: *idekey, PathMap: pathMap, ClaimTimeout: claimTimeout, + }) if err != nil { fmt.Fprintln(os.Stderr, err) return 2 diff --git a/cmd/dbgp/mcp_sessions.go b/cmd/dbgp/mcp_sessions.go index e697477..92f0db8 100644 --- a/cmd/dbgp/mcp_sessions.go +++ b/cmd/dbgp/mcp_sessions.go @@ -36,7 +36,8 @@ func (t *tools) addSessionTools(server *mcp.Server) { Name: "wait_for_session", Description: "Wait for PHP to connect and return the session, stopped at the start of the script. " + "Returns the oldest session this tool has not returned yet, so one that connected earlier is not missed, " + - "and makes it the session the other tools act on by default. Call listen first, then start the PHP code.", + "and makes it the session the other tools act on by default. Call listen first, then start the PHP code. " + + "A session not taken within a minute of connecting is detached, so its script runs on without the debugger.", Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true}, }, t.waitForSession) mcp.AddTool(server, &mcp.Tool{ @@ -111,6 +112,9 @@ func (t *tools) session(req *mcp.CallToolRequest, id int) (*dbgp.Session, error) if s == nil { return nil, fmt.Errorf("no session %d", id) } + // Named by id, it is taken: wait_for_session will not return it, and it + // is not detached for waiting too long. + t.srv.Claim(s) a.selected = id return s, nil } diff --git a/cmd/dbgp/mcp_sessions_test.go b/cmd/dbgp/mcp_sessions_test.go index 6b768fb..ecedbdb 100644 --- a/cmd/dbgp/mcp_sessions_test.go +++ b/cmd/dbgp/mcp_sessions_test.go @@ -203,6 +203,49 @@ func waitSessions(t *testing.T, srv *dbgp.Server, n int) { } } +// A session no agent takes is detached once the claim timeout passes, so +// its script runs on; one an agent names by id is taken, and kept. +func TestMCPDetachesUnclaimedSessions(t *testing.T) { + requireEngine(t) + const timeout = time.Second + cs, srv := newTestMCPWith(t, dbgp.Config{Addr: "127.0.0.1:0", ClaimTimeout: timeout}, true) + callTool(t, cs, "listen", nil, &listenOutput{}) + done := filepath.Join(t.TempDir(), "done") + startPHP(t, srv.Port(), "-r", fmt.Sprintf(`file_put_contents(%q, "done");`, done)) + waitSessions(t, srv, 1) + startPHP(t, srv.Port(), "-r", `echo "hi\n";`) + waitSessions(t, srv, 2) + callTool(t, cs, "output", map[string]any{"session": 2}, &outputOutput{}) + + select { + case <-srv.Session(1).Done(): + case <-time.After(timeout + 5*time.Second): + t.Fatal("the unclaimed session was not detached") + } + for deadline := time.Now().Add(5 * time.Second); ; time.Sleep(20 * time.Millisecond) { + if _, err := os.Stat(done); err == nil { + break + } else if time.Now().After(deadline) { + t.Fatal("the unclaimed session's script did not run on after detach") + } + } + + time.Sleep(timeout) + if srv.Session(2).State().Closed { + t.Fatal("the session named by id was detached") + } + var waited waitForSessionOutput + callTool(t, cs, "wait_for_session", map[string]any{"wait": 0.2}, &waited) + if waited.Connected { + t.Errorf("wait_for_session returned session %d, want none left", waited.Session.ID) + } + var st stateOutput + callTool(t, cs, "continue", nil, &st) + if st.Session != 2 || st.Status != "stopping" && st.Status != "ended" { + t.Errorf("continue = %+v, want session 2 run to its end", st) + } +} + // The session tools act on the session the agent picked, not the newest: // anything that can reach dbgp could connect and become that. Sessions show // where they connected from. diff --git a/cmd/dbgp/mcp_test.go b/cmd/dbgp/mcp_test.go index c7049b2..d7ae619 100644 --- a/cmd/dbgp/mcp_test.go +++ b/cmd/dbgp/mcp_test.go @@ -28,7 +28,13 @@ func newTestMCP(t *testing.T) (*mcp.ClientSession, *dbgp.Server) { // only if allowCode. func newTestMCPOn(t *testing.T, addr string, allowCode bool) (*mcp.ClientSession, *dbgp.Server) { t.Helper() - srv, err := dbgp.NewServer(dbgp.Config{Addr: addr}) + return newTestMCPWith(t, dbgp.Config{Addr: addr}, allowCode) +} + +// newTestMCPWith is newTestMCPOn with the server configured by cfg. +func newTestMCPWith(t *testing.T, cfg dbgp.Config, allowCode bool) (*mcp.ClientSession, *dbgp.Server) { + t.Helper() + srv, err := dbgp.NewServer(cfg) if err != nil { t.Fatal(err) } diff --git a/server.go b/server.go index bb66900..a4bc740 100644 --- a/server.go +++ b/server.go @@ -54,6 +54,11 @@ type Config struct { // PathMap maps local directories to the engine's, for PHP running in a // container or on a remote server. Sessions take and return local paths. PathMap []PathMapping + // ClaimTimeout, when set, detaches sessions that neither WaitForSession + // nor Claim has taken within this time of connecting, so their scripts + // run on without the debugger instead of waiting for someone to debug + // them. Default: sessions wait until taken. + ClaimTimeout time.Duration } // Limits on connections, so that a flood of them, from anything that can @@ -365,6 +370,21 @@ func (s *Server) handshake(conn net.Conn) { s.pruneSessions() close(s.arrived) s.arrived = make(chan struct{}) + if s.cfg.ClaimTimeout > 0 { + time.AfterFunc(s.cfg.ClaimTimeout, func() { s.detachUnclaimed(sess) }) + } +} + +// detachUnclaimed detaches sess if nobody has taken it, so its script runs +// on instead of waiting. +func (s *Server) detachUnclaimed(sess *Session) { + s.mu.Lock() + unclaimed := !s.closed && s.unqueueLocked(sess) + s.mu.Unlock() + if unclaimed { + _ = sess.Detach() + _ = sess.Close() + } } // detachConn lets the script on conn run on without the debugger. @@ -528,6 +548,26 @@ func (s *Server) WaitForSession(ctx context.Context) (*Session, error) { } } +// Claim takes sess as if WaitForSession had returned it, so WaitForSession +// does not return it and Config.ClaimTimeout no longer applies to it. It +// reports whether sess was still waiting to be taken. +func (s *Server) Claim(sess *Session) bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.unqueueLocked(sess) +} + +// unqueueLocked removes sess from the sessions WaitForSession has not +// returned yet, and reports whether it was there. +func (s *Server) unqueueLocked(sess *Session) bool { + i := slices.Index(s.queue, sess) + if i < 0 { + return false + } + s.queue = slices.Delete(s.queue, i, i+1) + return true +} + // AddBreakpoint adds a breakpoint to every session: now for sessions that // are stopped, and on their next Continue for sessions that are running. // The returned error lists sessions that rejected it. diff --git a/server_test.go b/server_test.go index a1a67fa..85f78a0 100644 --- a/server_test.go +++ b/server_test.go @@ -841,6 +841,71 @@ func TestServerLimitsLiveSessions(t *testing.T) { } } +// waitForSessionID waits up to 5s for srv to have session id, without +// taking it. +func waitForSessionID(t *testing.T, srv *Server, id int) *Session { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + if sess := srv.Session(id); sess != nil { + return sess + } + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("no session %d", id) + return nil +} + +// Sessions nobody takes within ClaimTimeout are detached, so their scripts +// run on; taken ones, by WaitForSession or Claim, are kept. +func TestServerDetachesUnclaimedSessions(t *testing.T) { + const timeout = 200 * time.Millisecond + srv := newTestServer(t, Config{ClaimTimeout: timeout}) + + unclaimed := dialFakeEngine(t, srv, fixture(t, "init"), standardHandlers()) + sess := waitForSessionID(t, srv, 1) + if !unclaimed.WaitClosed(3 * time.Second) { + t.Fatal("unclaimed session was not detached") + } + if cmd := unclaimed.Received(); cmd[len(cmd)-1].Name != "detach" { + t.Errorf("unclaimed session got %v, want detach last", cmd) + } + <-sess.Done() + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + if got, err := srv.WaitForSession(ctx); err == nil { + t.Errorf("WaitForSession returned detached session %d", got.ID()) + } + + waited := dialFakeEngine(t, srv, fixture(t, "init"), standardHandlers()) + waitSession(t, srv) + claimed := dialFakeEngine(t, srv, fixture(t, "init"), standardHandlers()) + if !srv.Claim(waitForSessionID(t, srv, 3)) { + t.Error("Claim of a new session = false") + } + if srv.Claim(srv.Session(3)) || srv.Claim(srv.Session(2)) { + t.Error("Claim of a taken session = true") + } + time.Sleep(3 * timeout) + for _, e := range []*fakeEngine{waited, claimed} { + for _, cmd := range e.Received() { + if cmd.Name == "detach" { + t.Errorf("taken session was detached") + } + } + } + for _, id := range []int{2, 3} { + if srv.Session(id).State().Closed { + t.Errorf("session %d ended", id) + } + } + ctx, cancel = context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + if got, err := srv.WaitForSession(ctx); err == nil { + t.Errorf("WaitForSession returned claimed session %d", got.ID()) + } +} + func TestSessionRemoteAddr(t *testing.T) { sess, _ := startFakeSession(t, standardHandlers()) if host, _, _ := net.SplitHostPort(sess.RemoteAddr().String()); host != "127.0.0.1" { From 3023951f8be26bbba61b71283cd9f524e55e6d44 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Mon, 19 Oct 2026 17:56:09 +0200 Subject: [PATCH 17/19] macOS port sharing --- listen_darwin.go | 78 +++++++++++++++++++++++++++++++++++++++++++ listen_darwin_test.go | 65 ++++++++++++++++++++++++++++++++++++ listen_other.go | 9 +++++ server.go | 9 +++++ 4 files changed, 161 insertions(+) create mode 100644 listen_darwin.go create mode 100644 listen_darwin_test.go create mode 100644 listen_other.go diff --git a/listen_darwin.go b/listen_darwin.go new file mode 100644 index 0000000..a995029 --- /dev/null +++ b/listen_darwin.go @@ -0,0 +1,78 @@ +//go:build darwin + +package dbgp + +import ( + "errors" + "net" + "strconv" + "syscall" +) + +// shadowedAddr finds an address another program listens on that l, which +// listens on every address, would share. Linux refuses to listen on every +// address then, but macOS does not, and connections to that address, such +// as PHP's to 127.0.0.1:9003, reach the other program instead. +// Binding to an address on l's port fails only when another socket is bound +// to that exact address: l itself does not get in the way. +func shadowedAddr(l net.Listener) string { + addr, ok := l.Addr().(*net.TCPAddr) + if !ok || !addr.IP.IsUnspecified() { + return "" + } + // Listening on IPv6's :: takes IPv4 addresses too; Go listens there + // even for 0.0.0.0, unless only IPv4 is available. + ipv4Only := addr.IP.To4() != nil + for _, ip := range localIPs() { + if ipv4Only && ip.To4() == nil { + continue + } + if bindInUse(ip, addr.Port) { + return net.JoinHostPort(ip.String(), strconv.Itoa(addr.Port)) + } + } + return "" +} + +// localIPs returns this machine's addresses, except link-local ones, +// always including the loopback addresses. +func localIPs() []net.IP { + ips := []net.IP{net.IPv4(127, 0, 0, 1), net.IPv6loopback} + addrs, _ := net.InterfaceAddrs() + for _, a := range addrs { + ipNet, ok := a.(*net.IPNet) + if !ok || ipNet.IP.IsLinkLocalUnicast() || ipNet.IP.IsLoopback() { + continue + } + ips = append(ips, ipNet.IP) + } + return ips +} + +// bindInUse reports whether binding a TCP socket to ip and port fails +// because another socket is bound there. The socket is only bound, never +// listening, so no connection reaches it. +func bindInUse(ip net.IP, port int) bool { + family := syscall.AF_INET6 + var sa syscall.Sockaddr + if ip4 := ip.To4(); ip4 != nil { + family = syscall.AF_INET + sa4 := &syscall.SockaddrInet4{Port: port} + copy(sa4.Addr[:], ip4) + sa = sa4 + } else { + sa6 := &syscall.SockaddrInet6{Port: port} + copy(sa6.Addr[:], ip.To16()) + sa = sa6 + } + fd, err := syscall.Socket(family, syscall.SOCK_STREAM, 0) + if err != nil { + return false + } + defer syscall.Close(fd) + // As Go sets on its listeners, so that only an exact match conflicts. + if err := syscall.SetsockoptInt(fd, syscall.SOL_SOCKET, syscall.SO_REUSEADDR, 1); err != nil { + return false + } + return errors.Is(syscall.Bind(fd, sa), syscall.EADDRINUSE) +} diff --git a/listen_darwin_test.go b/listen_darwin_test.go new file mode 100644 index 0000000..fe2635f --- /dev/null +++ b/listen_darwin_test.go @@ -0,0 +1,65 @@ +package dbgp + +import ( + "net" + "strings" + "testing" +) + +// macOS lets a server listen on every address while another program +// listens on one of them, which would then get the connections to it: +// StartListening refuses, as Linux does. +func TestListenRefusesShadowedAddress(t *testing.T) { + hosts := []string{"127.0.0.1", "::1"} + for _, ip := range localIPs() { + if !ip.IsLoopback() && ip.To4() != nil { + hosts = append(hosts, ip.String()) // a LAN address, as PHP in a container may use + break + } + } + for _, host := range hosts { + other, err := net.Listen("tcp", net.JoinHostPort(host, "0")) + if err != nil { + t.Fatal(err) + } + _, port, _ := net.SplitHostPort(other.Addr().String()) + srv, err := NewServer(Config{Addr: ":" + port}) + if err != nil { + t.Fatal(err) + } + err = srv.StartListening() + want := "another program is listening on " + net.JoinHostPort(host, port) + if err == nil || !strings.Contains(err.Error(), want) { + t.Errorf("with %s taken: StartListening = %v, want %q", host, err, want) + } + if srv.Listening() { + t.Errorf("with %s taken: listening", host) + } + + // Once the other program is gone, the port is free again. + _ = other.Close() + if err := srv.StartListening(); err != nil { + t.Errorf("after %s was freed: %v", host, err) + } + _ = srv.Close() + } +} + +// Go listens on IPv6 as well for 0.0.0.0, so a program on ::1 shares it too. +func TestListenOnIPv4AnyRefusesShadowedIPv6(t *testing.T) { + other, err := net.Listen("tcp", "[::1]:0") + if err != nil { + t.Skip("no IPv6 loopback:", err) + } + defer other.Close() + _, port, _ := net.SplitHostPort(other.Addr().String()) + srv, err := NewServer(Config{Addr: "0.0.0.0:" + port}) + if err != nil { + t.Fatal(err) + } + defer srv.Close() + want := "another program is listening on [::1]:" + port + if err := srv.StartListening(); err == nil || !strings.Contains(err.Error(), want) { + t.Errorf("StartListening = %v, want %q", err, want) + } +} diff --git a/listen_other.go b/listen_other.go new file mode 100644 index 0000000..94090d1 --- /dev/null +++ b/listen_other.go @@ -0,0 +1,9 @@ +//go:build !darwin + +package dbgp + +import "net" + +// shadowedAddr is only needed on macOS: Linux refuses to listen on every +// address when another program listens on one of them. +func shadowedAddr(net.Listener) string { return "" } diff --git a/server.go b/server.go index a4bc740..09149be 100644 --- a/server.go +++ b/server.go @@ -193,6 +193,15 @@ func (s *Server) StartListening() error { if len(listeners) == 0 { return fmt.Errorf("listen on %s: %s", s.addr, strings.Join(failures, "; ")) } + for _, listener := range listeners { + if shadowed := shadowedAddr(listener); shadowed != "" { + for _, l := range listeners { + _ = l.Close() + } + return fmt.Errorf("listen on %s: another program is listening on %s, so PHP connecting there would reach it instead", + s.addr, shadowed) + } + } if len(hosts) == 1 { s.addr = listeners[0].Addr().String() From ca13a03e38da81f0c67297de530657042ea6774e Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Mon, 19 Oct 2026 18:01:31 +0200 Subject: [PATCH 18/19] __get side effects --- cmd/dbgp/mcp.go | 18 ++++++++++++------ cmd/dbgp/mcp_test.go | 2 +- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/cmd/dbgp/mcp.go b/cmd/dbgp/mcp.go index c3656da..faa6d0e 100644 --- a/cmd/dbgp/mcp.go +++ b/cmd/dbgp/mcp.go @@ -45,6 +45,12 @@ that neither wait_for_session nor a tool naming it by id takes within a minute of connecting is detached, so its script runs on without the debugger: call wait_for_session right after starting the PHP code. +Reading an object property that does not exist, with variable or +variable_value, makes PHP call the object's __isset and __get methods: the +script's own code runs, and may change its state or have other side effects. +Listing variables and reading properties that exist run no code, so take +property names from what variables and variable list, and do not guess them. + The session tools act on the session wait_for_session last returned, or the last one you named by id. Each session shows the address PHP connected from: 127.0.0.1 or ::1 for PHP on this machine. dbgp accepts PHP connections from @@ -72,8 +78,8 @@ host.docker.internal from a Docker container.` // -no-eval, so agents know the code tools were left out on purpose. const noCodeInstructions = ` -The user started dbgp with -no-eval, which turns off everything that runs PHP -code in the debugged script: the eval and set_variable tools, and conditions on +The user started dbgp with -no-eval, which turns off running PHP code you +write in the debugged script: the eval and set_variable tools, and conditions on breakpoints. So you cannot evaluate PHP, change variables or set conditional breakpoints. This is the user's choice, not a missing feature or an outdated build: do not suggest rebuilding or restarting dbgp to get them. Read values @@ -123,7 +129,7 @@ func runMCP(args []string) int { phpPort := fs.Int("php-port", defaultPHPPort, "port to listen on for PHP connections, on every address of this machine") idekey := fs.String("idekey", "", "accept only sessions with this IDE key") listen := fs.Bool("listen", false, "listen for PHP connections from the start, instead of waiting for the listen tool") - noEval := fs.Bool("no-eval", false, "turn off everything that runs PHP code in the debugged script: the eval and set_variable tools, and breakpoint conditions") + noEval := fs.Bool("no-eval", false, "turn off running PHP code the agent writes in the debugged script: the eval and set_variable tools, and breakpoint conditions") mcpPort := fs.Int("mcp-port", defaultHTTPPort, "with start: the port agents connect to over HTTP, on "+httpHost+" only") var maps listFlag fs.Var(&maps, "map", "path mapping `LOCAL=REMOTE` for PHP in a container or on a server (repeatable)") @@ -211,8 +217,8 @@ func runMCP(args []string) int { } // newMCPServer returns an MCP server whose tools drive srv. allowCode adds -// what runs PHP code in the debugged script: the code tools, and conditions -// on breakpoints. shared is for the background server, which agents share. +// what runs the agent's PHP code in the debugged script: the code tools, and +// conditions on breakpoints. shared is for the background server, which agents share. // The tools are returned too, for what they keep about agents. func newMCPServer(srv *dbgp.Server, allowCode, shared bool) (*mcp.Server, *tools) { instructions := mcpInstructions @@ -343,7 +349,7 @@ func pageOf[T any](items []T, n int) ([]T, int, error) { // tools implements the MCP tools on a dbgp server. type tools struct { srv *dbgp.Server - // allowCode allows what runs PHP code in the debugged script. + // allowCode allows what runs the agent's PHP code in the debugged script. allowCode bool // mu guards agents, by agent id: see agentID. diff --git a/cmd/dbgp/mcp_test.go b/cmd/dbgp/mcp_test.go index d7ae619..1238ac7 100644 --- a/cmd/dbgp/mcp_test.go +++ b/cmd/dbgp/mcp_test.go @@ -127,7 +127,7 @@ func TestMCPTools(t *testing.T) { t.Error("status has no output schema") } for _, want := range []string{"listen", "php -v", "with PHP Debugger", "do not set xdebug.mode", "remove_breakpoint", - "in the background", "stop or detach", "in parts"} { + "in the background", "stop or detach", "in parts", "__get"} { if !strings.Contains(cs.InitializeResult().Instructions, want) { t.Errorf("instructions lack %q: %s", want, cs.InitializeResult().Instructions) } From 5b2ec2f484674359770fd02903b2cfa17bc53193 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Mon, 19 Oct 2026 18:41:24 +0200 Subject: [PATCH 19/19] Sessions left behind --- cmd/dbgp/mcp.go | 49 ++++++++++++++++++--- cmd/dbgp/mcp_sessions.go | 4 +- cmd/dbgp/mcp_sessions_test.go | 80 ++++++++++++++++++++++++++++++----- 3 files changed, 115 insertions(+), 18 deletions(-) diff --git a/cmd/dbgp/mcp.go b/cmd/dbgp/mcp.go index faa6d0e..4f928d7 100644 --- a/cmd/dbgp/mcp.go +++ b/cmd/dbgp/mcp.go @@ -365,6 +365,9 @@ type agentState struct { // wait_for_session last returned or a tool was last given by id. Not // simply the newest, which anything that connects would become. selected int + // taken holds the ids of the sessions the agent has taken, with + // wait_for_session or by naming them, to detach when it leaves. + taken map[int]bool // output and warnings return only what is new since their last call, // so they keep read positions per session id. outputFrom, warningsFrom map[int]int @@ -382,26 +385,60 @@ func (t *tools) agent(req *mcp.CallToolRequest) *agentState { if t.agents == nil { t.agents = map[string]*agentState{} } - a = &agentState{outputFrom: map[int]int{}, warningsFrom: map[int]int{}} + a = &agentState{taken: map[int]bool{}, outputFrom: map[int]int{}, warningsFrom: map[int]int{}} t.agents[id] = a } return a } +// take makes session id a's selected session, and one it has taken. +func (a *agentState) take(id int) { + a.selected = id + a.taken[id] = true +} + // leave forgets the agent with the given id, which has gone: it no longer -// keeps dbgp listening. +// keeps dbgp listening, and the sessions it took are detached, so their +// scripts run on instead of holding their PHP workers, unless another agent +// has taken them too. func (t *tools) leave(id string) error { t.mu.Lock() - defer t.mu.Unlock() a := t.agents[id] if a == nil { + t.mu.Unlock() return nil } delete(t.agents, id) - if !a.listening || t.listeners() > 0 { - return nil + var orphans []int + for sid := range a.taken { + if !t.taken(sid) { + orphans = append(orphans, sid) + } + } + var err error + if a.listening && t.listeners() == 0 { + err = t.srv.StopListening() + } + t.mu.Unlock() + + // Outside t.mu: detaching a stopped session waits for PHP's answer. + for _, sid := range orphans { + if s := t.srv.Session(sid); s != nil && !ended(s.State()) { + _ = s.Detach() + _ = s.Close() + } + } + return err +} + +// taken reports whether any agent has taken session id. t.mu must be held. +func (t *tools) taken(id int) bool { + for _, a := range t.agents { + if a.taken[id] { + return true + } } - return t.srv.StopListening() + return false } // listeners returns how many agents have called listen and not unlisten. diff --git a/cmd/dbgp/mcp_sessions.go b/cmd/dbgp/mcp_sessions.go index 92f0db8..db53a8f 100644 --- a/cmd/dbgp/mcp_sessions.go +++ b/cmd/dbgp/mcp_sessions.go @@ -115,7 +115,7 @@ func (t *tools) session(req *mcp.CallToolRequest, id int) (*dbgp.Session, error) // Named by id, it is taken: wait_for_session will not return it, and it // is not detached for waiting too long. t.srv.Claim(s) - a.selected = id + a.take(id) return s, nil } @@ -124,7 +124,7 @@ func (t *tools) session(req *mcp.CallToolRequest, id int) (*dbgp.Session, error) func (t *tools) selectSession(req *mcp.CallToolRequest, s *dbgp.Session) { t.mu.Lock() defer t.mu.Unlock() - t.agent(req).selected = s.ID() + t.agent(req).take(s.ID()) } // waitDuration converts a wait in seconds, 0 meaning the default. diff --git a/cmd/dbgp/mcp_sessions_test.go b/cmd/dbgp/mcp_sessions_test.go index ecedbdb..0fbf24a 100644 --- a/cmd/dbgp/mcp_sessions_test.go +++ b/cmd/dbgp/mcp_sessions_test.go @@ -403,6 +403,74 @@ func TestMCPAgentsSharingServer(t *testing.T) { // On the background server, dbgp listens while any agent that called // listen has not called unlisten, or left: one agent does not stop PHP // connections that another is waiting for. +// leaveAs tells the server at url that agent has gone, as its dbgp mcp +// does when it ends. +func leaveAs(t *testing.T, url, agent string) { + t.Helper() + client, err := tokenClient(url, "token", agent) + if err != nil { + t.Fatal(err) + } + leaveServer(client, url) +} + +// An agent that leaves without ending its sessions does not hold their PHP +// processes: they are detached, unless another agent took them too. +func TestMCPLeaveDetachesSessions(t *testing.T) { + requireEngine(t) + url, srv := newSharedMCP(t, "token") + a := connectAgent(t, url, "token", "agent-a") + b := connectAgent(t, url, "token", "agent-b") + callTool(t, a, "listen", nil, &listenOutput{}) + dir := t.TempDir() + start := func(cs *mcp.ClientSession, name string) int { + t.Helper() + startPHP(t, srv.Port(), "-r", fmt.Sprintf(`file_put_contents(%q, "done");`, filepath.Join(dir, name))) + var waited waitForSessionOutput + callTool(t, cs, "wait_for_session", map[string]any{"wait": 10}, &waited) + if !waited.Connected { + t.Fatalf("%s did not connect", name) + } + return waited.Session.ID + } + detached := func(id int) bool { + t.Helper() + select { + case <-srv.Session(id).Done(): + return true + case <-time.After(5 * time.Second): + return false + } + } + finished := func(name string) bool { + t.Helper() + for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); time.Sleep(20 * time.Millisecond) { + if _, err := os.Stat(filepath.Join(dir, name)); err == nil { + return true + } + } + return false + } + + own := start(a, "own") + shared := start(b, "shared") + callTool(t, a, "output", map[string]any{"session": shared}, &outputOutput{}) + + leaveAs(t, url, "agent-a") + if !detached(own) || !finished("own") { + t.Error("a's session was not detached when a left") + } + select { + case <-srv.Session(shared).Done(): + t.Error("the session b took too was detached when a left") + case <-time.After(300 * time.Millisecond): + } + leaveAs(t, url, "agent-b") + if !detached(shared) || !finished("shared") { + t.Error("the shared session was not detached when b left too") + } +} + func TestMCPListeningPerAgent(t *testing.T) { url, srv := newSharedMCP(t, "token") a := connectAgent(t, url, "token", "agent-a") @@ -429,19 +497,11 @@ func TestMCPListeningPerAgent(t *testing.T) { // An agent leaving, as its dbgp mcp tells the server when it ends, // counts as unlisten. - leaveAs := func(agent string) { - t.Helper() - client, err := tokenClient(url, "token", agent) - if err != nil { - t.Fatal(err) - } - leaveServer(client, url) - } - leaveAs("agent-b") + leaveAs(t, url, "agent-b") if !srv.Listening() { t.Error("b leaving stopped a's listening") } - leaveAs("agent-a") + leaveAs(t, url, "agent-a") if srv.Listening() { t.Error("still listening after a, the only agent listening, left") }