fix: security fixes for 1.1.4

SECURITY.md

@@ -44,7 +44,14 @@ What the server does contain:
   (`internal/tool/runner/env.go`).
 - Git credentials are supplied through short-lived `GIT_ASKPASS` scripts that
   are removed before the CLI starts. Tokens never reach the URL or
-  `.git/config`.
+  `.git/config`, and the script answers only for the host the session was
+  cloned from.
+- Git commands the server runs in a workspace treat the workspace as untrusted:
+  `.git/config` is rebuilt from an allowlist first, settings that run programs
+  (fsmonitor, hooks, credential helpers, submodule recursion) are pinned on the
+  command line, the global git config is ignored, and the environment is
+  allowlisted. Fetches and pushes go to the URL the session was cloned from
+  (`internal/tool/git/safe.go`).
 - Webhook-triggered work from authors without write access is refused by
   default (`code_review.allow_untrusted_authors`).
 - Opening a pull request always requires an explicit action. This is a

cmd/codeforge/main.go

@@ -270,6 +270,7 @@ func run() error {
 	var webhookReceiverHandler *handlers.WebhookReceiverHandler
 	if cfg.CodeReview.WebhookSecrets.GitHub != "" || cfg.CodeReview.WebhookSecrets.GitLab != "" {
 		webhookReceiverHandler = handlers.NewWebhookReceiverHandler(sessionService, rdb, cfg.CodeReview, settings.NewStore(rdb))
+		webhookReceiverHandler.SetGitLabMemberLookup(handlers.NewGitLabMemberLookup(keyResolver, cfg.CodeReview.DefaultKeyName))
 	}
 
 	// Initialize tenant service and handler

docs/configuration.md

@@ -113,6 +113,21 @@ instances and custom ports (e.g. `http://gitlab.example.com:8080`) work.
 |----------|---------|-------------|
 | `CODEFORGE_SUBSCRIPTION__ENABLED` | `false` | Enable the tenant subscription model. When disabled, only the static operator Bearer token is accepted and the per-session API-key (BYOK) flow is unchanged. When enabled, tenant API tokens (`cfk_...`) are also accepted and resolve to managed keys from the key pool. |
 
+A tenant session runs with the credentials the tenant brings. The operator's
+credentials never fill in for it:
+
+- Git access uses only the request's `access_token`. There is no fallback to
+  registered keys or `GITHUB_TOKEN`/`GITLAB_TOKEN`, so without a token only
+  public repositories can be cloned, and creating or updating a PR needs one.
+- `provider_key` is rejected (`403`), and `repo_url` must be an `http(s)` URL.
+- `config.tools` get only the config the tenant supplies; nothing is
+  auto-filled from registered keys.
+- The operator's registered MCP servers are not added; only the session's own
+  `config.mcp_servers` are.
+- `config.workspace_session_id` must name one of the tenant's own sessions.
+- Session routes (`/sessions/{id}/...`) answer `404` for another tenant's
+  session.
+
 ### Notifications
 
 Chat notifications for terminal session events. Disabled unless at least one webhook URL is set.
@@ -169,7 +184,11 @@ By default CodeForge therefore requires write access from the author:
   point in the system.
 - **GitLab** payloads carry no equivalent of `author_association`, so fork MRs
   (source project ≠ target project) are skipped outright and commands are
-  refused on them.
+  refused on them. On other MRs a command runs only when its author has
+  Developer access (30) or higher on the project. CodeForge looks that up
+  through the GitLab members API with the `default_key_name` token, so the token
+  must be able to read project members; if the lookup fails, the command is
+  refused.
 
 Setting `allow_untrusted_authors: true` disables all of the above. Only do that
 where each session is genuinely isolated — see

docs/deployment.md

@@ -193,7 +193,14 @@ What CodeForge does to contain this today:
   encryption key, operator token, webhook secrets, and Redis URL never cross
   into the session (`internal/tool/runner/env.go`).
 - Git credentials are used via short-lived `GIT_ASKPASS` scripts that are
-  removed before the CLI starts; tokens are never in the URL or `.git/config`.
+  removed before the CLI starts; tokens are never in the URL or `.git/config`,
+  and the script answers only for the host the session was cloned from.
+- Git commands the server runs in a workspace rebuild `.git/config` from an
+  allowlist, pin the settings that run programs on the command line, get an
+  allowlisted environment, and ignore the global git config (`~/.gitconfig`),
+  which the CLI could otherwise write. Configure TLS trust for a self-hosted
+  instance with `GIT_SSL_CAINFO` / `SSL_CERT_FILE` or the root-owned
+  `/etc/gitconfig`, not `~/.gitconfig`.
 - Webhook-triggered work from authors without write access is **off by
   default** (`code_review.allow_untrusted_authors`) — see
   [Configuration](configuration.md#who-can-trigger-a-webhook-review).

internal/server/handlers/sessions.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"log/slog"
 	"net/http"
+	"net/url"
 	"os"
 	"strconv"
 	"strings"
@@ -39,6 +40,12 @@ type tenantSessionCounter interface {
 	CountActiveByTenant(ctx context.Context, tenantID string) (int, error)
 }
 
+// sessionGetter loads a session by ID. Implemented by *session.Service; kept as
+// an interface so handler tests can fake it.
+type sessionGetter interface {
+	Get(ctx context.Context, sessionID string) (*session.Session, error)
+}
+
 // workspacePathResolver resolves a session's live workspace directory and
 // records workspace activity (Touch extends the TTL window on access).
 // Implemented by *workspace.Manager; kept as an interface so handler tests can fake it.
@@ -57,6 +64,7 @@ type SessionHandler struct {
 	domains        gitpkg.DomainsSource  // optional, nil = standard github.com/gitlab.com detection only
 	tenantService  *tenant.Service       // optional, nil = subscription disabled
 	sessionCounter tenantSessionCounter  // optional, nil = concurrency limit not enforced
+	sessions       sessionGetter         // nil = tenants cannot reference other sessions
 	workspaces     workspacePathResolver // optional, nil = diff endpoint reports workspace missing
 }
 
@@ -65,6 +73,7 @@ func NewSessionHandler(service *session.Service, prService *session.PRService, c
 	h := &SessionHandler{service: service, prService: prService, canceller: canceller, cliRegistry: cliRegistry, keyRegistry: keyRegistry, domains: domains, tenantService: tenantService, workspaces: workspaces}
 	if service != nil {
 		h.sessionCounter = service
+		h.sessions = service
 	}
 	return h
 }
@@ -204,25 +213,38 @@ func (h *SessionHandler) createSession(w http.ResponseWriter, r *http.Request, r
 // tenant_id); operator/no-tenant requests pass unconditionally. A mismatch returns
 // 404 (not 403) so a tenant cannot probe other tenants' session IDs. Routes without
 // a sessionID (List, Create) pass through and enforce their own scoping.
+//
+// The middleware reads {sessionID} with chi.URLParam, so it must be attached
+// where chi has already matched that param (r.With on the route, or r.Use inside
+// an r.Route("/{sessionID}", ...) subrouter). Attached with r.Use above the
+// pattern, the param is still empty and every request passes unchecked.
 func (h *SessionHandler) OwnershipMiddleware(next http.Handler) http.Handler {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		tnt := middleware.TenantFromContext(r.Context())
-		sessionID := chi.URLParam(r, "sessionID")
-		if tnt == nil || sessionID == "" {
+	return SessionOwnership(h.service.Get)(next)
+}
+
+// SessionOwnership builds the ownership check behind OwnershipMiddleware around a
+// session lookup, so the check can be exercised without Redis.
+func SessionOwnership(lookup func(ctx context.Context, sessionID string) (*session.Session, error)) func(http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			tnt := middleware.TenantFromContext(r.Context())
+			sessionID := chi.URLParam(r, "sessionID")
+			if tnt == nil || sessionID == "" {
+				next.ServeHTTP(w, r)
+				return
+			}
+			t, err := lookup(r.Context(), sessionID)
+			if err != nil {
+				writeAppError(w, err)
+				return
+			}
+			if t.TenantID != tnt.ID {
+				writeError(w, http.StatusNotFound, "session not found")
+				return
+			}
 			next.ServeHTTP(w, r)
-			return
-		}
-		t, err := h.service.Get(r.Context(), sessionID)
-		if err != nil {
-			writeAppError(w, err)
-			return
-		}
-		if t.TenantID != tnt.ID {
-			writeError(w, http.StatusNotFound, "session not found")
-			return
-		}
-		next.ServeHTTP(w, r)
-	})
+		})
+	}
 }
 
 // applyTenant enforces a subscription tenant's tier limits and assigns a managed
