fix(server): address PR #45 review — auth, secret handling, installer, races

- High: require admin on TestTunnel/RestartTunnel/ReconcileWebhooks — these
  decrypt PATs, register GitHub webhooks, restart the subprocess, or probe
  outbound, and were only behind requireAuth (any authenticated user).
  Hide the dashboard "Re-register webhooks" button from non-admins.
- Medium: pass the Cloudflare/ngrok tokens via env (TUNNEL_TOKEN /
  NGROK_AUTHTOKEN) instead of argv so they don't show in ps/proc cmdline.
- Medium: installer no longer tracks "latest" — pin cloudflared version
  (synced with the Dockerfile arg); compute the download SHA-256, verify it
  against a pinned map when present and warn (with the sum) when not; cap the
  download/extraction size to guard against a decompression bomb.
- Low: serialize Reconcile (mutex) so the boot double-reconcile can't create
  duplicate GitHub hooks; serialize Manager.Apply so concurrent applies can't
  orphan a provider; RestartTunnel uses a background-derived ctx so a client
  disconnect doesn't abort the spawn (matching UpdateTunnelConfig); capture
  readySignal under the lock before close to avoid a restart-time race.
- Nits: drop stale CIX_TUNNEL_ENABLED wording from handler messages/comments.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: dvcdsys

server/dashboard/src/modules/github-integration/WebhooksTab.tsx

@@ -2,6 +2,7 @@ import { useEffect, useState } from 'react';
 import { Link } from 'react-router-dom';
 import { AlertCircle, RefreshCw } from 'lucide-react';
 import { api } from '@/api/client';
+import { useAuth } from '@/auth/useAuth';
 import { Alert, AlertDescription, AlertTitle } from '@/ui/alert';
 import { Button } from '@/ui/button';
 import { Card, CardContent, CardDescription, CardHeader, CardTitle } from '@/ui/card';
