feat: align error responses with RFC 9457#79
Conversation
✱ Stainless preview buildsThis PR will update the Edit this comment to update it. It will appear in the SDK's changelogs. ✅ hub-openapi studio · code · diff
✅ hub-typescript studio · code · diff
This comment is auto-generated by GitHub Actions and is automatically kept up to date as you push. |
xernobyl
changed the title
WalkthroughThis PR implements RFC 9457–compliant problem details as the unified error response format across the HTTP API. The changes introduce stable problem type URIs and machine-readable 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/handlers/feedback_records_handler.go (1)
180-192:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftRequire tenant scope on
DeleteByUser.Line 190 can execute a bulk delete with
filters.TenantID == nil, which makes a normal feedback-records endpoint capable of deleting tenant-owned data across tenants. This needs to requiretenant_id, or be split into an explicitly named/documented GDPR-style all-tenant erasure flow with separate tests and logging.As per coding guidelines, "Tenant-owned data must never be read, enqueued, dispatched, cached, searched, embedded, or deleted across tenants without explicit tenant scope verification" and "Do not model
tenant_idas an optional filter for tenant-owned resources."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handlers/feedback_records_handler.go` around lines 180 - 192, The DeleteByUser handler currently allows bulk deletes when filters.TenantID is nil; update DeleteByUser to enforce explicit tenant scope by validating filters.TenantID is non-nil before calling h.service.DeleteFeedbackRecordsByUser and return a bad-request/authorization error if missing. Locate the filters variable (models.DeleteFeedbackRecordsByUserFilters) and add a guard after validation.ValidateAndDecodeQueryParams that checks filters.TenantID (or equivalent field) and rejects the request if nil, or alternatively split into a separate explicit "all-tenant erasure" flow with distinct endpoint/logic and tests; ensure h.service.DeleteFeedbackRecordsByUser is only called when tenant scope is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/handlers/search_handler.go`:
- Around line 67-69: The handler returns 405 without an Allow header; before
calling response.RespondProblem when rejecting non-POST requests (the r.Method
!= http.MethodPost check and the second 405 branch around the later check), set
the response header Allow to "POST" (e.g., w.Header().Set("Allow",
http.MethodPost)) so clients receive the permitted method; update both
occurrences in search_handler.go (the initial method check and the later 405
branch) to set the header prior to responding.
In `@internal/api/middleware/problem.go`:
- Around line 18-32: The problemErrorWriter currently embeds http.ResponseWriter
and overrides WriteHeader/Write but doesn't forward optional interfaces; add
method delegations on problemErrorWriter for http.Flusher (Flush), http.Hijacker
(Hijack), http.Pusher (Push), and io.ReaderFrom (ReadFrom) that check and call
the same method on the underlying ResponseWriter when it implements those
interfaces, returning appropriate results/errors otherwise; update the type to
use the embedded ResponseWriter field (already named ResponseWriter) for
assertions inside each delegated method so existing WriteHeader/Write behavior
is preserved while enabling interface upgrades/streaming for handlers that
require them.
In `@internal/api/response/errors.go`:
- Around line 99-102: The JSON error handling in the function around the
json.SyntaxError check currently misses truncated payloads (io.ErrUnexpectedEOF)
and treats them as 500s; add a check using errors.Is(err, io.ErrUnexpectedEOF)
(or a var ioErr = io.ErrUnexpectedEOF and errors.Is) and return
newProblem(http.StatusBadRequest, "Invalid JSON: "+err.Error()), true when that
matches; ensure the io package is imported if not present and place this check
alongside or just after the existing var syntaxErr check so truncated JSON is
classified as a client (400) error instead of a server error.
- Around line 113-121: Replace the broad substring check for "unknown field"
with a stricter JSON-decoder-origin check: in the branch that currently does if
strings.Contains(err.Error(), "unknown field") (around newValidationProblem /
InvalidParam usage), only treat it as a 400 validation error when the error
clearly comes from the json decoder (e.g., strings.HasPrefix(err.Error(), "json:
unknown field") or, better, use errors.As to match the JSON decode error type if
available). Update unknownJSONField(err) callsite to be invoked only after this
tighter check so unrelated internal errors are not misclassified as validation
problems.
In `@internal/api/response/response.go`:
- Around line 63-69: The response writer sets the problem body RequestID from
context but doesn't mirror it into the response headers; update the writeProblem
flow (where problem.RequestID is assigned and logProblem is called) to also set
the "X-Request-ID" response header to the same value (problem.RequestID or
observability.RequestIDFromContext(ctx)) before writing the body so header/body
parity is guaranteed regardless of middleware ordering.
In `@internal/api/validation/validation_test.go`:
- Around line 58-61: The test only asserts validationErrors[0].Field(), losing
detail-level checks; update the assertions on the validationErrors returned from
ValidateStruct to also assert validationErrors[0].Tag() equals "no_null_bytes"
and validationErrors[0].Value() equals the offending value (the string
containing the null byte) so the test
TestValidateStructPreservesValidationDetails ensures both the rule/tag and the
offending value are preserved; use the existing validationErrors variable and
its methods Tag() and Value() to add these assertions.
In `@openapi.yaml`:
- Around line 1446-1449: The OpenAPI schema defines the components field
invalid_params as an unbounded array; update the items schema for invalid_params
(the array using $ref '`#/components/schemas/InvalidParam`') to include an
explicit maximum size (e.g., add maxItems: 100) to cap the array length and
prevent unconstrained payloads so CKV_OPENAPI_21 is satisfied.
---
Outside diff comments:
In `@internal/api/handlers/feedback_records_handler.go`:
- Around line 180-192: The DeleteByUser handler currently allows bulk deletes
when filters.TenantID is nil; update DeleteByUser to enforce explicit tenant
scope by validating filters.TenantID is non-nil before calling
h.service.DeleteFeedbackRecordsByUser and return a bad-request/authorization
error if missing. Locate the filters variable
(models.DeleteFeedbackRecordsByUserFilters) and add a guard after
validation.ValidateAndDecodeQueryParams that checks filters.TenantID (or
equivalent field) and rejects the request if nil, or alternatively split into a
separate explicit "all-tenant erasure" flow with distinct endpoint/logic and
tests; ensure h.service.DeleteFeedbackRecordsByUser is only called when tenant
scope is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 647e50d5-c883-4be9-84a3-9341c4e77e70
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
cmd/api/app.gogo.modinternal/api/handlers/feedback_records_handler.gointernal/api/handlers/feedback_records_handler_test.gointernal/api/handlers/openapi_handler.gointernal/api/handlers/search_handler.gointernal/api/handlers/tenant_data_handler.gointernal/api/handlers/tenant_data_handler_test.gointernal/api/handlers/webhooks_handler.gointernal/api/middleware/auth.gointernal/api/middleware/body.gointernal/api/middleware/body_test.gointernal/api/middleware/problem.gointernal/api/middleware/problem_test.gointernal/api/response/errors.gointernal/api/response/problem.gointernal/api/response/response.gointernal/api/response/response_test.gointernal/api/validation/validation.gointernal/api/validation/validation_test.gointernal/observability/logging.goopenapi.yamltests/integration_test.go
1 participant
What does this PR do?
Brings the Hub API's error responses up to RFC 9457 (Problem Details for HTTP APIs, obsoletes 7807), with a centralized error-handling architecture optimized for SDK consumers and MCP/agentic clients.
Closes ENG-1058.
Response shape (every error)
{ "type": "https://hub.formbricks.com/problems/validation", "title": "Validation Error", "status": 400, "detail": "One or more request parameters are invalid", "instance": "/v1/feedback-records", "code": "validation", "request_id": "0190a1b2-c3d4-7e5f-8a9b-0c1d2e3f4a5b", "invalid_params": [ { "name": "field_type", "reason": "must be one of: text, categorical, nps, csat, ces, rating, number, boolean, date" } ] }Content-Type: application/problem+json,Cache-Control: no-store, andX-Request-ID: …are set on all error responses.Highlights
codeas a closed enum in the OpenAPI schema.request_idfor failure correlation.detailsfor optional structured context, e.g.resource_typeon 404.invalid_params:[{name, reason}]with self-correcting reasons that enumerate allowed values and constraints.tenant_id,query,cursor), query-decode failures, and JSON-decode failures all returnvalidation+invalid_paramswhere appropriate.response.RespondError(w, r, err); a central mapper translateshuberrors, validation, query-decode, JSON-decode, and cursor errors into the right problem response.ProblemErrorsmiddleware rewrites stdlibServeMuxplain-text 404/405 responses into problem+json, preservingAllowon 405.WWW-Authenticate: Beareron every 401 (RFC 9110 §11.6.1).Cache-Control: no-storeon every problem.response.InvalidCursorReason.Drive-by
golang.org/x/netv0.53.0 → v0.55.0(+ alignedcrypto/sys/text) to close GO-2026-5026 flagged by the security scan.Deferred to a follow-up
Aligning naming exactly with the v3 API (
request_id→requestId,unauthorized→not_authenticated, optionally collapsing the validation type intobad_request) is intentionally a separate PR. This lands an idiomatic RFC 9457 implementation in Hub-native snake_case first.How should this be tested?
go test ./cmd/api ./internal/api/...go test ./internal/... ./pkg/...golangci-lint run ./...— 0 issuesmake migrate-validatehub_smokedatabaseDATABASE_URL='postgres://postgres:postgres@localhost:5432/hub_smoke?sslmode=disable' API_KEY='test-api-key-12345' go test ./tests/... -v -timeout 120shub_smokeafterwardManual smoke expectations:
POST /v1/feedback-recordswith badfield_type→ 400validation+invalid_params.GET /v1/feedback-records?tenant_id=org-123&since=not-a-date→ 400validation+invalid_params:[{name:"since", reason:"must be in RFC3339 (ISO 8601) format"}].GET /v1/feedback-records/not-a-uuid→ 400validation+invalid_params:[{name:"id", reason:"must be a valid UUID"}].PUT /v1/feedback-records/<uuid>→ 405 problem+json withAllowheader.GET /v1/nonexistent→ 404 problem+json, not plain text.Authorization→ 401 problem+json withWWW-Authenticate: Bearer.Content-Type: application/problem+json,Cache-Control: no-store,X-Request-IDheader, and matchingrequest_idbody field.Checklist
Required
git pull origin mainmake migrate-validate(n/a — no schema changes)Appreciated
docs/if changes were necessary (n/a — no doc changes)