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

Add support for auth token in header #795

Merged
merged 1 commit into from Oct 4, 2022
Merged

Conversation

zachalam
Copy link
Contributor

@zachalam zachalam commented Oct 4, 2022

Some paid node providers require authentication to prevent abuse / DDoS attacks. A way of doing this is by proving an auth GET parameter in the server url when a “stellarsdk.Server” object is created. For security purposes, node providers may remove this auth token by purging it from the responses. And since the sdk uses some values in the response for future requests, the sdk no longer has context of the auth token. As a result, some calls to library methods like next() and prev() fail.

This PR adds support for a new custom header when a new stellar server object is created, namely X-Auth-Token. Adding support for this header allows us to authenticate against a node & have the sdk keep track of the authentication value.

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.

Two thoughts:

  1. I have no qualms with the feature, but some cursory research gives low confidence in X-Auth-Token as a meaningful header value. It's unspecified by IANA and would be unused by the public Horizon servers.
  2. I don't want to set a precedent for this kind of change every time a custom header is necessary, and instead would rather give a general way to allow custom headers (without finagling the HorizonAxiosClient directly).

How do you feel about a change that allows something like opts.extraHeaders: Array<{string: string}> (or just an object) that merges with customHeaders, instead?

@JakeUrban
Copy link
Contributor

@Shaptic I agree that an opts.extraHeaders or something similar is the ideal, but my concern is that we'll have two headers outside this object, opts.appName and opts.appVersion, and then the remaining headers in opts.extraHeaders.

Ideally, we'd migrate both of the existing headers into a single customHeaders object.

For now, can we move forward adding X-Auth-Token? This is the header that blockdaemon uses.

@Shaptic Shaptic merged commit ec168db into stellar:master Oct 4, 2022
@Shaptic Shaptic mentioned this pull request Oct 4, 2022
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.

None yet

3 participants