Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

myAchievements: achievement link validation(fixes #8114) #8115

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Mutugiii
Copy link
Member

Fixes #8114

Link validation for an achievement under the myAchievements

@Mutugiii Mutugiii linked an issue Jan 17, 2025 that may be closed by this pull request
Copy link
Member

@RheuX RheuX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its working well (for non http examples), but I did test out some edges case, something like this

image

image

image

It will allow it even though all of these are invalid links

@Mutugiii
Copy link
Member Author

Mutugiii commented Jan 23, 2025

@RheuX I've tackled the specific edge cases you mentioned, feel free to test again or even handle more edge cases

@Mutugiii Mutugiii requested a review from RheuX January 23, 2025 14:38
Copy link
Member

@jessewashburn jessewashburn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that anything after https:// or http:// (as long as it's not blank) is allowed.

image

image

image

@RheuX
Copy link
Member

RheuX commented Jan 24, 2025

the edge case I mentioned is now blocked which is great,

i know we can't block every link that invalid, but probably another one is to check if at least have a common tld (https://tld-list.com/). so things like the one jesse mentioned shouldn't be possible. If they passed all the basic link validation, but just not a valid link (doesn't link to anywhere at the end), than thats something that we probably shouldn't handle unless you want to do a DNS Lookup

So TL;DR, I think we should implemented a regex that handle (i gave example to test out for the regex - they should be invalid)

this should be good enough for a basic validation link @Mutugiii

@jessewashburn
Copy link
Member

I pushed some additional edge case handling with a regex. All the cases I mentioned plus support for multipart TLDs like .co.uk should now be handled appropriately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

achievement link validation
3 participants