chore(infrastructure): Fix GitHub API authentication bug #4420

acdvorak · 2019-02-15T18:47:58Z

A recent update to the GitHub Octokit API requires a new auth token format:

"token ABC..." instead of "ABC...".

This PR fixes errors like this:

https://travis-ci.com/material-components/material-components-web/jobs/178167993#L6363

API rate limit exceeded for 35.224.112.202. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)
HttpError: API rate limit exceeded for 35.224.112.202. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)
    at response.text.then.message (/home/travis/build/material-components/material-components-web/node_modules/@octokit/request/lib/request.js:55:27)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:189:7)
↳  called from:
Failed to get pull request number for branch "feat/typescript":
Error: GitHubApi.getPullRequestNumber()
    at GitHubApi.getPullRequestNumber (/home/travis/build/material-components/material-components-web/test/screenshot/infra/lib/github-api.js:138:20)
    at DiffBaseParser.parseDiffBase (/home/travis/build/material-components/material-components-web/test/screenshot/infra/lib/diff-base-parser.js:105:46)

In addition, this PR changes our custom GitHubApi wrapper class so that it no longer throws an exception when Octokit returns an error, so screenshot tests will no longer fail on Travis CI if posting a status check or PR comment fails, for example.

kfranqueiro · 2019-02-15T19:04:23Z

test/screenshot/infra/lib/diff-base-parser.js

