[WRONG BRANCH] Enforce absolute first-byte/header deadline for pinned HTTP requests - #298
[WRONG BRANCH] Enforce absolute first-byte/header deadline for pinned HTTP requests#298luvs01 wants to merge 1 commit into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Its title has been prefixed with |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe pinned HTTP client now starts first-byte timeout tracking for legacy and non-legacy socket configurations. A Bun integration test verifies timeout enforcement during incomplete response headers. ChangesHTTP timeout enforcement
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR adds an absolute first-byte/header timeout while preserving legacy idle-timeout behavior; no actionable merge-blocking risk remains at the current head. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db6114db0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const connectedEvent = parsed.protocol === "https:" ? "secureConnect" : "connect"; | ||
| if (!socket.connecting) { | ||
| if (!usesLegacyIdleTimeout) startFirstByteTimer(); | ||
| startFirstByteTimer(); |
There was a problem hiding this comment.
Preserve zero as the disabled legacy timeout
When a caller passes idleTimeoutMs: 0 to disable the legacy socket timeout, firstByteTimeoutMs also becomes zero and this unconditional call schedules an immediate JavaScript timer. Consequently, any response whose headers arrive on a later turn is rejected with first_byte_timeout; before this commit, req.setTimeout(0) and response.setTimeout(0) disabled the timeout and the same request succeeded. Preserve the established zero-timeout behavior by skipping the absolute timer when the inherited legacy value is zero (or otherwise normalize disabled legacy timeouts).
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
Motivation
idleTimeoutMsrelied on a resettable socket idle timeout and could be kept open by an attacker dripping partial response headers.Description
req.setTimeout(legacyIdleTimeoutMs)behavior so callers that rely on the idle socket timeout keep their existing compatibility semantics.tests/pinned-http.test.tsthat spins a local server which drips an unterminated header to verify the request now fails with a timeout instead of hanging indefinitely.src/lib/pinned-http.tsand addedtests/pinned-http.test.ts.Testing
bun test tests/pinned-http.test.ts tests/images/pinned-https-get.test.tsand the focused tests passed.bun run typecheckandbun run privacy:scanand both succeeded.bun run testwas attempted but produced unrelated environment-sensitive failures and timeouts, so verification relied on the focused tests and typecheck/privacy scan above.Codex Task
Summary by CodeRabbit
Bug Fixes
Tests