Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

Validate URL Fields like events.event_url and events.video_url #430

Closed
allella opened this issue Nov 1, 2020 · 10 comments · Fixed by #707
Closed

Validate URL Fields like events.event_url and events.video_url #430

allella opened this issue Nov 1, 2020 · 10 comments · Fixed by #707
Labels
Good First Issue This issue is beginner friendly! It shouldn't take much experience to get up and running.

Comments

@allella
Copy link
Contributor

allella commented Nov 1, 2020

Per #423 and #424, we agreed to add validation to check the user provided input is a valid URL.

It was suggested by @timmyichen new URL() wasn't the best solution and we agreed to merge in the database fields, but create a new issue.

These examples of url fields are described in #378 and were added via issues #380 and #381

@Ryuno-Ki
Copy link
Contributor

Ryuno-Ki commented Nov 1, 2020

What are the requirements? What kind of input do we want to accept and which should be rejected?

To me, new URL() will generally check on valid URL format. The point came up, that some protocols (like javascript:) would pass, too. So there needs to be additional checks.
Another argument I've read where „URLs” without a domain (e.g. freecodecamp.org), which should be accepted.

@allella
Copy link
Contributor Author

allella commented Nov 2, 2020

If the language or framework doesn't already have something built-in for our use case, then I believe any code that aims to match RFC 2396 would get us close.

In PHP this involves a core method filter_var($url, FILTER_VALIDATE_URL) . StackOverflow comments suggest RFC 2396 doesn't check the protocol, so we'd have to manually check for https:// or http:// if that's a limitation.

@vkWeb
Copy link
Member

vkWeb commented Nov 2, 2020

@Ryuno-Ki does the framework escapes the user input?

If yes then simply using input type url is the best way forward.

Else we need to think more.

@allella
Copy link
Contributor Author

allella commented Nov 2, 2020

@vkWeb I assume that's using HTML 5 validation on the front-end and we'd just not worry about older browsers.

I could get behind that too idea.

I can't imagine the output won't be sanitized by the framework, but that's certainly something we'll want for any user provided content that's printed back to the browser.

@allella
Copy link
Contributor Author

allella commented Nov 2, 2020

And/or, I'm not sure if this is active, but a package like https://www.npmjs.com/package/valid-url would seem to do the job of validating the form input.

@allella allella added Good First Issue This issue is beginner friendly! It shouldn't take much experience to get up and running. and removed Beginner Friendly labels Aug 19, 2021
@allella allella mentioned this issue Aug 19, 2021
1 task
@Ravichandra-C
Copy link
Contributor

@allella Looks like valid-url is not active anymore.

Can we implement something like this?

https://github.com/adrienv1520/node-uri#checkuriuri

or Is it enough if we just parse the URI and then check if the some parts of the URI are not empty?

Can we parse it as shown in the gist?

https://gist.github.com/curtisz/11139b2cfcaef4a261e0

@allella
Copy link
Contributor Author

allella commented Sep 6, 2021

@Zeko369 do you have a suggestion on using a node package vs a regex snippet?

@Zeko369
Copy link
Member

Zeko369 commented Sep 10, 2021

Even though the valid-url seems to be dead (while having 2M weekly downloads), it still works just fine and this is more of a nice to have than a required feature IMO.

For the MVP we just need to be able to show a small red error that there is something wrong, nothing special.

@Ravichandra-C if you think it would be easier with the Regex you linked you can use that.

@Ravichandra-C
Copy link
Contributor

Ravichandra-C commented Sep 16, 2021

@allella @Zeko369 @ojeytonwilliams, Can I use class-validator module decorators for fields to add the @isurl Restriction in the Mutation instead of writing a custom validator? I also saw that type graphql validations are disabled in server/src/app.ts. Could you please let me know if there any reason for that? and Can I enable them?

image

@Zeko369
Copy link
Member

Zeko369 commented Sep 16, 2021

YES, enable them in app.ts and use them here.

I'd just do IsUrl above Field to make it more consistent and nicer looking, but that's just a personal preference

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Good First Issue This issue is beginner friendly! It shouldn't take much experience to get up and running.
Projects
None yet
5 participants