@@ -233,6 +255,10 @@ func (h *SessionHandler) applyTenant(ctx context.Context, req *session.CreateSes
 		return 0, ""
 	}
 
+	if status, msg := h.checkTenantSources(ctx, req, tnt); status != 0 {
+		return status, msg
+	}
+
 	cli := h.cliRegistry.DefaultCLI()
 	if req.Config != nil && req.Config.CLI != "" {
 		cli = req.Config.CLI
@@ -308,6 +334,31 @@ func (h *SessionHandler) applyTenant(ctx context.Context, req *session.CreateSes
 	return 0, ""
 }
 
+// checkTenantSources rejects request fields that would let a tenant session run
+// with access the tenant did not bring: one of the operator's registered keys
+// (provider_key), a repository reached through the server's filesystem
+// (file:// and other non-HTTP URLs), or another tenant's workspace. The
+// executor separately keeps tenant sessions off the operator's fallback
+// credentials (see session.Session.UsesOperatorCredentials).
+func (h *SessionHandler) checkTenantSources(ctx context.Context, req *session.CreateSessionRequest, tnt *tenant.Tenant) (int, string) {
+	if req.ProviderKey != "" {
+		return http.StatusForbidden, "provider_key is not available to subscription tenants; pass access_token instead"
+	}
+	if u, err := url.Parse(req.RepoURL); err != nil || (u.Scheme != "https" && u.Scheme != "http") || u.Host == "" {
+		return http.StatusBadRequest, "repo_url must be an http(s) URL"
+	}
+	if req.Config != nil && req.Config.WorkspaceSessionID != "" {
+		if h.sessions == nil {
+			return http.StatusNotFound, "workspace session not found"
+		}
+		ref, err := h.sessions.Get(ctx, req.Config.WorkspaceSessionID)
+		if err != nil || ref.TenantID != tnt.ID {
+			return http.StatusNotFound, "workspace session not found"
+		}
+	}
+	return 0, ""
+}
+
 // stringInJSONList reports whether target is allowed by a JSON array allow-list like
 // `["claude-code","codex"]`. An empty/whitespace list means "no restriction" (allow).
 // A NON-empty but malformed list fails CLOSED (deny) — a corrupt restriction must not

internal/server/handlers/subscription_test.go

@@ -8,6 +8,7 @@ import (
 
 	_ "modernc.org/sqlite"
 
+	"github.com/freema/codeforge/internal/apperror"
 	"github.com/freema/codeforge/internal/crypto"
 	"github.com/freema/codeforge/internal/database"
 	"github.com/freema/codeforge/internal/session"
@@ -60,6 +61,10 @@ func testCLIRegistry() *runner.Registry {
 	return reg
 }
 
+// tenantRepo is a repository URL a tenant may use (applyTenant runs after the
+// request passed validation, so it always has one).
+const tenantRepo = "https://github.com/acme/repo.git"
+
 type fakeCounter struct{ active int }
 
 func (f fakeCounter) CountActiveByTenant(_ context.Context, _ string) (int, error) {
@@ -75,13 +80,13 @@ func TestApplyTenant_ConcurrencyLimit(t *testing.T) {
 	h := NewSessionHandler(nil, nil, nil, testCLIRegistry(), nil, nil, svc, nil)
 
 	h.sessionCounter = fakeCounter{active: tnt.MaxConcurrentSessions}
-	if status, _ := h.applyTenant(ctx, &session.CreateSessionRequest{}, tnt); status != 429 {
+	if status, _ := h.applyTenant(ctx, &session.CreateSessionRequest{RepoURL: tenantRepo}, tnt); status != 429 {
 		t.Fatalf("at concurrency limit: status = %d, want 429", status)
 	}
 
 	// Under the limit, a BYOK request passes (no pool needed).
 	h.sessionCounter = fakeCounter{active: tnt.MaxConcurrentSessions - 1}
-	req := &session.CreateSessionRequest{Config: &session.Config{AIApiKey: "byok"}}
+	req := &session.CreateSessionRequest{RepoURL: tenantRepo, Config: &session.Config{AIApiKey: "byok"}}
 	if status, msg := h.applyTenant(ctx, req, tnt); status != 0 {
 		t.Fatalf("under concurrency limit: status = %d (%s), want 0", status, msg)
 	}
@@ -111,15 +116,15 @@ func TestApplyTenant(t *testing.T) {
 	h := NewSessionHandler(nil, nil, nil, testCLIRegistry(), nil, nil, svc, nil)
 
 	t.Run("disallowed CLI -> 403", func(t *testing.T) {
-		req := &session.CreateSessionRequest{Config: &session.Config{CLI: "cursor"}}
+		req := &session.CreateSessionRequest{RepoURL: tenantRepo, Config: &session.Config{CLI: "cursor"}}
 		status, _ := h.applyTenant(ctx, req, tnt)
 		if status != 403 {
 			t.Fatalf("status = %d, want 403", status)
 		}
 	})
 
 	t.Run("allowed CLI, no BYOK -> pool key assigned + tenant_id stamped + budget capped", func(t *testing.T) {
-		req := &session.CreateSessionRequest{}
+		req := &session.CreateSessionRequest{RepoURL: tenantRepo}
 		status, msg := h.applyTenant(ctx, req, tnt)
 		if status != 0 {
 			t.Fatalf("status = %d (%s), want 0", status, msg)
@@ -136,7 +141,7 @@ func TestApplyTenant(t *testing.T) {
 	})
 
 	t.Run("BYOK key preserved, pool not consulted", func(t *testing.T) {
-		req := &session.CreateSessionRequest{Config: &session.Config{AIApiKey: "my-own-key"}}
+		req := &session.CreateSessionRequest{RepoURL: tenantRepo, Config: &session.Config{AIApiKey: "my-own-key"}}
 		status, _ := h.applyTenant(ctx, req, tnt)
 		if status != 0 {
 			t.Fatalf("status = %d, want 0", status)
@@ -152,10 +157,65 @@ func TestApplyTenant(t *testing.T) {
 		for i := 0; i < lt.MaxSessionsPerDay; i++ {
 			_ = store.LogUsage(ctx, &tenant.UsageLog{TenantID: lt.ID, SessionID: string(rune('a' + i)), CLI: "claude-code"})
 		}
-		req := &session.CreateSessionRequest{}
+		req := &session.CreateSessionRequest{RepoURL: tenantRepo}
 		status, _ := h.applyTenant(ctx, req, lt)
 		if status != 429 {
 			t.Fatalf("status = %d, want 429 after hitting daily limit", status)
 		}
 	})
 }
+
+type fakeSessions map[string]*session.Session
+
+func (f fakeSessions) Get(_ context.Context, id string) (*session.Session, error) {
+	if t, ok := f[id]; ok {
+		return t, nil
+	}
+	return nil, apperror.NotFound("session %s not found", id)
+}
+
+// TestApplyTenant_RejectsForeignCredentials covers request fields that would
+// let a tenant session run with the operator's or another tenant's access.
+func TestApplyTenant_RejectsForeignCredentials(t *testing.T) {
+	ctx := context.Background()
+	svc, store, _ := newTenantService(t)
+	res, err := svc.CreateTenant(ctx, "acme", "acme", tenant.TierFree)
+	if err != nil {
+		t.Fatalf("create tenant: %v", err)
+	}
+	tnt, err := store.GetTenant(ctx, res.Tenant.ID)
+	if err != nil {
+		t.Fatalf("get tenant: %v", err)
+	}
+
+	h := NewSessionHandler(nil, nil, nil, testCLIRegistry(), nil, nil, svc, nil)
+	h.sessions = fakeSessions{
+		"own":      {ID: "own", TenantID: tnt.ID},
+		"other":    {ID: "other", TenantID: "another-tenant"},
+		"operator": {ID: "operator"},
+	}
+
+	tests := []struct {
+		name       string
+		req        session.CreateSessionRequest
+		wantStatus int
+	}{
+		{"operator provider key", session.CreateSessionRequest{RepoURL: tenantRepo, ProviderKey: "operator-github"}, 403},
+		{"file URL", session.CreateSessionRequest{RepoURL: "file:///data/workspaces/other/"}, 400},
+		{"ssh URL", session.CreateSessionRequest{RepoURL: "ssh://git@github.com/acme/repo.git"}, 400},
+		{"another tenant's workspace", session.CreateSessionRequest{RepoURL: tenantRepo, Config: &session.Config{WorkspaceSessionID: "other"}}, 404},
+		{"an operator session's workspace", session.CreateSessionRequest{RepoURL: tenantRepo, Config: &session.Config{WorkspaceSessionID: "operator"}}, 404},
+		{"unknown workspace", session.CreateSessionRequest{RepoURL: tenantRepo, Config: &session.Config{WorkspaceSessionID: "missing"}}, 404},
+		// BYOK keeps the pool out of these so only the checks above decide.
+		{"own workspace", session.CreateSessionRequest{RepoURL: tenantRepo, Config: &session.Config{WorkspaceSessionID: "own", AIApiKey: "byok"}}, 0},
+		{"own access token", session.CreateSessionRequest{RepoURL: tenantRepo, AccessToken: "ghp_tenant", Config: &session.Config{AIApiKey: "byok"}}, 0},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			req := tt.req
+			if status, msg := h.applyTenant(ctx, &req, tnt); status != tt.wantStatus {
+				t.Fatalf("status = %d (%s), want %d", status, msg, tt.wantStatus)
+			}
+		})
+	}
+}