Unverified Commit ad4d256d authored by Lemon-miaow's avatar Lemon-miaow
Browse files

fix(submit): bound the untrusted upload lane — per-user caps + throttles (#75)

A logged-in user could file submissions without bound and stream a 1 GiB
context per submission. The only limits were the single-blob size cap and the
5 GiB uploads PVC (platform/workloads.go); nothing counted a user's rows or
bytes, so one account could fill the volume and every other user's upload
would start failing.

- Create: per-user pending_review cap (default 5) — the review queue cannot
  be parked full of one account's rows. Check-then-insert, documented soft.
- UploadContext: per-user stored-context budget (default 2 GiB) charged
  against the blob store's REAL sizes (new Blobs.Size on local/S3 stores), so
  the sum cannot drift from the volume; the write is capped at the remaining
  budget, so the excess is refused before it is persisted, and a re-upload is
  charged only for its new bytes.
- API: per-user create/upload throttles (30s/15s, cmd/felis-wired) on a
  dedicated cooldown keyspace, reserve→release so a failed attempt never
  burns the window and a burst collapses to one winner; ErrQuotaExceeded →
  403 submission_quota_exceeded (distinct from the 400 an oversize blob
  gets), 429 submission_cooldown for the throttles.
- Panel: zh/en copy for both codes; openapi documents 403/429 on the two
  user routes; pgint covers the pending-queue count.

Unit tests: submit package (cap, budget boundary/exact-fit/replacement,
oversize-vs-quota split) and api handlers (quota 403 both paths, throttle
429 + recovery + failure-release). go vet/go test/gofmt clean; panel
vitest 118 + typecheck green.
parent 3ed8bd7b
Loading
Loading
Loading
Loading
+5 −0
Changes for cmd/felis/api.go: 5 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -291,6 +291,11 @@ func cmdAPI(args []string, stdout, stderr io.Writer) int {
		AdminHostname: cfg.Auth.AdminHostname,
		PanelHostname: cfg.Auth.PanelHostname,
		WakeCooldown:  30 * time.Second,
		// The user-modpack lane's per-user throttles: a create spaces out
		// review-queue rows, an upload spaces out (up to 1 GiB) context streams.
		// Separate keys, so the normal create→upload sequence stays immediate.
		SubmitCreateCooldown: 30 * time.Second,
		SubmitUploadCooldown: 15 * time.Second,
		// Bound concurrent console/build-log SSE streams per principal. Generous enough
		// for legitimate multi-tab / multi-server watching, while capping how many
		// upstream follow connections a single caller can tie up if their streams stall.
+21 −2
Changes for docs/openapi.yaml: 21 added lines, 2 removed lines.
Original line number Diff line number Diff line
@@ -4266,6 +4266,15 @@ paths:
          $ref: '#/components/responses/BadRequest'
        '401':
          $ref: '#/components/responses/Unauthorized'
        '403':
          description: >-
            The per-user submission allowance is spent — too many of the
            caller's submissions are awaiting review, or their stored-upload
            budget is full (submission_quota_exceeded).
        '429':
          description: >-
            A submission was created within the per-user cooldown window
            (submission_cooldown).
        '503':
          $ref: '#/components/responses/ServiceUnavailable'
    get:
@@ -4307,8 +4316,10 @@ paths:
        principal; a submission the caller does not own is reported as 404, so
        this endpoint cannot upload to or probe another user's submission. Only a
        pending_review submission accepts a context (409 otherwise); a wrong-format
        or oversize body is rejected with 400. Returns 503 when the deployment's
        context store has no implemented upload transport.
        or oversize body is rejected with 400, and an upload that would push the
        caller past their per-user stored-context budget is refused with 403
        before the excess is persisted. Returns 503 when the deployment's context
        store has no implemented upload transport.
      x-felis-face: [external]
      x-felis-tier: app
      security: [{ accessJWT: [] }]
@@ -4329,10 +4340,18 @@ paths:
          $ref: '#/components/responses/BadRequest'
        '401':
          $ref: '#/components/responses/Unauthorized'
        '403':
          description: >-
            The upload would exceed the caller's per-user stored-context budget
            (submission_quota_exceeded).
        '404':
          $ref: '#/components/responses/NotFound'
        '409':
          $ref: '#/components/responses/Conflict'
        '429':
          description: >-
            An upload was accepted within the per-user cooldown window
            (submission_cooldown).
        '503':
          $ref: '#/components/responses/ServiceUnavailable'

+26 −0
Changes for internal/api/api.go: 26 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -123,6 +123,15 @@ type API struct {
	// on the wake lever). Zero disables throttling.
	WakeCooldown time.Duration

	// SubmitCreateCooldown / SubmitUploadCooldown throttle the user-modpack
	// submission lane per user: create bounds how quickly review-queue rows can
	// appear, upload bounds how often a user may stream a (up to 1 GiB) build
	// context. The keys are separate, so the lane's normal shape — create, then
	// upload — is never blocked by its own throttle. Zero disables each lever
	// (the same idiom as WakeCooldown); cmd/felis wires positive values.
	SubmitCreateCooldown time.Duration
	SubmitUploadCooldown time.Duration

	// MaxRunningServers caps how many servers may be desired-Running cluster-wide
	// (spec §9.1: the concurrency-上限 lever hanging on the same wake chokepoint as
	// cooldown and autostartPolicy). Zero — the default — disables it: §9.2 wires
@@ -158,6 +167,9 @@ type API struct {
	otpCooldownOnce sync.Once
	otpCooldown     *cooldownLimiter

	submitCooldownOnce sync.Once
	submitCooldown     *cooldownLimiter

	streamCapOnce sync.Once
	streamCap     *streamLimiter
}
@@ -204,6 +216,20 @@ func (a *API) otpLimiter() *cooldownLimiter {
	return a.otpCooldown
}

// submitLimiter lazily builds a SEPARATE cooldown limiter for the user-modpack
// submission lane, so its throttles never share state with the wake or OTP
// keyspaces. One limiter backs both levers with prefixed keys (see the
// submissionCreateKey/UploadKey constants), so create and upload never contend
// with each other. Like the other cooldowns it is process-local; with multiple
// api replicas the effective spacing is per-replica, the same accepted
// KNOWN-LIMITATION the OTP resend throttle carries.
func (a *API) submitLimiter() *cooldownLimiter {
	a.submitCooldownOnce.Do(func() {
		a.submitCooldown = &cooldownLimiter{now: a.now, last: map[string]time.Time{}}
	})
	return a.submitCooldown
}

// streamGate lazily builds the per-principal SSE stream cap bound to
// MaxStreamsPerPrincipal. A zero cap yields a disabled limiter that admits every
// stream, so a deployment (or test) that leaves it unset pays nothing.
+59 −3
Changes for internal/api/submissions.go: 59 added lines, 3 removed lines.
Original line number Diff line number Diff line
@@ -59,6 +59,15 @@ type createSubmissionRequest struct {
	DisplayName string `json:"display_name"`
}

// The submission lane's two cooldown keys, prefixed into the shared submit
// limiter's per-user keys. Create and upload are separate levers on purpose:
// creating a submission and then immediately uploading its context is the lane's
// normal shape, so one must never consume the other's window.
const (
	submissionCreateKey = "create:"
	submissionUploadKey = "upload:"
)

// rejectSubmissionRequest is the POST /submissions/{id}/reject body. A reason is
// required (the submit layer rejects an empty one with 400).
type rejectSubmissionRequest struct {
@@ -74,6 +83,26 @@ func (a *API) handleCreateSubmission(w http.ResponseWriter, r *http.Request) {
		return
	}
	p := principalFromContext(r.Context())
	// Reserve the per-user create cooldown BEFORE the store write. Unlike the
	// wake lever's allowed→record (whose real gate is the running cap and whose
	// effect is idempotent), a create is a non-idempotent row insertion with no
	// other bound on its rate, so a burst of truly concurrent creates must yield
	// exactly one winner per window. The deferred rollback frees the window
	// whenever the create fails — a 400 typo, a spent quota, a store error — so
	// only a row that was actually recorded consumes it.
	lim := a.submitLimiter()
	reservedAt, ok := lim.reserve(submissionCreateKey+p.UserID, a.SubmitCreateCooldown)
	if !ok {
		writeError(w, r, newError(http.StatusTooManyRequests, "submission_cooldown",
			"a submission was created recently; wait a moment before creating another"))
		return
	}
	committed := false
	defer func() {
		if !committed {
			lim.release(submissionCreateKey+p.UserID, reservedAt)
		}
	}()
	var body createSubmissionRequest
	if err := decodeJSON(w, r, &body); err != nil {
		writeError(w, r, err)
@@ -87,6 +116,7 @@ func (a *API) handleCreateSubmission(w http.ResponseWriter, r *http.Request) {
		writeSubmitError(w, r, err)
		return
	}
	committed = true
	a.audit(r, p.Email, "submission.create", sub.ID)
	writeJSON(w, http.StatusCreated, sub)
}
@@ -107,12 +137,34 @@ func (a *API) handleUploadSubmissionContext(w http.ResponseWriter, r *http.Reque
		return
	}
	p := principalFromContext(r.Context())
	// Reserve the per-user upload cooldown BEFORE streaming. The body is the
	// expensive part (up to the 1 GiB blob cap), so without a reservation the
	// throttle would never bound the resource it exists for: a caller could
	// repeatedly start long uploads and abort them. Reserving also collapses the
	// lane's parallel overshoot — a burst of concurrent uploads from one user
	// yields exactly one admitted stream per replica. The rollback keeps a failed
	// upload (aborted transfer, wrong format, spent quota) from burning the
	// window, so a legit retry after a genuine failure is not punished.
	lim := a.submitLimiter()
	reservedAt, ok := lim.reserve(submissionUploadKey+p.UserID, a.SubmitUploadCooldown)
	if !ok {
		writeError(w, r, newError(http.StatusTooManyRequests, "submission_cooldown",
			"an upload was accepted recently; wait a moment before uploading again"))
		return
	}
	committed := false
	defer func() {
		if !committed {
			lim.release(submissionUploadKey+p.UserID, reservedAt)
		}
	}()
	id := r.PathValue("id")
	sub, err := a.Submissions.UploadContext(r.Context(), id, p.UserID, r.Body)
	if err != nil {
		writeSubmitError(w, r, err)
		return
	}
	committed = true
	a.audit(r, p.Email, "submission.upload", sub.ID)
	writeJSON(w, http.StatusOK, sub)
}
@@ -250,9 +302,10 @@ var errSubmissionsUnavailable = newError(http.StatusServiceUnavailable, "submiss

// writeSubmitError maps submit-package errors onto HTTP status codes. Only the
// business sentinels are client-facing: a validation failure is 400, a missing
// submission is 404, an already-reviewed submission is 409, and an unconfigured
// upload transport is 503 (the store this deployment set has no implemented
// transport — an honest "not available here", not a client error). Everything
// submission is 404, an already-reviewed submission is 409, a spent per-user
// allowance is 403 (the same status the server-resource quota answers with), and
// an unconfigured upload transport is 503 (the store this deployment set has no
// implemented transport — an honest "not available here", not a client error). Everything
// else — including a build.ErrInvalid raised by the pre-CAS build.Validate (a
// platform registry/context MISCONFIGURATION, never client input, since every
// build input is platform-derived) and a post-CAS Submit hand-off failure — is a
@@ -267,6 +320,9 @@ func writeSubmitError(w http.ResponseWriter, r *http.Request, err error) {
	case errors.Is(err, submit.ErrAlreadyReviewed):
		writeError(w, r, newError(http.StatusConflict, "already_reviewed",
			"submission has already been reviewed"))
	case errors.Is(err, submit.ErrQuotaExceeded):
		writeError(w, r, newError(http.StatusForbidden, "submission_quota_exceeded",
			"submission quota reached"))
	case errors.Is(err, submit.ErrBlobNotFound):
		writeError(w, r, newError(http.StatusNotFound, "not_found", "no context uploaded for this submission"))
	case errors.Is(err, submit.ErrUploadsUnavailable):
+110 −0
Changes for internal/api/submissions_test.go: 110 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -9,6 +9,7 @@ import (
	"net/http"
	"strings"
	"testing"
	"time"

	"felis.lolicon.best/internal/build"
	"felis.lolicon.best/internal/submit"
@@ -567,3 +568,112 @@ func TestAdminSubmissionContextRoute(t *testing.T) {
		}
	})
}

// A spent per-user allowance is 403 submission_quota_exceeded on both the create
// and the upload path — distinctly NOT the 400 a malformed request gets, and not
// the 429 the cooldown answers with.
func TestSubmissionQuotaIs403(t *testing.T) {
	t.Run("create", func(t *testing.T) {
		fs := &fakeSubmissions{createErr: fmt.Errorf("%w: 5 submissions are already awaiting review", submit.ErrQuotaExceeded)}
		w := do(appSubAPI(fs).ExternalHandler(), "POST", "/api/v1/me/submissions", `{"display_name":"Pack"}`, nil)
		if w.Code != http.StatusForbidden {
			t.Fatalf("code = %d, want 403 (%s)", w.Code, w.Body.String())
		}
		if got := decodeErr(t, w); got != "submission_quota_exceeded" {
			t.Errorf("error code = %q, want submission_quota_exceeded", got)
		}
	})
	t.Run("upload", func(t *testing.T) {
		fs := &fakeSubmissions{uploadErr: fmt.Errorf("%w: exceeds your remaining storage allowance", submit.ErrQuotaExceeded)}
		w := do(appSubAPI(fs).ExternalHandler(), "POST", "/api/v1/me/submissions/sub-9/context", "\x1f\x8bdata", nil)
		if w.Code != http.StatusForbidden {
			t.Fatalf("code = %d, want 403 (%s)", w.Code, w.Body.String())
		}
		if got := decodeErr(t, w); got != "submission_quota_exceeded" {
			t.Errorf("error code = %q, want submission_quota_exceeded", got)
		}
	})
}

// The per-user create cooldown bounds review-queue growth: a second create in
// the same window is 429 submission_cooldown and never reaches the service; the
// window recovers afterwards.
func TestCreateSubmissionRateLimited(t *testing.T) {
	fs := &fakeSubmissions{}
	api := appSubAPI(fs)
	clock := time.Unix(1_700_000_000, 0)
	api.Now = func() time.Time { return clock }
	api.SubmitCreateCooldown = time.Minute
	eh := api.ExternalHandler()

	if w := do(eh, "POST", "/api/v1/me/submissions", `{"display_name":"First"}`, nil); w.Code != http.StatusCreated {
		t.Fatalf("first create: code = %d, want 201 (%s)", w.Code, w.Body.String())
	}
	w := do(eh, "POST", "/api/v1/me/submissions", `{"display_name":"Second"}`, nil)
	if w.Code != http.StatusTooManyRequests || decodeErr(t, w) != "submission_cooldown" {
		t.Fatalf("immediate second create: code = %d body %s, want 429 submission_cooldown", w.Code, w.Body.String())
	}
	// The gate sits before the body handling: even a malformed request is
	// refused while the window is closed, so it cannot be used to probe.
	if w := do(eh, "POST", "/api/v1/me/submissions", `{`, nil); w.Code != http.StatusTooManyRequests {
		t.Fatalf("malformed create during cooldown: code = %d, want 429", w.Code)
	}
	clock = clock.Add(time.Minute + time.Second)
	if w := do(eh, "POST", "/api/v1/me/submissions", `{"display_name":"Third"}`, nil); w.Code != http.StatusCreated {
		t.Fatalf("post-cooldown create: code = %d, want 201 (%s)", w.Code, w.Body.String())
	}
}

// A failed create frees the window: only a row that was actually recorded burns
// the cooldown, so a validation typo is not punished with a wait.
func TestCreateSubmissionFailureDoesNotBurnCooldown(t *testing.T) {
	fs := &fakeSubmissions{createErr: fmt.Errorf("%w: display name is required", submit.ErrInvalid)}
	api := appSubAPI(fs)
	api.Now = func() time.Time { return time.Unix(1_700_000_000, 0) }
	api.SubmitCreateCooldown = time.Minute
	eh := api.ExternalHandler()

	if w := do(eh, "POST", "/api/v1/me/submissions", `{"display_name":""}`, nil); w.Code != http.StatusBadRequest {
		t.Fatalf("failed create: code = %d, want 400", w.Code)
	}
	fs.createErr = nil
	if w := do(eh, "POST", "/api/v1/me/submissions", `{"display_name":"Fixed"}`, nil); w.Code != http.StatusCreated {
		t.Fatalf("retry at the same instant: code = %d, want 201 (%s)", w.Code, w.Body.String())
	}
}

// The per-user upload cooldown bounds context streaming: a second upload in the
// same window is 429 submission_cooldown, and a FAILED upload frees the window
// for an immediate retry.
func TestUploadSubmissionContextRateLimited(t *testing.T) {
	fs := &fakeSubmissions{}
	api := appSubAPI(fs)
	clock := time.Unix(1_700_000_000, 0)
	api.Now = func() time.Time { return clock }
	api.SubmitUploadCooldown = time.Minute
	eh := api.ExternalHandler()
	body := "\x1f\x8b\x08\x00 the modpack bytes"

	if w := do(eh, "POST", "/api/v1/me/submissions/sub-9/context", body, nil); w.Code != http.StatusOK {
		t.Fatalf("first upload: code = %d, want 200 (%s)", w.Code, w.Body.String())
	}
	w := do(eh, "POST", "/api/v1/me/submissions/sub-9/context", body, nil)
	if w.Code != http.StatusTooManyRequests || decodeErr(t, w) != "submission_cooldown" {
		t.Fatalf("immediate second upload: code = %d body %s, want 429 submission_cooldown", w.Code, w.Body.String())
	}

	// A failed upload releases its reservation, so the user is not punished for
	// a genuine failure (aborted transfer, spent quota) with a cooldown wait.
	fs2 := &fakeSubmissions{uploadErr: submit.ErrUploadsUnavailable}
	api2 := appSubAPI(fs2)
	api2.Now = func() time.Time { return clock }
	api2.SubmitUploadCooldown = time.Minute
	eh2 := api2.ExternalHandler()
	if w := do(eh2, "POST", "/api/v1/me/submissions/sub-9/context", body, nil); w.Code != http.StatusServiceUnavailable {
		t.Fatalf("failed upload: code = %d, want 503", w.Code)
	}
	fs2.uploadErr = nil
	if w := do(eh2, "POST", "/api/v1/me/submissions/sub-9/context", body, nil); w.Code != http.StatusOK {
		t.Fatalf("retry at the same instant after failure: code = %d, want 200 (%s)", w.Code, w.Body.String())
	}
}
Loading