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

Respect custom headers when using rpc.Server #988

Merged
merged 4 commits into from
Jun 18, 2024

Conversation

celestialkylin
Copy link
Contributor

@celestialkylin celestialkylin commented Jun 11, 2024

It's odd to compare the length with 0. In the original code, I need to do this to add a custom header:

let custom_header = {};
let server = new Server('https://url_to_server', {headers: custom_header});
custom_header['MyHeader'] = 'Some header content';

Shouldn't it check for non-zero to add the custom header?

It's odd to compare the length with 0. In the original code, I need to do this to add a custom header:

let custom_header = {};
let server = new Server('https://url_to_server', {headers: custom_header});
custom_header['MyHeader'] = 'Some header content';

Shouldn't it check for non-zero to add the custom header?
Shaptic
Shaptic previously approved these changes Jun 11, 2024
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

Hm, yeah, you're right; this is odd. Thanks for the fix!

@Shaptic Shaptic changed the title Update server.ts Respect custom headers when using rpc.Server Jun 11, 2024
@celestialkylin
Copy link
Contributor Author

May I add the custom header feature to the Horizon Server? Working with some endpoint providers requires authorization with a custom header

@Shaptic
Copy link
Contributor

Shaptic commented Jun 12, 2024

@celestialkylin can you write an integration test that confirms the Horizon behavior fails before you code change and passes afterward? that would help validate this issue! I feel like people have had success setting custom headers in the past, but maybe they had to use axios hooks.

You can reference/extend test/integration/client_headers_test.js.

@Shaptic Shaptic self-requested a review June 12, 2024 21:33
@Shaptic Shaptic dismissed their stale review June 12, 2024 21:33

New updates, testing needs

@celestialkylin
Copy link
Contributor Author

Apologies for the lack of rigor, fixed the compile error and added a test in client_headers_test.js.

By the way, when I wrote the test script, I encountered an issue where server.listen(port, (err) => {}) listens on IPv6 by default. This caused a connection timeout when I running the test. Could this be modified to increase ease of use?

Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

Thank you, no need to apologize! The IPv6 issue is odd, I'll take a look at that. What Node version were you using? Maybe that's a differentiator.

@Shaptic
Copy link
Contributor

Shaptic commented Jun 14, 2024

@celestialkylin ah shoot, we need all commits to be signed to merge; can you get that set up? Happy to help if you have Qs.

@celestialkylin
Copy link
Contributor Author

celestialkylin commented Jun 16, 2024

@celestialkylin ah shoot, we need all commits to be signed to merge; can you get that set up? Happy to help if you have Qs.

Ah, I am a GitHub beginner. I have recommitted a signed version now

Thank you, no need to apologize! The IPv6 issue is odd, I'll take a look at that. What Node version were you using? Maybe that's a differentiator.

My node version is v18.20.3, there is a document here "https://nodejs.org/api/net.html#serverlisten", it says:

"If host is omitted, the server will accept connections on the [unspecified IPv6 address] (::) when IPv6 is available, or the (0.0.0.0) otherwise."

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