diff --git a/server/internal/app/route_sweep_test.go b/server/internal/app/route_sweep_test.go index 3c174e54d..833d0c615 100644 --- a/server/internal/app/route_sweep_test.go +++ b/server/internal/app/route_sweep_test.go @@ -92,8 +92,12 @@ func TestEveryRouteAnswersWithoutAServerError(t *testing.T) { for _, rt := range routes { for _, at := range attempts { status := env.sweep(rt.method, sweepURL(rt.pattern, env.org.Slug, at.uid), at.token, at.body) - // 503 is how an unconfigured integration (WebAuthn, bots) says so. - if status >= http.StatusInternalServerError && status != http.StatusServiceUnavailable { + // 503 is how an unconfigured integration (WebAuthn, bots) says so; + // 502 is how Freebox pairing says the box is unreachable (the + // default base URL is a public host, so the answer depends on + // the network the test runs on: 502 when refused, none on timeout). + if status >= http.StatusInternalServerError && + status != http.StatusServiceUnavailable && status != http.StatusBadGateway { serverErrors = append(serverErrors, rt.method+" "+rt.pattern) } } diff --git a/server/internal/app/server.go b/server/internal/app/server.go index 1461cf182..ab2d0340f 100644 --- a/server/internal/app/server.go +++ b/server/internal/app/server.go @@ -2508,11 +2508,16 @@ func (s *Server) SetupRoutes(ctx context.Context) { mgmt.GET("/version", s.getVersion) mgmt.GET("/limits", s.getLimits) // POST /report is anonymous (no auth, no org required) and outside - // limitedPrefix, so it gets its own dedicated, much stricter limiter - // rather than riding on the unlimited /api/mgmt traffic assumption (spec - // 2026-09-25-26) — see reportRateLimitConfig. RateLimitRoute (not - // RateLimit) because the route itself is the scope here, not a path - // prefix. + // limitedPrefix, so it gets its own dedicated, much stricter per-IP + // limiter rather than riding on the unlimited /api/mgmt traffic + // assumption (spec 2026-09-25-26) — see reportRateLimitConfig. + // RateLimitRoute (not RateLimit) because the route itself is the scope + // here, not a path prefix. What it can STORE is bounded in + // feedback.Service (spec 2026-10-29-01): anonymous screenshots spend an + // instance-wide hourly budget (app.feedback_anon_screenshots_per_hour) + // and need a free GitHub issue dispatch slot, every screenshot also fits + // the per-org quota (checked atomically with the insert), and the + // state-cleanup job purges report files past app.feedback_retention_days. reportLimiter := middleware.NewRateLimiter(reportRateLimitConfig(s.config.Server.RateLimiting), ctx) mgmtReport := mainGroup.NewGroup("/api/mgmt").Use(reportLimiter.RateLimitRoute) mgmtReport.POST("/report", feedbackHandler.SubmitReport) diff --git a/server/internal/checkworker/repair_grant_agent_test.go b/server/internal/checkworker/repair_grant_agent_test.go index 61464b8d7..e7e0f1088 100644 --- a/server/internal/checkworker/repair_grant_agent_test.go +++ b/server/internal/checkworker/repair_grant_agent_test.go @@ -31,6 +31,10 @@ type agentGrantServer struct { mu sync.Mutex result *agents.ClientFrame done chan struct{} + // stop ends every connection the fake still holds (the agent may redial after + // the first one closed); handlers tracks them so the test can wait them out. + stop chan struct{} + handlers sync.WaitGroup } func (s *agentGrantServer) serve(writer http.ResponseWriter, req *http.Request) { @@ -39,7 +43,22 @@ func (s *agentGrantServer) serve(writer http.ResponseWriter, req *http.Request) return } - ctx := req.Context() + s.handlers.Add(1) + defer s.handlers.Done() + + ctx, cancel := context.WithCancel(req.Context()) + defer cancel() + + go func() { + select { + case <-s.stop: + cancel() + case <-ctx.Done(): + } + }() + + defer func() { _ = conn.CloseNow() }() + hello := agents.ServerFrame{Type: agents.MsgTypeHello, AgentUID: "agent-1", WorkerUID: "worker-1", Region: "r"} if req.Header.Get("Authorization") != "" { @@ -141,6 +160,7 @@ func runAgentGrantJob( region := "r" fake := &agentGrantServer{ done: make(chan struct{}), + stop: make(chan struct{}), job: agents.AgentJob{ UID: "job-1", CheckUID: "check-1", Type: string(checkerdef.CheckTypeJS), Config: stored, ConfigSealed: &sealed, Period: "1m", ScheduledAt: &due, Region: ®ion, @@ -148,7 +168,11 @@ func runAgentGrantJob( } srv := httptest.NewServer(http.HandlerFunc(fake.serve)) - t.Cleanup(srv.Close) + t.Cleanup(func() { + close(fake.stop) + srv.Close() + fake.handlers.Wait() + }) wsb := backend.NewWSBackend(srv.URL, "spe_token", "agent", &backend.Identity{AgentKeys: *keys}, nil, backend.WithPingInterval(400*time.Millisecond)) diff --git a/server/internal/config/config.go b/server/internal/config/config.go index 6f1f996a4..37941870a 100644 --- a/server/internal/config/config.go +++ b/server/internal/config/config.go @@ -1237,6 +1237,17 @@ type AppConfig struct { // (systemconfig.KeyFeedbackMaxStorageBytes), read at request time by // feedback.Service so a DB-set value takes effect without a restart. FeedbackMaxStorageBytes int64 `koanf:"feedback_max_storage_bytes"` + // FeedbackAnonScreenshotsPerHour caps how many screenshots ANONYMOUS bug + // reports may store per hour across the WHOLE instance (spec 2026-10-29-01). + // The per-org quota above is spent once per org, and the anonymous caller + // picks the org, so only an instance-wide budget bounds what they write. + // Counted through db.BumpGlobalCounter, so it holds across processes. + // Authenticated members are not counted. 0 or negative disables it. + FeedbackAnonScreenshotsPerHour int `koanf:"feedback_anon_screenshots_per_hour"` + // FeedbackRetentionDays is how long feedback-report screenshots are kept: + // the state-cleanup job purges (row and blob) every reports file older + // than this (spec 2026-10-29-01). 0 or negative keeps them forever. + FeedbackRetentionDays int `koanf:"feedback_retention_days"` } // AppGitHubConfig holds the GitHub credentials used for in-app feature integrations @@ -1254,13 +1265,33 @@ const DefaultFeedbackMaxStorageBytes int64 = 100 * 1024 * 1024 // AppConfig.FeedbackMaxStorageBytes. const EnvAppFeedbackMaxStorageBytes = "SP_APP_FEEDBACK_MAX_STORAGE_BYTES" -// applyFeedbackEnv binds SP_APP_FEEDBACK_MAX_STORAGE_BYTES. +// DefaultFeedbackAnonScreenshotsPerHour is the built-in instance-wide budget of +// anonymous bug-report screenshots per hour: the same as the GitHub issue +// dispatch cap, since a screenshot past that cap would never be filed. +const DefaultFeedbackAnonScreenshotsPerHour = 20 + +// EnvAppFeedbackAnonScreenshotsPerHour overrides +// AppConfig.FeedbackAnonScreenshotsPerHour. +const EnvAppFeedbackAnonScreenshotsPerHour = "SP_APP_FEEDBACK_ANON_SCREENSHOTS_PER_HOUR" + +// DefaultFeedbackRetentionDays is the built-in retention of bug-report +// screenshots. +const DefaultFeedbackRetentionDays = 90 + +// EnvAppFeedbackRetentionDays overrides AppConfig.FeedbackRetentionDays. +const EnvAppFeedbackRetentionDays = "SP_APP_FEEDBACK_RETENTION_DAYS" + +// applyFeedbackEnv binds SP_APP_FEEDBACK_MAX_STORAGE_BYTES, +// SP_APP_FEEDBACK_ANON_SCREENSHOTS_PER_HOUR and SP_APP_FEEDBACK_RETENTION_DAYS. // feedback_max_storage_bytes is snake_case, so koanf's env loader can never // reach it (SP_APP_FEEDBACK_MAX_STORAGE_BYTES would land on // app.feedback.max.storage.bytes, not app.feedback_max_storage_bytes). An // absent or unparseable variable leaves whatever config.yml / // config.local.yml (or the built-in default) already set alone. func applyFeedbackEnv(cfg *AppConfig) { + applyFeedbackIntEnv(EnvAppFeedbackAnonScreenshotsPerHour, &cfg.FeedbackAnonScreenshotsPerHour) + applyFeedbackIntEnv(EnvAppFeedbackRetentionDays, &cfg.FeedbackRetentionDays) + v := os.Getenv(EnvAppFeedbackMaxStorageBytes) if v == "" { return @@ -1271,6 +1302,19 @@ func applyFeedbackEnv(cfg *AppConfig) { } } +// applyFeedbackIntEnv sets *dst from the integer environment variable name. An +// absent or unparseable variable leaves *dst alone. +func applyFeedbackIntEnv(name string, dst *int) { + v := os.Getenv(name) + if v == "" { + return + } + + if n, err := strconv.Atoi(v); err == nil { + *dst = n + } +} + // AggregationConfig controls how aggressively raw/hour/day result data is compacted. // Each value is the number of completed periods of that tier to retain before // rolling up to the next tier. Minimum 1 (the previous behavior). @@ -2267,8 +2311,10 @@ func Load() (*Config, error) { App: AppConfig{ // No token and no repo: the in-app bug report is off unless the // operator sets both SP_APP_GITHUB_ISSUES_TOKEN and SP_APP_GITHUB_REPO. - GitHub: AppGitHubConfig{}, - FeedbackMaxStorageBytes: DefaultFeedbackMaxStorageBytes, + GitHub: AppGitHubConfig{}, + FeedbackMaxStorageBytes: DefaultFeedbackMaxStorageBytes, + FeedbackAnonScreenshotsPerHour: DefaultFeedbackAnonScreenshotsPerHour, + FeedbackRetentionDays: DefaultFeedbackRetentionDays, }, Google: GoogleOAuthConfig{Enabled: false}, GitHub: GitHubOAuthConfig{Enabled: false}, diff --git a/server/internal/config/envvars.go b/server/internal/config/envvars.go index 00db4a233..74e4535ea 100644 --- a/server/internal/config/envvars.go +++ b/server/internal/config/envvars.go @@ -192,6 +192,8 @@ func manualReaderServerEnvVars() []string { // applyFeedbackEnv — feedback_max_storage_bytes is snake_case, so // koanf's env loader cannot reach it. EnvAppFeedbackMaxStorageBytes, + EnvAppFeedbackAnonScreenshotsPerHour, + EnvAppFeedbackRetentionDays, // applyCheckersEnv — both keys have a snake_case segment, so koanf's // env loader cannot reach them (see BrowserCheckerConfig). "SP_CHECKERS_BROWSER_CDP_URL", diff --git a/server/internal/config/feedback_env_test.go b/server/internal/config/feedback_env_test.go new file mode 100644 index 000000000..9dafa87c1 --- /dev/null +++ b/server/internal/config/feedback_env_test.go @@ -0,0 +1,32 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestApplyFeedbackEnv_IntVars(t *testing.T) { + cfg := &AppConfig{FeedbackAnonScreenshotsPerHour: 20, FeedbackRetentionDays: 90} + + t.Setenv(EnvAppFeedbackAnonScreenshotsPerHour, "5") + t.Setenv(EnvAppFeedbackRetentionDays, "not-a-number") + applyFeedbackEnv(cfg) + + require.Equal(t, 5, cfg.FeedbackAnonScreenshotsPerHour) + require.Equal(t, 90, cfg.FeedbackRetentionDays, "an unparseable value leaves the setting alone") + + t.Setenv(EnvAppFeedbackRetentionDays, "0") + applyFeedbackEnv(cfg) + require.Zero(t, cfg.FeedbackRetentionDays) +} + +func TestLoad_FeedbackDefaults(t *testing.T) { + t.Setenv(EnvAppFeedbackAnonScreenshotsPerHour, "") + t.Setenv(EnvAppFeedbackRetentionDays, "") + + cfg, err := Load() + require.NoError(t, err) + require.Equal(t, DefaultFeedbackAnonScreenshotsPerHour, cfg.App.FeedbackAnonScreenshotsPerHour) + require.Equal(t, DefaultFeedbackRetentionDays, cfg.App.FeedbackRetentionDays) +} diff --git a/server/internal/db/postgres/postgres.go b/server/internal/db/postgres/postgres.go index d3c7d5ad0..3268aecf7 100644 --- a/server/internal/db/postgres/postgres.go +++ b/server/internal/db/postgres/postgres.go @@ -8027,6 +8027,37 @@ func (s *Service) SumFileSizeByGroup(ctx context.Context, orgUID, group string) return total.Int64, nil } +// ListFilesByGroup returns live files of every organization under the given +// storage group, created before `before`, oldest first, capped at limit. The +// group is matched on its "//" URI path segment, as in +// SumFileSizeByGroup. +func (s *Service) ListFilesByGroup( + ctx context.Context, group string, before time.Time, limit int, +) ([]*models.File, error) { + if group == "" { + return nil, nil + } + + var files []*models.File + + query := s.db.NewSelect(). + Model(&files). + Where("deleted_at IS NULL"). + Where("file_uri LIKE ? ESCAPE '\\'", "%/"+escapeLikePrefix(group)+"/%"). + Where("created_at < ?", before). + Order("created_at ASC") + + if limit > 0 { + query = query.Limit(limit) + } + + if err := query.Scan(ctx); err != nil { + return nil, err + } + + return files, nil +} + // ListAttachmentsByTopicPrefix returns live attachment rows across all orgs // under a topic prefix, older than `before`, capped at limit. Cross-org because // its only caller is the GC sweep. diff --git a/server/internal/db/service.go b/server/internal/db/service.go index a177d6c86..477b539a9 100644 --- a/server/internal/db/service.go +++ b/server/internal/db/service.go @@ -1458,6 +1458,11 @@ type Service interface { // the scheme/bucket portion varies by storage backend (local vs S3). Used // by the feedback-report per-org storage quota (spec 2026-09-25-26). SumFileSizeByGroup(ctx context.Context, orgUID, group string) (int64, error) + // ListFilesByGroup returns live files across ALL organizations whose + // storage URI belongs to the given filestorage.GroupType and which were + // created before `before`, oldest first, capped at limit. Cross-org by + // design: its only caller is the report-retention sweep (spec 2026-10-29-01). + ListFilesByGroup(ctx context.Context, group string, before time.Time, limit int) ([]*models.File, error) // CheckDependency operations CreateCheckDependency(ctx context.Context, dep *models.CheckDependency) error diff --git a/server/internal/db/sqlite/sqlite.go b/server/internal/db/sqlite/sqlite.go index 353c14837..1a36c2126 100644 --- a/server/internal/db/sqlite/sqlite.go +++ b/server/internal/db/sqlite/sqlite.go @@ -7991,6 +7991,37 @@ func (s *Service) SumFileSizeByGroup(ctx context.Context, orgUID, group string) return total.Int64, nil } +// ListFilesByGroup returns live files of every organization under the given +// storage group, created before `before`, oldest first, capped at limit. The +// group is matched on its "//" URI path segment, as in +// SumFileSizeByGroup. +func (s *Service) ListFilesByGroup( + ctx context.Context, group string, before time.Time, limit int, +) ([]*models.File, error) { + if group == "" { + return nil, nil + } + + var files []*models.File + + query := s.db.NewSelect(). + Model(&files). + Where("deleted_at IS NULL"). + Where("file_uri LIKE ? ESCAPE '\\'", "%/"+escapeLikePrefix(group)+"/%"). + Where("created_at < ?", before). + Order("created_at ASC") + + if limit > 0 { + query = query.Limit(limit) + } + + if err := query.Scan(ctx); err != nil { + return nil, err + } + + return files, nil +} + // ListAttachmentsByTopicPrefix returns live attachment rows across all orgs // under a topic prefix, older than `before`, capped at limit. Cross-org because // its only caller is the GC sweep. diff --git a/server/internal/handlers/feedback/budget_test.go b/server/internal/handlers/feedback/budget_test.go new file mode 100644 index 000000000..154f9c098 --- /dev/null +++ b/server/internal/handlers/feedback/budget_test.go @@ -0,0 +1,294 @@ +package feedback + +import ( + "bytes" + "context" + "mime/multipart" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/fclairamb/solidping/server/internal/db" + "github.com/fclairamb/solidping/server/internal/db/models" + "github.com/fclairamb/solidping/server/internal/db/sqlite" + "github.com/fclairamb/solidping/server/internal/handlers/files" + "github.com/fclairamb/solidping/server/internal/handlers/filestorage" +) + +// newBudgetSQLite returns a fresh in-memory SQLite database and n orgs. +func newBudgetSQLite(t *testing.T, slugs ...string) (db.Service, []*models.Organization) { + t.Helper() + r := require.New(t) + ctx := t.Context() + + dbSvc, err := sqlite.New(ctx, sqlite.Config{InMemory: true}) + r.NoError(err) + r.NoError(dbSvc.Initialize(ctx)) + t.Cleanup(func() { _ = dbSvc.Close() }) + + orgs := make([]*models.Organization, 0, len(slugs)) + + for _, slug := range slugs { + org := models.NewOrganization(slug, "Acme") + r.NoError(dbSvc.CreateOrganization(ctx, org)) + orgs = append(orgs, org) + } + + return dbSvc, orgs +} + +// newBudgetService builds a feedback service with a fake GitHub poster, the +// given per-org quota and anonymous screenshot budget, and a counter key of +// its own (tests sharing one Postgres must not share the budget). +func newBudgetService( + t *testing.T, dbSvc db.Service, quota int64, anonPerHour int, +) (*Service, *countingGitHubPoster) { + t.Helper() + + cfg := newFeedbackQuotaConfig(t, quota) + cfg.App.FeedbackAnonScreenshotsPerHour = anonPerHour + + poster := &countingGitHubPoster{} + svc := NewService(dbSvc, files.NewService(dbSvc, cfg), cfg, poster) + svc.anonCounterKey = anonScreenshotCounterKey + ":" + t.Name() + + return svc, poster +} + +// newMember creates a user and returns its UID, for authenticated reports. +func newMember(t *testing.T, dbSvc db.Service) string { + t.Helper() + + user := models.NewUser("member@acme.example") + require.NoError(t, dbSvc.CreateUser(t.Context(), user)) + + return user.UID +} + +// screenshotReport is a report with a one-byte screenshot. +func screenshotReport(orgSlug, userUID string) *SubmitReportRequest { + return &SubmitReportRequest{ + URL: "https://example.com/page", + OrgSlug: orgSlug, + UserUID: userUID, + Screenshot: strings.NewReader("x"), + ScreenshotSize: 1, + ScreenshotName: "s.png", + ScreenshotMIME: "image/png", + } +} + +// reportBytes sums the live `reports` bytes of the orgs; each test screenshot +// is one byte, so this is also the file count. +func reportBytes(ctx context.Context, t *testing.T, dbSvc db.Service, orgs ...*models.Organization) int64 { + t.Helper() + + var total int64 + + for _, org := range orgs { + used, err := dbSvc.SumFileSizeByGroup(ctx, org.UID, string(filestorage.GroupTypeReports)) + require.NoError(t, err) + + total += used + } + + return total +} + +// assertAnonBudgetIsInstanceWide is the spec 2026-10-29-01 regression: the +// anonymous screenshot budget is spent once for the whole instance, however +// the caller spreads its reports over orgs. +func assertAnonBudgetIsInstanceWide(t *testing.T, dbSvc db.Service, orgA, orgB *models.Organization) { + t.Helper() + r := require.New(t) + ctx := t.Context() + + svc, _ := newBudgetService(t, dbSvc, 1024, 3) + + for i := range 6 { + org := orgA + if i%2 == 1 { + org = orgB + } + + _, err := svc.SubmitReport(ctx, screenshotReport(org.Slug, "")) + if i < 3 { + r.NoError(err, "report %d is within the budget", i+1) + } else { + r.ErrorIs(err, ErrReportRateLimited, "report %d is past the instance budget", i+1) + } + } + + r.Equal(int64(3), reportBytes(ctx, t, dbSvc, orgA, orgB), + "exactly 3 screenshots are stored across both orgs") +} + +func TestSubmitReport_AnonBudgetIsInstanceWide_SQLite(t *testing.T) { + t.Parallel() + + dbSvc, orgs := newBudgetSQLite(t, "acme-a", "acme-b") + assertAnonBudgetIsInstanceWide(t, dbSvc, orgs[0], orgs[1]) +} + +// assertParallelQuotaHoldsOneFile: N parallel anonymous reports against an +// org whose quota has room for one file store at most one file. +func assertParallelQuotaHoldsOneFile(t *testing.T, dbSvc db.Service, org *models.Organization) { + t.Helper() + r := require.New(t) + ctx := t.Context() + + const parallel = 10 + + svc, _ := newBudgetService(t, dbSvc, 1, 0) + + var ( + wg sync.WaitGroup + mu sync.Mutex + accepted int + ) + + for range parallel { + wg.Go(func() { + _, err := svc.SubmitReport(ctx, screenshotReport(org.Slug, "")) + if err == nil { + mu.Lock() + accepted++ + mu.Unlock() + + return + } + + if !strings.Contains(err.Error(), ErrStorageQuotaExceeded.Error()) { + t.Errorf("unexpected error: %v", err) + } + }) + } + + wg.Wait() + + r.LessOrEqual(accepted, 1) + r.LessOrEqual(reportBytes(ctx, t, dbSvc, org), int64(1), "at most one file fits the quota") +} + +func TestSubmitReport_ParallelQuota_SQLite(t *testing.T) { + t.Parallel() + + dbSvc, orgs := newBudgetSQLite(t, "acme-par") + assertParallelQuotaHoldsOneFile(t, dbSvc, orgs[0]) +} + +// TestSubmitReport_DispatchCapStoresNoAnonScreenshot: with the issue dispatch +// budget spent, an anonymous screenshot would never be filed, so it is not +// stored. A member's screenshot, and a text-only anonymous report, are still +// accepted. +func TestSubmitReport_DispatchCapStoresNoAnonScreenshot(t *testing.T) { + t.Parallel() + r := require.New(t) + ctx := t.Context() + + dbSvc, orgs := newBudgetSQLite(t, "acme-cap") + svc, poster := newBudgetService(t, dbSvc, 1024, 20) + + svc.dispatchWindowStart = time.Now() + svc.dispatchCount = maxIssueDispatchesPerHour + member := newMember(t, dbSvc) + + _, err := svc.SubmitReport(ctx, screenshotReport(orgs[0].Slug, "")) + r.ErrorIs(err, ErrReportRateLimited) + r.Zero(reportBytes(ctx, t, dbSvc, orgs[0]), "no file is stored for a report that cannot be filed") + + _, err = svc.SubmitReport(ctx, screenshotReport(orgs[0].Slug, member)) + r.NoError(err, "a member's report keeps today's behavior") + r.Equal(int64(1), reportBytes(ctx, t, dbSvc, orgs[0])) + + _, err = svc.SubmitReport(ctx, &SubmitReportRequest{URL: "https://example.com/page", OrgSlug: orgs[0].Slug}) + r.NoError(err, "a text-only report is accepted") + r.Zero(poster.count(), "no issue is dispatched past the cap") +} + +// TestSubmitReport_MemberAndTextOnlyPastAnonBudget: the anonymous budget does +// not apply to authenticated members, nor to reports without a screenshot. +func TestSubmitReport_MemberAndTextOnlyPastAnonBudget(t *testing.T) { + t.Parallel() + r := require.New(t) + ctx := t.Context() + + dbSvc, orgs := newBudgetSQLite(t, "acme-member") + svc, _ := newBudgetService(t, dbSvc, 1024, 1) + member := newMember(t, dbSvc) + + _, err := svc.SubmitReport(ctx, screenshotReport(orgs[0].Slug, "")) + r.NoError(err) + + _, err = svc.SubmitReport(ctx, screenshotReport(orgs[0].Slug, "")) + r.ErrorIs(err, ErrReportRateLimited, "the anonymous budget is spent") + + _, err = svc.SubmitReport(ctx, screenshotReport(orgs[0].Slug, member)) + r.NoError(err, "an authenticated member's screenshot is still stored") + + _, err = svc.SubmitReport(ctx, &SubmitReportRequest{URL: "https://example.com/page", OrgSlug: orgs[0].Slug}) + r.NoError(err, "a report without a screenshot is accepted") + + r.Equal(int64(2), reportBytes(ctx, t, dbSvc, orgs[0])) +} + +// multipartReport builds the multipart body of an anonymous report with a +// screenshot, as the dash0 dialog sends it. +func multipartReport(t *testing.T, orgSlug string) (*bytes.Buffer, string) { + t.Helper() + + var body bytes.Buffer + + writer := multipart.NewWriter(&body) + require.NoError(t, writer.WriteField("url", "https://example.com/page")) + require.NoError(t, writer.WriteField("org", orgSlug)) + + part, err := writer.CreateFormFile("screenshot", "s.png") + require.NoError(t, err) + + _, err = part.Write([]byte("x")) + require.NoError(t, err) + require.NoError(t, writer.Close()) + + return &body, writer.FormDataContentType() +} + +// TestHandlerSubmitReport_AnonBudget: the first anonymous report with a +// screenshot answers 201, stores its file and dispatches one issue; past the +// budget the endpoint answers 429 RATE_LIMITED. +func TestHandlerSubmitReport_AnonBudget(t *testing.T) { + t.Parallel() + r := require.New(t) + ctx := t.Context() + + dbSvc, orgs := newBudgetSQLite(t, "acme-http") + svc, poster := newBudgetService(t, dbSvc, 1024, 1) + handler := NewHandler(svc, nil, svc.cfg) + + post := func() *httptest.ResponseRecorder { + body, contentType := multipartReport(t, orgs[0].Slug) + req := httptest.NewRequestWithContext(ctx, http.MethodPost, "/api/mgmt/report", body) + req.Header.Set("Content-Type", contentType) + + rec := httptest.NewRecorder() + r.NoError(handler.SubmitReport(rec, req)) + + return rec + } + + rec := post() + r.Equal(http.StatusCreated, rec.Code, rec.Body.String()) + r.Equal(int64(1), reportBytes(ctx, t, dbSvc, orgs[0])) + r.Eventually(func() bool { return poster.count() == 1 }, 5*time.Second, 10*time.Millisecond, + "the report dispatches one issue") + + rec = post() + r.Equal(http.StatusTooManyRequests, rec.Code) + r.Contains(rec.Body.String(), "RATE_LIMITED") + r.Equal(int64(1), reportBytes(ctx, t, dbSvc, orgs[0]), "the refused report stores nothing") +} diff --git a/server/internal/handlers/feedback/coverage_test.go b/server/internal/handlers/feedback/coverage_test.go new file mode 100644 index 000000000..2a1dad4d6 --- /dev/null +++ b/server/internal/handlers/feedback/coverage_test.go @@ -0,0 +1,129 @@ +package feedback + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/fclairamb/solidping/server/internal/config" + "github.com/fclairamb/solidping/server/internal/db" + "github.com/fclairamb/solidping/server/internal/db/models" + "github.com/fclairamb/solidping/server/internal/handlers/files" +) + +// TestSubmitReport_ResolveOrg covers the organization lookup: the named org, +// the first-org fallback, an unknown slug and an instance without any org. +func TestSubmitReport_ResolveOrg(t *testing.T) { + t.Parallel() + r := require.New(t) + ctx := t.Context() + + empty, _ := newBudgetSQLite(t) + svc, _ := newBudgetService(t, empty, 1024, 20) + + _, err := svc.SubmitReport(ctx, &SubmitReportRequest{URL: "https://example.com"}) + r.ErrorIs(err, ErrOrganizationNotFound, "no org at all") + + dbSvc, orgs := newBudgetSQLite(t, "acme-resolve") + svc, _ = newBudgetService(t, dbSvc, 1024, 20) + + resp, err := svc.SubmitReport(ctx, screenshotReport("", "")) + r.NoError(err, "an empty slug falls back to the first org") + r.NotEmpty(resp.UID) + r.Equal(int64(1), reportBytes(ctx, t, dbSvc, orgs[0])) + + _, err = svc.SubmitReport(ctx, &SubmitReportRequest{URL: "https://example.com", OrgSlug: "nope"}) + r.ErrorIs(err, ErrOrganizationNotFound, "unknown slug") +} + +// TestSubmitReport_AnonScreenshotWithoutGitHub: with no GitHub configured no +// issue slot exists, so an anonymous screenshot is refused and nothing is +// stored, while a member's report and a text-only one are still accepted. +func TestSubmitReport_AnonScreenshotWithoutGitHub(t *testing.T) { + t.Parallel() + r := require.New(t) + ctx := t.Context() + + dbSvc, orgs := newBudgetSQLite(t, "acme-nogh") + cfg := newFeedbackQuotaConfig(t, 1024) + cfg.App.GitHub = config.AppGitHubConfig{} + + svc := NewService(dbSvc, files.NewService(dbSvc, cfg), cfg, nil) + member := newMember(t, dbSvc) + + _, err := svc.SubmitReport(ctx, screenshotReport(orgs[0].Slug, "")) + r.ErrorIs(err, ErrReportRateLimited) + r.Zero(reportBytes(ctx, t, dbSvc, orgs[0])) + + _, err = svc.SubmitReport(ctx, screenshotReport(orgs[0].Slug, member)) + r.NoError(err) + + _, err = svc.SubmitReport(ctx, &SubmitReportRequest{URL: "https://example.com", OrgSlug: orgs[0].Slug}) + r.NoError(err) +} + +// TestSubmitReport_AnonBudgetDisabled: a budget of 0 lifts the anonymous cap. +func TestSubmitReport_AnonBudgetDisabled(t *testing.T) { + t.Parallel() + r := require.New(t) + ctx := t.Context() + + dbSvc, orgs := newBudgetSQLite(t, "acme-nobudget") + svc, _ := newBudgetService(t, dbSvc, 1024, 0) + + for range 3 { + _, err := svc.SubmitReport(ctx, screenshotReport(orgs[0].Slug, "")) + r.NoError(err) + } + + r.Equal(int64(3), reportBytes(ctx, t, dbSvc, orgs[0])) +} + +// assertListFilesByGroup checks the cross-org listing used by the retention +// sweep: group matching, the age cut-off, the limit and an empty group. +func assertListFilesByGroup(ctx context.Context, t *testing.T, dbSvc db.Service, orgA, orgB *models.Organization) { + t.Helper() + r := require.New(t) + + svc, _ := newBudgetService(t, dbSvc, 1024, 0) + + for _, org := range []*models.Organization{orgA, orgA, orgB} { + _, err := svc.SubmitReport(ctx, screenshotReport(org.Slug, "")) + r.NoError(err) + } + + future := time.Now().Add(time.Hour) + + all, err := dbSvc.ListFilesByGroup(ctx, "reports", future, 0) + r.NoError(err) + r.GreaterOrEqual(len(all), 3, "files of every org are listed, with no limit") + + limited, err := dbSvc.ListFilesByGroup(ctx, "reports", future, 2) + r.NoError(err) + r.Len(limited, 2) + + none, err := dbSvc.ListFilesByGroup(ctx, "reports", time.Now().Add(-24*time.Hour), 0) + r.NoError(err) + r.Empty(none, "nothing is older than a day") + + none, err = dbSvc.ListFilesByGroup(ctx, "", future, 0) + r.NoError(err) + r.Empty(none, "an empty group matches nothing") +} + +func TestListFilesByGroup_SQLite(t *testing.T) { + t.Parallel() + + dbSvc, orgs := newBudgetSQLite(t, "lfg-a", "lfg-b") + assertListFilesByGroup(t.Context(), t, dbSvc, orgs[0], orgs[1]) +} + +func TestListFilesByGroup_Postgres(t *testing.T) { + t.Parallel() + + dbSvc, orgA := newFeedbackPostgresOrg(t, "fb-lfg-pg-a") + _, orgB := newFeedbackPostgresOrg(t, "fb-lfg-pg-b") + assertListFilesByGroup(t.Context(), t, dbSvc, orgA, orgB) +} diff --git a/server/internal/handlers/feedback/dispatch_cap_test.go b/server/internal/handlers/feedback/dispatch_cap_test.go index b2af21396..435ff4b8a 100644 --- a/server/internal/handlers/feedback/dispatch_cap_test.go +++ b/server/internal/handlers/feedback/dispatch_cap_test.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "log/slog" + "sync" "testing" "time" @@ -16,23 +17,45 @@ import ( // countingGitHubPoster counts CreateIssue calls; it never talks to a real // GitHub API. Used to prove the per-hour dispatch cap actually stops calls // from reaching the poster, not just that a counter increments somewhere. +// Safe for the goroutines SubmitReport starts. type countingGitHubPoster struct { + mu sync.Mutex calls int } func (p *countingGitHubPoster) CreateIssue( _ context.Context, _, _, _, _ string, _ []string, ) error { + p.mu.Lock() + defer p.mu.Unlock() + p.calls++ return nil } +// count returns the number of CreateIssue calls so far. +func (p *countingGitHubPoster) count() int { + p.mu.Lock() + defer p.mu.Unlock() + + return p.calls +} + +// dispatchIfReserved mirrors SubmitReport's dispatch step synchronously: take +// a slot, and only then post the issue. +func dispatchIfReserved(svc *Service, req *SubmitReportRequest, fileUID string) { + ctx := context.Background() + if svc.reserveIssueDispatch(ctx) { + svc.dispatchGitHubIssue(ctx, req, "acme", fileUID, "", "") + } +} + // TestDispatchGitHubIssue_CapsAt20PerHour is the spec 2026-09-25-26 Tests // bullet: "21st dispatch within an hour is skipped with a WARN log (fake -// GitHub client)". dispatchGitHubIssue is called directly (rather than -// through SubmitReport, which fires it off in a goroutine) so the 21 calls -// are synchronous and deterministic. +// GitHub client)". The reserve-then-dispatch step is called directly (rather +// than through SubmitReport, which fires the dispatch off in a goroutine) so +// the 21 calls are synchronous and deterministic. func TestDispatchGitHubIssue_CapsAt20PerHour(t *testing.T) { t.Parallel() r := require.New(t) @@ -60,10 +83,10 @@ func TestDispatchGitHubIssue_CapsAt20PerHour(t *testing.T) { for i := range maxIssueDispatchesPerHour + 1 { // fileURI left empty: signedScreenshotURL short-circuits on it, so // this never needs a JWT secret to run. - svc.dispatchGitHubIssue(context.Background(), req, "acme", fmt.Sprintf("file-%d", i), "", "") + dispatchIfReserved(svc, req, fmt.Sprintf("file-%d", i)) } - r.Equal(maxIssueDispatchesPerHour, poster.calls, + r.Equal(maxIssueDispatchesPerHour, poster.count(), "the 21st dispatch within the hour must not reach GitHub") r.Contains(logBuf.String(), "bug_report: github issue dispatch capped", "the capped dispatch must log a WARN naming the cap") @@ -71,8 +94,8 @@ func TestDispatchGitHubIssue_CapsAt20PerHour(t *testing.T) { // The window resets after an hour: the next dispatch after it must go // through again rather than staying capped forever. svc.clock = func() time.Time { return time.Now().Add(time.Hour + time.Minute) } - svc.dispatchGitHubIssue(context.Background(), req, "acme", "file-after-window", "", "") - r.Equal(maxIssueDispatchesPerHour+1, poster.calls, + dispatchIfReserved(svc, req, "file-after-window") + r.Equal(maxIssueDispatchesPerHour+1, poster.count(), "a dispatch in a fresh hourly window must not stay capped") } diff --git a/server/internal/handlers/feedback/handler.go b/server/internal/handlers/feedback/handler.go index b6bb0ece7..6a6ff1cc6 100644 --- a/server/internal/handlers/feedback/handler.go +++ b/server/internal/handlers/feedback/handler.go @@ -108,6 +108,9 @@ func (h *Handler) SubmitReport(writer http.ResponseWriter, req *http.Request) er case errors.Is(err, ErrStorageQuotaExceeded): return h.WriteErrorErr(writer, req, http.StatusRequestEntityTooLarge, base.ErrorCodeQuotaExceeded, "Feedback storage quota exceeded for this organization", err) + case errors.Is(err, ErrReportRateLimited): + return h.WriteErrorErr(writer, req, http.StatusTooManyRequests, + base.ErrorCodeRateLimited, "Too many bug reports with a screenshot, try again later", err) default: return h.WriteInternalError(writer, req, err) } diff --git a/server/internal/handlers/feedback/quota_postgres_test.go b/server/internal/handlers/feedback/quota_postgres_test.go index 5eb2e1360..1acd8d49c 100644 --- a/server/internal/handlers/feedback/quota_postgres_test.go +++ b/server/internal/handlers/feedback/quota_postgres_test.go @@ -96,3 +96,23 @@ func TestSubmitReport_StorageQuota_Postgres(t *testing.T) { dbSvc, org := newFeedbackPostgresOrg(t, "fb-quota-pg") assertStorageQuotaEnforced(t.Context(), t, dbSvc, org.UID, org.Slug) } + +// TestSubmitReport_AnonBudgetIsInstanceWide_Postgres runs the instance-wide +// anonymous budget regression (spec 2026-10-29-01) on Postgres, whose global +// counter is a different implementation. +func TestSubmitReport_AnonBudgetIsInstanceWide_Postgres(t *testing.T) { + t.Parallel() + + dbSvc, orgA := newFeedbackPostgresOrg(t, "fb-anon-pg-a") + _, orgB := newFeedbackPostgresOrg(t, "fb-anon-pg-b") + assertAnonBudgetIsInstanceWide(t, dbSvc, orgA, orgB) +} + +// TestSubmitReport_ParallelQuota_Postgres is the Postgres half of the +// parallel per-org quota regression. +func TestSubmitReport_ParallelQuota_Postgres(t *testing.T) { + t.Parallel() + + dbSvc, org := newFeedbackPostgresOrg(t, "fb-par-pg") + assertParallelQuotaHoldsOneFile(t, dbSvc, org) +} diff --git a/server/internal/handlers/feedback/quota_test.go b/server/internal/handlers/feedback/quota_test.go index 6f17175b2..66f65c181 100644 --- a/server/internal/handlers/feedback/quota_test.go +++ b/server/internal/handlers/feedback/quota_test.go @@ -35,6 +35,10 @@ func newFeedbackQuotaConfig(t *testing.T, quotaBytes int64) *config.Config { cfg := &config.Config{} cfg.App.EnableBugReport = true cfg.App.FeedbackMaxStorageBytes = quotaBytes + // GitHub is wired (to a fake poster, see newTestService): an anonymous + // screenshot without an issue dispatch slot is refused (spec 2026-10-29-01). + cfg.App.GitHub = config.AppGitHubConfig{IssuesToken: "test-token", Repo: "acme/acme-repo"} + cfg.Auth.JWTSecret = "test-secret" cfg.FileStorage.Type = "local" cfg.FileStorage.LocalRoot = t.TempDir() @@ -64,7 +68,7 @@ func assertStorageQuotaEnforced(ctx context.Context, t *testing.T, dbSvc db.Serv r.NoError(dbSvc.CreateFile(ctx, seeded)) filesSvc := files.NewService(dbSvc, cfg) - svc := NewService(dbSvc, filesSvc, cfg, nil) + svc := NewService(dbSvc, filesSvc, cfg, &countingGitHubPoster{}) _, err := svc.SubmitReport(ctx, &SubmitReportRequest{ URL: "https://example.com/page", @@ -85,7 +89,7 @@ func assertStorageQuotaEnforced(ctx context.Context, t *testing.T, dbSvc db.Serv // broken store. roomy := *cfg roomy.App.FeedbackMaxStorageBytes = quota + 1024 - svcRoomy := NewService(dbSvc, filesSvc, &roomy, nil) + svcRoomy := NewService(dbSvc, filesSvc, &roomy, &countingGitHubPoster{}) resp, err := svcRoomy.SubmitReport(ctx, &SubmitReportRequest{ URL: "https://example.com/page", @@ -147,7 +151,7 @@ func TestSubmitReport_DisabledQuota_DoesNotBlock(t *testing.T) { r.NoError(dbSvc.CreateFile(ctx, seeded)) filesSvc := files.NewService(dbSvc, cfg) - svc := NewService(dbSvc, filesSvc, cfg, nil) + svc := NewService(dbSvc, filesSvc, cfg, &countingGitHubPoster{}) resp, err := svc.SubmitReport(ctx, &SubmitReportRequest{ URL: "https://example.com/page", diff --git a/server/internal/handlers/feedback/service.go b/server/internal/handlers/feedback/service.go index e52598d09..029b31cbe 100644 --- a/server/internal/handlers/feedback/service.go +++ b/server/internal/handlers/feedback/service.go @@ -17,6 +17,7 @@ import ( "github.com/fclairamb/solidping/server/internal/config" "github.com/fclairamb/solidping/server/internal/db" + "github.com/fclairamb/solidping/server/internal/db/models" "github.com/fclairamb/solidping/server/internal/handlers/files" "github.com/fclairamb/solidping/server/internal/handlers/files/signedurl" "github.com/fclairamb/solidping/server/internal/handlers/filestorage" @@ -38,8 +39,21 @@ var ( // auth and no other cost signal, so this is what bounds how much // local/S3 storage a single org's inbox can be made to fill. ErrStorageQuotaExceeded = errors.New("feedback storage quota exceeded") + // ErrReportRateLimited is returned when an anonymous report carries a + // screenshot and either the instance-wide anonymous screenshot budget + // (config.AppConfig.FeedbackAnonScreenshotsPerHour) is spent, or no GitHub + // issue dispatch slot is left this hour, so the screenshot would never be + // filed (spec 2026-10-29-01). Nothing is stored. + ErrReportRateLimited = errors.New("bug report rate limited") ) +// anonScreenshotCounterKey is the db.BumpGlobalCounter key of the +// instance-wide anonymous screenshot budget. +const anonScreenshotCounterKey = "feedback_report_anon" + +// anonScreenshotWindow is the window of the anonymous screenshot budget. +const anonScreenshotWindow = time.Hour + // maxIssueDispatchesPerHour caps GitHub issue creation across ALL orgs on this // process (spec 2026-09-25-26). The route-level rate limit and the per-org // storage quota bound how many reports can be filed; this additionally bounds @@ -97,6 +111,16 @@ type Service struct { dispatchMu sync.Mutex dispatchWindowStart time.Time dispatchCount int + + // quotaMu serializes the per-org quota check with the file insert, so + // parallel reports in this process cannot all pass the check and then all + // write (spec 2026-10-29-01). Across processes, storeScreenshot re-checks + // the quota after the insert and purges the file it just wrote. + quotaMu sync.Mutex + + // anonCounterKey is the global-counter key of the anonymous screenshot + // budget; tests sharing one database override it. + anonCounterKey string } // NewService constructs a feedback service. github may be nil — when so, a @@ -115,6 +139,8 @@ func NewService( github: github, logger: slog.Default(), clock: time.Now, + + anonCounterKey: anonScreenshotCounterKey, } } @@ -132,17 +158,60 @@ func (s *Service) SubmitReport( return nil, ErrURLRequired } - orgSlug := req.OrgSlug + org, orgUUID, err := s.resolveOrg(ctx, req.OrgSlug) + if err != nil { + return nil, err + } + + // Reserve the GitHub issue slot before anything is written: a screenshot + // stored past the dispatch cap is never referenced (spec 2026-10-29-01). + dispatch := s.reserveIssueDispatch(ctx) + + var fileUID, fileURI, fileMIME string + + if req.Screenshot != nil && req.ScreenshotSize > 0 { + file, err := s.admitAndStoreScreenshot(ctx, org.UID, orgUUID, req, dispatch) + if err != nil { + if dispatch { + s.releaseIssueDispatch() + } + + return nil, err + } + + fileUID = file.UID + fileURI = file.FileURI + fileMIME = file.MimeType + } + + if fileUID == "" { + // No screenshot: still return a stable UID so the frontend can show + // "report received". Generate ad-hoc; never persisted. + fileUID = uuid.NewString() + } + + if dispatch { + // Detached context: the HTTP request is already done. We log the + // goroutine err separately rather than propagating to the caller. + go s.dispatchGitHubIssue(context.WithoutCancel(ctx), req, org.Slug, fileUID, fileURI, fileMIME) + } + + return &SubmitReportResponse{UID: fileUID}, nil +} + +// resolveOrg finds the report's organization: the named one, or the first +// one when none is named. +func (s *Service) resolveOrg(ctx context.Context, orgSlug string) (*models.Organization, uuid.UUID, error) { if orgSlug == "" { // Fallback to the first non-deleted org. Used by status0 (subscriber-side) // where the user has no org context. orgs, err := s.db.ListOrganizations(ctx) if err != nil { - return nil, fmt.Errorf("list organizations: %w", err) + return nil, uuid.Nil, fmt.Errorf("list organizations: %w", err) } if len(orgs) == 0 { - return nil, ErrOrganizationNotFound + return nil, uuid.Nil, ErrOrganizationNotFound } orgSlug = orgs[0].Slug @@ -150,52 +219,106 @@ func (s *Service) SubmitReport( org, err := s.db.GetOrganizationBySlug(ctx, orgSlug) if err != nil { - return nil, ErrOrganizationNotFound + return nil, uuid.Nil, ErrOrganizationNotFound } orgUUID, err := uuid.Parse(org.UID) if err != nil { - return nil, fmt.Errorf("parse org uid: %w", err) + return nil, uuid.Nil, fmt.Errorf("parse org uid: %w", err) } - var fileUID, fileURI, fileMIME string + return org, orgUUID, nil +} - if req.Screenshot != nil && req.ScreenshotSize > 0 { - if err := s.checkStorageQuota(ctx, org.UID, req.ScreenshotSize); err != nil { +// admitAndStoreScreenshot stores the report's screenshot. An anonymous one +// must first be admitted by admitAnonymousScreenshot. +func (s *Service) admitAndStoreScreenshot( + ctx context.Context, orgUID string, orgUUID uuid.UUID, req *SubmitReportRequest, dispatch bool, +) (*models.File, error) { + if req.UserUID == "" { + if err := s.admitAnonymousScreenshot(ctx, dispatch); err != nil { return nil, err } + } - var createdBy *string - if req.UserUID != "" { - uid := req.UserUID - createdBy = &uid - } + return s.storeScreenshot(ctx, orgUID, orgUUID, req) +} + +// admitAnonymousScreenshot decides whether an anonymous report may store its +// screenshot (spec 2026-10-29-01). The caller of the anonymous endpoint picks +// the org, so the per-org quota alone does not bound what it writes: the +// screenshot must also fit the instance-wide hourly budget, and have an issue +// dispatch slot, or it would be stored and never filed. +func (s *Service) admitAnonymousScreenshot(ctx context.Context, dispatch bool) error { + if !dispatch { + return ErrReportRateLimited + } - file, ferr := s.files.CreateFile( - ctx, orgUUID, filestorage.GroupTypeReports, - req.ScreenshotName, req.ScreenshotMIME, createdBy, - req.Screenshot, req.ScreenshotSize, + limit := s.cfg.App.FeedbackAnonScreenshotsPerHour + if limit <= 0 { + return nil + } + + key := s.anonCounterKey + if key == "" { + key = anonScreenshotCounterKey + } + + limited, err := s.db.BumpGlobalCounter(ctx, key, limit, anonScreenshotWindow) + if err != nil { + return fmt.Errorf("spend anonymous screenshot budget: %w", err) + } + + if limited { + s.logger.WarnContext(ctx, "bug_report: anonymous screenshot budget spent", + "limit_per_hour", limit, ) - if ferr != nil { - return nil, fmt.Errorf("create file: %w", ferr) - } - fileUID = file.UID - fileURI = file.FileURI - fileMIME = file.MimeType + return ErrReportRateLimited } - if fileUID == "" { - // No screenshot: still return a stable UID so the frontend can show - // "report received". Generate ad-hoc; never persisted. - fileUID = uuid.NewString() + return nil +} + +// storeScreenshot writes the report's screenshot under the org's per-org +// quota. The check and the insert run under quotaMu, so parallel reports in +// this process cannot overshoot the quota; a re-check after the insert catches +// a race with another process, and purges the file just written. +func (s *Service) storeScreenshot( + ctx context.Context, orgUID string, orgUUID uuid.UUID, req *SubmitReportRequest, +) (*models.File, error) { + s.quotaMu.Lock() + defer s.quotaMu.Unlock() + + if err := s.checkStorageQuota(ctx, orgUID, req.ScreenshotSize); err != nil { + return nil, err + } + + var createdBy *string + if req.UserUID != "" { + uid := req.UserUID + createdBy = &uid } - // Detached context: the HTTP request is already done. We log the - // goroutine err separately rather than propagating to the caller. - go s.dispatchGitHubIssue(context.WithoutCancel(ctx), req, org.Slug, fileUID, fileURI, fileMIME) + file, err := s.files.CreateFile( + ctx, orgUUID, filestorage.GroupTypeReports, + req.ScreenshotName, req.ScreenshotMIME, createdBy, + req.Screenshot, req.ScreenshotSize, + ) + if err != nil { + return nil, fmt.Errorf("create file: %w", err) + } - return &SubmitReportResponse{UID: fileUID}, nil + if err := s.checkStorageQuota(ctx, orgUID, 0); err != nil { + if purgeErr := s.files.PurgeFile(ctx, file); purgeErr != nil { + s.logger.WarnContext(ctx, "bug_report: purge of over-quota screenshot failed", + "file_uid", file.UID, "err", purgeErr.Error()) + } + + return nil, err + } + + return file, nil } // checkStorageQuota returns ErrStorageQuotaExceeded when persisting `incoming` @@ -230,6 +353,39 @@ func (s *Service) checkStorageQuota(ctx context.Context, orgUID string, incoming return nil } +// reserveIssueDispatch takes one GitHub issue dispatch slot for a report, and +// reports whether it got one. It is false when GitHub is not configured or the +// hourly cap is spent; the report is then accepted without an issue. +func (s *Service) reserveIssueDispatch(ctx context.Context) bool { + github := s.cfg.App.GitHub + if github.IssuesToken == "" || github.Repo == "" { + s.logger.WarnContext(ctx, "bug_report: github not configured") + + return false + } + + if !s.allowIssueDispatch() { + s.logger.WarnContext(ctx, "bug_report: github issue dispatch capped", + "limit_per_hour", maxIssueDispatchesPerHour, + ) + + return false + } + + return true +} + +// releaseIssueDispatch gives back a slot taken by reserveIssueDispatch for a +// report that was refused before its issue was dispatched. +func (s *Service) releaseIssueDispatch() { + s.dispatchMu.Lock() + defer s.dispatchMu.Unlock() + + if s.dispatchCount > 0 { + s.dispatchCount-- + } +} + // allowIssueDispatch enforces maxIssueDispatchesPerHour across all orgs on // this process, using a fixed hourly window measured from the first dispatch // in it (not a rolling one) — simple and sufficient for smoothing a burst, @@ -254,7 +410,8 @@ func (s *Service) allowIssueDispatch() bool { } // dispatchGitHubIssue is the async side. Logs failures but never propagates -// to the caller — the report is already saved. +// to the caller — the report is already saved. The caller has already taken +// the dispatch slot (reserveIssueDispatch). func (s *Service) dispatchGitHubIssue( parent context.Context, req *SubmitReportRequest, orgSlug, fileUID, fileURI, fileMIME string, ) { @@ -262,21 +419,6 @@ func (s *Service) dispatchGitHubIssue( defer cancel() github := s.cfg.App.GitHub - if github.IssuesToken == "" || github.Repo == "" { - s.logger.WarnContext(ctx, "bug_report: github not configured") - - return - } - - if !s.allowIssueDispatch() { - s.logger.WarnContext(ctx, "bug_report: github issue dispatch capped", - "file_uid", fileUID, - "limit_per_hour", maxIssueDispatchesPerHour, - ) - - return - } - signedURL, exp := s.signedScreenshotURL(req.URL, fileUID, fileURI) input := &IssueInput{ diff --git a/server/internal/handlers/feedback/service_test.go b/server/internal/handlers/feedback/service_test.go index a7c2bc179..3e26488e3 100644 --- a/server/internal/handlers/feedback/service_test.go +++ b/server/internal/handlers/feedback/service_test.go @@ -106,7 +106,7 @@ func TestSubmitReport_DisabledStoresNothing(t *testing.T) { ScreenshotSize: 1, }) require.ErrorIs(t, err, ErrReportDisabled) - require.Zero(t, poster.calls) + require.Zero(t, poster.count()) } // TestHandlerSubmitReport_DisabledIs404 pins the endpoint contract: 404, not diff --git a/server/internal/jobs/jobtypes/job_state_cleanup.go b/server/internal/jobs/jobtypes/job_state_cleanup.go index e335f29bc..73aded390 100644 --- a/server/internal/jobs/jobtypes/job_state_cleanup.go +++ b/server/internal/jobs/jobtypes/job_state_cleanup.go @@ -7,6 +7,8 @@ import ( "github.com/fclairamb/solidping/server/internal/db/models" "github.com/fclairamb/solidping/server/internal/handlers/attachments" + "github.com/fclairamb/solidping/server/internal/handlers/files" + "github.com/fclairamb/solidping/server/internal/handlers/filestorage" "github.com/fclairamb/solidping/server/internal/jobs/jobdef" "github.com/fclairamb/solidping/server/internal/jobs/jobsvc" ) @@ -65,6 +67,7 @@ func (r *StateCleanupJobRun) Run(ctx context.Context, jctx *jobdef.JobContext) e sweepExpiredUnconfirmedSubscribers(ctx, jctx) sweepOrphanIncidentAttachments(ctx, jctx) sweepOrphanCheckAttachments(ctx, jctx) + sweepExpiredReportFiles(ctx, jctx) // Schedule next run in 2 hours // Skip if services are not available (e.g., in tests without full service setup) @@ -199,3 +202,47 @@ func sweepOrphanAttachments(ctx context.Context, jctx *jobdef.JobContext, entity func sweepOrphanCheckAttachments(ctx context.Context, jctx *jobdef.JobContext) { sweepOrphanAttachments(ctx, jctx, attachments.EntityChecks) } + +// reportSweepBatch bounds one pass of the report-retention sweep. +const reportSweepBatch = 500 + +// sweepExpiredReportFiles purges bug-report screenshots +// (filestorage.GroupTypeReports) older than app.feedback_retention_days, row +// and blob (spec 2026-10-29-01). Anyone can upload one through the anonymous +// report endpoint, and the GitHub issue only links it for a while, so without +// this the store only grows. A retention of 0 or less keeps them forever. +// Best-effort, like the other sweeps. +func sweepExpiredReportFiles(ctx context.Context, jctx *jobdef.JobContext) { + if jctx.AppConfig == nil || jctx.AppConfig.App.FeedbackRetentionDays <= 0 { + return + } + + log := jctx.Logger + retention := time.Duration(jctx.AppConfig.App.FeedbackRetentionDays) * 24 * time.Hour + + rows, err := jctx.DBService.ListFilesByGroup( + ctx, string(filestorage.GroupTypeReports), time.Now().Add(-retention), reportSweepBatch, + ) + if err != nil { + log.WarnContext(ctx, "Failed to list expired report files", "error", err) + + return + } + + filesSvc := files.NewService(jctx.DBService, jctx.AppConfig) + purged := 0 + + for _, row := range rows { + if purgeErr := filesSvc.PurgeFile(ctx, row); purgeErr != nil { + log.WarnContext(ctx, "Failed to purge expired report file", "fileUid", row.UID, "error", purgeErr) + + continue + } + + purged++ + } + + if purged > 0 { + log.InfoContext(ctx, "Purged expired report files", "count", purged) + } +} diff --git a/server/internal/jobs/jobtypes/job_state_cleanup_reports_test.go b/server/internal/jobs/jobtypes/job_state_cleanup_reports_test.go new file mode 100644 index 000000000..55df01a00 --- /dev/null +++ b/server/internal/jobs/jobtypes/job_state_cleanup_reports_test.go @@ -0,0 +1,123 @@ +package jobtypes + +import ( + "log/slog" + "strings" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "github.com/fclairamb/solidping/server/internal/config" + "github.com/fclairamb/solidping/server/internal/db/models" + "github.com/fclairamb/solidping/server/internal/db/sqlite" + "github.com/fclairamb/solidping/server/internal/handlers/files" + "github.com/fclairamb/solidping/server/internal/handlers/filestorage" + "github.com/fclairamb/solidping/server/internal/handlers/filestorage/localfs" + "github.com/fclairamb/solidping/server/internal/jobs/jobdef" +) + +// TestSweepExpiredReportFiles is the spec 2026-10-29-01 retention test: a +// `reports` file older than the retention is purged with its blob, a younger +// one is kept, and an equally old file of another group is not touched. +func TestSweepExpiredReportFiles(t *testing.T) { + t.Parallel() + + localfs.Register() + + r := require.New(t) + ctx := t.Context() + + dbSvc, err := sqlite.New(ctx, sqlite.Config{InMemory: true}) + r.NoError(err) + r.NoError(dbSvc.Initialize(ctx)) + t.Cleanup(func() { _ = dbSvc.Close() }) + + org := models.NewOrganization("acme-retention", "Acme") + r.NoError(dbSvc.CreateOrganization(ctx, org)) + + cfg := &config.Config{} + cfg.App.FeedbackRetentionDays = 90 + cfg.FileStorage.Type = "local" + cfg.FileStorage.LocalRoot = t.TempDir() + + filesSvc := files.NewService(dbSvc, cfg) + orgUUID := uuid.MustParse(org.UID) + + create := func(group filestorage.GroupType, name string, age time.Duration) *models.File { + file, createErr := filesSvc.CreateFile(ctx, orgUUID, group, name, "image/png", nil, + strings.NewReader("png"), 3) + r.NoError(createErr) + + _, updErr := dbSvc.DB().NewUpdate().Model((*models.File)(nil)). + Set("created_at = ?", time.Now().Add(-age)). + Where("uid = ?", file.UID). + Exec(ctx) + r.NoError(updErr) + + return file + } + + old := create(filestorage.GroupTypeReports, "old.png", 91*24*time.Hour) + young := create(filestorage.GroupTypeReports, "young.png", 89*24*time.Hour) + otherGroup := create(filestorage.GroupTypeScreenshots, "other.png", 200*24*time.Hour) + + jctx := &jobdef.JobContext{DBService: dbSvc, AppConfig: cfg, Logger: slog.Default()} + sweepExpiredReportFiles(ctx, jctx) + + _, err = dbSvc.GetFile(ctx, org.UID, old.UID) + r.Error(err, "a reports file past the retention must be deleted") + + _, err = filesSvc.OpenContent(ctx, old) + r.Error(err, "its blob must be removed from storage too") + + _, err = dbSvc.GetFile(ctx, org.UID, young.UID) + r.NoError(err, "a reports file younger than the retention must be kept") + + body, err := filesSvc.OpenContent(ctx, young) + r.NoError(err, "the kept file's blob must still be there") + r.NoError(body.Close()) + + _, err = dbSvc.GetFile(ctx, org.UID, otherGroup.UID) + r.NoError(err, "files of other groups are not under the report retention") + + // Retention 0 keeps report files forever. + cfg.App.FeedbackRetentionDays = 0 + ancient := create(filestorage.GroupTypeReports, "ancient.png", 1000*24*time.Hour) + sweepExpiredReportFiles(ctx, jctx) + + _, err = dbSvc.GetFile(ctx, org.UID, ancient.UID) + r.NoError(err, "a retention of 0 must keep report files") +} + +// TestSweepExpiredReportFiles_NoConfig: without an app config the sweep is a +// no-op rather than a nil dereference. +func TestSweepExpiredReportFiles_NoConfig(t *testing.T) { + t.Parallel() + + require.NotPanics(t, func() { + sweepExpiredReportFiles(t.Context(), &jobdef.JobContext{Logger: slog.Default()}) + }) +} + +// TestSweepExpiredReportFiles_ListError: a failing listing is logged and the +// sweep returns without purging anything. +func TestSweepExpiredReportFiles_ListError(t *testing.T) { + t.Parallel() + r := require.New(t) + + dbSvc, err := sqlite.New(t.Context(), sqlite.Config{InMemory: true}) + r.NoError(err) + r.NoError(dbSvc.Initialize(t.Context())) + r.NoError(dbSvc.Close()) + + cfg := &config.Config{} + cfg.App.FeedbackRetentionDays = 90 + + require.NotPanics(t, func() { + sweepExpiredReportFiles(t.Context(), &jobdef.JobContext{ + DBService: dbSvc, AppConfig: cfg, Logger: slog.Default(), + }) + }) +} diff --git a/server/internal/notifications/slack_test.go b/server/internal/notifications/slack_test.go index c1ca5da90..7457a5988 100644 --- a/server/internal/notifications/slack_test.go +++ b/server/internal/notifications/slack_test.go @@ -1682,6 +1682,12 @@ func (m *mockDBService) ListAttachmentsByTopicPrefix( return nil, nil } +func (m *mockDBService) ListFilesByGroup( + _ context.Context, _ string, _ time.Time, _ int, +) ([]*models.File, error) { + return nil, nil +} + func (m *mockDBService) SumFileSizeByGroup(_ context.Context, _, _ string) (int64, error) { return 0, nil } diff --git a/server/test/integration/files_public_feedback_test.go b/server/test/integration/files_public_feedback_test.go index 8f5e4aaf7..7834aa3f3 100644 --- a/server/test/integration/files_public_feedback_test.go +++ b/server/test/integration/files_public_feedback_test.go @@ -128,7 +128,13 @@ func TestSubmitReport_Endpoint(t *testing.T) { require.Equal(t, http.StatusBadRequest, postReport(t, ts, map[string]string{"comment": "x"}, nil)) require.Equal(t, http.StatusNotFound, postReport(t, ts, map[string]string{"url": "https://acme.com", "org": "nope"}, nil)) - require.Equal(t, http.StatusCreated, postReport(t, ts, map[string]string{ + // Anonymous screenshots need an issue dispatch slot (spec 2026-10-29-01): + // with no GitHub configured, the screenshot is refused with 429. + require.Equal(t, http.StatusTooManyRequests, postReport(t, ts, map[string]string{ "url": "https://acme.com", "org": TestOrgSlug, "comment": "broken", "context": `{"a":1}`, }, []byte("png-bytes"))) + // Without a screenshot the report is still accepted. + require.Equal(t, http.StatusCreated, postReport(t, ts, map[string]string{ + "url": "https://acme.com", "org": TestOrgSlug, "comment": "broken", "context": `{"a":1}`, + }, nil)) } diff --git a/specs/done/2026/10/2026-10-29-01-bug-report-upload-instance-budget.md b/specs/done/2026/10/2026-10-29-01-bug-report-upload-instance-budget.md new file mode 100644 index 000000000..91eac1e09 --- /dev/null +++ b/specs/done/2026/10/2026-10-29-01-bug-report-upload-instance-budget.md @@ -0,0 +1,64 @@ +--- +model: opus +effort: medium +--- + +# Anonymous bug-report screenshots are capped per org, not per instance + +## Problem + +Found by the security scan of 2026-10-29 (finding Y1). + +**Severity: low.** Anyone can trigger it, with no account, on an instance where bug reports are enabled (`app.github.issues_token` and `app.github.repo` set, `server/internal/config/config.go:3610-3612`). Since #701 the login page links to the same dialog, so the endpoint is now advertised to anonymous visitors. + +`POST /api/mgmt/report` is anonymous (`server/internal/app/server.go:2510-2518`). The only limits on what it stores are: + +- the 10 MB body cap (`server/internal/handlers/feedback/handler.go:19,53`); +- a per-IP limiter of 5 requests per minute, burst 10 (`server/internal/app/server.go:655,663-669`); +- a per-org quota of 100 MB of `reports` files (`server/internal/config/config.go:1249-1251`, checked in `server/internal/handlers/feedback/service.go:208-231`). + +The quota does not bound what the instance stores: + +1. **The caller picks the org.** The `org` form field names any org on the instance (`feedback/service.go:135-153`), so the 100 MB cap applies once per org. On SaaS, the total an anonymous caller can write is 100 MB times the number of orgs. Anyone can also add orgs by signing up. +2. **The screenshot is stored before, and whether or not, the GitHub issue is filed.** `CreateFile` runs in the request (`feedback/service.go:163-186`). The issue dispatch, which is the only consumer of the file, is capped at 20 per hour per process (`feedback/service.go:49`, `:271`). Every screenshot past that cap is written and never referenced. +3. **The quota check is not atomic.** `checkStorageQuota` sums the group and compares, then `CreateFile` writes (`feedback/service.go:167-176` and `:208-231`). Parallel requests from several IPs all pass the check and overshoot the cap by about 10 MB each. +4. **Nothing deletes report files.** No job touches `filestorage.GroupTypeReports` (`server/internal/handlers/filestorage/filestorage.go:27`). + +**Impact:** + +- One IP writes about 50 MB per minute (3 GB per hour) of arbitrary bytes into the file store. Several IPs multiply that. +- With the default `local` file backend (`config.go:1209`), the files share the server's data disk. Filling that disk can stop the database, and with it monitoring for every org. With S3 it is a storage cost. +- Each target org's report quota stays full for good, so its members' own bug reports with screenshots get 413 (`feedback/handler.go:104-106`). + +Spec 2026-09-25-26 added the per-org quota and the limiter as the storage protection. It assumed one org; this finding is the many-org case. + +## Proposal + +1. **Budget anonymous screenshots for the whole instance.** In `Service.SubmitReport` (`feedback/service.go:163`), when the report has no authenticated user (`req.UserUID == ""`), spend a global counter with `db.BumpGlobalCounter` before `CreateFile`. Use a key such as `feedback_report_anon` and a one-hour window, the primitive the status-page subscribe budget uses (`server/internal/handlers/statussubscribers/service.go`, `spendConfirmBudget`). Over budget, answer 429 `RATE_LIMITED` and store nothing. +2. **Do not store a screenshot that will never be filed.** Reserve the issue dispatch slot (`allowIssueDispatch`, `feedback/service.go:237`, today called from the goroutine started at `:196`) before `CreateFile`, not inside the goroutine. If no slot is left, refuse the screenshot as in step 1, before anything is written. +3. **Make the per-org quota check atomic** with the file insert, or spend the bytes through a counter, so parallel requests cannot overshoot it. +4. **Expire report files.** Add a retention to the state-cleanup job (`server/internal/jobs/jobtypes/job_state_cleanup.go`) that deletes `reports` files older than the retention, both the row and the stored blob. +5. Authenticated members keep today's behaviour, under the per-org quota only. +6. Update the comment at `server.go:2510-2515` to describe the real storage controls. + +## Tests + +- `server/internal/handlers/feedback` (SQLite, plus Postgres for the counter): + - **Regression (fails before the fix):** with the anonymous budget set to 3 and two orgs, send 6 anonymous reports with a screenshot, alternating the `org` field. The first 3 store their file, the next 3 get 429, and the `reports` group holds exactly 3 files across both orgs. + - **Regression:** with the dispatch budget spent, an anonymous report with a screenshot stores no file. + - **Regression:** N parallel anonymous reports against an org whose quota has room for one file store at most one file. + - **Positive controls:** + - a first anonymous report with a screenshot is stored, answers 201 and dispatches one issue; + - an authenticated member's report with a screenshot is still stored after the anonymous budget is spent; + - a report without a screenshot is accepted under the budget. +- `server/internal/jobs/jobtypes`: a `reports` file older than the retention is deleted with its blob, and a younger one is kept. + +## Open questions + +- Anonymous screenshot budget: **recommended** 20 per hour for the instance, the same as the issue dispatch cap, as a config value with that default. +- Report file retention: **recommended** 90 days, as a config value. + +## Resolved open questions + +- Anonymous screenshot budget: 20 per hour for the instance, same as the issue dispatch cap, as a config value with that default. (autopilot: recommended answer) +- Report file retention: 90 days, as a config value. (autopilot: recommended answer)