Skip to content
This repository was archived by the owner on Feb 12, 2024. It is now read-only.

Client config host breaks url #3476

Closed
v-stickykeys opened this issue Jan 13, 2021 · 3 comments
Closed

Client config host breaks url #3476

v-stickykeys opened this issue Jan 13, 2021 · 3 comments
Labels
need/triage Needs initial labeling and prioritization

Comments

@v-stickykeys
Copy link

  • Version:

"ipfs-http-client": "^48.1.2"

  • Platform:

Mac OSX

  • Subsystem:

ipfs-http-client

Severity:

Medium

Description:

When I pass host as a config option with the protocol scheme, the Client configuration breaks the url. For example
https://foo.bar.com becomes http://https.
The source of this issue is this line

const host = (opts.host || DEFAULT_HOST).split(':')[0]

The way this code is written assumes that (1) the developer is passing protocol, host, and port, and that (2) host does not include the protocol scheme. So these strings work as expected: foo.com or foo.com:5011
But this does not work: https://foo.com

I think this code is confusing because if all 3 options are required there shouldn't be a need to .split(':') on a colon at all.

Potential solutions would be:

  1. More explicit explanation of what should be passed in documentation
  2. Throw an error if only 1 of the 3 options are set
  3. Eliminate these options and just use url instead

Steps to reproduce the error:

  ipfsClient({
      host: 'https://foo.com',
      port: 5011
    });
FetchError: request to http://https:5011/api/v0/dag/put failed, reason: connect ETIMEDOUT
@lidel
Copy link
Member

lidel commented Jan 14, 2021

@valmack any reason why below did not work for you?

ipfsClient('https://foo.com:5011')

or equivalent:

  ipfsClient({
      protocol: 'https', 
      host: 'foo.com',
      port: 5011
    });

ps. I agree that splitting in mentioned line does not make much sense, unless we leverage [1] in port.
It is a cosmetic thing, but feel free to open a PR with a fix if you have time and will to clean this up.

@v-stickykeys
Copy link
Author

@lidel yeah I am now passing ipfsClient({url: https://foo.com:5011}) instead.

Happy to submit a pr but I guess just making it clear in the docs is easier. Doesn't strictly need a code change.

@achingbrain
Copy link
Member

I've merged #3478 which should hopefully make the constructor args clearer, thanks for pointing out the lack of docs.

But yes, TLDR is that you can't include a protocol in the host field, specify it in the protocol field instead, or pass the whole URL in as a string, multiaddr or URL, or use the url field of the options object.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
need/triage Needs initial labeling and prioritization
Projects
None yet
Development

No branches or pull requests

3 participants