Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

fix: throw on invalid multiaddr to constructor #934

Merged
merged 10 commits into from
Jan 29, 2019
Merged

fix: throw on invalid multiaddr to constructor #934

merged 10 commits into from
Jan 29, 2019

Conversation

olizilla
Copy link
Contributor

  • dont assume that invalid multiaddrs are valid hosts
  • if multiaddrOrHost starts with a /, assume its a multiaddr
  • throw if that multiaddr is invalid.

Previously, we'd try and use invalid multiaddrs as the host,
which would lead to requests like http:///dnsaddr/foobar.com:5001/api

see: ipfs/ipfs-webui#925

License: MIT
Signed-off-by: Oli Evans oli@tableflip.io

@olizilla
fix: throw on invalid multiaddr to constructor
cd682c2 
- dont assume that invalid multiaddrs are valid hosts
- if multiaddrOrHost starts with a /, assume its a multiaddr
- throw if that multiaddr is invalid.

Previously, we'd try and use invalid multiaddrs as the host,
which would lead to requests like http:///dnsaddr/foobar.com:5001/api

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@ghost ghost assigned olizilla Jan 25, 2019
@ghost ghost added the in progress label Jan 25, 2019
@olizilla
chore: fix linting. remove unused variable.
574a54f 
License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@alanshaw
Copy link
Contributor

Woah, CI is not happy at all with this...

@olizilla olizilla mentioned this pull request Jan 25, 2019
Closed
@olizilla
fix: refactor constructor arg parsing
75d76a7 
- explicitly handle all the cases we expect to be passed
- convert each param into an object so we can merge them
- presedence is right to left as with Object.assign

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@olizilla
fix: improve constructor param handling tests
0ebb098 
- ensure explicitly passed parameters are used; dont pass in the
default values and assume the are being set correctly

TODO: needs #935
to expose the protocol and api-path properties

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@olizilla
Copy link
Contributor Author

@alanshaw ok so, their was a bug that showed we were missing some test cases from the test/constructor.spec test suite. I've refactored the constructor more significantly and re-written the test/constructor.spec file to be unit test, checking that parameters are set as expected. The existing one often passes in default values for the host which means we werent really testing if that value was ever set becuase of the params passed in, or just becuase the default was being used.

To neatly check that the constructor params are updating the endpoint config that ends up being used, I need to additionally expose both the protocol and the api-path on the util.getEndpointConfig method. The other PR #935 already adds protocol and it's trivial to add api-path too, so I'm goint to update it in that PR. Once that is merged, I can update this PR to ensure that all the new constructor tests pass.

@olizilla olizilla mentioned this pull request Jan 28, 2019
Merged
@olizilla
Copy link
Contributor Author

Ready for review. I've updated it to make use of the protocol and apiPath from getEndpointConfig from #935

alanshaw
alanshaw approved these changes Jan 28, 2019
View reviewed changes
Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all the tests!

@ghost ghost assigned alanshaw Jan 28, 2019
@alanshaw
Copy link
Contributor

@olizilla looks like the browser tests are failing where the port is being detected from the browser URL - I guess karma is setup to run on port 9876.

@olizilla
fix: no args constructor test for browser
97c44ad 
License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@olizilla
Merge branch 'throw-on-invalid-multiaddr' of github.com:ipfs/js-ipfs-…
d9cf917 
…http-client into throw-on-invalid-multiaddr
@olizilla
Copy link
Contributor Author

@alanshaw I think I have everything apart from my commit messages correct now. I hope you can forgive that one.

@alanshaw alanshaw merged commit bcbf0d2 into master Jan 29, 2019
@ghost ghost removed the in progress label Jan 29, 2019
@alanshaw alanshaw deleted the throw-on-invalid-multiaddr branch January 29, 2019 12:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alanshaw alanshaw alanshaw approved these changes

Assignees

@olizilla olizilla

@alanshaw alanshaw

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@olizilla @alanshaw