@@ -83,7 +83,7 @@ class DiffBaseParser {
      if (process.env.TRAVIS_BRANCH) {
        baseBranch = `origin/${process.env.TRAVIS_BRANCH}`;
      } else {
-        baseBranch = await this.gitHubApi_.getPullRequestBaseBranch(prNumber);
+        baseBranch = await this.gitHubApi_.getPullRequestBaseBranch(prNumber) || baseBranch;


Wouldn't this cause incorrect diffs against base in cases where we're PR'ing against e.g. feat/typescript? Would it be more useful to just skip the base diff?

Also, this line suggests that getPullRequestBaseBranch just returns falsy in the problem case (it doesn't actually throw)?

Yes, it would cause incorrect base diffs for PRs merging into non-master branches if the GitHub API returns an error (e.g., due to rate limiting on an unauthenticated account).

I removed all the throw new Error() statements from the GitHubApi class because GitHub integration isn't totally critical to screenshot tests, so we probably shouldn't cause Travis CI to fail when a status check POST fails.

mdc-web-bot · 2019-02-15T19:46:04Z

All 624 screenshot tests passed for commit ed47614 vs. feat/typescript! 💯🎉

…ements

…ts` files

mdc-web-bot · 2019-02-15T22:55:42Z

All 624 screenshot tests passed for commit 94fd587 vs. feat/typescript! 💯🎉

…cript--github-api

mdc-web-bot · 2019-02-15T23:23:23Z

All 624 screenshot tests passed for commit 5bdfcfb vs. feat/typescript! 💯🎉

codecov-io · 2019-02-15T23:23:45Z

Codecov Report

Merging #4420 into feat/typescript will not change coverage.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           feat/typescript    #4420   +/-   ##
================================================
  Coverage            99.05%   99.05%           
================================================
  Files                   98       98           
  Lines                 6166     6166           
  Branches               808      808           
================================================
  Hits                  6108     6108           
  Misses                  57       57           
  Partials                 1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ec12b6...a917eb8. Read the comment docs.

mdc-web-bot · 2019-02-15T23:26:48Z

All 624 screenshot tests passed for commit 3d41e71 vs. feat/typescript! 💯🎉

test/screenshot/infra/commands/test.js

kfranqueiro · 2019-02-19T17:10:04Z

test/screenshot/infra/lib/github-api.js

+  error_(message, stackTrace, unsafeErr = '') {
+    let redactedError = nodeUtil.inspect(unsafeErr, /* showHidden */ false, /* depth */ 5);
+    while (this.authToken_ && redactedError.indexOf(this.authToken_) > -1) {
+      redactedError = redactedError.replace(this.authToken_, '[REDACTED]');


Is there anything else we need to redact? Should we really be spitting out this entire object?

I didn't see anything else. I figured it would be better to print the full error message for debugging purposes in case something like this happens again, but I could be persuaded otherwise.

For example, GitHub returns an HTTP 404 for an invalid auth token (instead of HTTP 401/403), and I wouldn't have been able to figure that out had I not been able to see the full error message.

WDYT?

Here's the raw error output:

Failed to set commit status:: Error: GitHubApi.setPullRequestStatus() at GitHubApi.setPullRequestStatus (/Users/advorak/dev/mdc-web/test/screenshot/infra/lib/github-api.js:84:20) at process._tickCallback (internal/process/next_tick.js:68:7) { HttpError: Not Found at response.text.then.message (/Users/advorak/dev/mdc-web/node_modules/@octokit/request/lib/request.js:55:27) at process._tickCallback (internal/process/next_tick.js:68:7) name: 'HttpError', status: 404, headers: { 'access-control-allow-origin': '*', 'access-control-expose-headers': 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type', connection: 'close', 'content-encoding': 'gzip', 'content-security-policy': 'default-src \'none\'', 'content-type': 'application/json; charset=utf-8', date: 'Wed, 20 Feb 2019 18:24:28 GMT', 'referrer-policy': 'origin-when-cross-origin, strict-origin-when-cross-origin', server: 'GitHub.com', status: '404 Not Found', 'strict-transport-security': 'max-age=31536000; includeSubdomains; preload', 'transfer-encoding': 'chunked', 'x-content-type-options': 'nosniff', 'x-frame-options': 'deny', 'x-github-media-type': 'github.v3; format=json', 'x-github-request-id': '8D00:62C7:34C324:813403:5C6D9B5C', 'x-ratelimit-limit': '60', 'x-ratelimit-remaining': '57', 'x-ratelimit-reset': '1550690663', 'x-xss-protection': '1; mode=block' }, request: { method: 'POST', url: 'https://api.github.com/repos/material-components/material-components-web/statuses/a917eb8ee10d077ff748f4a046da8aab32e7e43a', headers: { accept: 'application/vnd.github.v3+json', 'user-agent': 'octokit.js/16.15.0 Node.js/10.14.2 (macOS High Sierra; x64)', authorization: '[REDACTED]', 'content-type': 'application/json; charset=utf-8' }, body: '{"state":"pending","target_url":"https://github.com/material-components/material-components-web","description":"0 of 1 (0.0%) - 0 diffs","context":"screenshot-test/butter-bot"}', request: { validate: { context: { type: 'string' }, description: { type: 'string' }, owner: { required: true, type: 'string' }, repo: { required: true, type: 'string' }, sha: { required: true, type: 'string' }, state: { enum: [ 'error', 'failure', 'pending', 'success' ], required: true, type: 'string' }, target_url: { type: 'string' } } } }, documentation_url: 'https://developer.github.com/v3/repos/statuses/#create-a-status' }

test/screenshot/infra/lib/report-builder.js

mdc-web-bot · 2019-02-20T18:38:47Z

All 624 screenshot tests passed for commit a917eb8 vs. feat/typescript! 💯🎉

acdvorak · 2019-02-22T18:00:16Z

This PR was inadvertently included in #4407 which got merged first.

fix(infrastructure): Fix GitHub API authentication bug

d679b39

googlebot added the cla: yes label Feb 15, 2019

acdvorak added the do-not-merge label Feb 15, 2019

kfranqueiro reviewed Feb 15, 2019

View reviewed changes

WIP: Fix token format

ed47614

acdvorak added 2 commits February 15, 2019 14:38

WIP: Redact auth token from error messages; remove console.log() stat…

ab90fa5

…ements

WIP: Reduce log spam: Don't print unchanged files; don't upload `*.d.…

94fd587

…ts` files

acdvorak removed the do-not-merge label Feb 15, 2019

WIP: Remove unnecessary glob exclude

16764a1

acdvorak changed the title ~~fix(infrastructure): Fix GitHub API authentication bug~~ chore(infrastructure): Fix GitHub API authentication bug Feb 15, 2019

acdvorak added 2 commits February 15, 2019 15:07

WIP: Remove packages dir from npm run lint:js

5bdfcfb

Merge remote-tracking branch 'origin/feat/typescript' into feat/types…

3d41e71

…cript--github-api

kfranqueiro reviewed Feb 19, 2019

View reviewed changes

WIP: Revert changes to logging

a917eb8

acdvorak closed this Feb 22, 2019

acdvorak deleted the feat/typescript--github-api branch February 26, 2019 23:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(infrastructure): Fix GitHub API authentication bug #4420

chore(infrastructure): Fix GitHub API authentication bug #4420

acdvorak commented Feb 15, 2019 •

edited

Loading

kfranqueiro Feb 15, 2019 •

edited

Loading

acdvorak Feb 15, 2019

mdc-web-bot commented Feb 15, 2019

mdc-web-bot commented Feb 15, 2019

mdc-web-bot commented Feb 15, 2019

codecov-io commented Feb 15, 2019 •

edited

Loading

mdc-web-bot commented Feb 15, 2019

kfranqueiro Feb 19, 2019

acdvorak Feb 20, 2019

mdc-web-bot commented Feb 20, 2019

acdvorak commented Feb 22, 2019

chore(infrastructure): Fix GitHub API authentication bug #4420

chore(infrastructure): Fix GitHub API authentication bug #4420

Conversation

acdvorak commented Feb 15, 2019 • edited Loading

kfranqueiro Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

acdvorak Feb 15, 2019

Choose a reason for hiding this comment

mdc-web-bot commented Feb 15, 2019

mdc-web-bot commented Feb 15, 2019

mdc-web-bot commented Feb 15, 2019

codecov-io commented Feb 15, 2019 • edited Loading

Codecov Report

mdc-web-bot commented Feb 15, 2019

kfranqueiro Feb 19, 2019

Choose a reason for hiding this comment

acdvorak Feb 20, 2019

Choose a reason for hiding this comment

mdc-web-bot commented Feb 20, 2019

acdvorak commented Feb 22, 2019

acdvorak commented Feb 15, 2019 •

edited

Loading

kfranqueiro Feb 15, 2019 •

edited

Loading

codecov-io commented Feb 15, 2019 •

edited

Loading