feat: serve Windows PE blobs from Winbindex + Microsoft symbol server - #72
Conversation
Add a fast path in front of GET /blob/:hash: when the request carries ?filename= for a Windows PE file (.exe/.dll/.sys), resolve the file on Winbindex by content SHA-1 and stream verified bytes straight from Microsoft's public symbol server instead of proxying MinIO, so the public corpus never has to re-host Windows binaries. - src/winbindex.ts: isWindowsPEFilename, symbolServerUrl, resolveEntry (24h bounded in-memory cache of the per-filename index, negative-caches 404s), streamFromSymbolServer (on-the-fly SHA-1 verification, destroys the response on a post-send mismatch), tryServeFromWinbindex orchestrator. - rest-routes: fifth createRestRouter arg WinbindexConfig; the handler tries Winbindex first and falls through to the unchanged MinIO logic on every miss/failure except once bytes have been streamed. - index.ts: WINBINDEX_ENABLED / _DATA_URL / _SYMBOL_SERVER_URL / _FETCH_TIMEOUT_MS env vars, all optional with defaults. MinIO stays a pure GetObject proxy; nothing is ever written back. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NmATMvFbdupfLRq7Ldwabv
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NmATMvFbdupfLRq7Ldwabv
…headers Final whole-branch review fixes for the Winbindex blob source fast path: - Reject non-plain `filename` values (SAFE_PE_NAME) at the top of tryServeFromWinbindex, before any URL/cache/header work. Closes a blind single-host SSRF: `?`, `#` and percent-encoded segments survive basename() and previously flowed unencoded into the outbound Winbindex/symbol-server URLs. One gate makes the cache key, both URLs and Content-Disposition safe by construction. - Honour res.write() backpressure in streamFromSymbolServer: await `drain` (bailing on client `close`/`error`) instead of letting Node buffer the whole PE for a slow client. BlobResponse gains once()/off(). - Stream error before the first byte now returns not_available (MinIO fallback) instead of destroying the response; only a failure after bytes were sent yields failed_after_send + res.destroy(). Headers are set lazily on the first chunk so a pre-first-byte failure leaves the response pristine. - Forward upstream Content-Length only when the body is not content-encoded (undici may have transparently decompressed). - Set ETag: "<hash>" on the winbindex-served response, for parity with MinIO. - Widen tryServeFromWinbindex's try/catch to cover streamFromSymbolServer, matching its never-throws docblock. - tests/winbindex.test.ts: 23 -> 35 cases covering all the above, plus JSON-cache TTL expiry and the 200-entry eviction bound. console.warn/error stubbed; output pristine. - docs: blob-api.md query-parameter section, integrate-blob-api.md sample now sends ?filename=, winbindex-source.md negative-cache caveat + updated flow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NmATMvFbdupfLRq7Ldwabv
There was a problem hiding this comment.
🟡 Changes recommended
There are unresolved operational/performance issues in the new Winbindex path (cache policy mismatch and potential indefinite drain wait / event-loop blocking decompression) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a Winbindex-backed fast path to the existing /blob/:hash REST endpoint so Windows PE blobs can be served by proxying verified bytes from Microsoft’s public symbol server (using ?filename=), reducing the need to re-host Windows binaries while keeping MinIO as the default/fallback source.
Changes:
- Introduces
src/winbindex.tsto resolvehash + filenamevia Winbindex and stream+SHA1-verify bytes from the symbol server with backpressure handling and caching. - Integrates the Winbindex fast path into
GET /blob/:hash, plus adds env-driven configuration insrc/index.ts. - Adds comprehensive Jest coverage and documentation updates for the new
filenamequery parameter and Winbindex behavior.
File summaries
| File | Description |
|---|---|
| tests/winbindex.test.ts | Adds test coverage for Winbindex resolution, caching, streaming, and failure modes. |
| src/winbindex.ts | Implements Winbindex lookup, caching, and symbol-server streaming with on-the-fly SHA-1 verification. |
| src/rest-routes.ts | Hooks Winbindex fast path into the blob download handler before the MinIO/S3 path. |
| src/index.ts | Adds envalid configuration and wires Winbindex config into the REST router. |
| README.md | Documents new optional Winbindex environment variables. |
| docs/reference/winbindex-source.md | Adds reference documentation for the Winbindex blob source behavior and failure modes. |
| docs/reference/blob-api.md | Documents the new optional filename query parameter and its semantics. |
| docs/how-to/integrate-blob-api.md | Updates example client integration to pass filename when available. |
| CLAUDE.md | Updates repository overview docs to mention the Winbindex fast path. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses Copilot review feedback on #72: - The per-filename index cache now refreshes recency on read, so eviction is genuinely least-recently-used rather than least-recently-inserted; a hot filename survives churn from 200 other lookups. - Index decompression moves off the event loop (async gunzip instead of gunzipSync), so a multi-MB index cannot stall unrelated traffic. - waitForDrain now rejects after WINBINDEX_FETCH_TIMEOUT_MS, so a socket that never emits drain/close/error tears the transfer down instead of hanging the request handler and holding the upstream reader open. +2 tests (LRU recency, drain timeout -> failed_after_send); 37/37 in tests/winbindex.test.ts, full suite 89 passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NmATMvFbdupfLRq7Ldwabv
There was a problem hiding this comment.
🔵 Needs a closer look
There are a few concrete operational/test/doc issues to address (canceling unread fetch response bodies, restoring global fetch in tests, and a blob-api doc mismatch).
Review details
Suppressed comments (5)
Previously missed (3) — in code that hasn't changed since the last review.
src/winbindex.ts:183
- When the Winbindex index response is a 404, the code returns without consuming/canceling the response body; with undici/Node fetch this can keep the connection/body open longer than necessary and contribute to resource leaks under repeated misses. Best-effort cancel the body before returning.
This issue also appears in the following locations of the same file:
- line 184
- line 316
tests/winbindex.test.ts:188
- The test overrides globalThis.fetch in beforeEach but never restores it, which can leak mocks across test files and make future tests order-dependent. Save the original fetch once and restore it in afterEach.
docs/reference/blob-api.md:23 - Documentation mismatch: the server lowercases the basename before validating against
[a-z0-9._+-], so filenames containing uppercase letters (e.g.KERNEL32.DLL) are still accepted for the Winbindex fast path. The docs currently imply anything outside the lowercase set is ignored.
src/winbindex.ts:186
- On non-OK Winbindex responses (non-404), the response body is not consumed/canceled before returning. Canceling the body avoids leaving unread data attached to the undici connection pool.
if (!response.ok) {
return null;
}
src/winbindex.ts:318
- If the symbol server responds non-2xx, the code returns without canceling the response body. Best-effort cancel the body to avoid keeping unread response data/sockets around in the undici pool.
if (!upstream.ok || !upstream.body) {
return "not_available";
}
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
Second round of Copilot review feedback on #72: - fetchIndex and streamFromSymbolServer now best-effort cancel the response body on a 404 / non-OK / non-2xx return, so undici does not keep the socket and unread data in its pool under repeated misses. - winbindex.test.ts restores the real globalThis.fetch in afterEach instead of leaving the mock installed for later test files. - blob-api.md: clarify that the filename match is case-insensitive (the basename is lowercased before the [a-z0-9._+-] check), so KERNEL32.DLL is accepted. winbindex.test.ts 37/37, full suite 89 passing, build + ccode clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NmATMvFbdupfLRq7Ldwabv
There was a problem hiding this comment.
🟡 Changes recommended
The symbol-server streaming error path should cancel the upstream fetch body to avoid background downloads/sockets being held after failures/timeouts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
docs/reference/blob-api.md:23
- The docs say that path separators in
filenameskip the Winbindex fast path, but the implementation explicitly takesbasename(filename)first, sofilename=a/b/kernel32.dllwill still engage (usingkernel32.dll). Update this description to match the actual behavior.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
Third round of Copilot review feedback on #72: - streamFromSymbolServer cancels the upstream body reader in a finally block, so a mid-stream error or drain timeout aborts the symbol-server download instead of letting it run to completion in the background holding a socket. - blob-api.md: the server takes basename(filename) first, so a path like windows/system32/KERNEL32.DLL resolves as kernel32.dll and still engages the fast path; separators do not skip it. Corrected. Test asserts the upstream stream is cancelled on a drain timeout. winbindex.test.ts 37/37, full suite 89 passing, build + ccode clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NmATMvFbdupfLRq7Ldwabv
There was a problem hiding this comment.
🟡 Changes recommended
The Winbindex streaming path destroys the response and blocks MinIO fallback on a SHA-1 mismatch even when zero bytes were written to the client (zero-byte upstream body edge case).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
| const digest = hash.digest("hex"); | ||
| if (digest !== expectedSha1.toLowerCase()) { | ||
| console.warn( | ||
| `Winbindex: SHA-1 mismatch for ${name} (expected ${expectedSha1.toLowerCase()}, got ${digest}), destroying response`, | ||
| ); | ||
| res.destroy(); | ||
| return "failed_after_send"; | ||
| } |
The digest-mismatch branch in streamFromSymbolServer destroyed the response and returned failed_after_send even when zero bytes had reached the client (a symbol server 200 with an empty body for a hash that is not the empty-file hash). The response is still pristine in that case, so it now mirrors the stream-error branch and returns not_available, letting the request fall through to MinIO. Found by Copilot on #72 after merge. winbindex.test.ts 38/38. Claude-Session: https://claude.ai/code/session_01NmATMvFbdupfLRq7Ldwabv Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
What
Adds a Winbindex-backed fast path to
GET /blob/:hashfor Windows PE files (.exe/.dll/.sys). When the request carries a?filename=, the API resolves the file on Winbindex and streams verified bytes from Microsoft's public symbol server (msdl.microsoft.com), falling back to MinIO on any miss or failure. Callers that pass nofilenameare byte-for-byte unaffected.This lets the public corpus ship the Neo4j graph and metadata without re-hosting (or redistributing) Windows binaries. It is the same "point at Microsoft's own servers, don't re-host" model the
symbolsplugin already uses for PDBs.How it resolves
?filename=or extension not.exe/.dll/.sys→ MinIO, no external call./^[a-z0-9._+-]{1,255}$/(rejects path/query steering; one gate makes the cache key, both outbound URLs andContent-Dispositionsafe by construction).by_filename_compressed/<name>.json.gz), in-memory LRU cache (200 entries, 24h TTL, 404 → negative sentinel).fileInfo.sha1equals the requested blob hash (neogit blob hash is the raw file SHA-1).<symbol server>/<name>/<TimeDateStamp><SizeOfImage>/<name>,GETwithUser-Agent: Microsoft-Symbol-Server/10.0.0.0.res.write()+ drain backpressure, verifying SHA-1 on the fly. Post-stream mismatch → destroy the connection (failed download), no MinIO retry. Any earlier miss/failure → MinIO fallback.MinIO is never written to — pure proxy,
GetObjectonly.Config (envalid, all optional)
WINBINDEX_ENABLEDtrueWINBINDEX_DATA_URLhttps://winbindex.m417z.com/data/by_filename_compressedWINBINDEX_SYMBOL_SERVER_URLhttps://msdl.microsoft.com/download/symbolsWINBINDEX_FETCH_TIMEOUT_MS15000Tests
tests/winbindex.test.ts— 35 cases,fetchmocked, real gzip/stream/SHA-1 round trips: PE hit served + verified, entry withoutsha1→ MinIO, non-PE / no-filename → MinIO with no external call, unsafe filename → rejected with no fetch, symbol-server 500 → MinIO, pre-stream error → MinIO, post-stream mismatch → failed + destroyed,Content-Lengthdropped on gzip,ETagset, JSON cache hit, TTL expiry, 200-entry eviction.Full suite: 87 passing. (
tests/git-log/index.test.tsfails to compile on a pre-existingTS1378unrelated to this branch — present onmaster, untouched here.)Companion
Needs OSWatcher/frontend#60 so the web UI sends
?filename=.Reviewed
Built with subagent-driven development: per-task review, whole-branch review, one fix wave. One Low finding parked:
waitForDrainhas no timeout guard (not remotely inducible; one stuck request bounded by rate limiting).Docs: new
docs/reference/winbindex-source.md;docs/reference/blob-api.mdanddocs/how-to/integrate-blob-api.mdupdated for thefilenameparam.🤖 Generated with Claude Code
https://claude.ai/code/session_01NmATMvFbdupfLRq7Ldwabv