fix(video): avoid duplicate CORS headers in content proxy - #6576
Conversation
WalkthroughThe video proxy copies upstream response headers through a helper that excludes case-insensitive ChangesVideo proxy response header filtering
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
controller/video_proxy_test.go (1)
12-16: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a mixed-case CORS regression case.
All source keys currently use canonical casing. A future case-sensitive filter would still pass this test. Change one
Access-Control-*source key to mixed case and keep the existing assertion.Proposed regression input
src := http.Header{ - "Access-Control-Allow-Origin": {"https://upstream.example"}, + "aCcEsS-cOnTrOl-AlLoW-OrIgIn": {"https://upstream.example"}, "Access-Control-Allow-Headers": {"Authorization"},As per coding guidelines, backend tests must protect regression paths. The PR objective requires case-insensitive filtering for every
Access-Control-*header.🤖 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 `@controller/video_proxy_test.go` around lines 12 - 16, Update the src header fixture in the video proxy test to use mixed casing for one Access-Control-* key, while preserving its value and all existing assertions. Ensure the test continues covering case-insensitive filtering for Access-Control-* headers without changing unrelated headers.Source: Coding guidelines
🤖 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 `@controller/video_proxy_test.go`:
- Around line 3-7: Update the tests in controller/video_proxy_test.go to replace
reflect.DeepEqual checks with testify/assert and replace t.Fatalf with
testify/require for setup or assertions that must stop execution. Add the
appropriate testify imports and preserve the existing test behavior.
---
Nitpick comments:
In `@controller/video_proxy_test.go`:
- Around line 12-16: Update the src header fixture in the video proxy test to
use mixed casing for one Access-Control-* key, while preserving its value and
all existing assertions. Ensure the test continues covering case-insensitive
filtering for Access-Control-* headers without changing unrelated headers.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 2ded9641-3b95-4c54-bc2a-417a10486cb0
📒 Files selected for processing (2)
controller/video_proxy.gocontroller/video_proxy_test.go
| import ( | ||
| "net/http" | ||
| "reflect" | ||
| "testing" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the repository assertion packages in this backend test.
The test uses reflect.DeepEqual and t.Fatalf. Replace these checks with testify/assert. Use testify/require for any setup or assertion that must stop the test.
Proposed assertion update
import (
"net/http"
- "reflect"
"testing"
+
+ "github.com/stretchr/testify/assert"
)
...
- if got := dst.Values("Access-Control-Allow-Origin"); !reflect.DeepEqual(got, []string{"*"}) {
- t.Fatalf("Access-Control-Allow-Origin = %v, want existing middleware value", got)
- }
+ assert.Equal(t, []string{"*"}, dst.Values("Access-Control-Allow-Origin"))
...
- if got := dst.Values("Access-Control-Allow-Headers"); len(got) != 0 {
- t.Fatalf("Access-Control-Allow-Headers = %v, want no copied upstream values", got)
- }
+ assert.Empty(t, dst.Values("Access-Control-Allow-Headers"))
...
- if got := dst.Get("Content-Type"); got != "video/mp4" {
- t.Fatalf("Content-Type = %q, want video/mp4", got)
- }
+ assert.Equal(t, "video/mp4", dst.Get("Content-Type"))
...
- if got := dst.Values("X-Upstream-Value"); !reflect.DeepEqual(got, []string{"first", "second"}) {
- t.Fatalf("X-Upstream-Value = %v, want both upstream values", got)
- }
+ assert.Equal(t, []string{"first", "second"}, dst.Values("X-Upstream-Value"))As per coding guidelines, new Go backend tests must use testify/require for setup and fatal assertions and testify/assert for non-fatal checks.
Also applies to: 21-31
🤖 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 `@controller/video_proxy_test.go` around lines 3 - 7, Update the tests in
controller/video_proxy_test.go to replace reflect.DeepEqual checks with
testify/assert and replace t.Fatalf with testify/require for setup or assertions
that must stop execution. Add the appropriate testify imports and preserve the
existing test behavior.
Source: Coding guidelines
1 participant
📝 变更描述 / Description
视频内容代理目前会使用
Header().Add()复制上游的所有响应头。当上游也返回 CORS 响应头时，它们会与当前实例 CORS 中间件已经设置的响应头叠加，导致浏览器因多个Access-Control-Allow-Origin值而拒绝读取视频响应。本次修改在复制视频响应头时跳过上游的所有
Access-Control-*头，继续由当前实例的 CORS 中间件统一管理；其他响应头（包括普通多值头）保持原有透传行为。新增单元测试覆盖上述行为。🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
📸 运行证明 / Proof of Work
新增测试验证：
Access-Control-Allow-OriginAccess-Control-*响应头Content-TypeSummary by CodeRabbit