New security review

.github/workflows/govulncheck.yml

@@ -1,8 +1,9 @@
 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.
+# the standard library: of the oldest Go that go.mod allows, which go install
+# and local builds may use, and of the newest, which release builds use.
+# Weekly too, since new ones are found in code that has not changed.
 on:
   pull_request:
   push:
@@ -16,11 +17,21 @@ permissions:
 jobs:
   govulncheck:
     runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        go: [minimum, stable]
+    name: govulncheck (${{ matrix.go }} Go)
     steps:
       - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
         with:
           persist-credentials: false
-      - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0
+      - if: matrix.go == 'minimum'
+        uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0
+        with:
+          go-version-file: go.mod
+      - if: matrix.go == 'stable'
+        uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0
         with:
           go-version: stable
           check-latest: true

.github/workflows/tests.yml

@@ -34,31 +34,51 @@ jobs:
         run: go vet ./...
 
   test:
-    name: test (PHP ${{ matrix.php }})
-    runs-on: ubuntu-latest
+    # Linux with every PHP version, and Windows with the newest.
+    name: test (${{ matrix.os == 'windows-latest' && 'Windows, ' || '' }}PHP ${{ matrix.php }})
+    runs-on: ${{ matrix.os }}
     strategy:
       fail-fast: false
       matrix:
+        os: [ubuntu-latest]
         php: ['8.2', '8.3', '8.4', '8.5']
+        include:
+          - os: windows-latest
+            php: '8.5'
+    defaults:
+      run:
+        shell: bash
     steps:
       - 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
-      # latest php-debugger release.
+      # PHP Debugger, from its latest release: on Linux, a self-contained PHP
+      # interpreter with it compiled in; on Windows, where the release has only
+      # the extension, the official PHP build with the extension loaded.
       - name: Install PHP Debugger
         env:
           GH_TOKEN: ${{ github.token }}
         run: |
           dir="$RUNNER_TEMP/php-debugger/bin"
           mkdir -p "$dir"
-          gh release download --repo php-debugger/php-debugger \
-            --pattern "php-php${{ matrix.php }}-nts-linux-x86_64" \
-            --output "$dir/php"
-          chmod +x "$dir/php"
+          if [ "$RUNNER_OS" = Windows ]; then
+            curl -fsSL --retry 5 --retry-all-errors -o "$RUNNER_TEMP/php.zip" \
+              "https://downloads.php.net/~windows/releases/latest/php-${{ matrix.php }}-nts-Win32-vs17-x64-latest.zip"
+            unzip -q "$RUNNER_TEMP/php.zip" -d "$dir"
+            gh release download --repo php-debugger/php-debugger \
+              --pattern "php_php-debugger-php${{ matrix.php }}-nts-windows-x64.dll" \
+              --output "$dir/ext/php_php_debugger.dll"
+            # PHP reads the php.ini next to php.exe.
+            printf 'extension_dir="%s"\nzend_extension=php_debugger\n' "$(cygpath -w "$dir/ext")" > "$dir/php.ini"
+          else
+            gh release download --repo php-debugger/php-debugger \
+              --pattern "php-php${{ matrix.php }}-nts-linux-x86_64" \
+              --output "$dir/php"
+            chmod +x "$dir/php"
+          fi
           echo "$dir" >> "$GITHUB_PATH"
       - name: Show PHP and debugger versions
         run: |

README.md

@@ -81,3 +81,10 @@ php app.php
 Each script or request that connects becomes a session, stopped at its start. At the prompt, `run` runs to the next breakpoint, `next`, `step` and `out` step through the code, and `stack`, `vars` and `print $user["name"]` show where it stopped. On a terminal it has line editing, tab completion and history.
 
 Type `help` at the prompt for all the commands, and see `dbgp -h` for the options, such as `-map` for PHP in a container or on a server.
+
+## Security
+
+Like any debugger, dbgp trusts the PHP that connects to it and the agent that uses it:
+
+- **The PHP port.** DBGp has no authentication: anything that can reach the port, 9003 by default, can connect as PHP and show dbgp made-up code, variables and output, which agents then read. dbgp listens on every address of this machine so that PHP in a container or on another machine can connect, so make sure only your PHP hosts and container networks can reach that port, for example with a firewall. `-idekey` accepts only sessions with that IDE key, which keeps out other PHP, but it is sent in plain text and is not a password. To debug PHP across a network you do not trust, connect through an SSH tunnel or a VPN. `dbgp mcp` only listens while an agent is debugging, and every session shows the address it connected from.
+- **The agent.** An agent using dbgp can see everything the debugged script can: its variables, including passwords and keys, and any file PHP can read. `-no-eval` stops the agent from running PHP code of its own, but does not hide anything from it.

