Skip to content

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Jul 9, 2025

No description provided.

@anonrig anonrig requested review from a team as code owners July 9, 2025 18:38
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR prevents automatic redirect following in the Node HTTP client by using manual redirect mode, adds a corresponding test for this behavior, and updates test configurations to allow public network access.

  • Configure fetch to use redirect: 'manual' in the internal HTTP client
  • Add a new test (httpRedirectsAreNotFollowed) to assert a 301 response is returned
  • Update network permissions in the wd-test and Bazel rule to enable external HTTP requests

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/workerd/api/node/tests/http-client-nodejs-test.wd-test Expand “internet” environment to allow public network access
src/workerd/api/node/tests/http-client-nodejs-test.js Add httpRedirectsAreNotFollowed test using cloudflare.com
src/workerd/api/node/BUILD.bazel Tag the test rule with requires-network
src/node/internal/internal_http_client.ts Add redirect: 'manual' to fetch and adjust comments
Comments suppressed due to low confidence (2)

src/workerd/api/node/tests/http-client-nodejs-test.js:311

  • [nitpick] For consistency with other tests (e.g. testHttpSetTimeout), consider renaming this constant to testHttpRedirectsAreNotFollowed.
export const httpRedirectsAreNotFollowed = {

src/node/internal/internal_http_client.ts:345

  • [nitpick] The comment numbering is now out of sync after removing the redirect note. You may want to renumber or simplify the list to keep documentation accurate.
    // 2. Nothing is directly waiting for fetch promise here.

@anonrig anonrig enabled auto-merge (squash) July 9, 2025 19:09
@anonrig anonrig merged commit 7c9f73b into main Jul 9, 2025
21 checks passed
@anonrig anonrig deleted the yagiz/fix-fetch-redirect branch July 9, 2025 19:09
{
port: 80,
method: 'GET',
protocol: 'http:',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the copilot comment below was somehow relevant.

Can't we create a local redirect server? in the same way as we already have

      'PONG_SERVER_PORT',
      'ASD_SERVER_PORT',
      'TIMEOUT_SERVER_PORT',
      'HELLO_WORLD_SERVER_PORT',
      'HEADER_VALIDATION_SERVER_PORT',

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants