P1-2: localhost APIに認可トークン/Origin-Host検証/magic byte/上限/レート制限を追加 - #15
Conversation
同じPC上の別アプリや悪意あるWebページから、勝手にジョブを起動されたり 成果物を盗まれたりしないよう、server/security.mjs を新設して5点を実装。 - (A) 起動時トークン: generateStartupToken()。index.html配信時にのみ window.__KOSESPARK_TOKEN__として埋め込む(別オリジンはレスポンス本文を 読めないため盗めない) - (B) Origin/Host検証: isAllowedHost/isAllowedOrigin(DNS rebinding・ 他オリジンからのfetch/XHRを拒否) - (C) magic byte検証: looksLikeVideo(MP4/WebM/AVI/MPEG-TSのシグネチャ)。 該当しなければ415 - (D) アップロード上限: MAX_UPLOAD_BYTES(500MB)。宣言Content-Lengthの 事前チェックと受信バイト数の実測(宣言が無い/嘘の場合の保険)の両方で 強制し413 - (E) レート制限: createRateLimiter(固定ウィンドウ・60秒10回)。超過は429 server/index.mjsの全/api/*入口でトークン+Origin/Host検証を必須化。 webapp-mockup/app.jsをトークン付き呼び出し(fetchはヘッダ、EventSource/ ダウンロードリンクはカスタムヘッダを付けられないためクエリ)へ更新。 tests/smoke.mjsにP1-2-A〜E契約テスト8件を追加(計39 PASS)。ローカルで 実サーバーを起動し、curlで無トークン401・不正Origin401・非動画415・ 正常202を実地確認済み。 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01For7g76mPTJAQSBTfYBrwU
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds localhost-focused API hardening with per-startup token authorization, Host/Origin validation, upload signature and size checks, rate limiting, client token propagation, smoke tests, and roadmap updates marking the security controls complete. ChangesSecurity Hardening
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
CI(ci-green/roadmap-required/CodeQL)全て緑を確認し、P1-2-A〜Eの evidenceに該当CI run URLを記入してdone化。 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01For7g76mPTJAQSBTfYBrwU
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
video-shorts/server/index.mjs (2)
140-151: 🔒 Security & Privacy | 🔵 TrivialConsider a Referrer-Policy header given token-in-URL usage.
The startup token also travels via
?token=for EventSource/clip-download URLs (perapp.js'swithTokenQuery), since custom headers aren't possible there. That's a reasonable workaround for the API limitation, but it does mean the token could end up in aRefererheader if the page later loads any external resource. SettingReferrer-Policy: no-referrer(orsame-origin) on responses would close that residual leakage path at low cost.🤖 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 `@video-shorts/server/index.mjs` around lines 140 - 151, Update the response headers in the index.html handling within the request-serving flow to include a restrictive Referrer-Policy, preferably no-referrer, alongside Content-Type and Content-Length. Ensure the policy is applied to the token-bearing page response without changing the token injection behavior.
207-213: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMagic-byte check only inspects the first
dataevent's chunk.
checkedSignatureis only evaluated once, on whatever bytes happen to arrive in the firstreq.on("data")callback. If that first chunk is smaller than the minimum bytes a signature needs (e.g., under 8 bytes for the MP4ftypcheck), a legitimate video could be falsely rejected with 415 purely due to TCP segmentation, independent of file validity.Consider buffering a minimum number of bytes (e.g., 16) across chunks before running
looksLikeVideo.🤖 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 `@video-shorts/server/index.mjs` around lines 207 - 213, Update the request data handling around checkedSignature and looksLikeVideo to accumulate bytes across data chunks until at least the minimum signature length (for example, 16 bytes) is available, then perform the magic-byte check once. Preserve byte counting, rejection handling, and the existing 415 abort for invalid signatures.
🤖 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 `@video-shorts/server/index.mjs`:
- Around line 200-232: The abort handler in the upload streaming flow must
prevent rejected request bytes from remaining on a keep-alive connection. Update
abort to drain the request body or explicitly close the HTTP connection by
setting Connection: close and terminating the request/socket, while preserving
the existing 413/415 response and cleanup behavior.
In `@video-shorts/server/security.mjs`:
- Around line 46-69: The MPEG-TS predicate in VIDEO_SIGNATURES currently accepts
any buffer beginning with 0x47; require repeated 0x47 sync bytes at 188-byte
intervals, checking at least two or three points while preserving the existing
safe false result for short or invalid buffers. In
video-shorts/server/security.mjs lines 46-69 update this predicate; in
video-shorts/tests/smoke.mjs lines 404-413 add positive AVI and strengthened
MPEG-TS cases plus a negative case for non-TS data beginning with 0x47.
---
Nitpick comments:
In `@video-shorts/server/index.mjs`:
- Around line 140-151: Update the response headers in the index.html handling
within the request-serving flow to include a restrictive Referrer-Policy,
preferably no-referrer, alongside Content-Type and Content-Length. Ensure the
policy is applied to the token-bearing page response without changing the token
injection behavior.
- Around line 207-213: Update the request data handling around checkedSignature
and looksLikeVideo to accumulate bytes across data chunks until at least the
minimum signature length (for example, 16 bytes) is available, then perform the
magic-byte check once. Preserve byte counting, rejection handling, and the
existing 415 abort for invalid signatures.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bdab59e7-bd60-490c-ae55-48c5b870ab9b
📒 Files selected for processing (5)
docs/roadmap.htmlvideo-shorts/server/index.mjsvideo-shorts/server/security.mjsvideo-shorts/tests/smoke.mjsvideo-shorts/webapp-mockup/app.js
| const abort = (status, message) => { | ||
| rejection = { status, message }; | ||
| req.pause(); | ||
| ws.destroy(); | ||
| reject(new Error(message)); | ||
| }; | ||
|
|
||
| req.on("data", (chunk) => { | ||
| if (rejection) return; | ||
| receivedBytes += chunk.length; | ||
| if (!checkedSignature) { | ||
| checkedSignature = true; | ||
| if (!looksLikeVideo(chunk)) return abort(415, "動画ファイルとして認識できません"); | ||
| } | ||
| if (receivedBytes > MAX_UPLOAD_BYTES) return abort(413, "ファイルが大きすぎます"); | ||
| ws.write(chunk); | ||
| }); | ||
| req.on("end", () => { | ||
| if (!rejection) ws.end(); | ||
| }); | ||
| req.on("error", (e) => { | ||
| if (!rejection) reject(e); | ||
| }); | ||
| ws.on("finish", resolve); | ||
| ws.on("error", (e) => { | ||
| if (!rejection) reject(e); | ||
| }); | ||
| }); | ||
| } catch (e) { | ||
| fs.rmSync(workDir, { recursive: true, force: true }); | ||
| if (rejection) return jsonRes(res, rejection.status, { error: rejection.message }); | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Node.js http server: does destroying the request (req.destroy()) also close the response socket before res.end() flushes?
💡 Result:
Yes, calling req.destroy on a Node.js HTTP server request effectively destroys the underlying socket associated with the HTTP transaction [1]. Because the request and response share the same underlying socket, destroying the request closes that socket, which immediately terminates any pending response activity [2][3][1]. When req.destroy is invoked: 1. The socket is destroyed [4][5]. 2. Any pending data in the response stream is dropped [4][5]. 3. The response does not continue to flush or complete; the communication channel is severed [2][1]. As a result, if you call req.destroy before res.end has completed its work, the remaining buffered response data will not be sent, and the response process will be aborted [4][2]. In Node.js, this is treated as a connection-level action, and it is generally recommended to assume that the response is also destroyed once req.destroy is called [3].
Citations:
- 1: https://www.thenodebook.com/http/http-server-lifecycle
- 2: http: req.destroy coupling nodejs/node#32878
- 3: Stream finished does not always work with http incoming message nodejs/node#38657
- 4: https://nodejs.org/api/http.html
- 5: https://nodejs.org/dist/latest/docs/api/http.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant region with line numbers.
ast-grep outline video-shorts/server/index.mjs --view expanded || true
echo "----"
sed -n '150,280p' video-shorts/server/index.mjs | cat -nRepository: rahiseko-alt/ai-editer
Length of output: 5660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect response helper and server setup to see whether error responses keep the connection alive.
sed -n '90,140p' video-shorts/server/index.mjs | cat -n
echo "----"
sed -n '328,430p' video-shorts/server/index.mjs | cat -nRepository: rahiseko-alt/ai-editer
Length of output: 5286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '90,140p' video-shorts/server/index.mjs | cat -n
echo "----"
sed -n '328,430p' video-shorts/server/index.mjs | cat -nRepository: rahiseko-alt/ai-editer
Length of output: 5286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any explicit connection-close or request-destroy handling elsewhere in the server.
rg -n "destroy\(|Connection|keepAlive|setHeader\(\"Connection\"|req\.pause|req\.resume|unpipe" video-shorts/server/index.mjsRepository: rahiseko-alt/ai-editer
Length of output: 245
🌐 Web query:
Node.js HTTP server unread request body keep-alive pause unread bytes socket reuse
💡 Result:
In a Node.js HTTP server using keep-alive connections, failing to read or "drain" a request body can disrupt socket reuse and lead to connection hangs or corruption [1][2][3]. When an HTTP request includes a body and the server handler sends a response without fully reading that body, the unconsumed bytes remain in the TCP stream [1]. Because the server must maintain a clean state for the next pipelined request on the same keep-alive connection, these leftover bytes cause the Node.js HTTP parser to encounter data unexpectedly [1][3]. This can lead to the parser failing to identify the next request correctly, often resulting in the server resetting the connection (ECONNRESET) or effectively losing any subsequent requests sent on that same connection [1][4][2]. Key technical aspects include: 1. Backpressure and Pause: Node.js buffers incoming body data in the IncomingMessage object. If the application does not consume this stream, the buffer reaches its highWaterMark, and Node.js will pause the underlying socket to prevent memory exhaustion [1][3][5]. This pause stops further data from arriving, but it does not clear the data already buffered or the bytes still sitting in the socket's receive buffer [1][3]. 2. Socket Reuse Requirements: To reuse a socket for keep-alive, the server must reach a "clean" message end—meaning the entire request (headers and body) must be processed and accounted for [2][3]. If an application rejects a request (e.g., sends a 401 or 413 response) without consuming the body, it is a best practice to explicitly drain the remaining body bytes by calling req.resume or by attaching a data listener to consume and discard the stream [1][6][3]. 3. Connection Destruction: Many frameworks and developers choose to explicitly destroy the socket when a request is rejected and the body is unread [4][2]. This avoids the risk of data corruption on the next request, though it negates the performance benefits of keep-alive for that specific connection [2][3]. In summary, leaving a request body unread creates a discrepancy between the application's state and the TCP stream's state [3]. To ensure reliable keep-alive behavior, application code must either fully consume the request body or explicitly close the connection [1][3].
Citations:
- 1: Keep-alive connections hang when request bodies are not drained in adapter-node sveltejs/kit#15526
- 2: https://www.thenodebook.com/http/proxies-static-streaming
- 3: https://www.thenodebook.com/http/http-server-lifecycle
- 4: hono/body-limit middleware fails to return response if file is too large. honojs/node-server#327
- 5: node:http: emit 'pause' on req.socket once an unread body fills the IncomingMessage buffer oven-sh/bun#34740
- 6: https://stackoverflow.com/questions/25845733/is-it-necessary-to-call-end-on-a-received-http-request-in-node
Reject the upload by closing the connection. req.pause() leaves the rejected body unread on a keep-alive socket, so the 413/415 response can be followed by leftover bytes that break the next request on that connection. Drain the body or mark the response Connection: close and terminate the socket for this path.
🤖 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 `@video-shorts/server/index.mjs` around lines 200 - 232, The abort handler in
the upload streaming flow must prevent rejected request bytes from remaining on
a keep-alive connection. Update abort to drain the request body or explicitly
close the HTTP connection by setting Connection: close and terminating the
request/socket, while preserving the existing 413/415 response and cleanup
behavior.
| /** 既知の動画コンテナのmagic byteだけを許可する(C)。誤検知よりも「本物の動画ファイルのはず」を優先。 */ | ||
| const VIDEO_SIGNATURES = [ | ||
| // MP4/MOV系: バイト4-7が "ftyp" | ||
| (buf) => buf.length >= 8 && buf.subarray(4, 8).toString("ascii") === "ftyp", | ||
| // WebM/Matroska (EBML header) | ||
| (buf) => buf.length >= 4 && buf[0] === 0x1a && buf[1] === 0x45 && buf[2] === 0xdf && buf[3] === 0xa3, | ||
| // AVI (RIFF....AVI ) | ||
| (buf) => | ||
| buf.length >= 12 && | ||
| buf.subarray(0, 4).toString("ascii") === "RIFF" && | ||
| buf.subarray(8, 12).toString("ascii") === "AVI ", | ||
| // MPEG-TS (先頭が同期バイト0x47。188バイト境界で複数回現れるのが正だが、先頭のみ簡易確認) | ||
| (buf) => buf.length >= 1 && buf[0] === 0x47, | ||
| ]; | ||
|
|
||
| export function looksLikeVideo(buf) { | ||
| return VIDEO_SIGNATURES.some((check) => { | ||
| try { | ||
| return check(buf); | ||
| } catch (_) { | ||
| return false; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
MPEG-TS magic-byte check is a near-noop (single byte) and untested. looksLikeVideo's MPEG-TS signature only checks that the first byte equals 0x47, which arbitrary non-video content can trivially satisfy, and the smoke tests never exercise this branch (or the AVI branch) to catch that weakness.
video-shorts/server/security.mjs#L46-L69: strengthen the MPEG-TS check to verify the0x47sync byte repeats at the 188-byte boundary (check at least 2-3 sync points) instead of a single leading byte.video-shorts/tests/smoke.mjs#L404-L413: add positive cases for AVI and (the strengthened) MPEG-TS signatures, plus a negative case showing that a non-TS file starting with0x47is now correctly rejected.
📍 Affects 2 files
video-shorts/server/security.mjs#L46-L69(this comment)video-shorts/tests/smoke.mjs#L404-L413
🤖 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 `@video-shorts/server/security.mjs` around lines 46 - 69, The MPEG-TS predicate
in VIDEO_SIGNATURES currently accepts any buffer beginning with 0x47; require
repeated 0x47 sync bytes at 188-byte intervals, checking at least two or three
points while preserving the existing safe false result for short or invalid
buffers. In video-shorts/server/security.mjs lines 46-69 update this predicate;
in video-shorts/tests/smoke.mjs lines 404-413 add positive AVI and strengthened
MPEG-TS cases plus a negative case for non-TS data beginning with 0x47.
2 participants
Summary
video-shorts/server/security.mjsを新設し、localhost APIのハードニングを一元化generateStartupToken()。index.html配信時にのみwindow.__KOSESPARK_TOKEN__として埋め込む(別オリジンはレスポンス本文を読めないため盗めない)isAllowedHost/isAllowedOrigin(DNS rebinding・他オリジンからのfetch/XHRを拒否)looksLikeVideo(MP4/WebM/AVI/MPEG-TSのシグネチャ)。該当しなければ415MAX_UPLOAD_BYTES(500MB)。宣言Content-Lengthの事前チェックと受信バイト数の実測(宣言が無い/嘘の場合の保険)の両方で強制し413createRateLimiter(固定ウィンドウ・60秒10回)。超過は429server/index.mjsの全/api/*入口でトークン+Origin/Host検証を必須化。POST /api/jobsにレート制限・アップロード上限・magic byte検証を適用webapp-mockup/app.jsをトークン付き呼び出しへ更新(fetchはヘッダ、EventSource/ダウンロードリンクはカスタムヘッダを付けられないためクエリ)docs/roadmap.htmlのP1-2配下(原子ツリー)を更新Test plan
pnpm -r --if-present typecheck(該当パッケージ無し)pnpm -r --if-present lint(該当パッケージ無し)pnpm -r test(video-shorts:tests/smoke.mjs39 PASS +transcribe-corrections-check.py5 PASS)pnpm -r --if-present buildpnpm audit --audit-level moderate(既知の脆弱性なし)node scripts/verify-roadmap-evidence.mjs(roadmap evidence リンタ OK)CIが緑になり次第、
docs/roadmap.htmlのP1-2-A〜EのevidenceにCI run URLを記入する追いコミットを行います。Generated by Claude Code
Summary by CodeRabbit