Using with @octokit/graphql & GHES fails with 406

[lencioni]

Using with @octokit/graphql & GHES fails with 406

[lencioni]

@gr2m Yep, that works! Here's my diff:

 diff --git a/ghe.js b/ghe.js
index 06a915b..78763ea 100644
--- a/ghe.js
+++ b/ghe.js
@@ -14,13 +14,10 @@ function graphqlForInstallation(installationId) {
     id: GITHUB_APP_ISSUER_ID,
     privateKey: PEM,
     installationId,
-    request: request.defaults({
-      baseUrl: `${GITHUB_BASE_URL}/api/v3`,
-    }),
   });
 
   const graphqlWithAuth = graphql.defaults({
-    baseUrl: `${GITHUB_BASE_URL}/api`,
+    baseUrl: `${GITHUB_BASE_URL}/api/v3`,
     request: {
       hook: auth.hook,
     },
diff --git a/package-lock.json b/package-lock.json
index 4bb03c0..65b523a 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -37,6 +37,18 @@
         "@octokit/types": "^5.0.0",
         "before-after-hook": "^2.1.0",
         "universal-user-agent": "^6.0.0"
+      },
+      "dependencies": {
+        "@octokit/graphql": {
+          "version": "4.5.4",
+          "resolved": "https://registry.npmjs.org/@octokit/graphql/-/graphql-4.5.4.tgz",
+          "integrity": "sha512-ITpZ+dQc0cXAW1FmDkHJJM+8Lb6anUnin0VB5hLBilnYVdLC0ICFU/KIvT7OXfW9S81DE3U4Vx2EypDG1OYaPA==",
+          "requires": {
+            "@octokit/request": "^5.3.0",
+            "@octokit/types": "^5.0.0",
+            "universal-user-agent": "^6.0.0"
+          }
+        }
       }
     },
     "@octokit/endpoint": {
@@ -50,9 +62,8 @@
       }
     },
     "@octokit/graphql": {
-      "version": "4.5.3",
-      "resolved": "https://registry.npmjs.org/@octokit/graphql/-/graphql-4.5.3.tgz",
-      "integrity": "sha512-JyYvi3j2tOb5ofASEpcg1Advs07H+Ag+I+ez7buuZfNVAmh1IYcDTuxd4gnYH8S2PSGu+f5IdDGxMmkK+5zsdA==",
+      "version": "https://github.pika.dev/octokit/graphql.js/pr/186",
+      "integrity": "sha512-hUPAXu5w3+rcJ0AjzuGgoQNhYh5QYygRdxvDAP4Nhu/5t/I3dsjaywuapY6sEsjno+O3ObYzUNDzcr0w4FKGxQ==",
       "requires": {
         "@octokit/request": "^5.3.0",
         "@octokit/types": "^5.0.0",
diff --git a/package.json b/package.json
index 838756e..86a1197 100644
--- a/package.json
+++ b/package.json
@@ -11,7 +11,7 @@
   "license": "ISC",
   "dependencies": {
     "@octokit/auth-app": "^2.4.14",
-    "@octokit/graphql": "^4.5.3",
+    "@octokit/graphql": "https://github.pika.dev/octokit/graphql.js/pr/186",
     "@octokit/request": "^5.4.7",
     "@octokit/rest": "^18.0.3",
     "jsonwebtoken": "^8.5.1"

[trotzig]

I think we're going to try the same thing with github enterprise before we put together a PR (@lencioni and I).

[gr2m]

I did not have time to look into this yet. I will keep you posted.

[lencioni]

We've recently updated GHE to 2.20.12 and I tried this stuff again and I'm seeing similar behavior.

• Fresh install of required packages: 406 error
• Only change {} to { permissions: {} } in @octokit/auth-app/dist-node/index.js line 288: 406 error
• Only add requestOptions.url = requestOptions.url.replace('/api/app/installations/', '/api/v3/app/installations/'); to the fetchWrapper function in @octokit/request/dist-node/index.js (line 28): Successful request

So it seems that the newer version of GHE no longer needs the empty permissions object, but the URLs are still not able to be set correctly to get through a GraphQL request that also ends up making a REST request for authentication, since it tries to POST to /api/app/installations/<APP ISSUER ID>/access_tokens instead of /api/v3/app/installations/<APP ISSUER ID>/access_tokens.

[lencioni]

We have now updated GHE to 2.20.14, and I just wanted to report that the behavior is the same as I observed with 2.20.12.

[gr2m]

Apologies for not getting back to you. I'm looking into it right now

[gr2m]

Thanks Joe for reporting the issue. I'll be offline until Tuesday, but will look into it afterwards. I'd love a pull request if you get to it, I don't have a way to test against GHES 2.18 myself, I'm afraid

[lencioni]

@gr2m Thanks for the quick response! The best approach here is not super clear to me. Three options immediately come to mind:

1. Allow hook options to be passed to createAppAuth in some way and propagate them down the chain. If we go with this, I'm not sure if the options need to be scoped in some way so they only get passed to the getInstallationAuthentication call (e.g. via a getInstallationApplicationOptions property) or if we can add a more generally named property. Some guidance here would be appreciated.
2. Change the argument passed to getInstallationAuthentication from {} to { permissions: {} }.
3. Add a default value to get-installation-authentication.js (e.g. permissions: options.permissions || {}).

Of these options, I think I prefer 3 because the size of the change is relatively small and I think this would help solve this problem everywhere (might allow us to remove the caveat note from the docs). Failing that, my second choice would be 2 because it would still be a small change for me to make.

What is your preference here?

[lencioni]

@gr2m I hope you had a great holiday! How do you want to proceed here?

[gr2m]

Thanks! I've still lots of notifications to catch up with :)

For option 2. and 3. we have to double check that the token gets any permissions at all, If we explicitly set it to {} instead of not setting it at all. Could you maybe check that with github.com & GHE 2.18?

[trotzig]

I've confirmed that setting { permissions: {} } works well against github.com, at least with the permissions we need. I'm not entirely sure how permissions come into play in this case though, since we're not explicitly setting them. Here's how I tested:

1. changed line 280 of node_modules/@octokit/auth-app/dist-node/index.js into } = await getInstallationAuthentication(state, { permissions: {} }, request);
2. created a client with app auth:

const auth = createAppAuth({
  id: GITHUB_APP_ISSUER_ID,
  privateKey: PEM,
  installationId,
  request: request.defaults({
    baseUrl: `${GITHUB_BASE_URL}/api/v3`,
  }),
});
const client = graphql.defaults({
  baseUrl: GITHUB_BASE_URL,
  request: {
    hook: auth.hook,
  },
});

3. Ran a graphql query to fetch commits:

const data = await client(
  `
  query getCommits($name: String!, $owner: String!, $defaultBranch: String!, $since: GitTimestamp) {
    repository(name: $name, owner: $owner) {
      ref(qualifiedName: $defaultBranch) {
        target {
          ... on Commit {
            id
            history(first: 100, since: $since) {
              pageInfo {
                hasNextPage
                endCursor
              }
              edges {
                node {
                  oid
                  committer {
                    date
                  }
                }
              }
            }
          }
        }
      }
    }
  }

`,
  {
    ...repository,
    since: '2017-10-18T00:00:00.000Z',
  },
);

This scenario returns the same things with the getInstallationAuthentication options set to {} (current default) and { permissions: {} } (our modification).

[gr2m]

Okay thanks for checking! Would you like to get a pull request started? We can discuss details there

[lencioni]

It was tricky to make a successful request to test this. Here's the script I used:

const { createAppAuth } = require('@octokit/auth-app');
const { graphql } = require('@octokit/graphql');
const jsonwebtoken = require('jsonwebtoken');
const octokit = require('@octokit/rest');

const { request } = require('@octokit/request');

const { GITHUB_BASE_URL, GITHUB_APP_PEM_BASE64, GITHUB_APP_ISSUER_ID } = process.env;

const PEM = Buffer.from(GITHUB_APP_PEM_BASE64.trim(), 'base64').toString('ascii');

function graphqlForInstallation(installationId) {
  const auth = createAppAuth({
    id: GITHUB_APP_ISSUER_ID,
    privateKey: PEM,
    installationId,
    request: request.defaults({
      baseUrl: `${GITHUB_BASE_URL}/api/v3`,
    }),
  });

  const graphqlWithAuth = graphql.defaults({
    baseUrl: `${GITHUB_BASE_URL}/api`,
    request: {
      hook: auth.hook,
    },
  });

  return graphqlWithAuth;
}

async function main () {
  const client = graphqlForInstallation(1);

  let data;
  try {
    data = await client(
    `
      query getCommits($name: String!, $owner: String!, $defaultBranch: String!, $since: GitTimestamp) {
        repository(name: $name, owner: $owner) {
          ref(qualifiedName: $defaultBranch) {
            target {
              ... on Commit {
                id
                history(first: 100, since: $since) {
                  pageInfo {
                    hasNextPage
                    endCursor
                  }
                  edges {
                    node {
                      oid
                      committer {
                        date
                      }
                    }
                  }
                }
              }
            }
          }
        }
      }

    `,
      {
        name: 'repo-name',
        owner: 'repo-owner',
        defaultBranch: 'master',
        since: '2020-06-18T00:00:00.000Z',
      },
    );
  } catch (e) {
    throw e;
  }

  console.log(data);
}

main().catch(e => { console.error(e); });

I was getting stuck on only receiving 406 Not Acceptable responses. I've looked at issues like octokit/request.js#83 which suggest setting the accept header to application/json instead of application/vnd.github.machine-man-preview+json which I tried doing in a hacky way by adding this to @octokit/request/dist-node/index.js at line 27, but I still end up with the same result.

requestOptions.headers.accept = 'application/json';

Looking at the request URL that we are hitting, I suspected that the base URL here might not be correct. So I added this hacky thing to @octokit/request/dist-node/index.js at line 27 as well:

requestOptions.url = requestOptions.url.replace('/app/installations/', '/api/v3/app/installations/');

which gave me a new error response:

415 Unsupported Media Type

If I remove the accept header hack and leave the URL hack, it seems to get past this to actually make the graphql request!

If I leave getInstallationAuthentication with an empty object as the second arg as it is currently published, I get the following error:

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at set (/private/tmp/ghe/node_modules/@octokit/auth-app/dist-node/index.js:65:63)
    at getInstallationAuthentication (/private/tmp/ghe/node_modules/@octokit/auth-app/dist-node/index.js:161:9)
    at process._tickCallback (internal/process/next_tick.js:68:7)

If I set it to be { permissions: {} }, the request is successful.

So I think adding that empty object resolves part of the problem here for me, but I've also uncovered a different issue which is that I need two different baseUrl options here (one with /api/v3 for REST requests and one with /api for graphql requests). If I set the baseUrl in the graphql.defaults call to include /api/v3, then the auth request works but the graphql request tries to hit /api/v3/graphql, which fails. @gr2m do you have any thoughts on how we might be able to solve that problem? Or, am I just configuring something incorrectly here?

  const auth = createAppAuth({
    id: GITHUB_APP_ISSUER_ID,
    privateKey: PEM,
    installationId,
    request: request.defaults({
      baseUrl: `${GITHUB_BASE_URL}/api/v3`,
    }),
  });

  const graphqlWithAuth = graphql.defaults({
    baseUrl: `${GITHUB_BASE_URL}/api`,
    request: {
      hook: auth.hook,
    },
  });

[lencioni]

@gr2m did you find out anything useful yesterday?

[lencioni]

@gr2m is there any more information I can provide to help move this to the next step?

[gr2m]

I'm very sorry for not getting back to you. I had to focusing on wip/app#239 the past weeks, but that is coming to an end soon, after which I'll go through the open issues and PRs that I have neglected :(

[lencioni]

Thank you!

[lencioni]

@gr2m Can you think of a workaround that we could use in the meantime while this is still an issue?

[gr2m]

@lencioni can you try the following

1. install @octokit/graphql from octokit/graphql.js#186: npm install https://github.pika.dev/octokit/graphql.js/pr/186

2. Update the baseUrl setting

    const graphqlWithAuth = graphql.defaults({
   -  baseUrl: `${GITHUB_BASE_URL}/api`,
   +  baseUrl: `${GITHUB_BASE_URL}/api/v3`,
      request: {
        hook: auth.hook,
      },
    });

You can also remove

    request: request.defaults({
      baseUrl: `${GITHUB_BASE_URL}/api/v3`,
    }),

It has no effect in your case

Please let me know if that resolves the 406 error