start-proxy: Fix bug when language is not provided #2723
Conversation
marcogario
force-pushed
the
marcogario/start-proxy_tests
branch
2 times, most recently
from
e934ea2 to
88693ea
Compare
marcogario
force-pushed
the
marcogario/start-proxy_tests
branch
from
88693ea to
51bb5eb
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/start-proxy.test.ts
Dismissed
| startProxyExports.getCredentials( | ||
| getRunnerLogger(true), | ||
| undefined, | ||
| registryCredentials, | ||
| undefined, | ||
| ), |
Check failure
Code scanning / CodeQL
Untrusted data passed to external API with additional heuristic sources High Experimental
There was a problem hiding this comment.
I believe this is a false positive because this is part of the call to the test. The input is part of the input in the test.
| const parsed = JSON.parse(credentialsStr) as Credential[]; | ||
| const out: Credential[] = []; | ||
| for (const e of parsed) { | ||
| if (e.url === undefined && e.host === undefined) { |
There was a problem hiding this comment.
Is it also an error if both url and host are defined?
There was a problem hiding this comment.
I am not sure, I believe the proxy will use the url in that case. I will double check.
There was a problem hiding this comment.
Added a comment.
|
|
||
| // Filter credentials based on language if specified. `type` is the registry type. | ||
| // E.g., "maven_feed" for Java/Kotlin, "nuget_repository" for C#. | ||
| if (registryTypeForLanguage && e.type !== registryTypeForLanguage) { |
There was a problem hiding this comment.
If registryTypeForLanguage is undefined and e.type is defined, do we still want to include this?
eg- if e.type is maven_feed and the language is cpp, is this registry still relevant?
There was a problem hiding this comment.
Yes, this is the behavior that want mostly for backwards compatibility. If no language is specified, we keep the information for all registries. Otherwise, older versions that did not have this input will filter everything away.
| } catch { | ||
| // Don't log the error since it might contain sensitive information. | ||
| logger.error("Failed to parse the credentials data."); | ||
| throw new Error("Invalid credentials format."); |
There was a problem hiding this comment.
Can you convert this to a configuration error? This way, it won't count against our SLOs if this gets thrown.
| throw new Error("Invalid credentials format."); | |
| throw new ConfigurationError("Invalid credentials format."); |
| !isPrintable(e.password) || | ||
| !isPrintable(e.token) | ||
| ) { | ||
| throw new Error( |
There was a problem hiding this comment.
Same...configuration error would be better.
In #2680, I wanted the behavior to consider all existing credentials if the language argument was not provided. This was not the case, though.
I've fixed the bug and added regression tests.
Merge / deployment checklist