Skip to content
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.

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

Use key/value object in place of Array for requests headers #528

Closed
wants to merge 2 commits into from
Closed

Use key/value object in place of Array for requests headers #528

wants to merge 2 commits into from

Conversation

Elyahou
Copy link

@Elyahou Elyahou commented Jan 10, 2024

I came across an issue where the anonymizeProxy helper function was not working correctly, and upstream proxy was returning 407, even if the username/password was correctly set in the proxy url.

I found out that the upstream proxy was not receiving the credential header correctly. http and https request should contain header according to the API.

With the proposed changes, the anonymizeProxy works correctly, and I no longer have errors

Copy link
Member

@jancurn jancurn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, the changes look reasonable, I'm just surprised it used to work before...

Copy link
Member

@jancurn jancurn left a comment

Choose a reason for hiding this comment

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

Just the tests are failing now...

@Elyahou
Copy link
Author

Elyahou commented Jan 10, 2024

yes, so it seems that original implementation with array is to allow repeating headers, and my change break this.

@Elyahou
Copy link
Author

Elyahou commented Jan 10, 2024

It seems http support sending an array of value to support repeating headers, so I changed the forward function accordingly, now the tests pass

@Elyahou Elyahou requested a review from jancurn January 11, 2024 07:31
@Elyahou Elyahou closed this Jan 14, 2024
@jancurn
Copy link
Member

jancurn commented Jan 14, 2024

So it worked fine with the array too, instead of object? The reason why we use array of raw headers is to preserve all even duplicate headers to make the proxy transparent.

@mschfh
Copy link

mschfh commented Aug 16, 2024

The current implementation does not work at all for me (node v20.15.1), the proxy receives headers with an array index as key:

CONNECT www.ifconfig.co:443 HTTP/1.1
0: host
1: www.ifconfig.co:443
2: proxy-authorization
3: Basic Zm9vOmJhcg==
Host: 192.168.1.1:1234
Connection: keep-alive

opened #547

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