search

OpenUserJS / OpenUserJS.org

The home of FOSS user scripts.
https://openuserjs.org/
GNU General Public License v3.0
857 stars 305 forks source link

GitHub API Deprecations (repoManager and GH import pending failure) #1705

Open Martii opened 4 years ago

Martii commented 4 years ago

Got this in the email today (which is a little weird since I don't have OUJS GH access... but none-the-less):

Hello there!

On February 4th, 2020 at 13:20 (UTC) your application (OpenUserJS) used its client_id and client_secret (with the User-Agent NodeJS HTTP Client) as part of a set of query parameters to access an endpoint through the GitHub API:

https://api.github.com/user/_useridclipped_

Please use Basic Authentication instead as using OAuth credentials in query parameters has been deprecated.

Depending on your API usage, we'll be sending you this email reminder at most once every 3 days. Just one URL that was accessed with a User-Agent combination will be listed in the email reminder, not all.

Visit https://developer.github.com/changes/2019-11-05-deprecated-passwords-and-authorizations-api/#authenticating-using-query-parameters for more information.

Thanks, The GitHub Team

Target code point is probably https://github.com/OpenUserJS/OpenUserJS.org/blob/1261d16/libs/repoManager.js#L67.

I don't have much time to work on this but it seems that GH importing will fail eventually when it is EOL'd after deprecation. It is open for assignment atm.

See also:

@sizzlemctwizzle Seems like you will need to set up something with OAuth if I'm skimming this correctly.

For everyone else when/if it breaks, it breaks... the work around should be to paste your source into the browser from github raw url. It would be nice to have this OUJS feature continued as a lot of people do use it. As far as my past changes to the webhook this theoretically shouldn't change anything there. i.e. if your OUJS account has GH authentication then the webhook should still process push notices.

Cc: @Zren

Martii commented 4 years ago

Hmmm. Thinking out loud for a moment:

Re:

OAuth2 token (sent in a header)

curl -H "Authorization: token OAUTH-TOKEN" https://api.github.com

Note: GitHub recommends sending OAuth tokens using the Authorization header.

Read more about OAuth2. Note that OAuth2 tokens can be acquired using the web application flow for production applications.

Ignoring "Basic Authentication" for the moment which seems unclear... if the Header is sent that might work? EDIT But whose TOKEN... OUJS's or the user that is logged in (which I doubt the latter can be accessed but still investigating).

See also:

  1. Use the access token to access the API

The access token allows you to make requests to the API on a behalf of a user.

Authorization: token OAUTH-TOKEN GET https://api.github.com/user

For example, in curl you can set the Authorization header like this:

curl -H "Authorization: token OAUTH-TOKEN" https://api.github.com/user

Ref(s):

Martii commented 4 years ago

LOL *eyeroll*

Hi @Martii,

You recently used a password to access an endpoint through the GitHub API using curl/7.58.0. On July 1st, 2020, basic authentication using password to this endpoint will no longer work:

https://api.github.com/

We recommend using a personal access token (PAT) with the appropriate scope to access this endpoint instead. Visit https://github.com/settings/tokens for more information.

Thanks, The GitHub Team

Tried out their "Basic Authentication" and got this email from them. WT*beep*.

From some more skimming it seems that the authentication is needed to change the Rate Limiting on their end. May have to do this anonymously if possible... but could pose some additional problems.

Martii commented 4 years ago

Depending on your API usage, we'll be sending you this email reminder at most once every 3 days.

This part of GH is making it really difficult to debug. I got two today but not sure when it happened because GH's clocks are way off compared to when the events were actually initiated (NIST coordinated here but the messages are either earlier or later than what I tried). So I get to wait another day or more (up to three since they haven't quite got the reminder notice correct on max). There also seems to be some sort of API caching going on because I tried a third test and nothing in a trace and nothing in email yet.

I'm still not entirely sure if the "should" for the webhook will be a "won't".

