Repository navigation
Conversation
8d9efb0 to
654f5d9
Compare
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
🟡 Changes recommended
Auth-less details can still produce malformed token undefined headers, and the new unauthenticated path lacks direct coverage.
2 open findings
What changed in this PR
Enables future unauthenticated GitHub.com API requests from enterprise environments while centralizing GitHub instance details.
Changes:
- Makes
GitHubApiDetails.authoptional and constructs Octokit options directly. - Adds shared GitHub instance URL types and helpers.
- Updates call sites and relocates related tests.
| File | Description |
|---|---|
src/api-client.ts |
Supports unauthenticated Octokit clients. |
src/github.ts |
Adds GitHub instance types and helpers. |
src/github.test.ts |
Tests the new helpers. |
src/util.ts |
Removes relocated GitHub URL utilities. |
src/util.test.ts |
Removes relocated tests. |
src/upload-lib.ts |
Uses the current-instance helper. |
src/status-report.ts |
Imports the relocated Dotcom URL. |
src/setup-codeql.ts |
Uses the relocated Dotcom URL. |
src/setup-codeql-action.ts |
Uses centralized instance details. |
src/init-action.ts |
Uses centralized instance details. |
src/database-upload.ts |
Imports the relocated URL parser. |
src/codeql.test.ts |
Requires authentication in the relevant test helper. |
lib/entry-points.js |
Generated bundle file; content excluded from review. |
Files excluded by content exclusion policy (1)
- lib/entry-points.js
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
31e38bc to
6315034
Compare
This shouldn't be possible, but was previously handled by the library
Also add some tests for it
6315034 to
c164184
Compare
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
🟢 Approval recommended
The focused authentication refactor and accompanying regression tests leave no identified blocking issues.
0 open findings
2 resolved since last review
Files excluded by content exclusion policy (1)
- lib/entry-points.js
🧠 Review effort: Balanced
henrymercer
left a comment
There was a problem hiding this comment.
Thanks for the discussion on Slack and the thorough followup! Just a couple of last suggestions, which I'm happy for you to make in a followup PR.
| const getApiClient = sinon | ||
| .stub(apiClient, "getApiClient") | ||
| .returns(client); |
There was a problem hiding this comment.
Perhaps require that we get the expected API details argument (as a regression test)?
| // Ensure that the `apiDetails` that `getApiClient` uses match the ones we were given here, | ||
| // because we use `apiDetails.auth` below which must be used with the same instance that | ||
| // `apiDetails` refers to. |
There was a problem hiding this comment.
Nit: Update or remove this comment since we removed the apiDetails.auth change. It might be worth keeping it as it is still important that the auth matches the uploads URL constructed based on apiDetails below.
| * If {@link apiDetails} is not specified, then the GitHub instance URLs and auth token | ||
| * are taken from {@link env}. |
There was a problem hiding this comment.
Nit: the auth token comes from the Action input not from env.


Related to #4198 (comment).
We currently have a few different scenarios for where we fetch CodeQL CLI releases from depending on the environment that the CodeQL Action is running in. The usual case is that the running Action comes from
github.com/github/codeql-actionand is running in a workflow ongithub.com. In that case, thetokenwe have can be used withapi.github.com.The CodeQL Action may also be running on GHES or GitHub Enterprise Cloud with Data Residency. In those cases, the
tokenwe have is for the respective environment. Our logic for finding a release to download the CodeQL CLI from prioritises the more specific environments, but falls back to fetching a release fromgithub.com/github/codeql-actionif the release is not available more locally. In that case, we should not use thetokento authenticate any requests made from the host instance to the GitHub.com instance.Currently, for downloads, this is done with the help of
getAuthorizationHeaderForwhich tests whether a givenurlmatches the GitHub instance represented by a particularGitHubApiDetailsobject, which also contains thetoken. Thetokenis only returned if theurlis a match for the GitHub instance.We don't currently have an equivalent process for API requests from a different GitHub instance to GitHub.com, even though the GitHub.com API will accept unauthenticated requests for public resources (with much tighter rate limits than authenticated requests). This is, in part, because we use
getOctokitOptionsfrom@actions/githubto constructOctokitOptionsfor the API client, which requires a (non-empty)token.This PR tidies up some of the code related to this, but mainly replaces the (virtually unnecessary) call to
getOctokitOptionsso that we are constructing theOctokitOptionsourselves and allow notokento be provided. This is fine forOctokit. This relaxation of the requirements in ourapi-client.tslibrary isn't used here, but it will allow us to make some API requests from other GitHub instances to GitHub.com in the future when necessary.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, Code Quality, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.upload-sarifaction.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
.test.tsfiles).pr-checks).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist