jim800121/visionA
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
visionA/local-tool/.autoflow/05-implementation/reviews/M8-8-review.md
jim800121chen 8cd5751ce3 feat(local-tool): M8 重構 — Wails 控制台 + 瀏覽器 Web UI（R5 決策） 
依 R5 五輪決策把 visionA-local 從「Wails 內嵌 Next.js」重構為「Wails
本機伺服器控制台 + 瀏覽器 Web UI」模式（類比 Docker Desktop / Ollama）。

程式碼變動
  - M8-1 砍 yt-dlp 全套（後端 resolver / URL handler / 前端 URL tab /
    Makefile vendor / installer / bootstrap / CI workflow，-555 行）
  - M8-2 砍 Mock 模式全套（driver/mock、mock_camera、Settings runtimeMode、
    VISIONA_MOCK 環境變數，-528 行）
  - M8-3 ffmpeg 從 GPL 切換到 LGPL 混合方案：Windows/Linux 用 BtbN 現成
    LGPL binary，macOS 自 build minimal decoder-only 進 git
    （vendor/ffmpeg/macos/ffmpeg 5.7MB + ffprobe 5.6MB，比 GPL 版省 85% 空間）
  - M8-4 Wails Server Controller：state machine、log ring buffer 2000 行、
    preferences.json atomic write、boot-id、Gin SkipPaths、shutdown 7+1 秒、
    notify_*.go 三平台 OS 通知、watchServer 改 Error state 不 os.Exit
  - M8-4b 啟動階段管線 R5-E：6 階段進度 event、20s soft / 60s hard timeout、
    stage 5/6 skip 規則、sentinel file、RestartStartupSequence 5 步驟
  - M8-5 Wails 控制台 vanilla HTML/JS/CSS（9 檔 ~2012 行）取代 M7-B splash：
    state 視覺、log panel、startup progress panel、Stage 6 manual CTA
    pulse、shutdown modal、Settings、Dark Mode、i18n 中英雙語
  - M8-6 上傳影片副檔名擴充（mp4/avi/mov/mpeg/mpg）
  - M8-7 Web UI Server Offline Overlay（role=alertdialog + focus trap +
    wsEverConnected 容錯 + Page Visibility）
  - M8-8 CORS middleware（127.0.0.1/localhost only + suffix attack 防護）+
    ws/origin.go 獨立 WebSocket CheckOrigin 避 package cycle
  - MAJ-4 server:shutdown-imminent WebSocket broadcast 機制
    （/ws/system endpoint + notifyShutdownImminent helper）
  - M8-9 Boot-ID + 瀏覽器 tab 自動重連（sessionStorage loop guard）

品質
  - ~105+ 新 unit test + race detector (-count=2) 全綠
  - 10 個 milestone 全部通過 Reviewer 審查
  - 三方 v2 + v2.1 文件（PRD / Design Spec / TDD）+ 交叉互審紀錄
    收錄在 .autoflow/

交付前待處理（M8-10）
  - 重跑 make payload-macos 把舊 GPL 77MB binary 換成新 LGPL
  - 三平台 end-to-end build 驗證

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-15 17:57:54 +08:00

203 lines
11 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# 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（不阻擋）
Reference in New Issue View Git Blame Copy Permalink