Urgent: Fix breaking bug on url joining resulting in paths like ///path.

[STRML]

This commit can completely break production apps and this or a similar hotfix needs to be deployed ASAP.

[3rd-Eden]

@STRML Could you add test so @jcrugzz doesn't break it again ;-)?

[STRML]

Yep - working on that right now, just figured I should get the fix in ASAP.

On Sep 12, 2014, at 12:57 PM, Arnout Kazemier [email protected] wrote:

@STRML Could you add test so @jcrugzz doesn't break it again ;-)?

—
Reply to this email directly or view it on GitHub.

[3rd-Eden]

Yes, it should, but I can't release a new version anyways as I don't have publish rights (which is why I haven't merged it in yet either)

[STRML]

Ok. I have amended the commit to include a test.

On Sep 12, 2014, at 1:01 PM, Arnout Kazemier [email protected] wrote:

Yes, it should, but I can't release a new version anyways as I don't have publish rights (which is why I haven't merged it in yet either)

—
Reply to this email directly or view it on GitHub.

[3rd-Eden]

Thanks a lot for contribution. Will make sure that is out as fast as possible.

[cronopio]

Pubished as v1.4.3. Thanks for the reports and contributions.

[jcrugzz]

@STRML thanks a ton for the fix. I go and try and fix a windows bug and I do something terrible xD. Im very curious of the case that caused the path to be prefixed with /. Would you mind sharing?

[STRML]

@jcrugzz It could happen two ways, possibly resulting in ///. It is suggested, but not documented as a requirement, that there should be no trailing slash in the target or leading slash in the proxied path.

I had been using a target as a deep path, something like http://localhost:3000/api/, and my proxied paths were read from req.url so they were often something like /user/login. While path.join() handled this properly, your code would create http://localhost:3000/api///user/login.

Understandably there shouldn't be a trailing slash in the target, but it is very common for there to be a leading slash in the proxied path (from req.url), and this commit broke that case as well with a double slash.

[arnihermann]

Thanks for quickly responding to this issue.