HELM-520: Warn users about insecure HTTP Helm repository URLs and aut… - #16885
HELM-520: Warn users about insecure HTTP Helm repository URLs and aut…#16885sowmya-sl wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@sowmya-sl: This pull request references HELM-520 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sowmya-sl The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
component/helm
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Helm chart repository form warns when an HTTP URL is entered and probes for a reachable HTTPS equivalent during submission. The helper handles unsupported inputs, failures, and timeouts. Tests cover these cases. ChangesHelm repository HTTPS handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RepositoryForm
participant CreateHelmChartRepository
participant tryHttpsUpgrade
participant KubernetesResource
RepositoryForm->>CreateHelmChartRepository: Submit repository configuration
CreateHelmChartRepository->>tryHttpsUpgrade: Probe HTTP URL
tryHttpsUpgrade-->>CreateHelmChartRepository: HTTPS URL or null
CreateHelmChartRepository->>KubernetesResource: Create or update repository
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/helmchartrepository-create-utils.ts`:
- Around line 77-81: Normalize the HTTP scheme comparison case-insensitively so
tryHttpsUpgrade handles uppercase and mixed-case HTTP URLs while preserving the
existing HTTPS conversion behavior. Apply the same normalized check in
frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepositoryFormEditor.tsx
at lines 145-156 so warnings and upgrades remain consistent. Extend
frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/__tests__/helmchartrepository-create-utils-https-upgrade.spec.ts
at lines 26-39 with an uppercase-scheme test case.
- Around line 85-86: Update the HTTPS probe in the Helm chart repository upgrade
utility to upgrade only when the response is observably successful; avoid
treating opaque or resolved non-OK no-cors responses as success, otherwise
retain the original HTTP URL. In
frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/helmchartrepository-create-utils.ts:85-86,
adjust the fetch validation while preserving the persisted URL behavior in
CreateHelmChartRepository.tsx. In
frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/__tests__/helmchartrepository-create-utils-https-upgrade.spec.ts:10-24,
add resolved non-OK probe cases and assert that no HTTPS upgrade occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9fee68d5-7901-448d-b146-febc0a18ca80
📒 Files selected for processing (5)
frontend/packages/helm-plugin/locales/en/helm-plugin.jsonfrontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepository.tsxfrontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepositoryFormEditor.tsxfrontend/packages/helm-plugin/src/components/forms/HelmChartRepository/__tests__/helmchartrepository-create-utils-https-upgrade.spec.tsfrontend/packages/helm-plugin/src/components/forms/HelmChartRepository/helmchartrepository-create-utils.ts
| export const tryHttpsUpgrade = async (httpUrl: string): Promise<string | null> => { | ||
| if (!httpUrl?.startsWith('http://')) { | ||
| return null; | ||
| } | ||
| const httpsUrl = httpUrl.replace(/^http:\/\//, 'https://'); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/helmchartrepository-create-utils.ts \
frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepositoryFormEditor.tsx \
frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/__tests__/helmchartrepository-create-utils-https-upgrade.spec.ts
do
echo "### $f"
wc -l "$f"
echo
done
echo "### utils excerpt"
sed -n '1,180p' frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/helmchartrepository-create-utils.ts
echo "### form excerpt"
sed -n '120,190p' frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepositoryFormEditor.tsx
echo "### test excerpt"
sed -n '1,220p' frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/__tests__/helmchartrepository-create-utils-https-upgrade.spec.tsRepository: openshift/console
Length of output: 9159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = [
Path('frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/helmchartrepository-create-utils.ts'),
Path('frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepositoryFormEditor.tsx'),
Path('frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/__tests__/helmchartrepository-create-utils-https-upgrade.spec.ts'),
]
for p in files:
text = p.read_text()
print(f"\n### {p}")
for i, line in enumerate(text.splitlines(), 1):
if 'tryHttpsUpgrade' in line or "startsWith('http://')" in line or 'Alert' in line or 'HTTP is unencrypted' in line or 'https-upgrade' in line:
start = max(1, i-8)
end = min(len(text.splitlines()), i+20)
for j in range(start, end+1):
print(f"{j:4}: {text.splitlines()[j-1]}")
break
PYRepository: openshift/console
Length of output: 3792
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "startsWith\\('http://'\)|startsWith\\(\"http://\"\\)|tryHttpsUpgrade|HTTP is unencrypted" \
frontend/packages/helm-plugin/src/components/forms/HelmChartRepositoryRepository: openshift/console
Length of output: 3032
Normalize HTTP scheme checks
startsWith('http://') misses case variants like HTTP://repo.example, so both the warning and HTTPS upgrade path can be skipped. Reuse a normalized scheme check in helmchartrepository-create-utils.ts and CreateHelmChartRepositoryFormEditor.tsx, and add an uppercase-scheme test case.
📍 Affects 3 files
frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/helmchartrepository-create-utils.ts#L77-L81(this comment)frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepositoryFormEditor.tsx#L145-L156frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/__tests__/helmchartrepository-create-utils-https-upgrade.spec.ts#L26-L39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/helmchartrepository-create-utils.ts`
around lines 77 - 81, Normalize the HTTP scheme comparison case-insensitively so
tryHttpsUpgrade handles uppercase and mixed-case HTTP URLs while preserving the
existing HTTPS conversion behavior. Apply the same normalized check in
frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepositoryFormEditor.tsx
at lines 145-156 so warnings and upgrades remain consistent. Extend
frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/__tests__/helmchartrepository-create-utils-https-upgrade.spec.ts
at lines 26-39 with an uppercase-scheme test case.
60bbd46 to
d5e7cc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/__tests__/helmchartrepository-create-utils-https-upgrade.spec.ts`:
- Around line 53-66: Move Jest timer cleanup for the “should return null when
fetch times out” test into an afterEach hook or a finally block so it always
executes, including when assertions fail. Remove the test-local
jest.useRealTimers() call after the assertions while preserving the existing
fake-timer setup and timeout assertions.
In
`@frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepository.tsx`:
- Around line 114-115: Validate that
HelmChartRepositoryRes.spec?.connectionConfig?.url is a string before passing it
to tryHttpsUpgrade in the submission flow. If it is missing or invalid, set the
form error through the existing error-handling path and stop processing; only
invoke tryHttpsUpgrade for a valid URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 205dd47d-da6d-4d2c-b970-8addd4160abe
📒 Files selected for processing (5)
frontend/packages/helm-plugin/locales/en/helm-plugin.jsonfrontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepository.tsxfrontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepositoryFormEditor.tsxfrontend/packages/helm-plugin/src/components/forms/HelmChartRepository/__tests__/helmchartrepository-create-utils-https-upgrade.spec.tsfrontend/packages/helm-plugin/src/components/forms/HelmChartRepository/helmchartrepository-create-utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/helm-plugin/locales/en/helm-plugin.json
- frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepositoryFormEditor.tsx
| const currentUrl = HelmChartRepositoryRes.spec?.connectionConfig?.url; | ||
| const upgradedUrl = await tryHttpsUpgrade(currentUrl); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show file sizes and relevant excerpts with line numbers
wc -l frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepository.tsx
sed -n '1,220p' frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepository.tsx
printf '\n--- helper search ---\n'
rg -n "tryHttpsUpgrade|safeLoad|connectionConfig?.url|setStatus|resourceCall" frontend/packages/helm-plugin -SRepository: openshift/console
Length of output: 14649
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper implementation and any related types if present
helper_file=$(rg -l "function tryHttpsUpgrade|const tryHttpsUpgrade|export .*tryHttpsUpgrade" frontend/packages/helm-plugin -S | head -n 1 || true)
printf 'helper_file=%s\n' "${helper_file:-<none>}"
if [ -n "${helper_file:-}" ]; then
wc -l "$helper_file"
sed -n '1,220p' "$helper_file"
fi
printf '\n--- type definitions search ---\n'
rg -n "type HelmChartRepositoryType|interface HelmChartRepositoryType|connectionConfig" frontend/packages/helm-plugin -SRepository: openshift/console
Length of output: 9023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the helper uses string operations, inspect for explicit guards
rg -n "startsWith\\(|typeof .*url|isString|URL\\(" frontend/packages/helm-plugin/src -SRepository: openshift/console
Length of output: 2336
Validate spec.connectionConfig.url before tryHttpsUpgrade. safeLoad(values.yamlData) as HelmChartRepositoryType doesn’t enforce runtime shape, so a non-string URL can hit .startsWith() and abort submission before the form sets an error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepository.tsx`
around lines 114 - 115, Validate that
HelmChartRepositoryRes.spec?.connectionConfig?.url is a string before passing it
to tryHttpsUpgrade in the submission flow. If it is missing or invalid, set the
form error through the existing error-handling path and stop processing; only
invoke tryHttpsUpgrade for a valid URL.
Source: Path instructions
There was a problem hiding this comment.
Not a valid issue. tryHttpsUpgrade already handles missing/invalid input safely: line 78 checks if (!httpUrl?.startsWith('http://')) and returns null for undefined, null, empty string, or any non-HTTP URL. There's no crash path. Adding a separate string-type guard at the call site would be redundant defensive code.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
helper=$(fd -a '^helmchartrepository-create-utils\.ts$' frontend/packages/helm-plugin | head -n 1)
types=$(fd -a '^helm-types\.ts$' frontend/packages/helm-plugin | head -n 1)
printf '%s\n' '--- utility outline ---'
ast-grep outline "$helper" --items all --view expanded
printf '%s\n' '--- tryHttpsUpgrade implementation ---'
rg -n -A30 -B5 'tryHttpsUpgrade' "$helper"
printf '%s\n' '--- relevant type declarations ---'
rg -n -A20 -B5 'HelmChartRepositoryType|connectionConfig|url' "$types"Length of output: 4959
@sowmya-sl, the issue is still valid.
httpUrl?.startsWith('http://') safely handles null and undefined. It does not safely handle non-null, non-string YAML values.
For example, this valid YAML syntax passes through safeLoad:
spec:
connectionConfig:
url: 123safeLoad(...) as HelmChartRepositoryType only changes the TypeScript view. It does not validate the runtime value. In this case, tryHttpsUpgrade(123) evaluates 123?.startsWith(...) and throws because 123 has no startsWith method.
The helper should check typeof httpUrl === 'string' before calling startsWith, or the submission flow should validate the value before calling the helper.
You are interacting with an AI system.
…o-upgrade to HTTPS Show an inline warning when users enter an HTTP repository URL, alerting them that credentials may be exposed and content may be tampered with in transit. On submit, attempt to upgrade the URL to HTTPS by probing the server; fall back to HTTP if HTTPS is unavailable. Co-authored-by: Cursor <cursoragent@cursor.com>
d5e7cc7 to
e09397a
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/verified by @sowmya-sl |
openshift-ci-robot
added
the
verified
|
@sowmya-sl: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@sowmya-sl: This pull request references HELM-520 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Use plain { ok: true/false } objects instead of new Response() in
tryHttpsUpgrade tests since the CI Node.js version lacks the global
Response constructor.
Co-authored-by: Cursor <cursoragent@cursor.com>
openshift-ci-robot
removed
the
verified
|
@sowmya-sl: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
…o-upgrade to HTTPS
Show an inline warning when users enter an HTTP repository URL, alerting them that credentials may be exposed and content may be tampered with in transit. On submit, attempt to upgrade the URL to HTTPS by probing the server; fall back to HTTP if HTTPS is unavailable.
Analysis / Root cause:
Solution description:
Screenshots / screen recording:
Test setup:
Test cases:
Browser conformance:
Additional info:
Reviewers and assignees:
Summary by CodeRabbit