@@ -35,6 +36,8 @@ const ACTION_COLOR: Record<ReconcileOutcome['action'], string> = {
 // that provides the public URL is managed under Managed Tunnels — this tab
 // only consumes it.
 export default function WebhooksTab() {
+  const { user } = useAuth();
+  const isAdmin = user?.role === 'admin';
   const [tunnel, setTunnel] = useState<TunnelStatus | null>(null);
   const [result, setResult] = useState<ReconcileResult | null>(null);
   const [busy, setBusy] = useState(false);
@@ -98,12 +101,15 @@ export default function WebhooksTab() {
           <div className="flex flex-wrap items-center justify-between gap-3 pt-1">
             <p className="text-xs text-muted-foreground">
               Re-registration runs automatically on boot and when the tunnel URL
-              changes. Use this to trigger it manually.
+              changes.{' '}
+              {isAdmin ? 'Use this to trigger it manually.' : 'Manual re-registration requires an admin.'}
             </p>
-            <Button variant="outline" size="sm" disabled={busy} onClick={reconcile}>
-              <RefreshCw className="mr-1 size-4" />
-              {busy ? 'Re-registering…' : 'Re-register webhooks'}
-            </Button>
+            {isAdmin && (
+              <Button variant="outline" size="sm" disabled={busy} onClick={reconcile}>
+                <RefreshCw className="mr-1 size-4" />
+                {busy ? 'Re-registering…' : 'Re-register webhooks'}
+              </Button>
+            )}
           </div>
 
           {err && (

server/internal/httpapi/tunnels.go

@@ -12,9 +12,9 @@ import (
 	"github.com/dvcdsys/code-index/server/internal/tunnels"
 )
 
-// disabledTunnelStatus is the snapshot returned when no tunnel is wired
-// (CIX_TUNNEL_ENABLED=false). The cloudflare slot is "available but off";
-// ngrok is reported as not-yet-implemented.
+// disabledTunnelStatus is the snapshot returned when the tunnel manager is
+// not wired (no provider running). Both providers are available to enable
+// from the dashboard.
 func disabledTunnelStatus() tunnels.Status {
 	return tunnels.Status{Provider: tunnels.ProviderCloudflare, State: tunnels.StateDisabled, Available: true}
 }
@@ -177,10 +177,14 @@ func (s *Server) GetTunnelStatus(w http.ResponseWriter, _ *http.Request) {
 	writeJSON(w, http.StatusOK, st)
 }
 
-// TestTunnel — POST /api/v1/tunnels/test. End-to-end round-trip probe.
+// TestTunnel — POST /api/v1/tunnels/test. Admin-only. End-to-end round-trip
+// probe (initiates an outbound request through the tunnel).
 func (s *Server) TestTunnel(w http.ResponseWriter, r *http.Request) {
+	if _, ok := s.mustBeAdmin(w, r); !ok {
+		return
+	}
 	if s.Deps.Tunnel == nil {
-		writeError(w, http.StatusConflict, "no active tunnel (CIX_TUNNEL_ENABLED=false)")
+		writeError(w, http.StatusConflict, "no active tunnel (enable it under Managed Tunnels)")
 		return
 	}
 	res, err := s.Deps.Tunnel.TestConnection(r.Context())
@@ -195,13 +199,22 @@ func (s *Server) TestTunnel(w http.ResponseWriter, r *http.Request) {
 	writeJSON(w, http.StatusOK, res)
 }
 
-// RestartTunnel — POST /api/v1/tunnels/restart. Restart the subprocess.
+// RestartTunnel — POST /api/v1/tunnels/restart. Admin-only. Restarts the
+// subprocess (a soft DoS in the wrong hands, hence admin).
 func (s *Server) RestartTunnel(w http.ResponseWriter, r *http.Request) {
+	if _, ok := s.mustBeAdmin(w, r); !ok {
+		return
+	}
 	if s.Deps.Tunnel == nil {
-		writeError(w, http.StatusConflict, "no active tunnel (CIX_TUNNEL_ENABLED=false)")
+		writeError(w, http.StatusConflict, "no active tunnel (enable it under Managed Tunnels)")
 		return
 	}
-	if err := s.Deps.Tunnel.Restart(r.Context()); err != nil {
+	// Restart spawns a subprocess and waits for readiness; drive it on a
+	// background-derived deadline so a client disconnect doesn't abort the
+	// spawn mid-flight (matches UpdateTunnelConfig).
+	ctx, cancel := context.WithTimeout(context.Background(), 45*time.Second)
+	defer cancel()
+	if err := s.Deps.Tunnel.Restart(ctx); err != nil {
 		if errors.Is(err, tunnels.ErrNoActiveTunnel) {
 			writeError(w, http.StatusConflict, "no active tunnel")
 			return
@@ -212,9 +225,13 @@ func (s *Server) RestartTunnel(w http.ResponseWriter, r *http.Request) {
 	writeJSON(w, http.StatusOK, s.Deps.Tunnel.Status())
 }
 
-// ReconcileWebhooks — POST /api/v1/github/webhooks/reconcile. Re-register
-// every webhook_mode=auto repo against the current public base URL.
+// ReconcileWebhooks — POST /api/v1/github/webhooks/reconcile. Admin-only —
+// it decrypts stored PATs and registers webhooks on GitHub across every
+// webhook_mode=auto repo.
 func (s *Server) ReconcileWebhooks(w http.ResponseWriter, r *http.Request) {
+	if _, ok := s.mustBeAdmin(w, r); !ok {
+		return
+	}
 	if s.Deps.WebhookReconciler == nil || s.Deps.GitRepos == nil {
 		writeError(w, http.StatusServiceUnavailable, "GitHub repo support is not configured on this server")
 		return

server/internal/tunnels/cloudflare.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"log/slog"
 	"net/http"
+	"os"
 	"os/exec"
 	"regexp"
 	"strings"
@@ -121,7 +122,9 @@ func (p *cloudflareProvider) argv() []string {
 	case CloudflareModeQuick:
 		args = append(args, "--url", fmt.Sprintf("http://localhost:%d", p.localPort))
 	case CloudflareModeNamed:
-		args = append(args, "run", "--token", p.token)
+		// Token is passed via the TUNNEL_TOKEN env var (see spawn), not argv,
+		// so it doesn't appear in ps / /proc/<pid>/cmdline.
+		args = append(args, "run")
 	}
 	return args
 }
@@ -136,6 +139,11 @@ func (p *cloudflareProvider) spawn(ctx context.Context) error {
 	)
 
 	cmd := exec.Command(p.binPath, argv...)
+	// Pass the named-tunnel token via env (TUNNEL_TOKEN) rather than argv so
+	// it never appears in the process table.
+	if p.token != "" {
+		cmd.Env = append(os.Environ(), "TUNNEL_TOKEN="+p.token)
+	}
 	// cloudflared writes its logs (including the quick-tunnel URL) to stderr.
 	stdoutLog := newLineLogWriter(p.logger, "cloudflared.stdout", p.scanLine)
 	stderrLog := newLineLogWriter(p.logger, "cloudflared.stderr", p.scanLine)
@@ -151,7 +159,8 @@ func (p *cloudflareProvider) spawn(ctx context.Context) error {
 	p.mu.Lock()
 	p.cmd = cmd
 	p.startedAt = time.Now()
-	p.readySignal = make(chan struct{})
+	readyCh := make(chan struct{})
+	p.readySignal = readyCh
 	p.waiterDone = make(chan struct{})
 	// A fresh quick tunnel gets a new URL; clear the stale one so waitReady
 	// blocks until the new URL is parsed.
@@ -171,7 +180,7 @@ func (p *cloudflareProvider) spawn(ctx context.Context) error {
 		<-p.waiterDone
 		return fmt.Errorf("cloudflared not ready: %w", err)
 	}
-	close(p.readySignal)
+	close(readyCh)
 	p.lastSpawnErr.Store("")
 	p.logger.Info("cloudflared ready", "public_url", p.URL(), "elapsed", time.Since(p.startedAt).String())
 	return nil

server/internal/tunnels/installer.go

@@ -4,6 +4,8 @@ import (
 	"archive/tar"
 	"compress/gzip"
 	"context"
+	"crypto/sha256"
+	"encoding/hex"
 	"fmt"
 	"io"
 	"log/slog"
@@ -16,6 +18,24 @@ import (
 	"time"
 )
 
+// cloudflaredVersion pins the cloudflared release the managed installer
+// downloads. Pinned (not "latest") for reproducibility — keep it in sync
+// with the CLOUDFLARED_VERSION arg in server/Dockerfile*. ngrok is fetched
+// from its "v3-stable" channel (ngrok publishes no per-version URLs).
+const cloudflaredVersion = "2025.2.1"
+
+// maxBinaryBytes caps a managed download to guard against a decompression
+// bomb / runaway response. Comfortably above the real agents (~35 MB).
+const maxBinaryBytes = 300 << 20 // 300 MiB
+
+// pinnedSHA256 maps "<provider>/<version>/<goos>/<goarch>" → expected
+// hex SHA-256 of the downloaded artifact (raw binary for cloudflared/linux,
+// the .tgz for archived assets). When an entry exists the download is
+// verified against it; when absent the installer logs a warning with the
+// computed sum so an operator can pin it. Populate from the vendor's
+// published checksums.
+var pinnedSHA256 = map[string]string{}
+
 // BinaryStatus describes a provider's agent binary for the dashboard, so it
 // can render install instructions (local) or Install/Update buttons
 // (managed).
@@ -108,17 +128,37 @@ func (in *Installer) Install(ctx context.Context, provider string) error {
 	}
 	cleanup := func() { _ = out.Close(); _ = os.Remove(partial) }
 
+	// Hash the downloaded artifact (the .tgz for archived assets, the raw
+	// binary otherwise) while writing, bounded by maxBinaryBytes.
+	hasher := sha256.New()
+	src := io.TeeReader(io.LimitReader(resp.Body, maxBinaryBytes), hasher)
 	if archived {
-		if err := extractFromTarGz(resp.Body, member, out); err != nil {
+		if err := extractFromTarGz(src, member, out); err != nil {
 			cleanup()
 			return err
 		}
+		// Drain the rest of the archive so the hash covers the whole .tgz.
+		_, _ = io.Copy(io.Discard, src)
 	} else {
-		if _, err := io.Copy(out, resp.Body); err != nil {
+		if _, err := io.Copy(out, src); err != nil {
 			cleanup()
 			return fmt.Errorf("write binary: %w", err)
 		}
 	}
+
+	sum := hex.EncodeToString(hasher.Sum(nil))
+	key := provider + "/" + assetVersion(provider) + "/" + runtime.GOOS + "/" + runtime.GOARCH
+	if want, ok := pinnedSHA256[key]; ok {
+		if !strings.EqualFold(want, sum) {
+			cleanup()
+			return fmt.Errorf("checksum mismatch for %s: got %s, want %s", key, sum, want)
+		}
+		in.logger.Info("tunnel binary checksum verified", "provider", provider, "sha256", sum)
+	} else {
+		in.logger.Warn("tunnel binary downloaded without checksum verification (HTTPS from official source)",
+			"provider", provider, "url", url, "sha256", sum)
+	}
+
 	if err := out.Sync(); err != nil {
 		cleanup()
 		return fmt.Errorf("fsync binary: %w", err)
@@ -168,7 +208,8 @@ func extractFromTarGz(r io.Reader, member string, out io.Writer) error {
 func assetURL(provider, goos, goarch string) (url string, archived bool, member string, err error) {
 	switch provider {
 	case ProviderCloudflare:
-		const base = "https://github.com/cloudflare/cloudflared/releases/latest/download/"
+		// Pinned version (not "latest") for reproducibility.
+		base := "https://github.com/cloudflare/cloudflared/releases/download/" + cloudflaredVersion + "/"
 		switch goos {
 		case "linux":
 			return base + "cloudflared-linux-" + goarch, false, "", nil
@@ -185,6 +226,15 @@ func assetURL(provider, goos, goarch string) (url string, archived bool, member
 	}
 }
 
+// assetVersion is the version identifier used in the pinnedSHA256 key for a
+// provider (cloudflared is pinned; ngrok tracks its stable channel).
+func assetVersion(provider string) string {
+	if provider == ProviderNgrok {
+		return "v3-stable"
+	}
+	return cloudflaredVersion
+}
+
 // binaryVersion runs the agent's version subcommand best-effort. Returns ""
 // on any failure — it's informational only.
 func binaryVersion(ctx context.Context, provider, path string) string {

server/internal/tunnels/manager.go

@@ -60,6 +60,11 @@ type Manager struct {
 
 	installer *Installer // nil unless BinManaged
 
+	// applyMu serializes the whole Apply operation so two concurrent applies
+	// can't interleave their stop-old / start-new steps and leak an orphan
+	// provider. mu (below) only guards individual field reads/writes.
+	applyMu sync.Mutex
+
 	mu           sync.RWMutex
 	active       Provider
 	onURLChange  func(string)
@@ -144,6 +149,11 @@ func (m *Manager) InstallBinary(ctx context.Context, provider string) error {
 // can see the error and retry — Apply returns the error for the caller to
 // relay, it does not panic the server.
 func (m *Manager) Apply(ctx context.Context, set Settings) error {
+	// Serialize the entire apply so concurrent calls can't interleave the
+	// stop-old / start-new steps (which would orphan a running provider).
+	m.applyMu.Lock()
+	defer m.applyMu.Unlock()
+
 	m.mu.Lock()
 	m.lastSettings = set
 	old := m.active

server/internal/tunnels/ngrok.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"log/slog"
 	"net/http"
+	"os"
 	"os/exec"
 	"regexp"
 	"strconv"
@@ -104,8 +105,9 @@ func (p *ngrokProvider) Name() string { return ProviderNgrok }
 func (p *ngrokProvider) Start(ctx context.Context) error { return p.spawn(ctx) }
 
 func (p *ngrokProvider) argv() []string {
+	// The authtoken is passed via NGROK_AUTHTOKEN env (see spawn), not argv,
+	// so it doesn't appear in ps / /proc/<pid>/cmdline.
 	args := []string{"http", strconv.Itoa(p.localPort),
-		"--authtoken", p.token,
 		"--log", "stdout", "--log-format", "json"}
 	if p.cfg.Mode == ngrokModeNamed {
 		// --url takes a full origin (ngrok v3.5+); older agents used
@@ -120,6 +122,9 @@ func (p *ngrokProvider) spawn(ctx context.Context) error {
 	p.logger.Info("spawning ngrok", "bin", p.binPath, "mode", p.cfg.Mode, "local_port", p.localPort)
 
 	cmd := exec.Command(p.binPath, p.argv()...)
+	// Pass the authtoken via env (NGROK_AUTHTOKEN) rather than argv so it
+	// never appears in the process table.
+	cmd.Env = append(os.Environ(), "NGROK_AUTHTOKEN="+p.token)
 	stdoutLog := newLineLogWriter(p.logger, "ngrok.stdout", p.scanLine)
 	stderrLog := newLineLogWriter(p.logger, "ngrok.stderr", p.scanLine)
 	cmd.Stdout = stdoutLog
@@ -133,7 +138,8 @@ func (p *ngrokProvider) spawn(ctx context.Context) error {
 	p.mu.Lock()
 	p.cmd = cmd
 	p.startedAt = time.Now()
-	p.readySignal = make(chan struct{})
+	readyCh := make(chan struct{})
+	p.readySignal = readyCh
 	p.waiterDone = make(chan struct{})
 	p.publicURL = ""
 	p.mu.Unlock()
@@ -150,7 +156,7 @@ func (p *ngrokProvider) spawn(ctx context.Context) error {
 		<-p.waiterDone
 		return fmt.Errorf("ngrok not ready: %w", err)
 	}
-	close(p.readySignal)
+	close(readyCh)
 	p.lastSpawnErr.Store("")
 	p.logger.Info("ngrok ready", "public_url", p.URL(), "elapsed", time.Since(p.startedAt).String())
 	return nil

server/internal/tunnels/reconciler.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"log/slog"
 	"strings"
+	"sync"
 
 	"github.com/dvcdsys/code-index/server/internal/githubapi"
 	"github.com/dvcdsys/code-index/server/internal/gitrepos"
@@ -45,6 +46,14 @@ type Reconciler struct {
 	tokens TokenRevealer
 	gh     WebhookClient
 	logger *slog.Logger
+
+	// mu serializes Reconcile. On boot two reconciles can race (the
+	// OnURLChange callback for a freshly-parsed quick-tunnel URL + the
+	// explicit boot reconcile); without serialization both would take the
+	// create branch for a repo that has no WebhookID yet and register
+	// duplicate hooks on GitHub. Serializing means the second run re-reads
+	// the now-persisted WebhookID and PATCHes instead.
+	mu sync.Mutex
 }
 
 func NewReconciler(repos RepoStore, tokens TokenRevealer, gh WebhookClient, logger *slog.Logger) *Reconciler {
@@ -77,6 +86,9 @@ type ReconcileResult struct {
 // has the hook); one without is created. baseURL must be an absolute http(s)
 // origin — when empty (no live tunnel) reconcile is a no-op.
 func (r *Reconciler) Reconcile(ctx context.Context, baseURL string) (ReconcileResult, error) {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+
 	res := ReconcileResult{BaseURL: baseURL}
 	baseURL = strings.TrimRight(strings.TrimSpace(baseURL), "/")
 	if !strings.HasPrefix(baseURL, "http") {