filer: reject a proxyChunkId that isn't a well-formed fid (#10436)

LookupFileId only requires the fid to contain a single comma, and the value
is pasted straight into the volume server URL path, so ?proxyChunkId=3,x/../../status
resolves to volume 3 and then addresses an endpoint the caller never named:
Go sends the dot segments verbatim, the volume server's mux cleans the path
and redirects to /status, and the filer follows the redirect and relays the
body. That reaches any handler on the volume server -- status, stats, the UI --
past a filer that operators expect to be the only exposed surface.

Parse the fid before the lookup and answer 400 when it doesn't parse.

A trailing _N delta suffix from batch assigns is legal, so it is stripped
first, but only when it is a non-empty run of digits. The volume server strips
at the last "_" unconditionally, which is safe there because its fid came out
of a path the mux parsed and so cannot hold a "/"; here the value is raw query
input, and an unguarded strip would reduce "3,01637037d6_1/../../status" to a
valid fid and wave the traversal through.

weed/server/filer_server_handlers_proxy.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/seaweedfs/seaweedfs/weed/glog"
 	"github.com/seaweedfs/seaweedfs/weed/security"
+	"github.com/seaweedfs/seaweedfs/weed/storage/needle"
 	util_http "github.com/seaweedfs/seaweedfs/weed/util/http"
 	"github.com/seaweedfs/seaweedfs/weed/util/mem"
 	"github.com/seaweedfs/seaweedfs/weed/util/request_id"
@@ -13,6 +14,7 @@ import (
 	"io"
 	"math/rand/v2"
 	"net/http"
+	"strings"
 )
 
 // proxyReadConcurrencyPerVolumeServer limits how many concurrent proxy read
@@ -48,8 +50,50 @@ func releaseProxySemaphore(host string) {
 	}
 }
 
+// baseFileId strips the trailing _N delta suffix that batch assigns append to a
+// fid, and only that: the suffix must be a non-empty run of digits, otherwise
+// the fid is returned whole for the caller to reject.
+//
+// The volume server strips at the last "_" unconditionally, which is safe there
+// because its fid already came out of a path the mux parsed and so cannot hold
+// a "/". Here the value is raw query input, and an unguarded strip would reduce
+// "3,01637037d6_1/../../status" to a valid fid and wave the traversal through.
+func baseFileId(fileId string) string {
+	sepIndex := strings.LastIndex(fileId, "_")
+	if sepIndex <= 0 {
+		return fileId
+	}
+	delta := fileId[sepIndex+1:]
+	if delta == "" {
+		return fileId
+	}
+	for _, c := range delta {
+		if c < '0' || c > '9' {
+			return fileId
+		}
+	}
+	return fileId[:sepIndex]
+}
+
+// validateProxyChunkId rejects a proxyChunkId that is not a well-formed fid.
+// LookupFileId only requires a single comma, and the value is pasted into the
+// volume server URL path, so "3,x/../../status" resolves to a volume the caller
+// never named -- the volume server's mux cleans the dot segments and redirects
+// to /status, which the filer follows and relays.
+func validateProxyChunkId(fileId string) error {
+	_, err := needle.ParseFileIdFromString(baseFileId(fileId))
+	return err
+}
+
 func (fs *FilerServer) proxyToVolumeServer(w http.ResponseWriter, r *http.Request, fileId string) {
 	ctx := r.Context()
+
+	if err := validateProxyChunkId(fileId); err != nil {
+		glog.V(1).InfofCtx(ctx, "reject proxyChunkId %q: %v", fileId, err)
+		w.WriteHeader(http.StatusBadRequest)
+		return
+	}
+
 	urlStrings, err := fs.filer.MasterClient.GetLookupFileIdFunction()(ctx, fileId)
 	if err != nil {
 		glog.ErrorfCtx(ctx, "locate %s: %v", fileId, err)

weed/server/filer_server_handlers_proxy_test.go

@@ -2,12 +2,80 @@ package weed_server
 
 import (
 	"context"
+	"net/http"
+	"net/http/httptest"
 	"sync"
 	"sync/atomic"
 	"testing"
 	"time"
 )
 
+func TestValidateProxyChunkId(t *testing.T) {
+	for _, tc := range []struct {
+		fileId string
+		ok     bool
+	}{
+		{"3,01637037d6", true},
+		{"1,0c2b3f2f0f", true},
+		{"12,04f0e6ba1d", true},
+		{"3,01637037d6_1", true},  // batch-assign delta form
+		{"3,01637037d6_12", true}, // multi-digit delta
+		{"3,x/../../status", false},
+		{"3,01637037d6/../../status", false},
+		{"3,01637037d6/../../stats/counter", false},
+		{"3,../../status", false},
+		{"3,01637037d6/../../status_1", false}, // traversal wearing a delta suffix
+		// The suffix must be digits only, or stripping it would reduce a
+		// traversal payload to a valid fid and let it through.
+		{"3,01637037d6_1/../../status", false},
+		{"3,01637037d6_../../status", false},
+		{"3,01637037d6_1/../../stats/counter", false},
+		{"3,01637037d6_", false},
+		{"3,01637037d6_abc", false},
+		{"3,01637037d6_1a", false},
+		{"3,01637037d6?readDeleted=true", false},
+		{"3,01637037d6#frag", false},
+		{"3,", false},
+		{"3,abc", false},
+		{"3", false},
+		{"", false},
+	} {
+		err := validateProxyChunkId(tc.fileId)
+		if tc.ok && err != nil {
+			t.Errorf("validateProxyChunkId(%q) rejected a valid fid: %v", tc.fileId, err)
+		}
+		if !tc.ok && err == nil {
+			t.Errorf("validateProxyChunkId(%q) accepted a malformed fid", tc.fileId)
+		}
+	}
+}
+
+// A fid carrying dot segments must be rejected before the lookup, so it can
+// never be pasted into a volume server URL. Asserting on 400 (not merely "no
+// traversal") also proves the request never left the filer.
+func TestProxyRejectsTraversalBeforeLookup(t *testing.T) {
+	fs := &FilerServer{}
+
+	for _, fileId := range []string{
+		"3,x/../../status",
+		"3,01637037d6/../../status",
+		"3,01637037d6/../../stats/counter",
+		"3,01637037d6_1/../../status",
+		"3,01637037d6_../../status",
+	} {
+		r := httptest.NewRequest(http.MethodGet, "http://filer:8888/?proxyChunkId="+fileId, nil)
+		w := httptest.NewRecorder()
+
+		// fs.filer is nil: reaching the lookup would panic, so surviving this
+		// call is itself proof the fid was rejected first.
+		fs.proxyToVolumeServer(w, r, fileId)
+
+		if w.Code != http.StatusBadRequest {
+			t.Errorf("proxyChunkId=%q returned %d, want 400", fileId, w.Code)
+		}
+	}
+}
+
 func TestProxySemaphore_LimitsConcurrency(t *testing.T) {
 	host := "test-volume:8080"
 	defer proxySemaphores.Delete(host)