Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Attempt to recover from non-uri-encoded location headers #200

Closed
wants to merge 1 commit into from

3 participants

@jfhbrook

Hey Mikeal,

I came across a strange bug while trying to use request to download some files from github. From what I could tell, what was happening was that a redirect uri to cloud.github.com has a space in it, and that this space on the second request confused github's servers.

I was able to get a fix together for myself at least, so I'm sharing. I'm sure you can think of a better way to do this check, but at least this gives you an idea?

Anyway.

@mikeal
Owner

maybe the issue is that we're decoding the UI completely with resolve when it should be encoded in the header.

what happens when you remove that if statement and just urlEncode all location headers.

@jfhbrook

Some location headers are already encoded, and that just gets it more confused.

As an aside, html entities that start with a % will mess up a naive implementation of logref interpolation. Is yours guarded against this? Mine wasn't, and I got some preeetty funny logging messages.

@mikeal
Owner

i've seen that logref issue, it needs to get fixed in logref.

we need to find a way to detect if the url is escaped or not. we then need to resole it if it's relative without escaping it. that's the fix. i'm pretty sure that a server MUST escape urls but since we know some aren't we need to find a way to tolerate it.

@mikeal
Owner

would like to get @isaacs input on this.

@jfhbrook

Regarding logref, here's how I got around it but ymmv.

Yeah, I wasn't sure how to approach this problem either. What you're saying about the server-side url escaping feels right to me as well, so yeah, same page.

@mikeal
Owner

the safest fix for logref is to keep a minimum index around for the indexOf calls that is lastIndex + value.length where value is what you're adding in to the string. that would mean that the values put in to the string from the context are never re-interpolated.

@jfhbrook

That's a good idea. I'll have to give it a shot. If I come up with something good I'll try PRing it to logref too.

@mikeal
Owner

logref is my library, i don't need to be PR'd :)

@jfhbrook

well no, but it's the nice thing to do right?

@mikeal
Owner

haha, i read that as PR == Public Relations not PR === Pull Request. yes, a pull request would be excellent :)

@isaacs

we need to find a way to detect if the url is escaped or not.

There's no clean way to do this, unfortunately.

@mikeal
Owner

this doesn't merge cleanly, please reopen when it does.

@mikeal mikeal closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 3 additions and 0 deletions.
  1. +3 −0  main.js
View
3  main.js
@@ -439,6 +439,9 @@ Request.prototype.start = function () {
if (!isUrl.test(response.headers.location)) {
response.headers.location = url.resolve(self.uri.href, response.headers.location)
}
+ if ( ! /%25/.test(encodeURI(response.headers.location))) {
+ response.headers.location = encodeURI(response.headers.location)
+ }
self.uri = response.headers.location
self.redirects.push(
{ statusCode : response.statusCode
Something went wrong with that request. Please try again.