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 new file mode 100644 index 0000000..c6445b1 --- /dev/null +++ b/.github/workflows/govulncheck.yml @@ -0,0 +1,28 @@ +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@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + 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..fd20800 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -7,23 +7,37 @@ 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 - - uses: actions/setup-go@v7 + # Actions are pinned by commit, which a tag is not; Dependabot updates them. + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - go-version-file: go.mod + 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@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + 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 +49,36 @@ 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/" + # 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: 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/.github/workflows/tests.yml b/.github/workflows/tests.yml index d8362b1..1d47cc3 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: | @@ -93,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 133676e..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 @@ -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 @@ -49,9 +51,9 @@ 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. -- `-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,7 +63,9 @@ 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. +`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/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() {} 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 50e28c3..4f928d7 100644 --- a/cmd/dbgp/mcp.go +++ b/cmd/dbgp/mcp.go @@ -8,9 +8,11 @@ import ( "os" "os/signal" "path/filepath" + "slices" "strings" "sync" "syscall" + "time" "unicode/utf8" "github.com/google/jsonschema-go/jsonschema" @@ -18,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 @@ -33,7 +40,22 @@ 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. + +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 +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. @@ -45,20 +67,38 @@ 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. 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 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 +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 @@ -86,11 +126,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") + 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)") fs.Usage = func() { @@ -118,6 +158,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() @@ -125,12 +171,17 @@ 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, " ")) } - 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 } @@ -138,7 +189,9 @@ 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, ClaimTimeout: claimTimeout, + }) if err != nil { fmt.Fprintln(os.Stderr, err) return 2 @@ -150,9 +203,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, *httpAddr) + err = serveHTTP(ctx, server, t, *mcpPort) } else { err = server.Run(ctx, &mcp.StdioTransport{}) } @@ -164,15 +217,20 @@ 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. -func newMCPServer(srv *dbgp.Server, allowCode bool) *mcp.Server { +// 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 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} + t := &tools{srv: srv, allowCode: allowCode} mcp.AddTool(server, &mcp.Tool{ Name: "status", @@ -184,25 +242,30 @@ 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) 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) 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{ @@ -236,7 +299,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 @@ -286,19 +349,125 @@ 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 the agent's PHP code in the debugged script. + allowCode bool + + // 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 + // 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 + // 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{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, 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() + a := t.agents[id] + if a == nil { + t.mu.Unlock() + return nil + } + delete(t.agents, id) + 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 +} - // mu guards the read positions of output and warnings, per session id: - // those tools return only what is new since their last call. - mu sync.Mutex - outputFrom map[int]int - warningsFrom map[int]int +// 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 false +} + +// 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{} 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"` @@ -306,6 +475,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"` @@ -330,7 +500,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,22 +535,32 @@ 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"` } -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 } - return nil, listenOutput{Listening: true, Address: t.srv.Addr(), Warnings: t.srv.ListenWarnings()}, nil + 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 } @@ -393,7 +573,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"` @@ -402,12 +582,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 } @@ -417,6 +602,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 @@ -425,7 +614,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 } @@ -442,11 +631,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) } @@ -485,7 +688,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"` } @@ -498,7 +701,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 440eeb6..bedf8dd 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"` } @@ -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 } @@ -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"` @@ -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_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) + } +} diff --git a/cmd/dbgp/mcp_daemon.go b/cmd/dbgp/mcp_daemon.go index 2aa1a9a..6dd137c 100644 --- a/cmd/dbgp/mcp_daemon.go +++ b/cmd/dbgp/mcp_daemon.go @@ -2,6 +2,11 @@ package main import ( "context" + "crypto/hmac" + "crypto/rand" + "crypto/sha256" + "crypto/subtle" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -12,6 +17,7 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "time" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -20,15 +26,24 @@ 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. +// 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. 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 ( @@ -40,6 +55,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 +103,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. @@ -105,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 { @@ -116,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) @@ -136,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 { @@ -169,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 @@ -256,14 +362,21 @@ 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) + // 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 } @@ -291,25 +404,116 @@ func bridge(ctx context.Context, url string) error { return nil } -// 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 { - ln, err := net.Listen("tcp", addr) +// 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, agent: agent}}, nil +} + +// bearerTransport adds the token and the agent id to requests for host. +type bearerTransport struct { + host, token, agent string +} + +func (t *bearerTransport) RoundTrip(req *http.Request) (*http.Response, error) { + if req.URL.Host == t.host { + req = req.Clone(req.Context()) + if t.token != "" { + req.Header.Set("Authorization", "Bearer "+t.token) + } + if t.agent != "" { + req.Header.Set(agentHeader, t.agent) + } + } + 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 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 } - 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 } 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", handler) - 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. @@ -321,3 +525,15 @@ func serveHTTP(ctx context.Context, server *mcp.Server, addr string) 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 a938ce5..680c714 100644 --- a/cmd/dbgp/mcp_daemon_test.go +++ b/cmd/dbgp/mcp_daemon_test.go @@ -2,11 +2,22 @@ package main import ( "context" + "crypto/rand" + "encoding/json" + "errors" + "io" "net" + "net/http" + "net/http/httptest" + "os" "os/exec" "path/filepath" "regexp" + "runtime" + "slices" "strings" + "sync" + "syscall" "testing" "time" @@ -40,7 +51,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) @@ -54,6 +65,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") } @@ -66,11 +80,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 { @@ -116,7 +126,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) } @@ -137,15 +148,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: @@ -165,11 +188,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) @@ -188,13 +207,377 @@ 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()) } // 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, rand.Text()) + 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 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")+"|"+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", "agent-1") + 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|agent-1", "|"}; !slices.Equal(got, want) { + t.Errorf("Authorization|Dbgp-Agent 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") + } +} + +// 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_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_inspect.go b/cmd/dbgp/mcp_inspect.go index b9a1928..40996da 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" @@ -48,7 +50,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"` } @@ -59,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 } @@ -81,7 +83,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"` @@ -143,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 } @@ -164,7 +166,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"` @@ -178,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 } @@ -204,7 +206,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"` @@ -218,23 +220,30 @@ 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 } 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/mcp_output.go b/cmd/dbgp/mcp_output.go index 94c77c9..9e2e9ad 100644 --- a/cmd/dbgp/mcp_output.go +++ b/cmd/dbgp/mcp_output.go @@ -44,22 +44,9 @@ 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"` - File string `json:"file,omitempty" jsonschema:"the file, as a path on this machine; defaults to where the session stopped, or its script"` + 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 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"` } @@ -76,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 } @@ -99,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 { @@ -131,25 +118,27 @@ 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"` } -// 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"` @@ -157,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.latestSession(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) @@ -186,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.latestSession(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 != "= 0; i-- { - if !ended(sessions[i].State()) { - return sessions[i], nil + return addr +} + +// session returns the session with the given id, which it selects for the +// agent that sent req, or by default the agent's selected session, even +// one that has ended. +func (t *tools) session(req *mcp.CallToolRequest, id int) (*dbgp.Session, error) { + t.mu.Lock() + defer t.mu.Unlock() + a := t.agent(req) + if id == 0 { + if a.selected == 0 { + return nil, errors.New("no session: call listen, start the PHP code, then wait_for_session") } + id = a.selected } - return nil, errors.New("no session: call listen, start the PHP code, then wait_for_session") + s := t.srv.Session(id) + 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.take(id) + return s, nil +} + +// selectSession makes s the session the tools act on by default for the +// agent that sent req. +func (t *tools) selectSession(req *mcp.CallToolRequest, s *dbgp.Session) { + t.mu.Lock() + defer t.mu.Unlock() + t.agent(req).take(s.ID()) } // waitDuration converts a wait in seconds, 0 meaning the default. @@ -128,7 +157,7 @@ type waitForSessionOutput struct { Session *sessionInfo `json:"session,omitempty" jsonschema:"the session, when one connected"` } -func (t *tools) waitForSession(ctx context.Context, _ *mcp.CallToolRequest, in waitInput) (*mcp.CallToolResult, waitForSessionOutput, error) { +func (t *tools) waitForSession(ctx context.Context, req *mcp.CallToolRequest, in waitInput) (*mcp.CallToolResult, waitForSessionOutput, error) { wait, err := waitDuration(in.Wait) if err != nil { return nil, waitForSessionOutput{}, err @@ -138,6 +167,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(req, s) info := toSessionInfo(s) return nil, waitForSessionOutput{Connected: true, Session: &info}, nil } @@ -152,16 +182,17 @@ func (t *tools) waitForSession(ctx context.Context, _ *mcp.CallToolRequest, in w if err != nil { return nil, waitForSessionOutput{}, err } + t.selectSession(req, 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,13 +234,13 @@ 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 } -func (t *tools) continueSession(ctx context.Context, _ *mcp.CallToolRequest, in continueInput) (*mcp.CallToolResult, stateOutput, error) { - s, err := t.session(in.Session) +func (t *tools) continueSession(ctx context.Context, req *mcp.CallToolRequest, in continueInput) (*mcp.CallToolResult, stateOutput, error) { + s, err := t.session(req, in.Session) if err != nil { return nil, stateOutput{}, err } @@ -229,12 +260,12 @@ 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"` } -func (t *tools) waitForStop(ctx context.Context, _ *mcp.CallToolRequest, in waitForStopInput) (*mcp.CallToolResult, stateOutput, error) { - s, err := t.session(in.Session) +func (t *tools) waitForStop(ctx context.Context, req *mcp.CallToolRequest, in waitForStopInput) (*mcp.CallToolResult, stateOutput, error) { + s, err := t.session(req, in.Session) if err != nil { return nil, stateOutput{}, err } @@ -249,19 +280,23 @@ func (t *tools) waitForStop(ctx context.Context, _ *mcp.CallToolRequest, in wait return nil, toState(s, st), nil } -func (t *tools) stopSession(_ context.Context, _ *mcp.CallToolRequest, in sessionInput) (*mcp.CallToolResult, stateOutput, error) { - s, err := t.session(in.Session) +func (t *tools) stopSession(_ context.Context, req *mcp.CallToolRequest, in sessionInput) (*mcp.CallToolResult, stateOutput, error) { + s, err := t.session(req, in.Session) 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 } -func (t *tools) detachSession(_ context.Context, _ *mcp.CallToolRequest, in sessionInput) (*mcp.CallToolResult, stateOutput, error) { - s, err := t.session(in.Session) +func (t *tools) detachSession(_ context.Context, req *mcp.CallToolRequest, in sessionInput) (*mcp.CallToolResult, stateOutput, error) { + s, err := t.session(req, in.Session) if err != nil { return nil, stateOutput{}, err } diff --git a/cmd/dbgp/mcp_sessions_test.go b/cmd/dbgp/mcp_sessions_test.go index 27b62f7..0fbf24a 100644 --- a/cmd/dbgp/mcp_sessions_test.go +++ b/cmd/dbgp/mcp_sessions_test.go @@ -1,12 +1,19 @@ package main import ( + "context" "fmt" + "net/http" + "net/http/httptest" + "os" "os/exec" "path/filepath" "strings" "testing" "time" + + "github.com/modelcontextprotocol/go-sdk/mcp" + dbgp "github.com/php-debugger/dbgp-client" ) // startPHP runs php with the debugger pointed at port, killed at cleanup. @@ -110,8 +117,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) } } @@ -136,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) @@ -148,3 +156,364 @@ func TestMCPRunningSession(t *testing.T) { t.Errorf("detach = %+v", st) } } + +// 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() + 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) + } +} + +// 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. +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) + } +} + +// newSharedMCP serves the tools over HTTP as the background server does, +// with token, on a PHP port that is not listening yet. +func newSharedMCP(t *testing.T, token string) (string, *dbgp.Server) { + t.Helper() + srv, err := dbgp.NewServer(dbgp.Config{Addr: phpAddr(0)}) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = srv.Close() }) + server, tl := newMCPServer(srv, true, true) + hs := httptest.NewServer(httpHandler(server, tl, daemonState{Token: token})) + t.Cleanup(hs.Close) + return hs.URL + "/mcp", srv +} + +// connectAgent connects to the server at url as dbgp mcp does, as agent. +func connectAgent(t *testing.T, url, token, agent string) *mcp.ClientSession { + t.Helper() + client, err := tokenClient(url, token, agent) + if err != nil { + t.Fatal(err) + } + cs, err := mcp.NewClient(&mcp.Implementation{Name: agent, Version: "0"}, nil). + Connect(context.Background(), &mcp.StreamableClientTransport{Endpoint: url, HTTPClient: client}, nil) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = cs.Close() }) + return cs +} + +// Agents on the background server are told they share it; an agent on +// stdin and stdout has dbgp to itself. +func TestMCPSharedInstructions(t *testing.T) { + url, _ := newSharedMCP(t, "token") + shared := connectAgent(t, url, "token", "agent").InitializeResult().Instructions + if !strings.HasSuffix(shared, sharedInstructions) { + t.Errorf("background server instructions lack the shared ones:\n%s", shared) + } + own, _ := newTestMCP(t) + if strings.Contains(own.InitializeResult().Instructions, "other agents") { + t.Error("dbgp mcp on stdin and stdout says other agents may share it") + } +} + +// Agents sharing the background server each have their own selected +// session and place in a session's output, so one does not move the +// other's. +func TestMCPAgentsSharingServer(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{}) + startPHP(t, srv.Port(), "-r", `echo "one\n";`) + var waited waitForSessionOutput + callTool(t, a, "wait_for_session", map[string]any{"wait": 10}, &waited) + if !waited.Connected || waited.Session.ID != 1 { + t.Fatalf("a: wait_for_session = %+v", waited) + } + if msg := callToolError(t, b, "continue", nil); !strings.Contains(msg, "no session") { + t.Errorf("b: continue acted on a's session: %q", msg) + } + startPHP(t, srv.Port(), "-r", `echo "two\n";`) + callTool(t, b, "wait_for_session", map[string]any{"wait": 10}, &waited) + if !waited.Connected || waited.Session.ID != 2 { + t.Fatalf("b: wait_for_session = %+v", waited) + } + var st stateOutput + for name, agent := range map[string]*mcp.ClientSession{"a": a, "b": b} { + callTool(t, agent, "continue", nil, &st) + if want := map[string]int{"a": 1, "b": 2}[name]; st.Session != want || st.Status != "stopping" { + t.Errorf("%s: continue = %+v, want session %d stopping", name, st, want) + } + } + + // a reads session 1's output; b still gets all of it. + var printed outputOutput + for _, want := range []string{"one\n", ""} { + callTool(t, a, "output", nil, &printed) + if printed.Session != 1 || printed.Output != want { + t.Errorf("a: output = %+v, want session 1 %q", printed, want) + } + } + callTool(t, b, "output", map[string]any{"session": 1}, &printed) + if printed.Output != "one\n" { + t.Errorf("b: output of session 1 = %q, want all of it", printed.Output) + } + // Naming a session selects it for that agent only. + callTool(t, a, "output", map[string]any{"session": 2}, &printed) + callTool(t, b, "output", nil, &printed) + if printed.Session != 1 || printed.Output != "" { + t.Errorf("b: output = %+v, want session 1, which b named last, with nothing new", printed) + } +} + +// 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") + b := connectAgent(t, url, "token", "agent-b") + unlisten := func(name string, cs *mcp.ClientSession, listening bool, others int) { + t.Helper() + var out unlistenOutput + callTool(t, cs, "unlisten", nil, &out) + if out.Listening != listening || out.OtherAgents != others || srv.Listening() != listening { + t.Errorf("%s: unlisten = %+v, server listening %v; want listening %v for %d other agents", + name, out, srv.Listening(), listening, others) + } + } + + callTool(t, a, "listen", nil, &listenOutput{}) + callTool(t, b, "listen", nil, &listenOutput{}) + unlisten("a", a, true, 1) + unlisten("a again", a, true, 1) + unlisten("b", b, false, 0) + + // An agent that did not call listen does not stop it either. + callTool(t, a, "listen", nil, &listenOutput{}) + unlisten("b, not listening", b, true, 1) + + // An agent leaving, as its dbgp mcp tells the server when it ends, + // counts as unlisten. + leaveAs(t, url, "agent-b") + if !srv.Listening() { + t.Error("b leaving stopped a's listening") + } + leaveAs(t, url, "agent-a") + if srv.Listening() { + t.Error("still listening after a, the only agent listening, left") + } + + // Only agents with the token can leave. + callTool(t, a, "listen", nil, &listenOutput{}) + resp, err := http.Post(strings.TrimSuffix(url, "/mcp")+leavePath, "", nil) + if err != nil { + t.Fatal(err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusUnauthorized || !srv.Listening() { + t.Errorf("leave without the token: status %d, listening %v; want 401 and still listening", resp.StatusCode, srv.Listening()) + } +} diff --git a/cmd/dbgp/mcp_test.go b/cmd/dbgp/mcp_test.go index 5555558..1238ac7 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" @@ -27,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) } @@ -35,7 +42,8 @@ func newTestMCPOn(t *testing.T, addr string, allowCode bool) (*mcp.ClientSession ctx := context.Background() clientTransport, serverTransport := mcp.NewInMemoryTransports() - ss, err := newMCPServer(srv, allowCode).Connect(ctx, serverTransport, nil) + server, _ := newMCPServer(srv, allowCode, false) + ss, err := server.Connect(ctx, serverTransport, nil) if err != nil { t.Fatal(err) } @@ -119,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) } @@ -134,7 +142,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 +161,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 +172,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 +292,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...)} @@ -369,13 +378,12 @@ func callToolError(t *testing.T, cs *mcp.ClientSession, name string, args any) s func TestMCPAddBreakpoint(t *testing.T) { cs, _ := newTestMCP(t) - abs, _ := filepath.Abs("app/a.php") tests := []struct { args map[string]any want breakpointInfo }{ {map[string]any{"file": "/app/a.php", "line": 12}, breakpointInfo{ID: 1, Type: "line", File: "/app/a.php", Line: 12}}, - {map[string]any{"type": "line", "file": "app/a.php", "line": 3}, breakpointInfo{ID: 2, Type: "line", File: abs, Line: 3}}, + {map[string]any{"type": "line", "file": "/app/../app/b.php", "line": 3}, breakpointInfo{ID: 2, Type: "line", File: "/app/b.php", Line: 3}}, {map[string]any{"file": "/app/a.php", "line": 5, "condition": "$i > 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"}}, @@ -401,6 +409,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) @@ -435,7 +445,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 { @@ -443,9 +452,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) } @@ -456,6 +465,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. @@ -528,7 +541,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..d4fc9b4 100644 --- a/cmd/dbgp/repl.go +++ b/cmd/dbgp/repl.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "math" + "net" "os" "path/filepath" "slices" @@ -41,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) { @@ -69,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 { @@ -78,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 { @@ -107,13 +113,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 { @@ -335,13 +352,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 +384,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") } @@ -382,7 +399,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 } @@ -427,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 { @@ -774,7 +792,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 } @@ -952,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 7ae8a3b..98bc9c9 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" @@ -216,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) { @@ -429,21 +465,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 +520,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 +556,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 +597,43 @@ 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 + 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") { + 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() + } +} + +// 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/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 := "= s.maxHandshakes { s.mu.Unlock() _ = conn.Close() continue @@ -303,7 +334,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 @@ -317,20 +348,25 @@ 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() 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() @@ -343,6 +379,38 @@ 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. +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. @@ -489,6 +557,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. @@ -537,7 +625,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 4839d64..85f78a0 100644 --- a/server_test.go +++ b/server_test.go @@ -9,6 +9,7 @@ import ( "sort" "strconv" "strings" + "sync" "testing" "time" ) @@ -66,6 +67,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 +350,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 } @@ -566,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 @@ -657,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 } @@ -667,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() { @@ -681,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 @@ -761,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 @@ -771,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 } @@ -862,16 +941,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 5229638..857436e 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" @@ -173,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"}, @@ -489,10 +500,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{ @@ -811,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 { @@ -920,9 +1010,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) {