Skip to content

fix: send basic authorization from a link that includes it - #175

Open
rudycelekli wants to merge 1 commit into
anmolkapil:mainfrom
rudycelekli:fix/url-basic-authorization
Open

rudycelekli wants to merge 1 commit into
anmolkapil:mainfrom
rudycelekli:fix/url-basic-authorization

Conversation

@rudycelekli

Copy link
Copy Markdown
Contributor

A link such as https://user:p%40ss@files.example.com/dl/a.zip was fetched with no Authorization header. Node's HTTP client is given the hostname and path separately, and requestHeaders never read the username or password, so a server that requires those credentials answered 401. Probe and chunk downloads both build headers through requestHeaders.

The header is now Basic authentication of the decoded userinfo (user:p@ss, not the still-encoded password). A username with no password is sent as username:. A relative redirect stays on the same host and keeps the header. An absolute redirect, and a protocol-relative one (//cdn.example/b), builds a URL with no userinfo, so that password stays on the host named in the link.

Before the change, the two new browser-context cases saw Authorization undefined, and the local-server case received the body ok with authorization undefined. After the change:

PLEXO_E2E_SKIP_BUILD=1 npx playwright test e2e/browserContext.spec.ts e2e/network.spec.ts — 14 passed.

npx eslint on the three changed files, npx prettier --check on those files, npx tsc --noEmit -p tsconfig.node.json --composite false, and npx tsc -p tsconfig.e2e.json --noEmit all exited 0.

npm run test:e2e:smoke was not run locally. Dependencies were installed with npm ci --ignore-scripts, so the Electron binary is not present. CI runs the smoke suite.

Made with Cursor

@Arpit-Khandelwal Arpit-Khandelwal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, this is a clean fix and the tests are good. A link like https://user:pass@host/file now gets a Basic Authorization header, and an absolute redirect to another host does not carry the password, which is the right call.

The only blocker: the branch conflicts with main in e2e/network.spec.ts (main's #32 changed the imports at the top to connect as netConnect, type AddressInfo and ProxyConfig). Please rebase and keep both sets of imports. I resolved it locally that way and everything passed: typecheck, lint, format, and browserContext, network, download and probeRedirects specs (49 passed). Without your change in browserContext.ts, the three new auth assertions fail, so the tests do cover the bug.

Happy to approve after the rebase.

A link such as https://user:password@host/file was fetched with no Authorization header, so a server that requires those credentials answered 401.
@rudycelekli
rudycelekli force-pushed the fix/url-basic-authorization branch from 9d778a6 to 35b3024 Compare November 9, 2026 15:05
@rudycelekli

Copy link
Copy Markdown
Contributor Author

Rebased fix/url-basic-authorization onto upstream main (2e44a2f).

The conflict in e2e/network.spec.ts keeps both import sets: upstream's connect as netConnect, type AddressInfo, and ProxyConfig (with createServer as createTcpServer, type Socket, and connectThrough), and this branch's requestHeaders import. The browserContext.ts basic-authorization fix is unchanged.

Head: 35b3024c2b7cd3748ce7cb9ff1369dba9bfaedb3

This branch has not been deployed

No deployments
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.

2 participants