Fix new remote address being handled as local file by loadUserConfig#4012
Fix new remote address being handled as local file by loadUserConfig#4012mbg wants to merge 7 commits into
loadUserConfig#4012Conversation
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.
Pull request overview
Fixes shorthand remote configuration addresses being mistaken for local paths.
Changes:
- Refactors new-format remote address parsing.
- Revises local-versus-remote configuration resolution.
- Adds local and remote loading tests.
Show a summary per file
| File | Description |
|---|---|
src/config/remote-file.ts |
Extracts new-format address parsing. |
src/config-utils.ts |
Updates configuration source selection. |
src/config-utils.test.ts |
Adds loading behavior tests. |
lib/entry-points.js |
Generated artifact; excluded from review. |
Review details
Files excluded by content exclusion policy (1)
- lib/entry-points.js
- Files reviewed: 3/4 changed files
- Comments generated: 4
- Review effort level: Medium
| // If the path is relative to the workspace and the file exists, then we use it. | ||
| if (workspaceRelative && fs.existsSync(localFilePath)) { | ||
| configFile = localFilePath; | ||
| localFile = true; |
There was a problem hiding this comment.
For backwards-compatibility reasons, we cannot rely on local paths starting with ./. Adding such a requirement would break existing configurations.
That said, I am also not convinced that this is a real concern. While an analysis may end up using a local configuration file in a PR branch, rather than a remote one that was intended, this would be very obvious.
Currently, the same thing could be done in any repository that uses a custom workflow by changing the configuration file in the repo, adding one and setting config-file, or setting the config input.
| // If the FF for the new format is not enabled or the input path is explicitly local, | ||
| // throw the old errors depending on whether the file is outside of the workspace or not. | ||
| // Otherwise, we assume the path refers to a remote file. | ||
| if (isRelativePath(configFile) || !allowNewFormat) { |
There was a problem hiding this comment.
I don't believe that input paths are expected to be absolute or that it is really supported since paths have to be relative to the workspace root.
The
loadUserConfigfunction usesisLocalto determine if a providedconfigFilepath is local or remote. The logic inisLocalis based on theOLD_REMOTE_ADDRESS_FORMATwhere all components are required and the presence of an@character is used as an indicator that the address refers to a remote file.With the changes from #3973, the new remote address format makes all components, except the repository name, optional. Therefore, a new-style remote address may not contain an
@character. I added a test case which demonstrates the resulting failure in 3ac4070.Working around this issue is slightly complicated, because it is (in general) impossible to distinguish between a relative, local path like
foo.ymland a repository name (foo.ymlis also a valid repository name).The approach I have taken here to resolve this is principally that:
userConfigFromActionPathalways refers to a local file.isLocalpredicate holds:./), then we throw the relevant error depending on whether the absolute path is within the workspace or not.This has the effect that:
getRemoteConfig#3973 is off, the behaviour is as it was previously (all tests with the FF off behave as before).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.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