Sanity checks

[ShinySaana]

Add some sanity checks to the link check, just in case:

• The link is indeed github.com (as of now, the script catches "http://example.com/dummy?nonce=github.com").
• Just check that there isn't a "]" character, so that we are not susceptible to css injection.

I'll experiment and add some more checks later.

[augustozanellato]

Still vulnerable, you should escape ", not ]. Note that this issue has already been fixed tho

[ShinySaana]

Thanks for the tips, neckbeard.

[Bale001]

As @augustozanellato mentioned this is already fixed by #45, and your solution is still vulnerable- using a regex to parse anything is usually always a bad idea, much better to use a parser already given by a library

[ShinySaana]

Updated the parsing code. Changes:

• Ignores anything that isn't valid for a github username
• Handles 0 to n end slashes ("github.com/user/", "github.com/user///") (doesn't redirect to the canonical link)
• Handles www.github.com (redirects to github.com)
• Handles orgs ("github.com/orgs/some-org") (redirects to "github.com/some-org")
• Handles username that are less than 4 characters long
• Handles bad PR checks which merge links with multiple "https://" in them :)
• Cut the link to a (currently arbitrary) number of character before parsing

[gretchenfrage]

Procedurally modifying a URI with a regex concerns me. I think an approach based on using url.parse and other built-in URL-parsing machinery would be less likely to introduce bugs.

I prototyped this function which performs some of the checks your code does with a more url.parse-based approach. It seems to work decently from my manual testing, although I don't think that it performs all the checks that your code does:

function linkToGithubUsername(rawLink) {
    // if the link is a link to a GitHub user, return the username in
    // URI-escaped form. otherwise, return `null`.

    // truncate to avoid maliciously excessively long URLs
    let link = url.parse(rawLink.slice(0, 100));

    if (!(link.protocol == 'http:' || link.protocol == 'https:')) {
        // link is not HTTP(S)
        return null;
    }
    if (!(link.host == 'github.com' || link.host == 'www.github.com')) {
        // link is not github.com
        return null;
    }

    let pathParts = link.pathname.slice(1).split('/');
    if (pathParts.length != 1) {
        // link has the wrong number of path parts
        return null;
    }

    // url.parse automatically performs URI escaping
    return pathParts[0];
}

There are still some other sanity checks that this doesn't do. I think that a more secure approach could be based on using the GitHub API to check for actual pull requests to the original repository, and cross-check with that.

[sticks-stuff]

Does this fix https://github.com/rms-support-letter/rms-support-letter.github.io/pull/5037?

There are still some other sanity checks that this doesn't do. I think that a more secure approach could be based on using the GitHub API to check for actual pull requests to the original repository, and cross-check with that.

That's probably a smarter way, although it should be noted that there are a large number of GH users who signed just by commenting on https://github.com/rms-support-letter/rms-support-letter.github.io/issues/837

[davtur19]

Does this fix rms-support-letter/rms-support-letter.github.io#5037?

There are still some other sanity checks that this doesn't do. I think that a more secure approach could be based on using the GitHub API to check for actual pull requests to the original repository, and cross-check with that.

That's probably a smarter way, although it should be noted that there are a large number of GH users who signed just by commenting on rms-support-letter/rms-support-letter.github.io#837

At the current commit, no

[sticks-stuff]

Ah, ty for the heads-up!

[gretchenfrage]

Does this fix rms-support-letter/rms-support-letter.github.io#5037?

I assume you're responding to my code suggestion? Yeah, I ran the string "https://[email protected]/sticks-stuff" through my suggested function and it successfully returned null.

-- Edit --

If you're asking about the actual PR (sorry, I don't mean to sidetrack this), I haven't checked.

[sticks-stuff]

Don't be sorry! Feel free to make a PR with your own code and I'll merge it in :)

[ShinySaana]

I'll merge upstream and look on how to add the remaining checks with the current solution.

However, an API call for each of the 3500 users will get rate limited, so I'm not sure if it's the right approach.

I forgot about usernames, I'll see how this can be resolved.

[ShinySaana]

Added some changes:

• there are quite a few 4 characters long usernames, so takes those into account
• hardens the multiple slashes issue, which was part of a larger issue, relatives url (github.com/a/../b resolves to github.com/b)
• reintroduce the github orgs check

Don't have to be merged as-is.