# Reviewer 審查 M8-8 CORS middleware（2026-04-15） ## 摘要 - **總結論**：✅ **通過**。CORSMiddleware + WebSocket CheckOrigin 實作完全符合 `v2/cors-security.md` §3–§5，build / vet / test / race 全部 PASS，6 個 smoke test 親跑行為正確。 - **阻擋 M8-10 嗎**：不阻擋。所有 Critical / Major 皆零，僅 3 個 Minor / Suggestion，不影響後續任務。 --- ## A. Origin 判斷邏輯 `server/internal/api/middleware.go:33-46` - [x] 使用 `url.Parse` + `strings.ToLower(u.Hostname())` + map 查表（非 regex / prefix match） - [x] 白名單 map `allowedHosts`（第 17-22 行）包含 `127.0.0.1` / `localhost` / `::1` - [x] Scheme check 強制 `http`（第 41 行），自動擋掉 `https://` / `ws://` / `wss://` - [x] `url.Hostname()` 自動處理 IPv6 方括號：`http://[::1]:3721` → hostname `::1` - [x] 大小寫不敏感（`strings.ToLower`）— 測試 `http://LOCALHOST:9999` PASS - [x] 抗 suffix attack：`http://127.0.0.1.evil.com` → hostname 完整等於 `127.0.0.1.evil.com`，map 查表 miss → false（測試 case 驗證） - [x] 空字串 / `"null"` 明確拒絕（第 34 行） **符合 TDD §3.1 / §4.1 全部要求。** --- ## B. CORSMiddleware 行為 `server/internal/api/middleware.go:62-106` | 情境 | 實作 | TDD 要求 | 結果 | |------|------|---------|------| | Same-origin（無 Origin）非 OPTIONS | 直接 `c.Next()`，不回 ACA | §4.1 注釋 | ✅ | | Same-origin OPTIONS | 回 204 即停 | §4.1 | ✅ | | 白名單 Origin 非 OPTIONS | 回 ACA-Origin/Methods/Headers/Credentials + `Vary: Origin` → next | §4.1 | ✅ | | 白名單 OPTIONS 預檢 | 回完整 ACA + 204 | §3.2 | ✅（smoke 3 驗證） | | 非白名單 POST/PUT/DELETE/PATCH/OPTIONS | 403，不回 ACA | §3.2 / §4.1 | ✅ | | 非白名單 GET/HEAD | 執行 handler 但無 ACA | §3.3 | ✅（smoke 5 驗證） | - [x] 砍掉 v1 的 `X-Relay-Token`（relay 功能 M1 已砍） - [x] `Vary: Origin` 已加（第 98 行） - [x] `Access-Control-Allow-Credentials: true`（第 97 行） --- ## C. Test 覆蓋度 ### `middleware_test.go` - **TestIsAllowedOrigin**（19 case）： - 白名單 7 case（含 IPv6、大小寫、無 port） - scheme 3 case（https × 2、ws × 1） - hostname 5 case（192.168.x / example.com / malicious.local / suffix 攻擊 × 2） - 特殊 4 case（空字串、"null"、"http://"、"not-a-url"） - **TestCORSMiddleware_* × 7**： - AllowedOriginGET / LocalhostAllowed / DisallowedOriginPOST / DisallowedOriginGET / PreflightAllowed / PreflightDisallowed / SameOrigin - 涵蓋 `Vary: Origin`、`ACA-Credentials`、非白名單不得回 ACA 三項關鍵斷言。 ### `ws/origin_test.go` - **TestCheckOrigin**（9 sub-test）：empty / 127.0.0.1 / localhost / `[::1]` / https / 192.168 / evil / null / suffix 攻擊 **測試覆蓋度完整，符合 TDD §4.2 要求（TDD 只列了 10 case，實作更完整，算加分）。** --- ## D. WebSocket Origin 獨立 package - [x] `ws/origin.go` 不 import `api` package（只 import `net/http` / `net/url` / `strings`），避免 `api ↔ ws` 循環（`origin.go:1-7` + 注釋 14-17 有明確說明） - [x] 邏輯與 `api/middleware.go` 的 `isAllowedOrigin` **一致**（scheme、ToLower、hostname 白名單） - [x] `ws/device_events_ws.go:13-15` 宣告 package 層級共用 `var upgrader`，`CheckOrigin: CheckOrigin`；其他 WS handler 全部共用同一個 upgrader（`flash_ws.go:11`、`inference_ws.go:13`、`server_logs_ws.go:14`、`system_ws.go:29` 皆 `upgrader.Upgrade(...)`，grep 驗證） - [x] `device_events_ws.go` 已移除 `net/http` import（依任務描述），建立 upgrader 不再使用 inline anonymous `CheckOrigin: func(r *http.Request) bool { return true }` **唯一與 HTTP middleware 的行為差異**：`CheckOrigin` 對空 Origin 回 `true`（same-origin 或 websocat / Postman 非瀏覽器 client），而 HTTP middleware 對空 Origin 走「same-origin 快路徑 → next」。兩者在語意上一致：瀏覽器 same-origin 不送 Origin，非瀏覽器 client（如 websocat）也不會被誤擋。此處符合 TDD §5 注釋。 --- ## E. Router 整合 `server/internal/api/router.go:44-45` - [x] `CORSMiddleware()` 掛在 `broadcasterLogger` **後面**、`api := r.Group("/api")` 之前，位置合理 - [x] M8-4 的 `broadcasterLoggerSkipPaths`（`/api/system/boot-id` + `/api/system/health`）沒被動到（行 123-128、144-147） - [x] M8-4b 的 Hub sentinel 機制沒被動到（`hub_sentinel_test.go` 跑過 race 仍 PASS，`ws/*.go` 未修改 Hub 相關 code） - [x] WS route 註冊（行 97-102）沒動，所有 WS handler 繼續透過共用 `upgrader` 自動走新 `CheckOrigin` **順序微注意**：`CORSMiddleware` 掛在 `broadcasterLogger` 之後，代表 403 的 preflight 會被寫 access log。這對排障有幫助，不是問題，符合預期。 --- ## F. Build / Test / Race 結果 ``` $ cd server && go build ./... → PASS（無輸出） $ cd server && go vet ./... → PASS（無輸出） $ cd server && go test ./... → ALL PASS ok visiona-local/server/internal/api 0.921s ok visiona-local/server/internal/api/ws 0.585s ok visiona-local/server/internal/device ok visiona-local/server/internal/model $ cd server && go test -race ./... → ALL PASS（api 2.069s / ws 1.629s） ``` `go test -v -run 'TestIsAllowedOrigin|TestCORSMiddleware|TestCheckOrigin'` 所有 case 逐一 PASS，無任何 FAIL / SKIP。 --- ## G. Smoke test 結果 於 `127.0.0.1:3721`（本機已啟動 server，binary 為今日 go-build 產物）親跑： | # | curl | 預期 | 實測 | |---|------|------|------| | 1 | `GET /api/system/health -H "Origin: http://127.0.0.1:9999"` | 200 + ACA | ✅ 200 + `ACA-Origin: http://127.0.0.1:9999` + `Vary: Origin` + `ACA-Credentials: true` | | 2 | `POST /api/devices/scan -H "Origin: http://evil.com"` | 403 | ✅ 403，無 ACA header | | 3 | `OPTIONS /api/camera/start -H "Origin: http://127.0.0.1:9999" -H "Access-Control-Request-Method: POST"` | 204 + 完整 ACA | ✅ 204 + `ACA-Methods: GET,POST,PUT,DELETE,OPTIONS` + `ACA-Origin: http://127.0.0.1:9999` | | 4 | `POST /api/devices/scan -H "Origin: null"` | 403 | ✅ 403 | | 5 | `GET /api/system/health -H "Origin: http://evil.com"` | 200 **無** ACA | ✅ 200，response header 只有 content-type / date / length | | 6 | `POST /api/devices/scan -H "Origin: https://127.0.0.1:3721"` | 403 | ✅ 403（scheme mismatch） | 全部 6 條行為與 TDD §9 驗收表對應欄位完全吻合。 --- ## H. 安全檢查 - [x] IPv6 `http://[::1]:3721` parse 正確（hostname 回 `::1`，map 命中） - [x] Origin 大小寫不敏感（`strings.ToLower` 處理 `HTTP://127.0.0.1` 的 hostname 部分；但注意 `u.Scheme` 是小寫，因為 `url.Parse` 會 normalize scheme；若原字串為 `HTTP://...`，`u.Scheme` 仍回 `http`，測試 case `http://LOCALHOST:9999` PASS） - [x] Port 任意放行（map 只查 hostname） - [x] `ws://` / `wss://` **不支援**：測試 case `ws://127.0.0.1:3721` 明確斷言 false ✅。WebSocket 本身不用 Origin 比對 scheme（HTTP upgrade 的 Origin header 本來就是 http/https），所以這個行為正確。 - [x] hostname 完整 match：`127.0.0.1` ≠ `127.0.0.10`（map 查表天然安全），亦擋 `127.0.0.1.evil.com` - [x] IPv4 loopback 只允許 `127.0.0.1`（`127.0.0.2` 等非標準 loopback 不放行，符合 TDD） - [x] `u.Scheme != "http"` 擋掉 `https`（smoke 6 驗證） **無發現攻擊面。** --- ## I. 遺漏項 ### TDD §4.3 的「二道防線」`requireSameOriginOrNoOrigin` 未實作 TDD `v2/cors-security.md:210-244` 建議在 `/api` group 再掛一層 origin check 作為 defense-in-depth，但實作只有 CORSMiddleware（單層）。 **判讀**：TDD 本身在 §4.3 的開頭寫的是「額外掛一層 origin check，確保即使 CORSMiddleware 有 bug 也不會出事」，屬於 defensive bonus，而不是主線必須。§9 的驗收表格沒有列對應 case。因此： - **不算 Critical / Major** - 歸為 **Minor** 建議（見下方清單） ### 其他可優化但不阻擋 - `allowedHosts` map 同時有 `"[::1]"` 與 `"::1"` 兩個 key。`url.Parse("http://[::1]:3721").Hostname()` 只會回 `::1`（不帶方括號），所以 `"[::1]"` 是 dead entry。 - `ws/origin.go:36` 的 switch 同樣寫 `case "127.0.0.1", "localhost", "::1", "[::1]"`，`[::1]` 永不命中。 **不影響正確性**，但有誤導閱讀的風險。 --- ## J. 問題清單 ### Critical 無。 ### Major 無。 ### Minor | # | 檔案 | 行數 | 問題描述 | 建議修改方式 | |---|------|------|----------|-------------| | 1 | `server/internal/api/middleware.go` | 20 | `allowedHosts["[::1]"] = true` 為 dead entry，`url.Hostname()` 不會回傳帶方括號版本 | 移除 `"[::1]": true,` 這一行，並在 `::1` 同列加注釋「IPv6 loopback，`url.Hostname()` 會拆掉方括號」 | | 2 | `server/internal/api/ws/origin.go` | 36 | 同上，switch 的 `"[::1]"` case 永不命中 | 移除 `"[::1]"` case | | 3 | `server/internal/api/router.go` | 52 | 未實作 TDD §4.3 的 `requireSameOriginOrNoOrigin` 二道防線 | 若使用者 / Architect 要求 defense-in-depth，補上 `api.Use(requireSameOriginOrNoOrigin())`；否則在 TDD §4.3 加 note 說明「已評估，單層 CORSMiddleware 足夠，不做」 | ### Suggestion | # | 檔案 | 行數 | 建議內容 | |---|------|------|----------| | 1 | `server/internal/api/middleware.go` | 94 | `ACA-Methods` 固定 `GET, POST, PUT, DELETE, OPTIONS`，沒有 `HEAD` / `PATCH`。若未來有 PATCH handler 可能要補上。目前 router 無 PATCH 路由，不緊急 | | 2 | `server/internal/api/ws/origin.go` | 18-40 | 可考慮讓 `api.isAllowedOrigin` export 成 `api.IsAllowedOrigin`，再讓 `ws/origin.go` import；不過這會引入 `ws → api` 依賴，需評估層級。目前重複實作 + 注釋交代清楚也可接受 | --- ## K. 結論 **✅ 通過 Review，M8-8 可標記為完成，不阻擋 M8-10。** 實作品質摘要： - 邏輯嚴謹：URL parse + map 查表，天然抗 suffix attack；scheme 強制 `http`；大小寫不敏感 - 測試完整：19 + 7 + 9 = 35 個 assertion，含正向 / 反向 / 邊界 - 架構乾淨：`ws/origin.go` 獨立實作避免 package cycle，注釋清楚交代理由；所有 WS handler 共用一個 package 層級 `upgrader` 變數，單點維護 - 驗證紮實：build / vet / test / race 全部 PASS，6 條 smoke test 對齊 TDD §9 驗收表 建議下一輪（或與後續任務一併）處理的 Minor：移除 `[::1]` dead entry × 2、決定是否實作 §4.3 二道防線。皆非阻擋項。 --- **Reviewer**：Autoflow Reviewer Agent **日期**：2026-04-15 **審查輪次**：第 1 輪 **下游任務**：M8-10（不阻擋）