FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

start-proxy: Fix bug when language is not provided by marcogario · Pull Request #2723 · github/codeql-action · GitHub

/ codeql-action Public

start-proxy: Fix bug when language is not provided - #2723

Merged
aeisenberg merged 4 commits into
mainfrom
marcogario/start-proxy_tests
Jan 27, 2025
ConversationCommits (4)ChecksFiles changed
Merged

start-proxy: Fix bug when language is not provided#2723
aeisenberg merged 4 commits into
mainfrom
marcogario/start-proxy_tests

Conversation

marcogario commented Jan 24, 2025

Copy link
Copy Markdown
Contributor

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

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

marcogario force-pushed the marcogario/start-proxy_tests branch 2 times, most recently from e934ea2 to 88693ea Compare January 24, 2025 16:39
marcogario force-pushed the marcogario/start-proxy_tests branch from 88693ea to 51bb5eb Compare January 24, 2025 16:40
github-advanced-security AI found potential problems Jan 24, 2025
View reviewed changes
Comment thread src/start-proxy.test.ts
Comment on lines +36 to +41
startProxyExports.getCredentials(
getRunnerLogger(true),
undefined,
registryCredentials,
undefined,
),

Check failure

Code scanning / CodeQL

Untrusted data passed to external API with additional heuristic sources

Call to ava/types/assertions.ThrowsAssertion()() \[callback 0 result\] with untrusted data from [e.password](1).

marcogario Jan 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

marcogario marked this pull request as ready for review January 24, 2025 16:49
marcogario requested a review from a team as a code owner January 24, 2025 16:49
aeisenberg reviewed Jan 24, 2025
View reviewed changes
Comment thread src/start-proxy.ts
Comment thread src/start-proxy.ts Outdated
Comment thread src/start-proxy.ts
const parsed = JSON.parse(credentialsStr) as Credential[];
const out: Credential[] = [];
for (const e of parsed) {
if (e.url === undefined && e.host === undefined) {

aeisenberg Jan 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is it also an error if both url and host are defined?

marcogario Jan 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I am not sure, I believe the proxy will use the url in that case. I will double check.

marcogario Jan 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Added a comment.

Comment thread src/start-proxy.ts

// 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) {

aeisenberg Jan 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

marcogario Jan 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

github-advanced-security AI found potential problems Jan 24, 2025
View reviewed changes
Comment thread src/start-proxy.test.ts Fixed
github-advanced-security AI found potential problems Jan 24, 2025
View reviewed changes
Comment thread src/start-proxy.test.ts
Comment on lines +103 to +108
startProxyExports.getCredentials(
getRunnerLogger(true),
undefined,
credentialsInput,
undefined,
),

Check failure

Code scanning / CodeQL

Untrusted data passed to external API with additional heuristic sources

Call to ava/types/assertions.ThrowsAssertion()() \[callback 0 result\] with untrusted data from [e.password](1).
marcogario requested a review from aeisenberg January 24, 2025 20:55
aeisenberg reviewed Jan 24, 2025
View reviewed changes

aeisenberg left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for fixing. One more thing. I should have mentioned that in the original comment.

Comment thread src/start-proxy.ts Outdated
Comment thread src/start-proxy.ts Outdated
marcogario force-pushed the marcogario/start-proxy_tests branch from a65575b to 7c2eafa Compare January 27, 2025 10:09
marcogario requested a review from aeisenberg January 27, 2025 10:23
aeisenberg merged commit 297e89a into main Jan 27, 2025
aeisenberg deleted the marcogario/start-proxy_tests branch January 27, 2025 17:26
github-actions Bot mentioned this pull request Jan 27, 2025
Merged
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aeisenberg aeisenberg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL