Skip to content

Conversation

@sj26
Copy link

@sj26 sj26 commented Dec 5, 2018

Prototype of #512: Allow baking a base URI into an HTTP::Client. Super useful for creating an API client then requesting on paths alone, and making endpoints configurable:

client = HTTP.uri("https://api.stripe.com/v1/").auth(...)
client.get("charges")
client.post("customers", json: {...})
# ...

Persistent gets part way there, but only allows specifying the origin excluding path prefixes, and requires opting in to persistent connections.


This change is Reviewable

sj26 added 3 commits December 5, 2018 11:55
Useful for creating an API client then requesting on paths:

   client = HTTP.uri("https://api.stripe.com/v1/").auth(...)
   client.get("charges")
   client.post("customers", json: {...})
   ...

Persistent gets part way there, but only allows specifying the origin
excluding path prefixes, and requires opting in to persistent
connections.
@ixti ixti self-requested a review December 5, 2018 15:06
@ixti ixti self-assigned this Dec 5, 2018
@ixti
Copy link
Member

ixti commented Dec 5, 2018

That's somewhat duplicate of #493

@bjeanes
Copy link

bjeanes commented Dec 10, 2018

I like this interface better than mine in #493 though. Strong 👍 for merging this.

@tarcieri
Copy link
Member

tarcieri commented Dec 11, 2018

#493 is definitely a different kind of API. Where it was scoped to a host (or as I suggested, origin) this generally operates on (potentially) merging potentially relative paths to URIs.

I think it's at least worth pointing out (not as a blocker on this PR, just as a general note) that path joining is an area that's fraught with peril, especially if the base URI is assumed to be "secure" but the joined parameter is attacker-controlled and the attacker can exploit either relative path behaviors or completely overwrite an existing path with an absolute one.

I'd personally like to see at least some tests for cases where relative and absolute paths are provided and what the behavior is, and make sure we're ok with how it behaves. I get, to a certain extent, this is testing Addressable::URI, but where its URI#join was largely an implementation detail in the past, this API puts it in the forefront, so I want to make sure we're clear on the behavior and potentially override it. Unexpected behavior in Addressable::URI has been the source of many past bugs.

Some cases I think are important:

  • URI with path component joined with absolute path: (e.g. https://example.com/foo and /bar"). Does this result in /foo/bar or /bar?
  • URI with path component with absolute prefix but relative path elements (e.g. /bar/../)
  • Paths that start with relative path elements (../bar)

@bjeanes
Copy link

bjeanes commented Dec 11, 2018

@tarcieri excellent, excellent points. My goal in #493 is just about hosts but I think this API offers a bit more flexibility for other use cases. But you are 100% right that this flexibility needs to be tempered by security and usability considerations that merit further discussion.

I think we should have that discussion here before deciding to merge #493 or a derivative thereof in case there is some merit to unifying the proposals.

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.

4 participants