fix(server): resolve relative redirects and release intermediate responses - #1714
OskarEichler wants to merge 6 commits into
Conversation
Greptile SummaryThis PR makes the same-method/body HTTP helper resolve relative redirects, release intermediate responses, enforce explicit redirect failures, and apply a default response timeout.
Confidence Score: 4/5The PR appears safe to merge, with the non-blocking concern that its new redirect and cleanup contracts are not covered by automated tests. The changed implementation is compatible with the current replayable JSON caller and node-fetch runtime, while the remaining actionable issue is the absence of regression coverage for several newly introduced branches. Files Needing Attention: src/shadowbox/infrastructure/follow_redirects.ts
|
| Filename | Overview |
|---|---|
| src/shadowbox/infrastructure/follow_redirects.ts | Correctly adds relative redirect resolution, intermediate-body cleanup, explicit failure handling, and a default timeout, but the new behavior lacks automated regression coverage. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Send request with manual redirects] --> B{Standard redirect status?}
B -- No --> C[Return final response]
B -- Yes --> D[Destroy intermediate body]
D --> E{Location present?}
E -- No --> F[Throw missing Location error]
E -- Yes --> G[Resolve Location against current URL]
G --> H{Redirect limit reached?}
H -- No --> A
H -- Yes --> I[Throw too many redirects error]
Reviews (1): Last reviewed commit: "fix(server): resolve relative redirects ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (let i = 0; i < 10; i++) { | ||
| response = await fetch(url, manualRedirectOptions); | ||
| if (response.status >= 300 && response.status < 400) { | ||
| url = response.headers.get('location'); | ||
| } else { | ||
| break; | ||
| const response = await fetch(url, manualRedirectOptions); | ||
| if (![301, 302, 303, 307, 308].includes(response.status)) { | ||
| return response; | ||
| } | ||
| const location = response.headers.get('location'); | ||
| // Intermediate bodies are never read. Release the socket before redirecting. | ||
| (response.body as Readable).destroy(); | ||
| if (!location) { | ||
| throw new Error('Redirect response is missing a Location header'); | ||
| } | ||
| url = new URL(location, url).toString(); | ||
| } | ||
| return response; | ||
| throw new Error('Too many redirects'); |
There was a problem hiding this comment.
The new relative-resolution, intermediate-body cleanup, timeout, missing-Location, and redirect-limit branches have no automated coverage; the shared-metrics specs use a fake collector and never exercise this helper, leaving these changed contracts vulnerable to undetected regressions.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Added direct coverage for relative redirects, method/body preservation, missing Location, redirect limits, and timeouts. I also fixed two test calls that omitted the required options argument. TypeScript compilation and the focused redirect suite pass: 4 specs, 0 failures. |
This branch has not been deployed
1 participant
Why
The explicit same-method/body redirect helper does not resolve relative locations, release unread intermediate bodies, or clearly reject missing/exhausted redirect chains.
Changes
Locationagainst the current URL for standard redirect statuses.Verification
After splitting, all 65 existing manager-metrics, shared-metrics, access-key, and file-operation specs passed again on this isolated branch under Node 22.23.2.
Controlled local HTTP checks passed for relative POST redirects, preservation of method/body, missing Location, and redirect loops.
Targeted semantic TypeScript and formatting checks passed.
Patch isolation and
git diff --checkpassed. Recombining all focused patches reproduces the reviewed source changes exactly.No test/spec files were added or modified; controlled probe drivers are kept outside the repository.
Compatibility and remaining validation
Full npm installation/build checks remain incomplete: npm 12 rejected existing lockfile Git dependencies (
EALLOWGIT). Isolated registry-only tooling was used without disabling that safeguard.Scope
This is one focused part of the reliability review, based directly on upstream
master; it does not include the other review patches. No installed client, live VPN/DNS setting, or production service was changed by this patch.Second review — 2026-08-27
A socket-level regression check showed that destroying node-fetch 2's proxy body did not close the underlying HTTP connection when a redirect response never ended. Use a per-hop AbortController, forward caller cancellation, abort intermediate requests explicitly, and abort final requests when their returned body closes/errors. Remove forwarded signal listeners on completion/failure.
Actual local sockets now close after intermediate/final disposal, caller cancellation before/after headers, missing Location, and timeouts. Listener cleanup is checked as well as POST/body preservation and bounded redirect loops. These checks pass independently and in combination with the reporting PR. All 65 selected existing server specs and targeted TypeScript checks passed on this branch.
All touched files and nearby callers were reviewed again. Each focused patch was also checked in combination with the other seven patches for this repository. External probe drivers remain outside the repositories; no test/spec files were added or modified. Existing full-build and platform-validation limitations above still apply.
Security review follow-up — 2026-09-03
Cross-origin redirects are not allowed.git diff --checkpass.