fix: set the correct custom hub catalog type#2371
Conversation
Summary of ChangesHello @pratap0007, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the handling of custom hub catalogs by introducing automatic type detection. Previously, custom catalogs without a specified type would default to Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🔍 PR Lint Feedback
|
There was a problem hiding this comment.
Code Review
This pull request introduces a useful feature to auto-detect the hub type for custom catalogs when it's not specified. The implementation looks good, and the tests cover the new logic. I have a couple of suggestions to improve code clarity and test robustness.
First, in pkg/params/settings/default.go, there's an opportunity to simplify the logic for auto-detection by removing some redundant code.
Second, in pkg/params/settings/default_test.go, the new test case could be made more robust and comprehensive by using httptest to cover both success and failure scenarios for the API-based detection, avoiding reliance on external network conditions.
Overall, great work on this enhancement!
862aa60 to
bdaa5c0
Compare
|
@pratap0007 you should fix go-testing failing test. |
bdaa5c0 to
0febbe2
Compare
5e01e1b to
b49aef8
Compare
|
/retest go-testing |
|
/test go-testing |
|
@pratap0007 i am confused about what this commits those 😓 can you explain it a bit for me pleaase? |
Hi @chmouel, Currently, we always set the catalog type to artifacthub for custom hub catalog. This commit simply checks the hub catalog URL and assigns the custom hub catalog type, following the same behavior we already done to set the default catalog type in this PR |
|
@pratap0007 sounds good, please amend your commit with better details and update the pr description |
e90afcb to
3bee27c
Compare
|
it may be useful for you to use cursor/gemini to help producing quality commit and pull request description |
3bee27c to
8c02956
Compare
|
@chmouel thank you for review, I have updated the PR and could you please take another look? |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly sets the custom hub catalog type by validating the catalog URL via an API call. The changes are logical and well-tested. I've provided a suggestion for improving performance when detecting types for multiple custom catalogs, and another to enhance debuggability by adding logging to the type detection logic, also noting the use of magic numbers and strings.
| catalogType := config[fmt.Sprintf("%s-type", cPrefix)] | ||
| if catalogType == "" { | ||
| catalogType = hubtypes.ArtifactHubType // default to artifact hub if not specified | ||
| catalogType = getHubCatalogTypeViaAPI(config[fmt.Sprintf("%s-url", cPrefix)], httpClient) |
There was a problem hiding this comment.
This change introduces a network call inside a loop to determine the catalog type. While this works, it could lead to performance issues if there are multiple custom catalogs configured without an explicit type. The calls to getHubCatalogTypeViaAPI are sequential, and each has a 5-second timeout. With N catalogs, this could delay processing by up to N * 5 seconds.
Consider refactoring to perform these network calls in parallel. You could gather all catalogs that need type detection, launch a goroutine for each detection, and wait for them to complete before storing the results. This would significantly improve performance for configurations with many custom catalogs.
There was a problem hiding this comment.
let's hope people don't have more than one
| catalogType := config[fmt.Sprintf("%s-type", cPrefix)] | ||
| if catalogType == "" { | ||
| catalogType = hubtypes.ArtifactHubType // default to artifact hub if not specified | ||
| catalogType = getHubCatalogTypeViaAPI(config[fmt.Sprintf("%s-url", cPrefix)], httpClient) |
There was a problem hiding this comment.
let's hope people don't have more than one
pkg/params/settings/default.go
Outdated
|
|
||
| func getHubCatalogTypeViaAPI(hubURL string) string { | ||
| func getHubCatalogTypeViaAPI(hubURL string, httpClient *http.Client) string { | ||
| if httpClient == nil { |
There was a problem hiding this comment.
just pass http.DefaultClient from the caller on line 17 instead of having this condition here?
but then now that i think about you should use r.Clients.HTTP it has many things for security and timeout handling, which mean you need to refactor it to pass it from the initial call to SyncConfig
8c02956 to
7188aa7
Compare
|
/retest |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a Server-Side Request Forgery (SSRF) vulnerability due to inadequate validation of the hub URL when automatically detecting the hub catalog type via an API call. Additionally, there's a suggestion to improve the readability and simplify the logic for determining the default hub type.
| } | ||
|
|
||
| resp, err := http.DefaultClient.Do(req) | ||
| resp, err := httpClient.Do(req) |
There was a problem hiding this comment.
The function getHubCatalogTypeViaAPI makes an HTTP GET request to a URL constructed from user-controlled configuration (hub-url or custom catalog URLs) without sufficient validation. An attacker who can modify the Pipelines as Code ConfigMap can specify arbitrary internal URLs, leading to a Server-Side Request Forgery (SSRF) vulnerability. This can be used to probe the internal network from the PAC controller or a user's machine running the tknpac CLI.
To remediate this, implement a strict allow-list for hub URLs or validate that the provided URLs do not point to internal, private, or reserved IP addresses. Additionally, ensure that the URL scheme is restricted to http or https.
433ad50 to
44b4dde
Compare
|
i have pushed to this PR a fix for the loop, ptal |
|
please use proper conventional commit in your commit message as well.. we need this to generate release note categorization |
Earlier, the custom hub catalog type was always set to artifacthub. With this fix, the custom hub catalog URL is validated and correct custom hub catalog type is set accordingly. Signed-off-by: Shiv Verma <shverma@redhat.com>
44b4dde to
15f042c
Compare
Thank you, @chmouel! |

📝 Description of the Change
Earlier, the custom hub catalog type was always set to artfacthub.
With this fix, the custom hub catalog URL is validated and correct custom hub
catalog type is set accordingly.
👨🏻 Linked Jira
🔗 Linked GitHub Issue
Fixes #
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Gemini gemini@google.com
Co-authored-by: ChatGPT noreply@chatgpt.com
Co-authored-by: Claude noreply@anthropic.com
Co-authored-by: Cursor noreply@cursor.com
Co-authored-by: Copilot Copilot@users.noreply.github.com
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.