-
Notifications
You must be signed in to change notification settings - Fork 305
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
GitHub API Deprecations (repoManager and GH import pending failure) #1705
Comments
Hmmm. Thinking out loud for a moment: Re:
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:
Ref(s):
|
LOL *eyeroll*
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. |
* Too many nested if/else's makes it highly unreadable between OAuth and OpenID i.e. easier commit history searching. Been thinking about doing this since removal commit... doing it. * Probable location to add `aToken` to which is actually `aAccessToken` in dep README's. Possible location could be `aReq.session.accessToken` Applies to OpenUserJS#1705 Post OpenUserJS#889 OpenUserJS#564
* Too many nested if/else's makes it highly unreadable between OAuth and OpenID i.e. easier commit history searching. Been thinking about doing this since removal commit... doing it. * Probable location to add `aToken` to which is actually `aAccessToken` in dep README's. Possible location could be `aReq.session.accessToken` Applies to #1705 Post #889 #564 Auto-merge
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:
Total mess having them deprecate the QSP's imho atm. Misc note(s):
|
* Please read their CHANGELOGs * Delete op retested * *request* is deprecated and another suitable alternative needs to be found. See request/request/issues/3143 and request/request/issues/3142. Note we didn't get the deprecation warning in dev but examined with CHANGELOGs/commits on this package. * Affected code *(a lot)*: * app.js *(TLS certficate ping)* * libs/githubClient.js *(github import/browse)* Related to OpenUserJS#1705 * libs/repoManager *(github import/browse)* Related to OpenUserJS#1705 * controllers/scriptStorage.js *(webhook hooks)*
* Please read their CHANGELOGs * Delete op retested * *request* is deprecated and another suitable alternative needs to be found. See request/request/issues/3143 and request/request/issues/3142. Note we didn't get the deprecation warning in dev but examined with CHANGELOGs/commits on this package. * Affected code *(a lot)*: * app.js *(TLS certficate ping)* * libs/githubClient.js *(github import/browse)* Related to #1705 * libs/repoManager *(github import/browse)* Related to #1705 * controllers/scriptStorage.js *(webhook hooks)* Auto-merge
Keeping this open for a bit in case more reports come through. |
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):
|
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. |
* Disables *github* dep authorization which in turn disable importing. When activated use paste to site via Write Script Online or file upload via Upload Script until the dep can be successfully migrated. If authorization is enabled and there's a dep issue a 500 Server error could occur if GH changes something. * More bookmarks... can't quite tell if it's Fx but the bookmarks are shooting low on auto browser scroll... will try another browser at a future date. * Some verbiage clarity. Applies to OpenUserJS#1705
* Disables *github* dep authorization which in turn disable importing. When activated use paste to site via Write Script Online or file upload via Upload Script until the dep can be successfully migrated. If authorization is enabled and there's a dep issue a 500 Server error could occur if GH changes something. * More bookmarks... can't quite tell if it's Fx but the bookmarks are shooting low on auto browser scroll... will try another browser at a future date. * Some verbiage clarity. Applies to #1705 Auto-merge
* Translate GH's 403 to 503 * If/when the dep gets updated, gracefully handle GH's 403 as much as possible at authenticated and non-authenticated requests. * The *@octokit/auth-token* dep returns lower casings for `authentication` and `basic`... not sure if matching this makes a difference but today I've been getting 45 maximum on unauthenticated GH rate limiting with this. Post OpenUserJS#1729 Applies to OpenUserJS#1705 OpenUserJS#37
* Translate GH's 403 to 503 * If/when the dep gets updated, gracefully handle GH's 403 as much as possible at authenticated and non-authenticated requests. * The *@octokit/auth-token* dep returns lower casings for `authentication` and `basic`... not sure if matching this makes a difference but today I've been getting 45 maximum on unauthenticated GH rate limiting with this. Post #1729 Applies to #1705 #37 Auto-merge
* Authentication confirmation doesn't happen at all on object construction. This routine is instead used to store the values for later usage. A call to a URL will show if there is a higher rate limit. In the newer dep `authorization` is calculated as well. * Clean up and add existence checks in repoManager. Don't necessarily have to terminate the app if API Keys for GH aren't there... should just use lower rate limit... although other features might depend on those values. Appears to be part of the webhook but not confirmed. Applies to OpenUserJS#1705 OpenUserJS#37 and post OpenUserJS#1729
* Authentication confirmation doesn't happen at all on object construction. This routine is instead used to store the values for later usage. A call to a URL will show if there is a higher rate limit. In the newer dep `authorization` is calculated as well. * Clean up and add existence checks in repoManager. Don't necessarily have to terminate the app if API Keys for GH aren't there... should just use lower rate limit... although other features might depend on those values. Appears to be part of the webhook but not confirmed. Applies to #1705 #37 and post #1729 Auto-merge
Dead code found at: $ rgrep fetchRecentRepos
libs/repoManager.js:// RepoManager.prototype.fetchRecentRepos = function (aCallback) { GRR! This also means the entire DOUBLE GRR!
|
* Also at some point *node* changed the way to detect if debugging was in process. Updated original comment at OpenUserJS#429 (comment) Been a while since I've been in this mode for deep debug examination. * Fix missing custom headers for UA. This is optional but didn't know this until now that it had that option for the *github* dep. Post OpenUserJS#1753 Applies to OpenUserJS#1705 Ref(s): * https://nodejs.org/api/inspector.html#inspector_inspector_url * https://nodejs.org/api/debugger.html
* Also at some point *node* changed the way to detect if debugging was in process. Updated original comment at #429 (comment) Been a while since I've been in this mode for deep debug examination. * Fix missing custom headers for UA. This is optional but didn't know this until now that it had that option for the *github* dep. Post #1753 Applies to #1705 Ref(s): * https://nodejs.org/api/inspector.html#inspector_inspector_url * https://nodejs.org/api/debugger.html Auto-merge
Misc note for anyone wanting to follow along... or better yet add your 2 cents... trials and tribulations at 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. 😸 I will definitely be removing the dead code but will probably do that at/near EOL of the github dep.... when they disable QSPs. |
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):
|
* Transform GH 401 to 503 . Mentioned at https://developer.github.com/changes/2020-02-10-deprecating-auth-through-query-param/ and encountered with newer migrated dep. * Spec for authorization shows uppercasing... so new dep has it with incorrect casing I believe... may not matter on their server but consistancy here. Reverting. * Add a few more important comments Applies to OpenUserJS#1705 OpenUserJS#37 and post OpenUserJS#1799 Ref(s): * https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.8 * https://developer.mozilla.org/docs/Web/HTTP/Authentication
* Transform GH 401 to 503 . Mentioned at https://developer.github.com/changes/2020-02-10-deprecating-auth-through-query-param/ and encountered with newer *(attempted)* migrated dep. * Spec for authorization shows uppercasing... so new dep has it with incorrect casing I believe... may not matter on their server but consistancy here. Reverting. * Add a few more important comments Applies to #1705 #37 and post #1799 Ref(s): * https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.8 * https://developer.mozilla.org/docs/Web/HTTP/Authentication Auto-merge
Interesting... blog post changed:
... modifying milestone again. Good day now, May 5th, to test these changes and how it will affect us. 😸 |
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.)
... SUMMARYDev test (started manually)
Pro test (already running)
Pro test (restarted ~20:02 UTC)
NOTE(S):
... whereas "503 Service Unavailable" usually means:
... 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)
... |
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. |
* I'm guessing the failure that was encountered at OpenUserJS#1705 (comment) may have been a fluke on the GH back-end. * GH Docs and blog posts are a little too vague for my tastes with `username` and `password`... however upon manual deconstruction and inspection of the *github* dep at https://github.com/OpenUserJS/rest.js/blob/29dedb3022649c27ac1dad79326270b6b2a48047/index.js#L730 it uses nearly what the maintainer mentioned at [the discussion](octokit/octokit.js#2070) and what joeytwiddle did at OpenUserJS#1729 in the recently discovered dead code. *nodes* default for `Buffer` is `utf8` whereas *github*@0.2.4 via commit ref uses `ascii`. Since hashes usually only contain ASCII should not be an issue. * Newest dep at the time of testing with *@octokit/rest* for this seems to be unstable so going with this for a while since it currently gives the higher rate limit. This could change depending on what they do in the back-end. Applies to OpenUserJS#1705 NOTE(s) * a GH url changed recently from https://github.com/octokit/rest.js to https://github.com/octokit/octokit.js ... may affect things even further.
* I'm guessing the failure that was encountered at #1705 (comment) may have been a fluke on the GH back-end. * GH Docs and blog posts are a little too vague for my tastes with `username` and `password`... however upon manual deconstruction and inspection of the *github* dep at https://github.com/OpenUserJS/rest.js/blob/29dedb3022649c27ac1dad79326270b6b2a48047/index.js#L730 it uses nearly what the maintainer mentioned at [the discussion](octokit/octokit.js#2070) and what joeytwiddle did at #1729 in the recently discovered dead code. *nodes* default for `Buffer` is `utf8` whereas *github*@0.2.4 via commit ref uses `ascii`. Since hashes usually only contain ASCII should not be an issue. * Newest dep at the time of testing with *@octokit/rest* for this seems to be unstable so going with this for a while since it currently gives the higher rate limit. This could change depending on what they do in the back-end. Applies to #1705 NOTE(s) * a GH url changed recently from https://github.com/octokit/rest.js to https://github.com/octokit/octokit.js ... may affect things even further. Auto-merge
* This finally warns using *npm* under `engines` with *node* * Also startup with strict instead of lax. Has been in place for about a month as a test for login issues. Still `lax` is needed for OAuth/etc. unfortunately. * Delete op, sanitize, markdown, moderation flagging, overall UI... retested * Move git repo linkage in package.json to https. Seems like https://github.blog/2021-09-01-improving-git-protocol-security-github/#how-do-i-prepare with "anonymous" git might be affecting this from what I gather. See also https://docs.npmjs.com/cli/v7/configuring-npm/package-json#git-urls-as-dependencies . Works on current dev stack... haven't ever tried this on pro... so \*crosses things\* Indirectly applies to OpenUserJS#1705 . After March 15, 2022 can retry `git:` but preventative atm.
*node* bump * This finally warns using *npm* under `engines` with *node* * Also startup with strict instead of lax. Has been in place for about a month as a test for login issues. Still `lax` is needed for OAuth/etc. unfortunately. * Delete op, sanitize, markdown, moderation flagging, overall UI... retested * Move git repo linkage in package.json to https. Seems like https://github.blog/2021-09-01-improving-git-protocol-security-github/#how-do-i-prepare with "anonymous" git might be affecting this from what I gather. See also https://docs.npmjs.com/cli/v7/configuring-npm/package-json#git-urls-as-dependencies . Works on current dev stack... haven't ever tried this on pro... so \*crosses things\* Indirectly applies to #1705 . After March 15, 2022 can retry `git:` but preventative atm.
Got this in the email today (which is a little weird since I don't have OUJS GH access... but none-the-less):
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
The text was updated successfully, but these errors were encountered: