8000 [Bug]: net.fetch fails on manual redirect · Issue #43715 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Bug]: net.fetch fails on manual redirect #43715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
3 tasks done
chrmarti opened this issue Sep 13, 2024 · 14 comments
Open
3 tasks done

[Bug]: net.fetch fails on manual redirect #43715

chrmarti opened this issue Sep 13, 2024 · 14 comments
Assignees
@deepak1556
Labels
bug 🪲 component/net has-repro-gist Issue can be reproduced with code at https://gist.github.com/ platform/macOS

Comments

@chrmarti
Copy link

Preflight Checklist

Electron Version

30.4.0, 32.1.0

What operating system(s) are you using?

macOS

Operating System Version

Sonoma

What arch are you using?

arm64 (including Apple Silicon)

Last Known Working Electron version

unknown

Expected Behavior

using redirect: 'manual' net.fetch should return with the redirect response.

Actual Behavior

using redirect: 'manual' net.fetch fails with an 'Error: Redirect was cancelled'

Testcase Gist URL

https://gist.github.com/510aba6af66a8bbecf31682d8098cf25

Additional Information

No response
@electron-issue-triage electron-issue-triage bot added has-repro-gist Issue can be reproduced with code at https://gist.github.com/ platform/macOS labels Sep 13, 2024
@MarshallOfSound
Copy link
Member

This is working as documented, the request is cancelled if you don't call followRedirect inside the redirect event when redirect mode is manual.

Ref: https://www.electronjs.org/docs/latest/api/client-request#event-redirect

@MarshallOfSound MarshallOfSound closed this as not planned Won't fix, can't repro, duplicate, stale Sep 13, 2024
@chrmarti
Copy link
Author

I see ClientRequest being return from net.request, but I don't see how I would get a ClientRequest with net.fetch. It seems the only way for redirect: 'manual' to work with net.fetch, the returned Response would have to hold the redirect status. This is how Node.js' fetch implements redirect: 'manual'.

I'm trying use net.fetch as a drop-in for the global fetch from Node.js because net.fetch comes with proxy support and loads certificates from the OS.

@deepak1556
Copy link
Member

@MarshallOfSound this might be a bug in our net.fetch implementation. The spec mentions if mode is not navigate then an opaqueResponse should be provided. We are missing the handling of redirect event in the implementation today.

Node.js fetch response also seems different from the spec, ref wpt tests https://github.com/web-platform-tests/wpt/blob/master/fetch/api/redirect/redirect-mode.any.js

> fetch(`http://localhost:3001/redirect`, { redirect: 'manual' }).then(console.log, console.error)
Promise {
  <pending>,
  [Symbol(async_id_symbol)]: 140,
  [Symbol(trigger_async_id_symbol)]: 139
}
> Response {
  status: 302,
  statusText: 'Found',
  headers: Headers {
    location: '/',
    date: 'Sat, 14 Sep 2024 01:55:46 GMT',
    connection: 'keep-alive',
    'keep-alive': 'timeout=5',
    'transfer-encoding': 'chunked'
  },
  body: ReadableStream { locked: false, state: 'readable', supportsBYOB: true },
  bodyUsed: false,
  ok: false,
  redirected: false,
  type: 'basic',
  url: 'http://localhost:3001/redirect'
}

@chrmarti chrmarti mentioned this issue Sep 16, 2024
Open
@deepak1556
Copy link
Member

Okay Node.js implementation was initially following the spec but it was later changed per nodejs/undici#1193.

What should be the behavior we match in our net.Fetch API for this case ?

@deepak1556 deepak1556 reopened this Sep 16, 2024
@deepak1556
Copy link
Member

For browsers opaque responses are needed for preventing xss attacks https://fetch.spec.whatwg.org/#atomic-http-redirect-handling. In Electron, users already can use Node.js fetch in these process to get those responses, so if we were to align net.fetch behavior we are not increasing the risk any higher. Also, the api is meant to offer easy migration for users from Node.js fetch to use chromium network stack, so aligning their behaviors would be better in this case.

@jaouadeljadiri
Copy link

#43774

@kieferrm kieferrm mentioned this issue Oct 7, 2024
Closed
34 tasks
@jhudson8
Copy link
jhudson8 commented Oct 8, 2024 •
edited
Loading

this also prevents (AFAIK) conditionally but completely intercepting http/s protocol by preventing the browser from following redirects.

Appologies for the dumb question but Is anyone aware of an alternative solution to implement this currently using net.request (the issue here being a complete reproduction of the GlobalRequest to ClientRequest to listen for the 'redirect' event)?

protocol.handle('http', (request) => {
  if (shouldOverride) {
    // do my own thing
  } else {
    // if I want to leverage Chromium for Kerberos SSO and other benefits but the renderer must follow redirects so orgin is correct on future requests
   return net.fetch(request, {
     bypassCustomProtocolHandlers: true,
     redirect: 'manual'
    });
  }
}

@kieferrm kieferrm mentioned this issue Nov 3, 2024
Closed
57 tasks
@deepak1556 deepak1556 self-assigned this Nov 19, 2024
@deepak1556 deepak1556 mentioned this issue Nov 19, 2024
Open
@kieferrm kieferrm mentioned this issue Jan 5, 2025
Closed
62 tasks
@kieferrm kieferrm mentioned this issue Feb 10, 2025
Closed
57 tasks
@electron-issue-triage
Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

@deepak1556 deepak1556 removed the stale label Feb 18, 2025
@xiaoshuangLi
Copy link
xiaoshuangLi commented Feb 26, 2025 •
edited
Loading

I have the same issue, is there any workaround ??

@tianfeng98
Copy link

Same issue +1

4 similar comments
@lifuquan
Copy link

Same issue +1

@978847302
Copy link

Same issue +1

@violarong
Copy link

Same issue +1

@namonakimono
Copy link

Same issue +1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@deepak1556 deepak1556

Labels
bug 🪲 component/net has-repro-gist Issue can be reproduced with code at https://gist.github.com/ platform/macOS
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0