Options:

  1. If it is for sure the import routine failing we might be able to send the accessToken (aToken to be specific) in it's place. This is the least complicated for the end user rather than creating a personal access token but untested atm. OUJS might be able to have it's own token but that may prove to have GH Rate Limiting if we have a ton of users sync at the same time (e.g. they'll detect our token as a DoS attack and limit (if not kill) retrieval). EDIT Currently this is synonymous with the behaviour and usage of QSP's

  2. If it's the webhook that is pending failure then we'll need to allow each user to store a personal access token because they won't always be logged into OUJS. If we utilize a stored personal access token only the single user gets rate limited. I'm personally leaning towards this although it does compromise some anonymity/privacy but passes DoS responsibility onto the end user.

Total mess having them deprecate the QSP's imho atm.

Misc note(s):

Martii commented 4 years ago

Keeping this open for a bit in case more reports come through.

Martii commented 4 years ago

Reports have come through and are from #1432 starting at lines 714 forward:

        if (this.auth) {
            var basic;
            switch (this.auth.type) {
                case "oauth":
                    if (this.auth.token) {
                        path += (path.indexOf("?") === -1 ? "?" : "&") +
                            "access_token=" + encodeURIComponent(this.auth.token);
                    } else {
                        path += (path.indexOf("?") === -1 ? "?" : "&") +
                            "client_id=" + encodeURIComponent(this.auth.key) +
                            "&client_secret=" + encodeURIComponent(this.auth.secret);
                    }
                    break;

So migration will need to occur soon, if possible. A fork of the fixed version may be needed in the short term... presuming it's available.

Ref(s):

Martii commented 3 years ago

Just a misc note... the import and webhooks are probably going to have be rebuilt from scratch. Was given a nice pointer (direction) on where to start so I'll go try a few things... but will probably disable both importing and the webhook sometime before/during deprecation. Sorry in advance... but this will take some time since only one other has volunteered to contribute to this issue.

Martii commented 3 years ago

As I feared if the dependency isn't authenticated it doesn't matter at this time that #1729 was done.

Confirmed with above note of:

fetchRaw is currently unauthenticated ... reverse trace (manual):

... which is what is used currently at ... https://github.com/OpenUserJS/OpenUserJS.org/blob/5772c7297908052acac5520fd4714953ece3a9f0/libs/repoManager.js#L223 ... with only three parameters.

*le sigh*. Appreciate the try though. :smiling_face_with_three_hearts:

What's interesting is when the github dependency is not authenticated I did a test on how many reloads it would take to get:

[Error: {"message":"API rate limit exceeded for 24.8.195.48. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting"}
] {
  code: 403
}

... which returns a 500 Server Error message landing status code page for us...

but the kicker is it was around 30 times and seemingly not 60 like they advertise for unauthenticated (unless somewhere hidden it's doing things twice to get a result... a refactor from scratch should find this out hopefully). (also shows that the refactor of this after @sizzlemctwizzle 's creation, of he who went dark and ignored my reaching out earlier with a Cc: initially, didn't account for the rate limiting by reading the headers GH sends)

Next test...on production with disabling existing authentication and see how that affects the webhook.

Cc: @joeytwiddle

Martii commented 3 years ago

:relieved: webhook still works unauthenticated... although no rate limiting investigated... that's the next test...

Martii commented 3 years ago

Okay... so webhook doesn't appear to be directly coupled with github dependency authorization... since most recent webhook test commit reference worked after I forced the rate limit (30 times again on production this time) on the repoManager. (poor mans testing btw ;)

So in short importing will be broken (but I put in a kill switch above) until this can get resolved properly. Buys a little bit of time by losing the import feature but at least it's not everything so far.

I think I'm going to do some random "brown out"s similar, but longer and very random, to what GH did at their blog post to simulate if any unforseen bugs appear... so be aware.

Martii commented 3 years ago

Dead code found at:

$ rgrep fetchRecentRepos
libs/repoManager.js:// RepoManager.prototype.fetchRecentRepos = function (aCallback) {

GRR!

This also means the entire Repo object is dead code too. This is why #1729 had no effect... because part of that is dead code too with fetchJSON.

DOUBLE GRR!

rgrep is my alias to recursively search excluding node_modules btw.

Martii commented 3 years ago

Misc note for anyone wanting to follow along... or better yet add your 2 cents... trials and tribulations at https://github.com/octokit/rest.js/discussions/2070 https://github.com/octokit/octokit.js/discussions/2070

I'm bullet proofing the github dep and site as much as I can but I have no idea what GH in the back end is going to do to this dep... hopefully it doesn't throw an unexpected error and cause a bazillion restarts... but we'll know soon. :smile_cat:

I will definitely be removing the dead code but will probably do that at/near EOL of the github dep.... when they disable QSPs.

Martii commented 3 years ago

Hmmm... seems that https://github.com/octokit/rest.js/discussions has been disabled today. :\

Ahhh I see, I think, they went through a GH name change... new location is https://github.com/octokit/octokit.js/discussions

NOTE(s):

Martii commented 3 years ago

Interesting... blog post changed:

Brownouts

During a brownout, authentication using query parameters will temporarily fail. The goal is to trigger alerts (assuming there are any) on our customers' services to help them find unmigrated authentication calls.

The brownouts are scheduled for:

  • May 5, 2021: For 12 hours starting at 14:00 UTC

  • June 9, 2021: For 24 hours starting at 14:00 UTC

    • August 11, 2021: For 48 hours starting at 14:00 UTC

Removal date

All authentication using query parameters will return a status code of 401 like all other auth failures starting on:

  • September 8 2021 at 14:00 UTC

... modifying milestone again.

Good day now, May 5th, to test these changes and how it will affect us. :smile_cat:

Martii commented 3 years ago

Test forced rate limit on dev... Wasn't totally expecting a 403 because docs says 401 but close enough (kind of figured this with #1799 and the vague wording that Sept 8, 2021 is probably going to be the actual failure point with 401 ):

(NOTE: Some sanitized and beautified output)

$ node inspect app.js
...
C
...
REQUEST:  {
  host: 'api.github.com',
  port: 443,
  path: '/users/Martii?client_id={client_id}',
  method: 'get',
  headers: {
    host: 'api.github.com',
    'content-length': '0',
    'user-agent': 'OpenUserJS/0.5.2 (Linux; x64; rv:249e16f) OUJS/20131106 OpenUserJS.org/249e16f',
    accept: 'application/vnd.github.v3+json'
  }
}
STATUS: 403
HEADERS: {
  "server": "GitHub.com",
  "date": "Wed, 05 May 2021 19:34:34 GMT",
  "content-type": "application/json; charset=utf-8",
  "content-length": "276",
  "x-ratelimit-limit": "60",
  "x-ratelimit-remaining": "0",
  "x-ratelimit-reset": "1620246698",
  "x-ratelimit-used": "60",
  "x-ratelimit-resource": "core",
  "x-github-media-type": "github.v3; format=json",
  "access-control-expose-headers": "ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, Deprecation, Sunset",
  "access-control-allow-origin": "*",
  "strict-transport-security": "max-age=31536000; includeSubdomains; preload",
  "x-frame-options": "deny",
  "x-content-type-options": "nosniff",
  "x-xss-protection": "0",
  "referrer-policy": "origin-when-cross-origin, strict-origin-when-cross-origin",
  "content-security-policy": "default-src 'none'",
  "vary": "Accept-Encoding, Accept, X-Requested-With",
  "x-github-request-id": "{X-GITHUB-REQUEST-ID}",
  "connection": "close"
}
[error] [Error: {"message":"API rate limit exceeded for {IP}. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting"}] {
[error]   code: 403
[error] } null Martii
API rate limit exceeded for {IP}. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)
...

SUMMARY

Dev test (started manually)

  1. Started up normally.
  2. Import still worked during lower rate limit in all three tested routes.
  3. When rate limit exceeded gave 403 in all three routes back-end and 503 front-end.
  4. Rate limit is still halved. i.e. two requests are happening currently per page.

Pro test (already running)

  1. Running normally.
  2. Import still worked during lower rate limit in all three tested routes.
  3. When rate limit exceeded gave 403 in all three routes back-end and 503 front-end.
  4. Rate limit is still halved. i.e. two requests are happening currently per page.
  5. Webhook route still works. YAY!

Pro test (restarted ~20:02 UTC)

  1. Running normally.
  2. Import still worked during lower rate limit in all three tested routes.
  3. When rate limit exceeded gave 403 in all three routes back-end and 503 front-end.
  4. Rate limit is still halved. i.e. two requests are happening currently per page.

NOTE(S):

... whereas "503 Service Unavailable" usually means:

The server (OUJS) cannot handle the request (because it is overloaded (GH) or down for maintenance). Generally, this is a temporary state.

... is more accurate... plus I think we've had enough of non-descript status codes (still) from the USO days.

var Octokit = require("@octokit/rest").Octokit;

var clientId = 'Can not post this publicly but it is accurate';
var clientSecret = 'Can not post this publicly but it is accurate';

var oauthToken = Buffer.from([`${clientId}`, `${clientSecret}`].join(':')).toString('base64');

var test = new Octokit({
  auth: `basic ${oauthToken}`
});

test.paginate("GET /users/{username}/repos", { username: "Martii" }).then(repositories => {
  console.log("%d repositories found", repositories.length)
});

RESULT:

...
(node:48411) UnhandledPromiseRejectionWarning: HttpError: Bad credentials
    at ~/repo/git/OpenUserJS.org/martii/OpenUserJS.org/node_modules/@octokit/request/dist-node/index.js:68:23
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async Object.next (~/repo/git/OpenUserJS.org/martii/OpenUserJS.org/node_modules/@octokit/plugin-paginate-rest/dist-node/index.js:62:26)
...
Martii commented 3 years ago

For a note with #1802 #1799, this is what should be showing for #37 compliance with GH rate limit (GH RL) exceeded:

Since we don't currently know if GH RL is rolling using the message ~"...back later".

Loosely related at jaredhanson/passport-github#75 followup is needed... we don't currently nab emails but still might be an issue at some point down the line.