cmd/dbgp/mcp.go

@@ -191,6 +191,8 @@ func runMCP(args []string) int {
 
 	srv, err := dbgp.NewServer(dbgp.Config{
 		Addr: phpAddr(*phpPort), IDEKey: *idekey, PathMap: pathMap, ClaimTimeout: claimTimeout,
+		// The tools never read the engine mapping that the probe reports.
+		NoEval: *noEval,
 	})
 	if err != nil {
 		fmt.Fprintln(os.Stderr, err)
@@ -352,9 +354,12 @@ type tools struct {
 	// allowCode allows what runs the agent's PHP code in the debugged script.
 	allowCode bool
 
-	// mu guards agents, by agent id: see agentID.
+	// mu guards agents, by agent id (see agentID), and detaching.
 	mu     sync.Mutex
 	agents map[string]*agentState
+	// detaching holds the ids of the sessions being detached because the
+	// agents that took them left: no agent can take them meanwhile.
+	detaching map[int]bool
 }
 
 // agentState is what the tools keep for one agent. Agents sharing the
@@ -402,33 +407,53 @@ func (a *agentState) take(id int) {
 // scripts run on instead of holding their PHP workers, unless another agent
 // has taken them too.
 func (t *tools) leave(id string) error {
+	orphans, err := t.release(id)
+	t.detachSessions(orphans)
+	return err
+}
+
+// release forgets the agent with the given id, stops listening if it was
+// the last agent listening, and returns the live sessions only it took,
+// marked as detaching, so no other agent takes one before it is detached.
+func (t *tools) release(id string) ([]*dbgp.Session, error) {
 	t.mu.Lock()
+	defer t.mu.Unlock()
 	a := t.agents[id]
 	if a == nil {
-		t.mu.Unlock()
-		return nil
+		return nil, nil
 	}
 	delete(t.agents, id)
-	var orphans []int
+	var orphans []*dbgp.Session
 	for sid := range a.taken {
-		if !t.taken(sid) {
-			orphans = append(orphans, sid)
+		if t.taken(sid) {
+			continue
+		}
+		if s := t.srv.Session(sid); s != nil && !ended(s.State()) {
+			if t.detaching == nil {
+				t.detaching = map[int]bool{}
+			}
+			t.detaching[sid] = true
+			orphans = append(orphans, s)
 		}
 	}
-	var err error
 	if a.listening && t.listeners() == 0 {
-		err = t.srv.StopListening()
+		return orphans, t.srv.StopListening()
 	}
-	t.mu.Unlock()
+	return orphans, nil
+}
 
-	// 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()
-		}
+// detachSessions detaches the sessions release returned, outside t.mu:
+// detaching a stopped session waits for PHP's answer. Once they have ended,
+// agents can name them again, to read their output.
+func (t *tools) detachSessions(sessions []*dbgp.Session) {
+	for _, s := range sessions {
+		_ = s.Detach()
+		_ = s.Close()
+		<-s.Done()
+		t.mu.Lock()
+		delete(t.detaching, s.ID())
+		t.mu.Unlock()
 	}
-	return err
 }
 
 // taken reports whether any agent has taken session id. t.mu must be held.

cmd/dbgp/mcp_daemon.go

@@ -18,6 +18,7 @@ import (
 	"os/exec"
 	"path/filepath"
 	"strconv"
+	"sync"
 	"time"
 
 	"github.com/modelcontextprotocol/go-sdk/mcp"
@@ -375,7 +376,11 @@ func bridge(ctx context.Context, st *daemonState) error {
 	if err != nil {
 		return err
 	}
-	defer leaveServer(client, st.URL)
+	presence, err := attachServer(ctx, client, st.URL)
+	if err != nil {
+		return err
+	}
+	defer presence.Close()
 	remote, err := (&mcp.StreamableClientTransport{Endpoint: st.URL, HTTPClient: client}).Connect(ctx)
 	if err != nil {
 		return err
@@ -404,36 +409,40 @@ func bridge(ctx context.Context, st *daemonState) error {
 	return nil
 }
 
-// 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
+// attachPath is where dbgp mcp tells the server its agent is there, for as
+// long as it runs: it keeps the request open, and the server takes the
+// agent as gone when the request ends. The operating system ends it when
+// dbgp mcp ends, even when it crashes or is killed.
+const attachPath = "/attach"
 
-// 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) {
+// attachServer tells the server at rawURL, through client, that the agent
+// client sends the id of is there, until the returned body is closed or
+// ctx is done.
+func attachServer(ctx context.Context, client *http.Client, rawURL string) (io.Closer, error) {
 	u, err := url.Parse(rawURL)
 	if err != nil {
-		return
+		return nil, err
 	}
-	u.Path = leavePath
-	ctx, cancel := context.WithTimeout(context.Background(), leaveTimeout)
-	defer cancel()
+	u.Path = attachPath
 	req, err := http.NewRequestWithContext(ctx, http.MethodPost, u.String(), nil)
 	if err != nil {
-		return
+		return nil, err
 	}
-	if resp, err := client.Do(req); err == nil {
+	resp, err := client.Do(req)
+	if err != nil {
+		return nil, fmt.Errorf("attach to the background server: %w", err)
+	}
+	if resp.StatusCode != http.StatusOK {
 		_ = resp.Body.Close()
+		return nil, fmt.Errorf("attach to the background server: %s", resp.Status)
 	}
+	return resp.Body, nil
 }
 
-// leave forgets the agent each request is from, by its agentHeader.
-func leave(t *tools) http.Handler {
+// attach holds each request open until the agent it is from, by its
+// agentHeader, has gone, then forgets that agent and closes its MCP
+// sessions.
+func attach(t *tools, sessions *mcpSessions) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		if req.Method != http.MethodPost {
 			w.Header().Set("Allow", http.MethodPost)
@@ -445,14 +454,72 @@ func leave(t *tools) http.Handler {
 			http.Error(w, "no "+agentHeader+" header", http.StatusBadRequest)
 			return
 		}
+		w.WriteHeader(http.StatusOK)
+		_ = http.NewResponseController(w).Flush()
+		// Done when dbgp mcp closes the connection or the server stops.
+		<-req.Context().Done()
 		if err := t.leave(id); err != nil {
-			http.Error(w, err.Error(), http.StatusInternalServerError)
-			return
+			fmt.Fprintf(os.Stderr, "dbgp: agent left: %v\n", err)
 		}
-		w.WriteHeader(http.StatusNoContent)
+		sessions.close(id)
+	})
+}
+
+// mcpSessionHeader carries the id of the MCP session a request belongs to,
+// which the MCP library assigns in its answer to initialize.
+const mcpSessionHeader = "Mcp-Session-Id"
+
+// mcpSessions records which MCP sessions each agent uses, to close them when
+// it leaves: the MCP library keeps a session until its client ends it, which
+// a dbgp mcp that crashed or was killed never does.
+type mcpSessions struct {
+	server *mcp.Server
+
+	mu      sync.Mutex
+	byAgent map[string]map[string]bool // session ids, by agent id
+}
+
+// track passes requests on to next, recording the MCP session each is from
+// for the agent it is from.
+func (m *mcpSessions) track(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+		agent := req.Header.Get(agentHeader)
+		m.add(agent, req.Header.Get(mcpSessionHeader))
+		next.ServeHTTP(w, req)
+		// The answer to initialize, which the session starts with.
+		m.add(agent, w.Header().Get(mcpSessionHeader))
 	})
 }
 
+func (m *mcpSessions) add(agent, session string) {
+	if agent == "" || session == "" {
+		return
+	}
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	if m.byAgent == nil {
+		m.byAgent = map[string]map[string]bool{}
+	}
+	if m.byAgent[agent] == nil {
+		m.byAgent[agent] = map[string]bool{}
+	}
+	m.byAgent[agent][session] = true
+}
+
+// close closes the MCP sessions of the agent with the given id, which has
+// left, and forgets them.
+func (m *mcpSessions) close(agent string) {
+	m.mu.Lock()
+	ids := m.byAgent[agent]
+	delete(m.byAgent, agent)
+	m.mu.Unlock()
+	for ss := range m.server.Sessions() {
+		if ids[ss.ID()] {
+			_ = ss.Close()
+		}
+	}
+}
+
 // agentHeader carries the id of the agent a request is from, which its
 // dbgp mcp chooses: see agentID.
 const agentHeader = "Dbgp-Agent"
@@ -526,14 +593,15 @@ func serveHTTP(ctx context.Context, server *mcp.Server, t *tools, port int) erro
 	return nil
 }
 
-// httpHandler serves MCP at /mcp and agents leaving, to requests with st's
+// httpHandler serves MCP at /mcp and agents' presence, 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()})
+	sessions := &mcpSessions{server: server}
 	mux := http.NewServeMux()
-	mux.Handle("/mcp", requireToken(st.Token, handler))
-	mux.Handle(leavePath, requireToken(st.Token, leave(t)))
+	mux.Handle("/mcp", requireToken(st.Token, sessions.track(handler)))
+	mux.Handle(attachPath, requireToken(st.Token, attach(t, sessions)))
 	mux.Handle(identifyPath, identify(st))
 	return mux
 }