Skip to content

incorrect port parsing #7525

Open
exenza opened this Issue · 19 comments

3 participants

@exenza

Challenge URL Shortener Microservice has an issue.
User Agent is: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36.
Please describe how to reproduce this issue, and include links to screenshots if possible.

If you pass a port i.e foo.com:80 itoutputs an error rather then processing the URL.
https://shurli.herokuapp.com/new/foo.com:80

@exenza

I found using regEX much more realiable than using node.js url.parse

REGEX used:

/[-a-zA-Z0-9@:%+.~#?&//=]{2,256}.[a-z]{2,4}\b(\/[-a-zA-Z0-9@:%+.~#?&//=]*)?/gi

@exenza

if you click the link:
https://shurli.herokuapp.com/new/foo.com:80

The output I get in Chrome on Win is:

{"error":{"code":"ENOTFOUND","errno":"ENOTFOUND","syscall":"getaddrinfo","hostname":"foo.com:80","host":"foo.com:80","port":80}}

@exenza

I ditched the regex and implemented the following which seems solid:

//validate function

function validate(url) {

  url = url.split('/new/')[1]

  //Test for second level domain as url module is accepting topLevel domain i.e. "foo" as a valid URL

  if(!url.split('.')[1]) return false

  var testUrl=urlp.parse(url).protocol

  // if fails retest with http://

  if (!testUrl) url='http://'+url; testUrl=urlp.parse(url).host

  if (!testUrl) return false

  return url
}
@raisedadead
Free Code Camp member

@exenza Is this resolved in that case? If yes can you close this?
Waiting for your confirmation.

@raisedadead raisedadead added the blocked label
@exenza

it works on my heroku, but I did not do it on the original repository, can I simply go on the original repository and branch it?

@raisedadead raisedadead added confirmed and removed blocked labels
@raisedadead
Free Code Camp member

@exenza Thanks.

Seems we need to update the original heroku instance of the demo. Or replace it with a working one (a fork). I am not sure who is the original owner of this. Will find out and get back.

@raisedadead
Free Code Camp member

@Rafase282 Lol : One Google search later its you!
Can you please help us out on this? as pointed by @exenza

Thanks.

@Rafase282

This is what i use to validate:

function validateURL(url) {
    // Checks to see if it is an actual url
    // Regex from https://gist.github.com/dperini/729294
    var regex = /^(?:(?:https?|ftp):\/\/)(?:\S+(?::\S*)?@)?(?:(?!(?:10|127)(?:\.\d{1,3}){3})(?!(?:169\.254|192\.168)(?:\.\d{1,3}){2})(?!172\.(?:1[6-9]|2\d|3[0-1])(?:\.\d{1,3}){2})(?:[1-9]\d?|1\d\d|2[01]\d|22[0-3])(?:\.(?:1?\d{1,2}|2[0-4]\d|25[0-5])){2}(?:\.(?:[1-9]\d?|1\d\d|2[0-4]\d|25[0-4]))|(?:(?:[a-z\u00a1-\uffff0-9]-*)*[a-z\u00a1-\uffff0-9]+)(?:\.(?:[a-z\u00a1-\uffff0-9]-*)*[a-z\u00a1-\uffff0-9]+)*(?:\.(?:[a-z\u00a1-\uffff]{2,}))\.?)(?::\d{2,5})?(?:[/?#]\S*)?$/i;
    return regex.test(url);
  }

I haven't looked at the code of the original but mine would parse the example if I add http:// to it. I can probably improve on mine and work on implementing your concerns but I would think it is more of an open challenge than anything as the requirements seem flexible or are not specific enough.

Give it a try, https://little-url.herokuapp.com/

I found something interesting that I might have to work on but overall yes, regex is the way to go.

@raisedadead
Free Code Camp member

@Rafase282 This one works! :boom:

I recommend we use yours! replacing the one in the Challenge.

And apologies, the original URL is not indeed yours, Google just confused me!
Sorry about that.

Tested : https://little-url.herokuapp.com/new/http://foo.com:80
Result : {"original_url":"http://foo.com:80","short_url":"https://little-url.herokuapp.com/8170"}

:+1:

It would be awesome if you create a PR? You want to modify anything about the RegEx?

Thanks a lot for jumping in so fast!

@Rafase282

The regex is not originally mine which is why I kept the original link in comments here and in my code but if you point me to the original I can create a PR for it.

@exenza

wouldn't be better practice to use url.parse rather than a regEx?

@Rafase282

I suppose you could use it. I myself was not aware of it, I took a look here. However, if like me; you were not aware at the time then regex would be the best way to go, particularly if you are trying to avoid adding dependencies for small projects as much as possible like I did. Yet it seem this is part of node itself even though you need a require('url') to utilize it.

@raisedadead
Free Code Camp member

@Rafase282 Unfortunately I don't know the original author. We just can replace the link with yours, with more enhancements if you feel that's necessary.

@Rafase282

Well in that case we can use mine after testing and updating to make everything clear.
Feel free to check the code and open issues if you find any problems, I'll take a look at it as soon as possible. https://github.com/Rafase282/URL-Shortener

@raisedadead
Free Code Camp member

Sure thing! Thanks.

@Rafase282

From the top of my head I know I need to update the README page on the repo, and posibly the home page unless you want to let the campers know they don't need to make it like mine. I just like to add my header or footer on my projects to share my links.

@raisedadead
Free Code Camp member

@Rafase I don't that needs to be updated except for any reference to the old link that I stumbled earlier, if any. I'll try and give ya feedback's tonight(your early morning).

But far just make a PR, so we can address anything. Which I think are none.

Other than that the repo and the README etc. looks good, to me s they are.

@Rafase282

Where do I send the PR against though? I haven't contribute to challenges since last year. Would it be to switch the links? Wouldn't the video explanation need to be updated too?

@Rafase282

@raisedadead I just updated my code, it now has examples that don't link to the other app and also has the test case you used to let people know that it should also handle port numbers.

Also I fixed the error messages, it was saying that the page was not found when it should be saying that the url is not valid.

@Rafase282 Rafase282 added a commit to Rafase282/FreeCodeCamp that referenced this issue
@Rafase282 Rafase282 Changes demo app
The purpose is to switch the demo app to another one that works better as the original one does not work well with url that specify a port as discussed at FreeCodeCamp#7525

Closes #7525
5e8c096
This was referenced
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.