diff --git a/internal/submit/pgstore.go b/internal/submit/pgstore.go new file mode 100644 index 0000000..781b258 --- /dev/null +++ b/internal/submit/pgstore.go @@ -0,0 +1,159 @@ +package submit + +import ( + "context" + "database/sql" + "time" +) + +// PGStore is the Postgres-backed Store (image_submissions, migration 0002). It +// is the only submit component that holds database credentials and exposes only +// the narrow operations the Manager needs — no generic UPDATE escape hatch. It +// compiles here but is exercised by integration tests against a live database; +// the Manager's logic is unit-tested against the in-memory fake instead. +type PGStore struct { + db *sql.DB +} + +// NewPGStore wraps an open *sql.DB. +func NewPGStore(db *sql.DB) *PGStore { return &PGStore{db: db} } + +// Compile-time proof PGStore satisfies the Store interface. +var _ Store = (*PGStore)(nil) + +const submissionColumns = `id, submitted_by, display_name, context_ref, status, + image_ref, build_id, reviewed_by, reject_reason, created_at, reviewed_at` + +func (s *PGStore) CreateSubmission(ctx context.Context, sub *Submission) error { + const q = `INSERT INTO image_submissions + (id, submitted_by, display_name, context_ref, status, created_at) + VALUES ($1, $2, $3, $4, $5, $6)` + _, err := s.db.ExecContext(ctx, q, + sub.ID, sub.SubmittedBy, sub.DisplayName, sub.ContextRef, string(sub.Status), sub.CreatedAt) + return err +} + +func (s *PGStore) GetSubmission(ctx context.Context, id string) (*Submission, error) { + const q = `SELECT ` + submissionColumns + ` FROM image_submissions WHERE id = $1` + return scanSubmission(s.db.QueryRowContext(ctx, q, id)) +} + +func (s *PGStore) ListSubmissions(ctx context.Context) ([]Submission, error) { + const q = `SELECT ` + submissionColumns + ` FROM image_submissions ORDER BY created_at DESC` + return s.querySubmissions(ctx, q) +} + +func (s *PGStore) ListSubmissionsBy(ctx context.Context, submittedBy string) ([]Submission, error) { + const q = `SELECT ` + submissionColumns + ` + FROM image_submissions WHERE submitted_by = $1 ORDER BY created_at DESC` + return s.querySubmissions(ctx, q, submittedBy) +} + +// cas executes a compare-and-set UPDATE and reports whether THIS call moved the +// row. The `status = 'pending_review'` guard is the actual CAS predicate and is +// deliberately kept INLINE in each caller's query — it is security-visible, so a +// reader auditing "can a non-pending row be flipped?" must see it next to the SET. +// cas only folds the shared ExecContext + RowsAffected tail so the two reviewers +// (approve, reject) cannot drift in how they report a lost race or a RowsAffected +// error. n == 0 means a concurrent review already won the row — reported as +// won=false (never an error), which the Manager maps to ErrAlreadyReviewed. +func (s *PGStore) cas(ctx context.Context, q string, args ...any) (bool, error) { + res, err := s.db.ExecContext(ctx, q, args...) + if err != nil { + return false, err + } + n, err := res.RowsAffected() + return n > 0, err +} + +// ApproveSubmission is the approve CAS: it flips the row only while it is still +// pending_review, so a concurrent reviewer cannot also win. +func (s *PGStore) ApproveSubmission(ctx context.Context, id, reviewedBy, imageRef string, at time.Time) (bool, error) { + const q = `UPDATE image_submissions + SET status = 'approved', image_ref = $2, reviewed_by = $3, reviewed_at = $4 + WHERE id = $1 AND status = 'pending_review'` + return s.cas(ctx, q, id, imageRef, reviewedBy, at) +} + +// RejectSubmission is the reject CAS, mirroring ApproveSubmission. +func (s *PGStore) RejectSubmission(ctx context.Context, id, reviewedBy, reason string, at time.Time) (bool, error) { + const q = `UPDATE image_submissions + SET status = 'rejected', reviewed_by = $2, reject_reason = $3, reviewed_at = $4 + WHERE id = $1 AND status = 'pending_review'` + return s.cas(ctx, q, id, reviewedBy, reason, at) +} + +func (s *PGStore) LinkBuild(ctx context.Context, id, buildID string) error { + const q = `UPDATE image_submissions SET build_id = $2 WHERE id = $1` + res, err := s.db.ExecContext(ctx, q, id, buildID) + if err != nil { + return err + } + n, err := res.RowsAffected() + if err != nil { + return err + } + if n == 0 { + return ErrNotFound + } + return nil +} + +func (s *PGStore) querySubmissions(ctx context.Context, q string, args ...any) ([]Submission, error) { + rows, err := s.db.QueryContext(ctx, q, args...) + if err != nil { + return nil, err + } + defer rows.Close() + var out []Submission + for rows.Next() { + sub, err := scanSubmissionRows(rows) + if err != nil { + return nil, err + } + out = append(out, *sub) + } + return out, rows.Err() +} + +// rowScanner is the read surface shared by *sql.Row (single) and *sql.Rows (in a +// list loop), so one scan body serves both without copy-paste. +type rowScanner interface { + Scan(dest ...any) error +} + +// scanSubmission scans a single row, translating no-rows into ErrNotFound. +func scanSubmission(row *sql.Row) (*Submission, error) { + sub, err := scanSubmissionRows(row) + if err == sql.ErrNoRows { + return nil, ErrNotFound + } + return sub, err +} + +// scanSubmissionRows decodes one row's columns, mapping nullable text/time +// columns through sql.Null* (NULLIF/absent values become zero, omitted in JSON). +func scanSubmissionRows(row rowScanner) (*Submission, error) { + var ( + sub Submission + status string + imageRef, buildID, reviewedBy, rejectReason sql.NullString + reviewedAt sql.NullTime + ) + if err := row.Scan( + &sub.ID, &sub.SubmittedBy, &sub.DisplayName, &sub.ContextRef, &status, + &imageRef, &buildID, &reviewedBy, &rejectReason, &sub.CreatedAt, &reviewedAt, + ); err != nil { + return nil, err + } + sub.Status = Status(status) + sub.ImageRef = imageRef.String + sub.BuildID = buildID.String + sub.ReviewedBy = reviewedBy.String + sub.RejectReason = rejectReason.String + if reviewedAt.Valid { + t := reviewedAt.Time + sub.ReviewedAt = &t + } + return &sub, nil +} diff --git a/internal/submit/submit.go b/internal/submit/submit.go new file mode 100644 index 0000000..444be3f --- /dev/null +++ b/internal/submit/submit.go @@ -0,0 +1,418 @@ +// Package submit implements the user-uploaded modpack approval lane. +// +// PROVENANCE — read this before trusting the spec citations elsewhere. This lane +// is a USER-DIRECTED extension, not a feature of Felis-Spec-V4.1 as written. The +// spec's §16 build subsystem deliberately DELETED a manual-approve flow ("简化掉 +// (admin 信任,删):人工 approve 流程 → 删 ... 无人工 approve 端点") under a trust +// model where "上传仅 SysAdmin" (§1 invariant 9: "build 鉴权简化(admin-only)"). +// The product owner subsequently required the opposite: ordinary, untrusted +// users may upload their own modpacks, and an admin must approve each one before +// it is usable. Re-introducing untrusted-origin uploads is exactly the condition +// under which §16's deleted gate must come BACK — the spec removed the approve +// step only because it had assumed every uploader was already a trusted SysAdmin. +// So this lane departs from §16's *ingress* simplification on purpose, with the +// owner's instruction as the authority; it does NOT relax anything §16 calls +// non-negotiable. (Do not cite "spec §8" for this lane — §8 is Readiness/优雅关闭 +// /Console. Earlier drafts mis-cited it; those citations have been corrected.) +// +// What is preserved (§16, non-negotiable). The build subsystem treats the +// Dockerfile as hostile at runtime (build/build.go): Kaniko reads the Dockerfile +// from inside the uploaded context, so every build — admin or user — runs in the +// isolated felis-build namespace under a weak service account behind a default- +// deny egress policy, and a CRITICAL CVE fails the Trivy gate ("运行时隔离不能 +// 省"). This lane changes none of that: an approved submission is built through +// the SAME gated Builder.Submit. The approval gate is layered in FRONT of the +// still-mandatory Trivy scan, never instead of it — a human "yes" does not skip +// the scan, so a CRITICAL CVE still blocks admission after approval. +// +// Two further guards close the gap an untrusted origin opens: +// +// - The push target (image_ref) is DERIVED by the Manager as +// {registry}/user-uploads/{submissionID}:latest — a namespace platform refs +// never use, so a submitter can neither collide with nor target the platform +// image namespace. +// - The build context (context_ref) is DERIVED as {contextStore}/{id}/... +// too, NOT taken as free-form user input. Kaniko interpolates the context +// ref straight into its `--context=` argument (build/jobspec.go); a user- +// chosen ref would let an untrusted origin point the build at an arbitrary +// source. By deriving it from the submission id the user selects nothing +// that reaches the executor — only the modpack blob behind the pinned, +// id-namespaced location (uploaded by a separate, deferred transport). +// +// Source of truth. submissions is a NEW Postgres business-truth domain, added to +// §1 invariant 2's enumeration (owner/claim/accounts/audit/quota/images/builds/ +// backups) by the same owner instruction that introduced this lane. The +// image_submissions row is the business authority — the approval record — while +// the build is the execution record in image_builds. Approve never copies the +// build's authoritative fields; it records the derived push target it asked the +// Builder to build plus the reviewer identity, and links the build id once +// Submit succeeds. +// +// The Manager depends on the Store and Builds interfaces, so creation, the +// approve CAS, rejection, and build hand-off are all unit-tested against +// in-memory fakes. The Postgres implementation (PGStore) compiles here but is +// exercised only by integration tests against a live database. +package submit + +import ( + "context" + "crypto/rand" + "encoding/hex" + "errors" + "fmt" + "regexp" + "strings" + "time" + + "felis.lolicon.best/internal/build" +) + +// Status mirrors the submission_status enum (spec §6, migration 0002). +type Status string + +const ( + StatusPendingReview Status = "pending_review" + StatusApproved Status = "approved" + StatusRejected Status = "rejected" +) + +// Sentinels. The API layer maps ErrInvalid→400, ErrNotFound→404 and +// ErrAlreadyReviewed→409; they are kept distinct from store/cluster failures so +// those surface as 500. +var ( + ErrInvalid = errors.New("submit: invalid request") + ErrNotFound = errors.New("submit: submission not found") + ErrAlreadyReviewed = errors.New("submit: submission already reviewed") +) + +// invalidf wraps ErrInvalid so every malformed-request case maps to one 400. +func invalidf(format string, a ...any) error { + return fmt.Errorf("%w: "+format, append([]any{ErrInvalid}, a...)...) +} + +const ( + maxDisplayName = 200 + maxRejectReason = 1000 +) + +// displayNameRE constrains the user-supplied label to a calm, single-line set: +// it is the only free-form string a submission carries, and although JSON +// encoding already neutralises it in responses, rejecting control characters +// and odd whitespace here keeps it clean in the admin review queue and any log +// line. Length is bounded separately so this stays a charset check. +var displayNameRE = regexp.MustCompile(`^[\p{L}\p{N} ._:()\[\]/+-]+$`) + +// Submission mirrors an image_submissions row (spec §6, migration 0002): a +// user's request to have a modpack built into a runnable image, plus the admin +// review verdict. The user supplies only DisplayName; ContextRef/ImageRef/ +// BuildID/ReviewedBy are filled by the platform, never by the submitter. +type Submission struct { + ID string `json:"id"` + SubmittedBy string `json:"submitted_by"` + DisplayName string `json:"display_name"` + ContextRef string `json:"context_ref"` + Status Status `json:"status"` + ImageRef string `json:"image_ref,omitempty"` + BuildID string `json:"build_id,omitempty"` + ReviewedBy string `json:"reviewed_by,omitempty"` + RejectReason string `json:"reject_reason,omitempty"` + CreatedAt time.Time `json:"created_at"` + ReviewedAt *time.Time `json:"reviewed_at,omitempty"` +} + +// Store is the business-layer persistence the Manager depends on. It is an +// interface so the Manager is tested against an in-memory fake; the Postgres +// implementation (PGStore) is integration-tested only. +type Store interface { + // CreateSubmission inserts a pending_review row. + CreateSubmission(ctx context.Context, s *Submission) error + // GetSubmission loads one submission, or ErrNotFound. + GetSubmission(ctx context.Context, id string) (*Submission, error) + // ListSubmissions returns every submission, newest first (admin queue). + ListSubmissions(ctx context.Context) ([]Submission, error) + // ListSubmissionsBy returns one user's submissions, newest first. + ListSubmissionsBy(ctx context.Context, submittedBy string) ([]Submission, error) + // ApproveSubmission atomically flips pending_review -> approved, recording the + // derived image_ref, the reviewer and reviewed_at. It reports whether THIS + // call won the transition: false means a concurrent review already moved the + // row, so the caller must NOT start a build. + ApproveSubmission(ctx context.Context, id, reviewedBy, imageRef string, at time.Time) (won bool, err error) + // RejectSubmission atomically flips pending_review -> rejected, recording the + // reviewer, the reason and reviewed_at. Reports whether THIS call won. + RejectSubmission(ctx context.Context, id, reviewedBy, reason string, at time.Time) (won bool, err error) + // LinkBuild records the build id on an approved submission. It runs only after + // Builder.Submit succeeds, so the implication is ONE-WAY: build_id non-null ⇒ + // a build started. The converse does NOT hold — if Submit succeeds but this + // link write then fails, build_id stays NULL while a build is genuinely running + // (Approve surfaces that distinctly so remediation does not double-build; see + // the Approve ordering note and the package KNOWN-LIMITATION). + LinkBuild(ctx context.Context, id, buildID string) error +} + +// Builds is the slice of the build subsystem the approval lane drives. An +// approved submission is built through the SAME gated Builder.Submit as an +// admin's direct build, so the Trivy scan-gate applies identically (spec §16). +type Builds interface { + Submit(ctx context.Context, req build.Request) (*build.Build, error) +} + +// Manager orchestrates the approval lane. It holds no mutable state; the clock +// and id generator are injectable for hermetic tests. +type Manager struct { + Store Store + Builds Builds + + // Registry is the internal registry host[:port] the derived push target + // addresses. It MUST be the SAME value the Builder is configured with + // (build.Config.RegistryURL): the Manager pre-validates the derived ref + // against this host before the approve CAS, and the Builder re-validates + // against its own config at Submit — if the two disagree the pre-check passes + // but Submit rejects, stranding an approved row. cmd/felis wires both from one + // field. The derived ref is {Registry}/user-uploads/{id}:latest. + Registry string + // ContextStore is the pinned Kaniko build-context base for user uploads, e.g. + // "s3://felis-user-uploads" (mirrors a configured object store). The derived + // context ref is {ContextStore}/{id}/context.tar.gz; the modpack blob is + // placed there by a separate upload transport (deferred — see package doc). + ContextStore string + + Now func() time.Time + IDGen func() string +} + +func (m *Manager) now() time.Time { + if m.Now != nil { + return m.Now() + } + return time.Now() +} + +func (m *Manager) newID() string { + if m.IDGen != nil { + return m.IDGen() + } + // The id namespaces the derived image ref (deriveImageRef) and is the PK of + // image_submissions, so it must be lowercase (imageNameRE in build/validate.go), + // path/argv-safe, and collision-resistant. 8 bytes of crypto/rand hex give all + // three: lowercase hex never contains a slash or shell metachar, and the id is + // unguessable rather than a sequential timestamp. crypto/rand.Read only fails on + // a broken entropy source; fall back to a nanosecond stamp so creation degrades + // instead of panicking (a PK clash would surface as a normal insert error). + var b [8]byte + if _, err := rand.Read(b[:]); err != nil { + return fmt.Sprintf("sub-%d", time.Now().UnixNano()) + } + return "sub-" + hex.EncodeToString(b[:]) +} + +// deriveImageRef is the platform-controlled push target for a submission: a pure +// function of the registry and the submission id, in a user-uploads/ path no +// platform ref uses, so an untrusted submitter can never collide with or target +// the platform image namespace. +func (m *Manager) deriveImageRef(id string) string { + return fmt.Sprintf("%s/user-uploads/%s:latest", strings.TrimRight(m.Registry, "/"), id) +} + +// deriveContextRef is the platform-controlled build context for a submission. +// Like the image ref it is a pure function of the submission id, so the user +// selects nothing that reaches Kaniko's --context argument; only the blob behind +// this pinned, id-namespaced location (placed by the upload transport) varies. +func (m *Manager) deriveContextRef(id string) string { + return fmt.Sprintf("%s/%s/context.tar.gz", strings.TrimRight(m.ContextStore, "/"), id) +} + +// auditDockerfile is the audit-archive Dockerfile recorded on the build row. It +// is NOT what Kaniko executes — Kaniko reads the real Dockerfile from inside the +// uploaded context (build/jobspec.go) — so this honestly documents the +// provenance instead of implying a recipe the platform controls. It satisfies +// build.Validate's non-empty requirement. +func auditDockerfile(id, contextRef string) string { + return fmt.Sprintf( + "# felis user-modpack submission %s\n"+ + "# The executed Dockerfile is provided by the uploaded build context:\n"+ + "# %s\n"+ + "# Built in the isolated felis-build sandbox and Trivy-gated (spec §16).\n", + id, contextRef) +} + +// CreateRequest is the validated user upload. The user supplies ONLY a +// human-friendly name; identity comes from the authenticated principal and the +// build inputs (image/context refs) are platform-derived, never from the body. +type CreateRequest struct { + DisplayName string + SubmittedBy string +} + +// Create records a new pending_review submission. It does NOT start a build: +// nothing is built until an admin approves (the whole point of the lane). +func (m *Manager) Create(ctx context.Context, req CreateRequest) (*Submission, error) { + name := strings.TrimSpace(req.DisplayName) + switch { + case name == "": + return nil, invalidf("display name is required") + case len(name) > maxDisplayName: + return nil, invalidf("display name exceeds %d characters", maxDisplayName) + case !displayNameRE.MatchString(name): + return nil, invalidf("display name contains unsupported characters") + } + if strings.TrimSpace(req.SubmittedBy) == "" { + return nil, invalidf("submitter identity is required") + } + + id := m.newID() + s := &Submission{ + ID: id, + SubmittedBy: req.SubmittedBy, + DisplayName: name, + ContextRef: m.deriveContextRef(id), + Status: StatusPendingReview, + CreatedAt: m.now(), + } + if err := m.Store.CreateSubmission(ctx, s); err != nil { + return nil, err + } + return s, nil +} + +// Approve is the admin gate. It atomically claims the pending_review -> approved +// transition (CAS) and ONLY the winner starts the build, so concurrent approvals +// can never double-build. The build runs through the SAME gated Builder.Submit as +// an admin's direct build: approval is layered in FRONT of the Trivy scan, never +// instead of it (spec §16), so a CRITICAL CVE still fails the build and nothing +// is admitted even after a human approved. +// +// Ordering and its two post-CAS partial states. Deterministic validation (a +// misconfigured registry/context, an id that does not form a valid ref) runs +// BEFORE the CAS via build.Validate, so such failures never leave a stuck +// approved row. The CAS is then committed before Submit. Two failures can occur +// after it, and they are NOT equally benign: +// +// - Submit fails (transient cluster error): the row is approved with build_id +// NULL and NO build is running. Benign and recoverable — an admin re-drives +// it via the direct build path. +// - Submit SUCCEEDS but the follow-up LinkBuild fails: the row is approved with +// build_id NULL while a build IS running and will push to the derived +// image_ref. On the row this is indistinguishable from the benign case, so a +// blind "re-drive" would double-build and double-push to :latest (no unique +// constraint stops it). Approve therefore returns a DISTINCT error naming the +// running build id and warning not to re-submit. +// +// Both are labeled KNOWN-LIMITATIONs (see package doc); the second is the worse +// one and the reason the error is differentiated. The alternative ordering +// (Submit-then-CAS) would either double-build under a concurrent approve or +// orphan a build on a lost race, both worse still. +func (m *Manager) Approve(ctx context.Context, id, reviewedBy string) (*Submission, error) { + if strings.TrimSpace(reviewedBy) == "" { + return nil, invalidf("reviewer identity is required") + } + + sub, err := m.Store.GetSubmission(ctx, id) + if err != nil { + return nil, err + } + if sub.Status != StatusPendingReview { + return nil, ErrAlreadyReviewed + } + + imageRef := m.deriveImageRef(id) + req := build.Request{ + ImageRef: imageRef, + Dockerfile: auditDockerfile(id, sub.ContextRef), + ContextRef: sub.ContextRef, + BaseImage: "", // declared inside the uploaded context, unknown here + RequestedBy: reviewedBy, + } + // Pre-validate against the SAME registry the Builder enforces, BEFORE the CAS, + // so a deterministic config error cannot strand the row in approved. + if err := build.Validate(req, build.Config{RegistryURL: m.Registry}); err != nil { + return nil, err + } + + now := m.now() + won, err := m.Store.ApproveSubmission(ctx, id, reviewedBy, imageRef, now) + if err != nil { + return nil, err + } + if !won { + // Lost the race to a concurrent approve/reject. + return nil, ErrAlreadyReviewed + } + + // Only the CAS winner starts the build. + bld, err := m.Builds.Submit(ctx, req) + if err != nil { + // Approved but unbuilt and NOTHING is running — admin remediation via the + // direct build path is safe (the benign partial state). + return nil, fmt.Errorf("submit: approved but build hand-off failed: %w", err) + } + if err := m.Store.LinkBuild(ctx, id, bld.ID); err != nil { + // The worse partial state: the build IS running and will push to image_ref, + // but the row still shows build_id NULL. A blind re-drive would double-build. + // Name the running build id and warn explicitly so an operator reconciles it + // (link it by hand / let it finish) instead of re-submitting. + return nil, fmt.Errorf("submit: approved and build %s started but linking it to the submission failed (build is running — do NOT re-submit, reconcile build_id by hand): %w", bld.ID, err) + } + + reviewedAt := now + sub.Status = StatusApproved + sub.ImageRef = imageRef + sub.BuildID = bld.ID + sub.ReviewedBy = reviewedBy + sub.ReviewedAt = &reviewedAt + return sub, nil +} + +// Reject is the admin's other verdict: it atomically claims +// pending_review -> rejected with a required reason and starts NO build. +func (m *Manager) Reject(ctx context.Context, id, reviewedBy, reason string) (*Submission, error) { + if strings.TrimSpace(reviewedBy) == "" { + return nil, invalidf("reviewer identity is required") + } + reason = strings.TrimSpace(reason) + switch { + case reason == "": + return nil, invalidf("a reject reason is required") + case len(reason) > maxRejectReason: + return nil, invalidf("reject reason exceeds %d characters", maxRejectReason) + } + + sub, err := m.Store.GetSubmission(ctx, id) + if err != nil { + return nil, err + } + if sub.Status != StatusPendingReview { + return nil, ErrAlreadyReviewed + } + + now := m.now() + won, err := m.Store.RejectSubmission(ctx, id, reviewedBy, reason, now) + if err != nil { + return nil, err + } + if !won { + return nil, ErrAlreadyReviewed + } + + reviewedAt := now + sub.Status = StatusRejected + sub.ReviewedBy = reviewedBy + sub.RejectReason = reason + sub.ReviewedAt = &reviewedAt + return sub, nil +} + +// List returns every submission, newest first (the admin review queue). +func (m *Manager) List(ctx context.Context) ([]Submission, error) { + return m.Store.ListSubmissions(ctx) +} + +// ListBy returns one user's submissions, newest first (the "my uploads" view). +func (m *Manager) ListBy(ctx context.Context, submittedBy string) ([]Submission, error) { + if strings.TrimSpace(submittedBy) == "" { + return nil, invalidf("submitter identity is required") + } + return m.Store.ListSubmissionsBy(ctx, submittedBy) +} + +// Compile-time proof that the production build subsystem satisfies Builds. +var _ Builds = (*build.Builder)(nil) diff --git a/internal/submit/submit_test.go b/internal/submit/submit_test.go new file mode 100644 index 0000000..01c3454 --- /dev/null +++ b/internal/submit/submit_test.go @@ -0,0 +1,454 @@ +package submit + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + "felis.lolicon.best/internal/build" +) + +// testNow is the frozen clock for hermetic assertions. +var testNow = time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + +const ( + testRegistry = "registry.felis.svc:5000" + testContext = "s3://felis-user-uploads" +) + +// fakeStore is an in-memory Store with CAS semantics and error injection. +type fakeStore struct { + subs map[string]*Submission + + createErr error + approveErr error + rejectErr error + linkErr error + + linked []string // "id=buildID" recorder +} + +func newFakeStore() *fakeStore { return &fakeStore{subs: map[string]*Submission{}} } + +func (f *fakeStore) CreateSubmission(_ context.Context, s *Submission) error { + if f.createErr != nil { + return f.createErr + } + cp := *s + f.subs[s.ID] = &cp + return nil +} + +func (f *fakeStore) GetSubmission(_ context.Context, id string) (*Submission, error) { + s, ok := f.subs[id] + if !ok { + return nil, ErrNotFound + } + cp := *s + return &cp, nil +} + +func (f *fakeStore) ListSubmissions(_ context.Context) ([]Submission, error) { + out := make([]Submission, 0, len(f.subs)) + for _, s := range f.subs { + out = append(out, *s) + } + return out, nil +} + +func (f *fakeStore) ListSubmissionsBy(_ context.Context, by string) ([]Submission, error) { + var out []Submission + for _, s := range f.subs { + if s.SubmittedBy == by { + out = append(out, *s) + } + } + return out, nil +} + +func (f *fakeStore) ApproveSubmission(_ context.Context, id, reviewedBy, imageRef string, at time.Time) (bool, error) { + if f.approveErr != nil { + return false, f.approveErr + } + s, ok := f.subs[id] + if !ok || s.Status != StatusPendingReview { + return false, nil // CAS lost / nonexistent + } + s.Status = StatusApproved + s.ImageRef = imageRef + s.ReviewedBy = reviewedBy + t := at + s.ReviewedAt = &t + return true, nil +} + +func (f *fakeStore) RejectSubmission(_ context.Context, id, reviewedBy, reason string, at time.Time) (bool, error) { + if f.rejectErr != nil { + return false, f.rejectErr + } + s, ok := f.subs[id] + if !ok || s.Status != StatusPendingReview { + return false, nil + } + s.Status = StatusRejected + s.ReviewedBy = reviewedBy + s.RejectReason = reason + t := at + s.ReviewedAt = &t + return true, nil +} + +func (f *fakeStore) LinkBuild(_ context.Context, id, buildID string) error { + if f.linkErr != nil { + return f.linkErr + } + s, ok := f.subs[id] + if !ok { + return ErrNotFound + } + s.BuildID = buildID + f.linked = append(f.linked, id+"="+buildID) + return nil +} + +// fakeBuilds records Submit calls and can inject a failure. +type fakeBuilds struct { + submitErr error + calls int + got []build.Request +} + +func (f *fakeBuilds) Submit(_ context.Context, req build.Request) (*build.Build, error) { + f.calls++ + f.got = append(f.got, req) + if f.submitErr != nil { + return nil, f.submitErr + } + return &build.Build{ID: "bld-x", ImageRef: req.ImageRef, Status: build.StatusBuilding}, nil +} + +// newManager wires the fakes with a frozen clock and a deterministic lowercase +// id generator (sub-1, sub-2, ...). +func newManager() (*Manager, *fakeStore, *fakeBuilds) { + st := newFakeStore() + bl := &fakeBuilds{} + n := 0 + m := &Manager{ + Store: st, + Builds: bl, + Registry: testRegistry, + ContextStore: testContext, + Now: func() time.Time { return testNow }, + IDGen: func() string { n++; return "sub-" + string(rune('0'+n)) }, + } + return m, st, bl +} + +func TestCreatePendingDoesNotBuild(t *testing.T) { + m, st, bl := newManager() + + sub, err := m.Create(context.Background(), CreateRequest{ + DisplayName: "My Modpack", + SubmittedBy: "user-1", + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + if sub.Status != StatusPendingReview { + t.Fatalf("status = %q, want pending_review", sub.Status) + } + if sub.ID != "sub-1" { + t.Fatalf("id = %q, want sub-1", sub.ID) + } + // Context ref is derived, never user-supplied. + if want := testContext + "/sub-1/context.tar.gz"; sub.ContextRef != want { + t.Fatalf("context_ref = %q, want %q", sub.ContextRef, want) + } + if sub.ImageRef != "" || sub.BuildID != "" { + t.Fatalf("image/build set before approval: %+v", sub) + } + if bl.calls != 0 { + t.Fatalf("Create started %d builds, want 0", bl.calls) + } + if _, ok := st.subs["sub-1"]; !ok { + t.Fatal("submission not persisted") + } +} + +func TestCreateValidation(t *testing.T) { + cases := []struct { + name string + req CreateRequest + }{ + {"empty name", CreateRequest{DisplayName: " ", SubmittedBy: "user-1"}}, + {"oversize name", CreateRequest{DisplayName: strings.Repeat("a", maxDisplayName+1), SubmittedBy: "user-1"}}, + {"control chars", CreateRequest{DisplayName: "bad\nname", SubmittedBy: "user-1"}}, + {"no submitter", CreateRequest{DisplayName: "ok", SubmittedBy: ""}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m, st, bl := newManager() + _, err := m.Create(context.Background(), tc.req) + if !errors.Is(err, ErrInvalid) { + t.Fatalf("err = %v, want ErrInvalid", err) + } + if len(st.subs) != 0 || bl.calls != 0 { + t.Fatal("invalid Create persisted or built") + } + }) + } +} + +func TestApproveStartsExactlyOneBuildAndLinks(t *testing.T) { + m, st, bl := newManager() + seed, _ := m.Create(context.Background(), CreateRequest{DisplayName: "Pack", SubmittedBy: "user-1"}) + + sub, err := m.Approve(context.Background(), seed.ID, "admin@example.test") + if err != nil { + t.Fatalf("Approve: %v", err) + } + if sub.Status != StatusApproved { + t.Fatalf("status = %q, want approved", sub.Status) + } + wantImage := testRegistry + "/user-uploads/sub-1:latest" + if sub.ImageRef != wantImage { + t.Fatalf("image_ref = %q, want %q", sub.ImageRef, wantImage) + } + if sub.BuildID != "bld-x" { + t.Fatalf("build_id = %q, want bld-x", sub.BuildID) + } + if sub.ReviewedBy != "admin@example.test" || sub.ReviewedAt == nil { + t.Fatalf("review metadata missing: %+v", sub) + } + // Exactly one build, carrying the derived refs + reviewer + audit Dockerfile. + if bl.calls != 1 { + t.Fatalf("builds started = %d, want 1", bl.calls) + } + req := bl.got[0] + if req.ImageRef != wantImage { + t.Fatalf("build ImageRef = %q, want %q", req.ImageRef, wantImage) + } + if req.ContextRef != seed.ContextRef { + t.Fatalf("build ContextRef = %q, want %q", req.ContextRef, seed.ContextRef) + } + if req.RequestedBy != "admin@example.test" { + t.Fatalf("build RequestedBy = %q, want admin", req.RequestedBy) + } + if strings.TrimSpace(req.Dockerfile) == "" { + t.Fatal("audit Dockerfile must be non-empty (build.Validate requires it)") + } + // The persisted row links the build. + if got := st.subs[seed.ID]; got.BuildID != "bld-x" || got.Status != StatusApproved { + t.Fatalf("persisted row not linked/approved: %+v", got) + } + if len(st.linked) != 1 { + t.Fatalf("LinkBuild called %d times, want 1", len(st.linked)) + } +} + +func TestApproveDerivedRefValidates(t *testing.T) { + // The derived push target must satisfy the build subsystem's own admission + // rules, else Approve would CAS then fail at Submit. Prove it against the SAME + // validator the Builder uses. + m, _, _ := newManager() + ref := m.deriveImageRef("sub-1") + req := build.Request{ + ImageRef: ref, + Dockerfile: auditDockerfile("sub-1", m.deriveContextRef("sub-1")), + ContextRef: m.deriveContextRef("sub-1"), + } + if err := build.Validate(req, build.Config{RegistryURL: testRegistry}); err != nil { + t.Fatalf("derived build request does not validate: %v", err) + } + if !strings.HasPrefix(ref, testRegistry+"/user-uploads/") { + t.Fatalf("derived ref %q is not in the user-uploads namespace", ref) + } +} + +func TestApproveIsCASNoDoubleBuild(t *testing.T) { + m, _, bl := newManager() + seed, _ := m.Create(context.Background(), CreateRequest{DisplayName: "Pack", SubmittedBy: "user-1"}) + + if _, err := m.Approve(context.Background(), seed.ID, "admin@x"); err != nil { + t.Fatalf("first Approve: %v", err) + } + // A second approve loses the CAS and must NOT start another build. + _, err := m.Approve(context.Background(), seed.ID, "admin@x") + if !errors.Is(err, ErrAlreadyReviewed) { + t.Fatalf("second Approve err = %v, want ErrAlreadyReviewed", err) + } + if bl.calls != 1 { + t.Fatalf("builds started = %d, want 1 (no double-build)", bl.calls) + } +} + +func TestApproveRejectedSubmission(t *testing.T) { + m, _, bl := newManager() + seed, _ := m.Create(context.Background(), CreateRequest{DisplayName: "Pack", SubmittedBy: "user-1"}) + if _, err := m.Reject(context.Background(), seed.ID, "admin@x", "nope"); err != nil { + t.Fatalf("Reject: %v", err) + } + _, err := m.Approve(context.Background(), seed.ID, "admin@x") + if !errors.Is(err, ErrAlreadyReviewed) { + t.Fatalf("Approve after reject err = %v, want ErrAlreadyReviewed", err) + } + if bl.calls != 0 { + t.Fatalf("builds started = %d, want 0", bl.calls) + } +} + +func TestApproveUnknown(t *testing.T) { + m, _, _ := newManager() + _, err := m.Approve(context.Background(), "sub-nope", "admin@x") + if !errors.Is(err, ErrNotFound) { + t.Fatalf("err = %v, want ErrNotFound", err) + } +} + +func TestApproveBuildFailureLeavesApprovedUnlinked(t *testing.T) { + // The post-CAS Submit failure is the documented KNOWN-LIMITATION: the row is + // left approved with build_id NULL, recoverable by an admin via the direct + // build path. It must NOT roll back to pending or double-count. + m, st, bl := newManager() + bl.submitErr = errors.New("apiserver unreachable") + seed, _ := m.Create(context.Background(), CreateRequest{DisplayName: "Pack", SubmittedBy: "user-1"}) + + _, err := m.Approve(context.Background(), seed.ID, "admin@x") + if err == nil { + t.Fatal("Approve should surface the build hand-off failure") + } + if bl.calls != 1 { + t.Fatalf("builds attempted = %d, want 1", bl.calls) + } + got := st.subs[seed.ID] + if got.Status != StatusApproved { + t.Fatalf("status = %q, want approved (CAS already committed)", got.Status) + } + if got.BuildID != "" { + t.Fatalf("build_id = %q, want empty (Submit failed before LinkBuild)", got.BuildID) + } + if len(st.linked) != 0 { + t.Fatal("LinkBuild must not run when Submit failed") + } +} + +func TestApproveLinkFailureNamesRunningBuild(t *testing.T) { + // The WORSE post-CAS partial state: Submit SUCCEEDS (a build is genuinely + // running and will push to image_ref) but the follow-up LinkBuild write fails, + // so the row is approved with build_id still NULL. On the row this is + // indistinguishable from the benign Submit-fail case above, so a blind re-drive + // would double-build/double-push to :latest. Approve must therefore return a + // DISTINCT error that names the running build id and forbids re-submission — + // this exercises that branch (the benign test above does not). + m, st, bl := newManager() + st.linkErr = errors.New("db write timeout") + seed, _ := m.Create(context.Background(), CreateRequest{DisplayName: "Pack", SubmittedBy: "user-1"}) + + _, err := m.Approve(context.Background(), seed.ID, "admin@x") + if err == nil { + t.Fatal("Approve must surface the link-write failure (a build is running)") + } + // A build WAS started — that is precisely why re-driving is unsafe. + if bl.calls != 1 { + t.Fatalf("builds started = %d, want 1 (Submit succeeded, build is running)", bl.calls) + } + // The error must name the running build id and the do-not-resubmit warning, and + // wrap the underlying store error so callers can still inspect the cause. + if !strings.Contains(err.Error(), "bld-x") { + t.Fatalf("error must name the running build id bld-x: %v", err) + } + if !strings.Contains(err.Error(), "do NOT re-submit") { + t.Fatalf("error must warn against re-submission: %v", err) + } + if !errors.Is(err, st.linkErr) { + t.Fatalf("error must wrap the underlying link failure: %v", err) + } + // The row is approved (CAS committed) but build_id stays empty — the very + // ambiguity the distinct error compensates for. + got := st.subs[seed.ID] + if got.Status != StatusApproved { + t.Fatalf("status = %q, want approved (CAS already committed)", got.Status) + } + if got.BuildID != "" { + t.Fatalf("build_id = %q, want empty (LinkBuild failed to record it)", got.BuildID) + } + if len(st.linked) != 0 { + t.Fatal("LinkBuild errored before recording — must not appear linked") + } +} + +func TestApproveValidatesBeforeCAS(t *testing.T) { + // A misconfigured registry makes the derived ref fail build.Validate. That + // deterministic failure must happen BEFORE the CAS, so the row stays pending + // and no build is attempted — never a stranded approved row. + m, st, bl := newManager() + m.Registry = "" // derived ref becomes "/user-uploads/...", not host-qualified + seed, _ := m.Create(context.Background(), CreateRequest{DisplayName: "Pack", SubmittedBy: "user-1"}) + + _, err := m.Approve(context.Background(), seed.ID, "admin@x") + if !errors.Is(err, build.ErrInvalid) { + t.Fatalf("err = %v, want build.ErrInvalid (pre-CAS validation)", err) + } + if got := st.subs[seed.ID]; got.Status != StatusPendingReview { + t.Fatalf("status = %q, want still pending_review (CAS not reached)", got.Status) + } + if bl.calls != 0 { + t.Fatalf("builds started = %d, want 0", bl.calls) + } +} + +func TestRejectDoesNotBuild(t *testing.T) { + m, st, bl := newManager() + seed, _ := m.Create(context.Background(), CreateRequest{DisplayName: "Pack", SubmittedBy: "user-1"}) + + sub, err := m.Reject(context.Background(), seed.ID, "admin@x", "ships a coin miner") + if err != nil { + t.Fatalf("Reject: %v", err) + } + if sub.Status != StatusRejected || sub.RejectReason != "ships a coin miner" { + t.Fatalf("reject verdict not recorded: %+v", sub) + } + if sub.ReviewedBy != "admin@x" || sub.ReviewedAt == nil { + t.Fatalf("review metadata missing: %+v", sub) + } + if bl.calls != 0 { + t.Fatalf("builds started = %d, want 0", bl.calls) + } + if st.subs[seed.ID].Status != StatusRejected { + t.Fatal("rejection not persisted") + } +} + +func TestRejectRequiresReason(t *testing.T) { + m, st, _ := newManager() + seed, _ := m.Create(context.Background(), CreateRequest{DisplayName: "Pack", SubmittedBy: "user-1"}) + _, err := m.Reject(context.Background(), seed.ID, "admin@x", " ") + if !errors.Is(err, ErrInvalid) { + t.Fatalf("err = %v, want ErrInvalid", err) + } + if got := st.subs[seed.ID]; got.Status != StatusPendingReview { + t.Fatalf("status = %q, want pending_review", got.Status) + } +} + +func TestListBy(t *testing.T) { + m, _, _ := newManager() + if _, err := m.Create(context.Background(), CreateRequest{DisplayName: "A", SubmittedBy: "user-1"}); err != nil { + t.Fatal(err) + } + if _, err := m.Create(context.Background(), CreateRequest{DisplayName: "B", SubmittedBy: "user-2"}); err != nil { + t.Fatal(err) + } + mine, err := m.ListBy(context.Background(), "user-1") + if err != nil { + t.Fatalf("ListBy: %v", err) + } + if len(mine) != 1 || mine[0].SubmittedBy != "user-1" { + t.Fatalf("ListBy scoped wrong: %+v", mine) + } + if _, err := m.ListBy(context.Background(), ""); !errors.Is(err, ErrInvalid) { + t.Fatalf("empty submitter err = %v, want ErrInvalid", err) + } +}