feat: add CORS headers#424
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces CORS support by implementing a new middleware and applying it to the server's router group. It also includes unit tests for the middleware, updates the .gitignore file to exclude local configuration, and reorders imports for consistency. The review feedback suggests improving the CORS implementation by adding an Access-Control-Max-Age header to cache preflight responses and recommends allowing configurable origins instead of using a wildcard for better security.
There was a problem hiding this comment.
Code Review
This pull request introduces CORS support to the server, allowing for configurable allowed origins via settings or environment variables. The implementation includes a new middleware that handles origin validation, preflight OPTIONS requests, and appropriate headers. Feedback was provided regarding the CORS middleware to ensure the Vary: Origin header is consistently applied when specific origins are configured to prevent cache poisoning, and to restrict OPTIONS request interception to actual CORS preflights.
fank
left a comment
There was a problem hiding this comment.
On the splitCSV workaround — verified not needed
I ran an empirical check against viper v1.21.0 (this project's pinned version) with the same struct shape and tags (json/yaml only, no mapstructure):
os.Setenv("OCAP_CORS_ALLOWEDORIGINS", "https://a.com,https://b.com")
os.Setenv("OCAP_AUTH_ADMINSTEAMIDS", "id1,id2,id3")
// ... viper.AutomaticEnv + SetDefault + Unmarshal
// → cors len=2 ["https://a.com", "https://b.com"]
// → auth len=3 ["id1", "id2", "id3"]Viper v1.21 auto-splits comma-separated env values into []string slices on Unmarshal. The comment in setting.go:132-133 ("Viper doesn't split comma-separated env var strings into slices") is stale — it predates v1.21. Commit 3397682 already dropped a similar Viper workaround for the same reason.
Note: setting_test.go only covers AdminSteamIDs via the JSON config file path, not the env var path — there's no test asserting that env-var CSV expansion actually requires splitCSV in production code, so the helper has been unverified.
Ask: Drop the new splitCSV(setting.CORS.AllowedOrigins) line + comment. Since you're already touching that block, drop splitCSV(setting.Auth.AdminSteamIDs), the comment, and the now-dead splitCSV helper + its unit test in this same PR — same legacy assumption, atomic cleanup, no separate follow-up needed.
Rest of the PR
Middleware logic is good:
- Wildcard
*whenallowedOriginsis empty; explicit allowlist withVary: Originotherwise. - Preflight only intercepted when
Originheader is present — correct, lets non-CORS OPTIONS pass through. - No
Access-Control-Allow-Credentials— right call given Bearer JWT auth (no cookies). - 86400s preflight cache is reasonable.
Tests cover wildcard, allowlist, matching/non-matching origin, and missing-Origin OPTIONS pass-through. Solid coverage.
.gitignore and README additions are appropriate.
Summary
Allow cross-origin requests to the app by adding CORS headers.
Also, add to .gitignore Claude local settings for local development
Checklist
go test ./...passes