Skip to content

Conversation

@wechuli
Copy link

@wechuli wechuli commented Feb 4, 2024

The way the action is currently handling fetching the installation id of an App is problematic:

    const installations: listInstallationsResponse =
      await appOctokit.apps.listInstallations();
    let installationId = installations.data[0].id;
    if (scope !== '') {
      const scopedData = installations.data.find(
        (item) =>
          item.account &&
          'login' in item.account &&
          item.account?.login === scope
      );
      if (scopedData === undefined) {
        throw new Error(`set scope is ${scope}, but installation is not found`);
      }
      installationId = scopedData.id;
    }

In case a user has not specified the scope, the action will fetch the first page of all the installations (default is 30 results per page) and simply pick the first installation of these. This is problematic for the simple reason that the owner of the repository where the workflow is running might not be the only organization/user where the App is installed.

The bug is easy to reproduce, create a GitHub App and install it on two different organizations or users. Without specifying the scope, the action will always fetch the installation access token for where you installed the GitHub App last, that surely cannot be the intended behavior.

Additionally, even if scope is specified, the appOctokit.apps.listInstallations call will fetch the first 30 results, if the GitHub App is installed in more than 30 organizations/users, it is possible the owner specified in the scope is not among the first 30 results returned. Pagination would solve this but a much simpler way exists.

The pull request is aimed at fixing that behavior. The default scope is the owner of the repository where the workflow is running. This is a more reliable way of fetching the installation id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant