diff --git a/cmd/admin-api/main.go b/cmd/admin-api/main.go index 925b12c..11be2de 100644 --- a/cmd/admin-api/main.go +++ b/cmd/admin-api/main.go @@ -21,6 +21,7 @@ import ( ginSwagger "github.com/swaggo/gin-swagger" "github.com/sweetrpg/admin-api/constants" "github.com/sweetrpg/admin-api/docs" + "github.com/sweetrpg/admin-api/feedback" "github.com/sweetrpg/admin-api/models" "github.com/sweetrpg/admin-api/server" apiconstants "github.com/sweetrpg/api-core.go/constants" @@ -111,7 +112,8 @@ func main() { r.Use(ratelimit.Middleware(redisPool, ratelimit.DefaultOptions())) authzClient := authz.NewClient(util.GetEnv(constants.AUTH_API_URL, ""), util.GetEnv(constants.USERS_API_URL, "")) - server.SetupHandlers(r, authzClient) + issueCreator := feedback.NewGitHubIssueClient(util.GetEnv(constants.GITHUB_FEEDBACK_TOKEN, "")) + server.SetupHandlers(r, authzClient, issueCreator, feedback.RateLimitMiddleware(redisPool)) _ = r.Run(util.GetEnv(apiconstants.BIND_ADDRESS, ":8000")) } diff --git a/constants/constants.go b/constants/constants.go index 5335e46..a14bcaf 100644 --- a/constants/constants.go +++ b/constants/constants.go @@ -1,5 +1,7 @@ package constants +import "time" + // Environment variable names const ( HEALTH_TOKEN = "HEALTH_TOKEN" @@ -16,6 +18,16 @@ const ( // subject to its canonical users._id for write-path created_by/updated_by stamps. See // canonical-user-ids-across-services in sweetrpg/platform. USERS_API_URL = "USERS_API_URL" + + // GITHUB_FEEDBACK_TOKEN authenticates the feedback package's GitHub Issues API calls. + // Scoped to Issues:write on sweetrpg/platform only - see design.md. + GITHUB_FEEDBACK_TOKEN = "GITHUB_FEEDBACK_TOKEN" + + // RATE_LIMIT_FEEDBACK and RATE_LIMIT_FEEDBACK_WINDOW_SECONDS tune the feedback-specific + // rate-limit tier, stricter than api-core.go/ratelimit's standard tier. See + // feedback.RateLimitMiddleware. + RATE_LIMIT_FEEDBACK = "RATE_LIMIT_FEEDBACK" + RATE_LIMIT_FEEDBACK_WINDOW_SECONDS = "RATE_LIMIT_FEEDBACK_WINDOW_SECONDS" ) // Value constants @@ -42,4 +54,26 @@ const ( // AdminActionAuditLogCollection is the MongoDB collection name for write-route // audit records. AdminActionAuditLogCollection = "admin_action_audit_logs" + + // RateLimitFeedbackDefault and RateLimitFeedbackWindowDefault are the feedback tier's + // fallback budget when RATE_LIMIT_FEEDBACK[_WINDOW_SECONDS] aren't set: 5 submissions + // per 5 minutes per client, well under the platform-default standard tier (see + // design.md's "dedicated feedback-specific rate-limit tier" decision). + RateLimitFeedbackDefault = 5 + RateLimitFeedbackWindowDefault = 300 + + // FeedbackTitleMaxLength and FeedbackBodyMaxLength cap submitted text, rejected with a + // 400 when exceeded (see the anonymous-feedback-submission spec's "Oversized submission + // rejected" scenario). + FeedbackTitleMaxLength = 200 + FeedbackBodyMaxLength = 10000 + + // FeedbackMinFillTime is the minimum time between the form rendering (client-reported + // rendered_at) and submission for a human to plausibly have filled it out. Submissions + // faster than this are silently discarded - see design.md's honeypot/fill-time decision. + FeedbackMinFillTime = 3 * time.Second + + // FeedbackSourceLabel is applied to every issue created from a feedback submission, so a + // spam wave can be bulk-filtered/closed without affecting other issues. + FeedbackSourceLabel = "source:in-app-feedback" ) diff --git a/docs/docs.go b/docs/docs.go index 340e8ac..45e2af9 100644 --- a/docs/docs.go +++ b/docs/docs.go @@ -524,6 +524,64 @@ const docTemplate = `{ } } }, + "/feedback": { + "post": { + "description": "Accept an anonymous bug report or feature request and file it as a GitHub Issue on sweetrpg/platform, labeled source:in-app-feedback plus bug or feature. No GitHub credentials are ever requested from or exposed to the caller. Subject to a feedback-specific rate limit stricter than this service's default.", + "consumes": [ + "application/json" + ], + "produces": [ + "application/json" + ], + "tags": [ + "feedback" + ], + "summary": "Submit feedback", + "parameters": [ + { + "description": "Feedback submission", + "name": "body", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/server.createFeedbackRequest" + } + } + ], + "responses": { + "200": { + "description": "OK", + "schema": { + "$ref": "#/definitions/server.feedbackAcceptedResponse" + } + }, + "400": { + "description": "missing/invalid type, title, or body, or title/body exceeds the length cap", + "schema": { + "$ref": "#/definitions/vo.ErrorVO" + } + }, + "429": { + "description": "feedback-specific rate limit exceeded", + "schema": { + "$ref": "#/definitions/vo.ErrorVO" + } + }, + "502": { + "description": "GitHub API call failed or timed out", + "schema": { + "$ref": "#/definitions/vo.ErrorVO" + } + }, + "503": { + "description": "rate-limit backend unavailable", + "schema": { + "$ref": "#/definitions/vo.ErrorVO" + } + } + } + } + }, "/maintenance-modes": { "get": { "description": "Returns every maintenance-mode record (platform and every known service scope) regardless of enabled state, for admin-web's management view.", @@ -1031,6 +1089,51 @@ const docTemplate = `{ } } }, + "server.createFeedbackRequest": { + "type": "object", + "required": [ + "body", + "title", + "type" + ], + "properties": { + "body": { + "type": "string", + "example": "Searching for 9780786965601 returns zero results, even though the volume exists in the catalog." + }, + "rendered_at": { + "type": "string" + }, + "reporter_email": { + "type": "string", + "example": "reporter@example.com" + }, + "source": { + "type": "string", + "example": "catalog-web:/catalog/search" + }, + "title": { + "type": "string", + "example": "Catalog search returns no results for exact ISBN match" + }, + "type": { + "type": "string", + "example": "bug" + }, + "website": { + "type": "string" + } + } + }, + "server.feedbackAcceptedResponse": { + "type": "object", + "properties": { + "status": { + "type": "string", + "example": "accepted" + } + } + }, "server.maintenanceModeRequest": { "type": "object", "required": [ diff --git a/docs/swagger.json b/docs/swagger.json index 1a36e71..dbed88a 100644 --- a/docs/swagger.json +++ b/docs/swagger.json @@ -516,6 +516,64 @@ } } }, + "/feedback": { + "post": { + "description": "Accept an anonymous bug report or feature request and file it as a GitHub Issue on sweetrpg/platform, labeled source:in-app-feedback plus bug or feature. No GitHub credentials are ever requested from or exposed to the caller. Subject to a feedback-specific rate limit stricter than this service's default.", + "consumes": [ + "application/json" + ], + "produces": [ + "application/json" + ], + "tags": [ + "feedback" + ], + "summary": "Submit feedback", + "parameters": [ + { + "description": "Feedback submission", + "name": "body", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/server.createFeedbackRequest" + } + } + ], + "responses": { + "200": { + "description": "OK", + "schema": { + "$ref": "#/definitions/server.feedbackAcceptedResponse" + } + }, + "400": { + "description": "missing/invalid type, title, or body, or title/body exceeds the length cap", + "schema": { + "$ref": "#/definitions/vo.ErrorVO" + } + }, + "429": { + "description": "feedback-specific rate limit exceeded", + "schema": { + "$ref": "#/definitions/vo.ErrorVO" + } + }, + "502": { + "description": "GitHub API call failed or timed out", + "schema": { + "$ref": "#/definitions/vo.ErrorVO" + } + }, + "503": { + "description": "rate-limit backend unavailable", + "schema": { + "$ref": "#/definitions/vo.ErrorVO" + } + } + } + } + }, "/maintenance-modes": { "get": { "description": "Returns every maintenance-mode record (platform and every known service scope) regardless of enabled state, for admin-web's management view.", @@ -1023,6 +1081,51 @@ } } }, + "server.createFeedbackRequest": { + "type": "object", + "required": [ + "body", + "title", + "type" + ], + "properties": { + "body": { + "type": "string", + "example": "Searching for 9780786965601 returns zero results, even though the volume exists in the catalog." + }, + "rendered_at": { + "type": "string" + }, + "reporter_email": { + "type": "string", + "example": "reporter@example.com" + }, + "source": { + "type": "string", + "example": "catalog-web:/catalog/search" + }, + "title": { + "type": "string", + "example": "Catalog search returns no results for exact ISBN match" + }, + "type": { + "type": "string", + "example": "bug" + }, + "website": { + "type": "string" + } + } + }, + "server.feedbackAcceptedResponse": { + "type": "object", + "properties": { + "status": { + "type": "string", + "example": "accepted" + } + } + }, "server.maintenanceModeRequest": { "type": "object", "required": [ diff --git a/docs/swagger.yaml b/docs/swagger.yaml index 6116111..7038d9d 100644 --- a/docs/swagger.yaml +++ b/docs/swagger.yaml @@ -138,6 +138,39 @@ definitions: - scope_type - severity type: object + server.createFeedbackRequest: + properties: + body: + example: Searching for 9780786965601 returns zero results, even though the + volume exists in the catalog. + type: string + rendered_at: + type: string + reporter_email: + example: reporter@example.com + type: string + source: + example: catalog-web:/catalog/search + type: string + title: + example: Catalog search returns no results for exact ISBN match + type: string + type: + example: bug + type: string + website: + type: string + required: + - body + - title + - type + type: object + server.feedbackAcceptedResponse: + properties: + status: + example: accepted + type: string + type: object server.maintenanceModeRequest: properties: description: @@ -577,6 +610,49 @@ paths: summary: Update a banner message tags: - banners + /feedback: + post: + consumes: + - application/json + description: Accept an anonymous bug report or feature request and file it as + a GitHub Issue on sweetrpg/platform, labeled source:in-app-feedback plus bug + or feature. No GitHub credentials are ever requested from or exposed to the + caller. Subject to a feedback-specific rate limit stricter than this service's + default. + parameters: + - description: Feedback submission + in: body + name: body + required: true + schema: + $ref: '#/definitions/server.createFeedbackRequest' + produces: + - application/json + responses: + "200": + description: OK + schema: + $ref: '#/definitions/server.feedbackAcceptedResponse' + "400": + description: missing/invalid type, title, or body, or title/body exceeds + the length cap + schema: + $ref: '#/definitions/vo.ErrorVO' + "429": + description: feedback-specific rate limit exceeded + schema: + $ref: '#/definitions/vo.ErrorVO' + "502": + description: GitHub API call failed or timed out + schema: + $ref: '#/definitions/vo.ErrorVO' + "503": + description: rate-limit backend unavailable + schema: + $ref: '#/definitions/vo.ErrorVO' + summary: Submit feedback + tags: + - feedback /maintenance-modes: get: description: Returns every maintenance-mode record (platform and every known diff --git a/feedback/client.go b/feedback/client.go new file mode 100644 index 0000000..b290eb7 --- /dev/null +++ b/feedback/client.go @@ -0,0 +1,62 @@ +// Package feedback implements the anonymous-feedback-to-GitHub-Issue proxy: a single +// IssueCreator interface backed by the GitHub Issues API, kept separate from the HTTP handler +// in server/feedback.go so the handler can be unit-tested against a fake creator. +package feedback + +import ( + "context" + "fmt" + "net/http" + "time" + + "github.com/google/go-github/v76/github" +) + +// Owner and Repo identify the sweetrpg/platform meta-repo that every feedback submission +// becomes an issue on - see design.md's "no per-service repo routing" decision. +const ( + Owner = "sweetrpg" + Repo = "platform" + + // createIssueTimeout bounds the GitHub API call so a stalled connection fails fast and + // the caller gets a prompt 502 rather than hanging past their own timeout. + createIssueTimeout = 10 * time.Second +) + +// IssueCreator creates a GitHub Issue from a feedback submission. Implemented by +// GitHubIssueClient; tests substitute a fake. +type IssueCreator interface { + CreateIssue(ctx context.Context, title, body string, labels []string) error +} + +// GitHubIssueClient creates issues on sweetrpg/platform via the GitHub REST API using a +// service-owned token scoped to Issues:write on that one repo (see design.md). +type GitHubIssueClient struct { + client *github.Client +} + +// NewGitHubIssueClient builds a client authenticated with the given token. +func NewGitHubIssueClient(token string) *GitHubIssueClient { + return &GitHubIssueClient{client: github.NewClient(nil).WithAuthToken(token)} +} + +// CreateIssue creates a labeled issue on sweetrpg/platform. A non-nil error means the caller +// should treat the submission as failed (502/503) - this never retries internally, per +// design.md's "no partial state to reconcile" decision. +func (c *GitHubIssueClient) CreateIssue(ctx context.Context, title, body string, labels []string) error { + ctx, cancel := context.WithTimeout(ctx, createIssueTimeout) + defer cancel() + + _, resp, err := c.client.Issues.Create(ctx, Owner, Repo, &github.IssueRequest{ + Title: &title, + Body: &body, + Labels: &labels, + }) + if err != nil { + return fmt.Errorf("feedback: create github issue: %w", err) + } + if resp.StatusCode != http.StatusCreated { + return fmt.Errorf("feedback: create github issue: unexpected status %d", resp.StatusCode) + } + return nil +} diff --git a/feedback/client_test.go b/feedback/client_test.go new file mode 100644 index 0000000..2a59625 --- /dev/null +++ b/feedback/client_test.go @@ -0,0 +1,69 @@ +package feedback + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/google/go-github/v76/github" +) + +func newTestClient(t *testing.T, handler http.HandlerFunc) *GitHubIssueClient { + t.Helper() + server := httptest.NewServer(handler) + t.Cleanup(server.Close) + + gh := github.NewClient(nil).WithAuthToken("test-token") + base, err := url.Parse(server.URL + "/") + if err != nil { + t.Fatalf("parse test server URL: %v", err) + } + gh.BaseURL = base + + return &GitHubIssueClient{client: gh} +} + +func TestCreateIssue_Success(t *testing.T) { + var gotPath string + var gotBody github.IssueRequest + + client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + if err := json.NewDecoder(r.Body).Decode(&gotBody); err != nil { + t.Fatalf("decode request body: %v", err) + } + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(github.Issue{Number: github.Ptr(1)}) + }) + + err := client.CreateIssue(context.Background(), "Bug: thing broke", "Steps to reproduce...", []string{"source:in-app-feedback", "bug"}) + if err != nil { + t.Fatalf("CreateIssue returned error: %v", err) + } + + wantPath := "/repos/" + Owner + "/" + Repo + "/issues" + if gotPath != wantPath { + t.Errorf("request path = %q, want %q", gotPath, wantPath) + } + if gotBody.Title == nil || *gotBody.Title != "Bug: thing broke" { + t.Errorf("request title = %v, want %q", gotBody.Title, "Bug: thing broke") + } + if gotBody.Labels == nil || len(*gotBody.Labels) != 2 || (*gotBody.Labels)[0] != "source:in-app-feedback" || (*gotBody.Labels)[1] != "bug" { + t.Errorf("request labels = %v, want [source:in-app-feedback bug]", gotBody.Labels) + } +} + +func TestCreateIssue_GitHubError(t *testing.T) { + client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"internal error"}`)) + }) + + err := client.CreateIssue(context.Background(), "Bug", "Body", []string{"source:in-app-feedback", "bug"}) + if err == nil { + t.Fatal("CreateIssue returned nil error, want non-nil on GitHub API failure") + } +} diff --git a/feedback/ratelimit.go b/feedback/ratelimit.go new file mode 100644 index 0000000..c8849df --- /dev/null +++ b/feedback/ratelimit.go @@ -0,0 +1,62 @@ +package feedback + +import ( + "net/http" + + "github.com/gin-gonic/gin" + "github.com/gomodule/redigo/redis" + "github.com/sweetrpg/admin-api/constants" + apicoreconstants "github.com/sweetrpg/api-core.go/constants" + "github.com/sweetrpg/api-core.go/ratelimit" + "github.com/sweetrpg/api-core.go/util" + apiv "github.com/sweetrpg/api-core.go/vo" + "github.com/sweetrpg/common.go/logging" +) + +// tierFeedback is this route's own rate-limit tier name, kept separate from +// api-core.go/ratelimit's cheap/standard tiers so it doesn't change the budget for any other +// admin-api route (see design.md's "dedicated feedback-specific rate-limit tier" decision). +const tierFeedback = "feedback" + +// RateLimitMiddleware enforces a per-client budget on just the route it's mounted on, stricter +// than and layered on top of admin-api's existing service-wide limiter. Fails closed (503) if +// pool is nil or Redis is unreachable, matching api-core.go/ratelimit.Middleware's behavior. +func RateLimitMiddleware(pool *redis.Pool) gin.HandlerFunc { + limiter := ratelimit.New(pool, map[string]ratelimit.Tier{ + tierFeedback: { + Limit: util.GetEnvInt(constants.RATE_LIMIT_FEEDBACK, constants.RateLimitFeedbackDefault), + Window: util.GetEnvInt(constants.RATE_LIMIT_FEEDBACK_WINDOW_SECONDS, constants.RateLimitFeedbackWindowDefault), + }, + }) + + return func(c *gin.Context) { + if pool == nil { + logging.Logger.Error("Feedback rate-limit store not configured; rejecting request (fail closed)") + c.AbortWithStatusJSON(http.StatusServiceUnavailable, apiv.ErrorVO{ + Error: apicoreconstants.ErrorRateLimitUnavailable, + Message: "Rate limiting is temporarily unavailable", + }) + return + } + + clientKey := ratelimit.ClientKey(c) + allowed, err := limiter.Allow(c.Request.Context(), clientKey, tierFeedback) + if err != nil { + logging.Logger.Error("Feedback rate-limit backend unreachable; rejecting request (fail closed)", "error", err.Error()) + c.AbortWithStatusJSON(http.StatusServiceUnavailable, apiv.ErrorVO{ + Error: apicoreconstants.ErrorRateLimitUnavailable, + Message: "Rate limiting is temporarily unavailable", + }) + return + } + if !allowed { + logging.Logger.Warn("Feedback rate limit exceeded", "client", clientKey) + c.AbortWithStatusJSON(http.StatusTooManyRequests, apiv.ErrorVO{ + Error: apicoreconstants.ErrorRateLimited, + Message: "Limit exceeded", + }) + return + } + c.Next() + } +} diff --git a/go.mod b/go.mod index 0400f2f..a0bcd08 100644 --- a/go.mod +++ b/go.mod @@ -3,9 +3,12 @@ module github.com/sweetrpg/admin-api go 1.26.5 require ( + github.com/alicebob/miniredis/v2 v2.39.0 github.com/getsentry/sentry-go v0.49.0 github.com/gin-contrib/cors v1.7.8 github.com/gin-gonic/gin v1.12.0 + github.com/gomodule/redigo v1.9.3 + github.com/google/go-github/v76 v76.0.0 github.com/grafana/pyroscope-go v1.4.2 github.com/joho/godotenv v1.5.1 github.com/penglongli/gin-metrics v0.1.13 @@ -56,7 +59,7 @@ require ( github.com/goccy/go-json v0.10.6 // indirect github.com/goccy/go-yaml v1.19.2 // indirect github.com/golang/snappy v1.0.0 // indirect - github.com/gomodule/redigo v1.9.3 // indirect + github.com/google/go-querystring v1.1.0 // indirect github.com/google/uuid v1.6.0 // indirect github.com/grafana/pyroscope-go/godeltaprof v0.1.11 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.29.0 // indirect @@ -95,6 +98,7 @@ require ( github.com/xdg-go/scram v1.2.0 // indirect github.com/xdg-go/stringprep v1.0.4 // indirect github.com/youmark/pkcs8 v0.0.0-20240726163527-a2c0da244d78 // indirect + github.com/yuin/gopher-lua v1.1.1 // indirect github.com/zerodha/logf v0.5.5 // indirect go.jtlabs.io/query v1.6.1 // indirect go.mongodb.org/mongo-driver/v2 v2.8.1 // indirect diff --git a/go.sum b/go.sum index 8c4710a..5b79c65 100644 --- a/go.sum +++ b/go.sum @@ -93,8 +93,13 @@ github.com/golang/snappy v1.0.0 h1:Oy607GVXHs7RtbggtPBnr2RmDArIsAefDwvrdWvRhGs= github.com/golang/snappy v1.0.0/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q= github.com/gomodule/redigo v1.9.3 h1:dNPSXeXv6HCq2jdyWfjgmhBdqnR6PRO3m/G05nvpPC8= github.com/gomodule/redigo v1.9.3/go.mod h1:KsU3hiK/Ay8U42qpaJk+kuNa3C+spxapWpM+ywhcgtw= +github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= +github.com/google/go-github/v76 v76.0.0 h1:MCa9VQn+VG5GG7Y7BAkBvSRUN3o+QpaEOuZwFPJmdFA= +github.com/google/go-github/v76 v76.0.0/go.mod h1:38+d/8pYDO4fBLYfBhXF5EKO0wA3UkXBjfmQapFsNCQ= +github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8= +github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17icRSOU623lUBU= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= @@ -333,6 +338,7 @@ golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc golang.org/x/tools v0.48.0 h1:3+hClM1aLL5mjMKm5ovokw9epgRXPuu2tILgismM6RE= golang.org/x/tools v0.48.0/go.mod h1:08xX0orndb/F7jJxGDicx061tyd5pcMto75YMAXr6lk= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= +golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= gonum.org/v1/gonum v0.17.0 h1:VbpOemQlsSMrYmn7T2OUvQ4dqxQXU+ouZFQsZOx50z4= gonum.org/v1/gonum v0.17.0/go.mod h1:El3tOrEuMpv2UdMrbNlKEh9vd86bmQ6vqIcDwxEOc1E= google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa h1:Kjn0N0tCrDgiAFW+lGO4JZ3ck44CehvJQMAwj9QF0G8= diff --git a/kubernetes/base/deployment.yaml b/kubernetes/base/deployment.yaml index c0928dd..a5ca359 100644 --- a/kubernetes/base/deployment.yaml +++ b/kubernetes/base/deployment.yaml @@ -43,6 +43,8 @@ spec: name: api-db - secretRef: name: api-misc + - secretRef: + name: api-feedback - secretRef: name: api-cache env: diff --git a/kubernetes/overlays/dev/configmaps.yaml b/kubernetes/overlays/dev/configmaps.yaml index 0614b16..8e306d3 100644 --- a/kubernetes/overlays/dev/configmaps.yaml +++ b/kubernetes/overlays/dev/configmaps.yaml @@ -28,7 +28,11 @@ data: RATE_LIMIT_CHEAP_WINDOW_SECONDS: "60" RATE_LIMIT_STANDARD: "30" RATE_LIMIT_STANDARD_WINDOW_SECONDS: "60" - # Namespace cache Service, backs the distributed rate-limit counters. REDIS_PASS comes from an - # ExternalSecret (#106). + # Namespace cache Service, backs the distributed rate-limit counters (platform-default tiers + # above, and the feedback-specific tier below). REDIS_PASS comes from an ExternalSecret (#106). REDIS_HOST: cache.sweetrpg-admin.svc.cluster.local REDIS_PORT: "6379" + # main-web's feedback widget is the first browser-side (cross-origin) caller of this API - + # every prior consumer (main-web, catalog-web) fetches server-side, which setupCORS never + # gated. Without this, the Gin CORS middleware stays disabled entirely. + ALLOWED_ORIGINS: https://dev.sweetrpg.com diff --git a/kubernetes/overlays/dev/kustomization.yaml b/kubernetes/overlays/dev/kustomization.yaml index c184ebd..25a962e 100644 --- a/kubernetes/overlays/dev/kustomization.yaml +++ b/kubernetes/overlays/dev/kustomization.yaml @@ -14,6 +14,7 @@ resources: - secrets/cache.yaml - secrets/db-creds.yaml - secrets/db.yaml + - secrets/feedback.yaml - secrets/misc.yaml labels: diff --git a/kubernetes/overlays/dev/secrets/feedback.yaml b/kubernetes/overlays/dev/secrets/feedback.yaml new file mode 100644 index 0000000..cd4bec8 --- /dev/null +++ b/kubernetes/overlays/dev/secrets/feedback.yaml @@ -0,0 +1,18 @@ +--- +apiVersion: external-secrets.io/v1 +kind: ExternalSecret + +metadata: + name: admin-api-feedback + +spec: + refreshInterval: 1h + secretStoreRef: + name: akeyless + kind: ClusterSecretStore + target: + name: api-feedback + creationPolicy: Owner + dataFrom: + - extract: + key: /sweetrpg/admin/api/feedback diff --git a/server/feedback.go b/server/feedback.go new file mode 100644 index 0000000..43281d6 --- /dev/null +++ b/server/feedback.go @@ -0,0 +1,130 @@ +package server + +import ( + "fmt" + "net/http" + "strings" + "time" + + "github.com/gin-gonic/gin" + "github.com/sweetrpg/admin-api/constants" + "github.com/sweetrpg/admin-api/feedback" + apiv "github.com/sweetrpg/api-core.go/vo" + "github.com/sweetrpg/common.go/logging" +) + +// feedbackTypeBug and feedbackTypeFeature are the only accepted values for createFeedbackRequest.Type. +const ( + feedbackTypeBug = "bug" + feedbackTypeFeature = "feature" +) + +// createFeedbackRequest is the payload accepted by POST /feedback. Website and RenderedAt back +// the bot-detection check (honeypot + minimum fill-time); real users never populate Website +// and never submit faster than FeedbackMinFillTime after RenderedAt - see design.md. +type createFeedbackRequest struct { + Type string `json:"type" binding:"required" example:"bug"` + Title string `json:"title" binding:"required" example:"Catalog search returns no results for exact ISBN match"` + Body string `json:"body" binding:"required" example:"Searching for 9780786965601 returns zero results, even though the volume exists in the catalog."` + ReporterEmail string `json:"reporter_email,omitempty" example:"reporter@example.com"` + Source string `json:"source,omitempty" example:"catalog-web:/catalog/search"` + Website string `json:"website,omitempty"` + RenderedAt *time.Time `json:"rendered_at,omitempty"` +} + +// feedbackAcceptedResponse is the uniform success body - identical whether a GitHub Issue was +// actually created or the submission was silently discarded as bot traffic. Deliberately gives +// a bot no signal about which outcome occurred. +type feedbackAcceptedResponse struct { + Status string `json:"status" example:"accepted"` +} + +func setupFeedbackHandlers(g *gin.Engine, issueCreator feedback.IssueCreator, rateLimit gin.HandlerFunc) { + logging.Logger.Info("Setting up feedback endpoint handlers...") + + g.POST("/feedback", rateLimit, createFeedback(issueCreator)) +} + +// Submit feedback. +// +// @Summary Submit feedback +// @Description Accept an anonymous bug report or feature request and file it as a GitHub Issue on sweetrpg/platform, labeled source:in-app-feedback plus bug or feature. No GitHub credentials are ever requested from or exposed to the caller. Subject to a feedback-specific rate limit stricter than this service's default. +// @Tags feedback +// @Accept json +// @Produce json +// @Param body body createFeedbackRequest true "Feedback submission" +// @Success 200 {object} feedbackAcceptedResponse +// @Failure 400 {object} apiv.ErrorVO "missing/invalid type, title, or body, or title/body exceeds the length cap" +// @Failure 429 {object} apiv.ErrorVO "feedback-specific rate limit exceeded" +// @Failure 502 {object} apiv.ErrorVO "GitHub API call failed or timed out" +// @Failure 503 {object} apiv.ErrorVO "rate-limit backend unavailable" +// @Router /feedback [post] +func createFeedback(issueCreator feedback.IssueCreator) gin.HandlerFunc { + return func(c *gin.Context) { + var req createFeedbackRequest + if err := c.ShouldBindJSON(&req); err != nil { + c.JSON(http.StatusBadRequest, apiv.ErrorVO{Error: "invalid_request", Message: err.Error()}) + return + } + + if req.Type != feedbackTypeBug && req.Type != feedbackTypeFeature { + c.JSON(http.StatusBadRequest, apiv.ErrorVO{Error: "invalid_type", Message: "type must be \"bug\" or \"feature\""}) + return + } + if strings.TrimSpace(req.Title) == "" || strings.TrimSpace(req.Body) == "" { + c.JSON(http.StatusBadRequest, apiv.ErrorVO{Error: "invalid_request", Message: "title and body are required"}) + return + } + if len(req.Title) > constants.FeedbackTitleMaxLength { + c.JSON(http.StatusBadRequest, apiv.ErrorVO{Error: "title_too_long", Message: fmt.Sprintf("title exceeds %d characters", constants.FeedbackTitleMaxLength)}) + return + } + if len(req.Body) > constants.FeedbackBodyMaxLength { + c.JSON(http.StatusBadRequest, apiv.ErrorVO{Error: "body_too_long", Message: fmt.Sprintf("body exceeds %d characters", constants.FeedbackBodyMaxLength)}) + return + } + + if looksAutomated(req) { + logging.Logger.Warn("Discarding feedback submission that failed bot-detection check") + c.JSON(http.StatusOK, feedbackAcceptedResponse{Status: "accepted"}) + return + } + + body := issueBody(req) + labels := []string{constants.FeedbackSourceLabel, req.Type} + if err := issueCreator.CreateIssue(c.Request.Context(), req.Title, body, labels); err != nil { + logging.Logger.Error("Failed to create GitHub issue from feedback submission", "error", err.Error()) + c.JSON(http.StatusBadGateway, apiv.ErrorVO{Error: "github_unavailable", Message: "failed to record feedback; please try again shortly"}) + return + } + + c.JSON(http.StatusOK, feedbackAcceptedResponse{Status: "accepted"}) + } +} + +// looksAutomated reports whether a submission fails the bot-detection check: a populated +// honeypot field (real users never see or fill it), or submission faster than +// constants.FeedbackMinFillTime after the client-reported render time. +func looksAutomated(req createFeedbackRequest) bool { + if req.Website != "" { + return true + } + if req.RenderedAt != nil && time.Since(*req.RenderedAt) < constants.FeedbackMinFillTime { + return true + } + return false +} + +// issueBody renders the submitted body plus any optional fields, appended only when present +// per the "Submission without optional fields" scenario - no empty placeholders. +func issueBody(req createFeedbackRequest) string { + var b strings.Builder + b.WriteString(req.Body) + if req.ReporterEmail != "" { + fmt.Fprintf(&b, "\n\n---\nReporter email: %s", req.ReporterEmail) + } + if req.Source != "" { + fmt.Fprintf(&b, "\n\nSource: %s", req.Source) + } + return b.String() +} diff --git a/server/feedback_test.go b/server/feedback_test.go new file mode 100644 index 0000000..fa81518 --- /dev/null +++ b/server/feedback_test.go @@ -0,0 +1,287 @@ +package server + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/alicebob/miniredis/v2" + "github.com/gin-gonic/gin" + "github.com/gomodule/redigo/redis" + "github.com/sweetrpg/admin-api/feedback" + "github.com/sweetrpg/common.go/logging" +) + +func TestMain(m *testing.M) { + logging.Init() + m.Run() +} + +// fakeIssueCreator records calls and returns a configurable error, so tests can assert both +// "an issue was created with these labels/title/body" and "GitHub failure surfaces as 502" +// without making a real network call. +type fakeIssueCreator struct { + called bool + title string + body string + labels []string + err error +} + +func (f *fakeIssueCreator) CreateIssue(_ context.Context, title, body string, labels []string) error { + f.called = true + f.title = title + f.body = body + f.labels = labels + return f.err +} + +func newFeedbackTestRouter(creator feedback.IssueCreator) *gin.Engine { + gin.SetMode(gin.TestMode) + r := gin.New() + setupFeedbackHandlers(r, creator, func(c *gin.Context) { c.Next() }) + return r +} + +func postFeedback(r *gin.Engine, body map[string]any) *httptest.ResponseRecorder { + b, _ := json.Marshal(body) + req := httptest.NewRequest(http.MethodPost, "/feedback", bytes.NewReader(b)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + return w +} + +func TestCreateFeedback_ValidBugReport(t *testing.T) { + creator := &fakeIssueCreator{} + r := newFeedbackTestRouter(creator) + + w := postFeedback(r, map[string]any{ + "type": "bug", + "title": "Catalog search broken", + "body": "Searching for an exact ISBN returns no results.", + }) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String()) + } + if !creator.called { + t.Fatal("expected CreateIssue to be called for a valid submission") + } + if creator.title != "Catalog search broken" { + t.Errorf("issue title = %q, want %q", creator.title, "Catalog search broken") + } + wantLabels := []string{"source:in-app-feedback", "bug"} + if len(creator.labels) != 2 || creator.labels[0] != wantLabels[0] || creator.labels[1] != wantLabels[1] { + t.Errorf("issue labels = %v, want %v", creator.labels, wantLabels) + } +} + +func TestCreateFeedback_MissingOrInvalidFields(t *testing.T) { + cases := []struct { + name string + body map[string]any + }{ + {"missing type", map[string]any{"title": "t", "body": "b"}}, + {"invalid type", map[string]any{"type": "complaint", "title": "t", "body": "b"}}, + {"missing title", map[string]any{"type": "bug", "body": "b"}}, + {"missing body", map[string]any{"type": "bug", "title": "t"}}, + {"blank title", map[string]any{"type": "bug", "title": " ", "body": "b"}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + creator := &fakeIssueCreator{} + r := newFeedbackTestRouter(creator) + + w := postFeedback(r, tc.body) + + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body = %s", w.Code, w.Body.String()) + } + if creator.called { + t.Error("expected CreateIssue not to be called for a rejected submission") + } + }) + } +} + +func TestCreateFeedback_OversizedFieldsRejected(t *testing.T) { + longTitle := make([]byte, 201) + for i := range longTitle { + longTitle[i] = 'a' + } + longBody := make([]byte, 10001) + for i := range longBody { + longBody[i] = 'b' + } + + creator := &fakeIssueCreator{} + r := newFeedbackTestRouter(creator) + + w := postFeedback(r, map[string]any{ + "type": "feature", + "title": string(longTitle), + "body": "ok", + }) + if w.Code != http.StatusBadRequest { + t.Fatalf("oversized title: status = %d, want 400", w.Code) + } + + w = postFeedback(r, map[string]any{ + "type": "feature", + "title": "ok", + "body": string(longBody), + }) + if w.Code != http.StatusBadRequest { + t.Fatalf("oversized body: status = %d, want 400", w.Code) + } + if creator.called { + t.Error("expected CreateIssue not to be called for an oversized submission") + } +} + +func TestCreateFeedback_HoneypotPopulated_SilentlyAccepted(t *testing.T) { + creator := &fakeIssueCreator{} + r := newFeedbackTestRouter(creator) + + w := postFeedback(r, map[string]any{ + "type": "bug", + "title": "t", + "body": "b", + "website": "http://spam.example", + }) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (silent accept); body = %s", w.Code, w.Body.String()) + } + if creator.called { + t.Error("expected CreateIssue not to be called when honeypot is populated") + } +} + +func TestCreateFeedback_SubmittedTooFast_SilentlyAccepted(t *testing.T) { + creator := &fakeIssueCreator{} + r := newFeedbackTestRouter(creator) + + renderedAt := time.Now() + w := postFeedback(r, map[string]any{ + "type": "bug", + "title": "t", + "body": "b", + "rendered_at": renderedAt.Format(time.RFC3339Nano), + }) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (silent accept); body = %s", w.Code, w.Body.String()) + } + if creator.called { + t.Error("expected CreateIssue not to be called for a too-fast submission") + } +} + +func TestCreateFeedback_OptionalFieldsIncludedInBody(t *testing.T) { + creator := &fakeIssueCreator{} + r := newFeedbackTestRouter(creator) + + w := postFeedback(r, map[string]any{ + "type": "feature", + "title": "t", + "body": "b", + "reporter_email": "reporter@example.com", + "source": "main-web:/", + }) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String()) + } + if !creator.called { + t.Fatal("expected CreateIssue to be called") + } + if !contains(creator.body, "reporter@example.com") || !contains(creator.body, "main-web:/") { + t.Errorf("issue body = %q, want it to include both optional fields", creator.body) + } +} + +func TestCreateFeedback_OptionalFieldsOmittedWhenAbsent(t *testing.T) { + creator := &fakeIssueCreator{} + r := newFeedbackTestRouter(creator) + + w := postFeedback(r, map[string]any{"type": "bug", "title": "t", "body": "b"}) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String()) + } + if creator.body != "b" { + t.Errorf("issue body = %q, want exactly the submitted body with no placeholders", creator.body) + } +} + +func TestCreateFeedback_GitHubFailure_Returns502(t *testing.T) { + creator := &fakeIssueCreator{err: errors.New("github: 500 internal server error")} + r := newFeedbackTestRouter(creator) + + w := postFeedback(r, map[string]any{"type": "bug", "title": "t", "body": "b"}) + + if w.Code != http.StatusBadGateway { + t.Fatalf("status = %d, want 502; body = %s", w.Code, w.Body.String()) + } +} + +func contains(s, substr string) bool { + return len(s) >= len(substr) && (func() bool { + for i := 0; i+len(substr) <= len(s); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false + })() +} + +// TestFeedbackRateLimit_ExceedsBudgetWithoutAffectingOtherRoutes exercises +// feedback.RateLimitMiddleware directly against a miniredis-backed pool: once the feedback +// tier's budget is exhausted, /feedback 429s while an unrelated route mounted alongside it +// keeps responding normally. +func TestFeedbackRateLimit_ExceedsBudgetWithoutAffectingOtherRoutes(t *testing.T) { + t.Setenv("RATE_LIMIT_FEEDBACK", "1") + t.Setenv("RATE_LIMIT_FEEDBACK_WINDOW_SECONDS", "60") + + mr := miniredis.RunT(t) + pool := &redis.Pool{ + Dial: func() (redis.Conn, error) { + return redis.Dial("tcp", mr.Addr()) + }, + } + t.Cleanup(func() { _ = pool.Close() }) + + gin.SetMode(gin.TestMode) + r := gin.New() + creator := &fakeIssueCreator{} + setupFeedbackHandlers(r, creator, feedback.RateLimitMiddleware(pool)) + r.GET("/other", func(c *gin.Context) { c.Status(http.StatusOK) }) + + body := map[string]any{"type": "bug", "title": "t", "body": "b"} + + w1 := postFeedback(r, body) + if w1.Code != http.StatusOK { + t.Fatalf("first submission: status = %d, want 200; body = %s", w1.Code, w1.Body.String()) + } + + w2 := postFeedback(r, body) + if w2.Code != http.StatusTooManyRequests { + t.Fatalf("second submission: status = %d, want 429; body = %s", w2.Code, w2.Body.String()) + } + + other := httptest.NewRequest(http.MethodGet, "/other", nil) + otherW := httptest.NewRecorder() + r.ServeHTTP(otherW, other) + if otherW.Code != http.StatusOK { + t.Errorf("unrelated route status = %d, want 200 - feedback rate limit must not affect other routes", otherW.Code) + } +} diff --git a/server/handlers.go b/server/handlers.go index 8ea06fc..229316d 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -2,12 +2,14 @@ package server import ( "github.com/gin-gonic/gin" + "github.com/sweetrpg/admin-api/feedback" "github.com/sweetrpg/authz-client.go/authz" ) -func SetupHandlers(g *gin.Engine, authzClient *authz.Client) { +func SetupHandlers(g *gin.Engine, authzClient *authz.Client, issueCreator feedback.IssueCreator, feedbackRateLimit gin.HandlerFunc) { setupBannerHandlers(g, authzClient) setupMaintenanceModeHandlers(g, authzClient) setupAppCardStatusHandlers(g, authzClient) setupStatusHandlers(g) + setupFeedbackHandlers(g, issueCreator, feedbackRateLimit) } diff --git a/server/write_auth_integration_test.go b/server/write_auth_integration_test.go index c62bc76..18609eb 100644 --- a/server/write_auth_integration_test.go +++ b/server/write_auth_integration_test.go @@ -1,6 +1,7 @@ package server import ( + "context" "encoding/json" "net/http" "net/http/httptest" @@ -11,6 +12,12 @@ import ( "github.com/sweetrpg/common.go/logging" ) +// noopIssueCreator is a stub feedback.IssueCreator for tests that don't exercise /feedback +// itself but need a valid SetupHandlers call. +type noopIssueCreator struct{} + +func (noopIssueCreator) CreateIssue(_ context.Context, _, _ string, _ []string) error { return nil } + // TestWriteRoutesRequireAuth registers every route admin-api exposes (the // same SetupHandlers wiring main.go uses) and asserts every write route // (POST/PUT/DELETE) 401s without credentials, while every read route (GET) is @@ -29,7 +36,7 @@ func TestWriteRoutesRequireAuth(t *testing.T) { _ = json.NewEncoder(w).Encode(authz.CheckResponse{Allowed: true, Roles: []string{authz.RoleAdmin}, Sub: "auth0|user-sub"}) })) t.Cleanup(authAPI.Close) - SetupHandlers(r, authz.NewClient(authAPI.URL, authAPI.URL)) + SetupHandlers(r, authz.NewClient(authAPI.URL, authAPI.URL), noopIssueCreator{}, func(c *gin.Context) { c.Next() }) writeRoutes := []struct { method string