fix: org/re.po#123 links do not redirect to our togithub domain #17819
Conversation
MaronHatoum
requested review from
Gabriel-Ladzaretti,
nabeelsaabna and
PhilipAbed
|
I have a concern that this is making a large change to our link approach to solve only a small edge case. The impact of a mistake would be very high in terms of bad reputation, so we try to be very careful about changes to backlinks. Before merging, I would like to ask this: if it causes any mass backlink spam by accident (e.g. a regression error in our backlink prevention) then do we have any way to detect this quickly without waiting for some outraged OSS maintainer to tell us? |
in this PR I tried to cover all the links we are using and convert them from |
|
@rarkins I don't see any a big risk to false backlinks generated by this pr |
| @@ -11,12 +11,19 @@ interface UrlMatch { | |||
| } | |||
|
|
|||
| const urlRegex = | |||
| /(?:https?:)?(?:\/\/)?(?:www\.)?(?<!api\.)(?:to)?github\.com\/[-_a-z0-9]+\/[-_a-z0-9]+\/(?:discussions|issues|pull)\/[0-9]+(?:#[-_a-z0-9]+)?/i; // TODO #12872 (?<!re) after text not matching | |||
| /(?:https?:)?(?:\/\/)?(?:www\.)?(?<!api\.)(?:to)?github\.com\/[-_a-z0-9]+\/[a-zA-Z1-9\-_.]+\/(?:discussions|issues|pull)\/[0-9]+(?:#[-_a-z0-9]+)?/i; // TODO #12872 (?<!re) after text not matching | |||
There was a problem hiding this comment.
@viceice if we look at the changes here, this is the only change in the middle:
/[a-zA-Z1-9-_.]+/
/[-_a-z0-9]+/
you approved this so hopefully you can explain to me
is it correct to remove capital letters backward slashes and dots(which not skipped for some reason) here? is the URL always with small letters? or is it just so we won't catch any user names that have capital letters?
the intention isn't clear to me here.
There was a problem hiding this comment.
nevermind, there's insensitive flag at the end it means no need for big letters...
There was a problem hiding this comment.
😉 allowed usernames1
Footnotes
| /(?:https?:)?(?:\/\/)?(?:www\.)?(?<!api\.)(?:to)?github\.com\/[-_a-z0-9]+\/[a-zA-Z1-9\-_.]+\/(?:discussions|issues|pull)\/[0-9]+(?:#[-_a-z0-9]+)?/i; // TODO #12872 (?<!re) after text not matching | ||
|
|
||
| const reduceUrlRegex = | ||
| /(?:https?:)?(?:\/\/)?(?:www\.)?github\.com\/(?<org>[a-zA-Z1-9\-_.]*)\/(?<repo>[a-zA-Z1-9\-_.]*)\/([a-zA-Z1-9\-_.]*)\/(?<number>[\d]+)/g; |
There was a problem hiding this comment.
@viceice
can't we just use the same URL above and add groups to it, for reducing? why do we make here another regex
WDYT?
There was a problem hiding this comment.
i dont see any reason to keep this regex
There was a problem hiding this comment.
you can try to integrate it. 🤷♂️
There was a problem hiding this comment.
@viceice maroon made this regex because of
(?<!api\.)
this is failing validation on regEx, so he made a new almost identical regex without it
what is this? (?<!api\.) ???
|
@rarkins do not merge this, im changing it. |
|
closing this as maroon is no longer working on it, opened #18944 instead |
Changes
.in GitHub pull requests/discussions/issues URLs.Check the release notes in this real repository example:
MaronHatoum/17284#11
Context
org/re.po#123links do not redirect to ourtogithubdomain #17284Documentation (please check one with an [x])
How I've tested my work (please tick one)
I have verified these changes via: