HELM-520: Warn users about insecure HTTP Helm repository URLs and aut… - #16885
Draft
sowmya-sl wants to merge 1 commit into
Draft
HELM-520: Warn users about insecure HTTP Helm repository URLs and aut…#16885sowmya-sl wants to merge 1 commit into
sowmya-sl wants to merge 1 commit into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@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
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[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
Contributor
WalkthroughThe Helm chart repository form warns when an HTTP URL is entered and probes for a reachable HTTPS equivalent during submission. The HTTPS upgrade helper includes timeout and failure handling, with tests covering supported and unsupported inputs. 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
🚥 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}
Contributor
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
Comment on lines
+77
to
+81
| export const tryHttpsUpgrade = async (httpUrl: string): Promise<string | null> => { | ||
| if (!httpUrl?.startsWith('http://')) { | ||
| return null; | ||
| } | ||
| const httpsUrl = httpUrl.replace(/^http:\/\//, 'https://'); |
Contributor
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.
…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>
sowmya-sl
force-pushed
the
add-http-warning
branch
from
July 30, 2026 11:15
60bbd46 to
d5e7cc7
Compare
Contributor
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
Comment on lines
+53
to
+66
| it('should return null when fetch times out', async () => { | ||
| global.fetch = jest.fn().mockImplementation( | ||
| (_url, options) => | ||
| new Promise((_resolve, reject) => { | ||
| options.signal.addEventListener('abort', () => reject(new DOMException('Aborted'))); | ||
| }), | ||
| ); | ||
| jest.useFakeTimers(); | ||
| const promise = tryHttpsUpgrade('http://slow-server.com/repo'); | ||
| jest.advanceTimersByTime(3000); | ||
| const result = await promise; | ||
| expect(result).toBeNull(); | ||
| jest.useRealTimers(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore Jest timers in afterEach.
jest.useRealTimers() runs after the assertions, so a failure can leave fake timers installed and affect subsequent tests. Move timer cleanup into afterEach or a finally block.
🤖 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/__tests__/helmchartrepository-create-utils-https-upgrade.spec.ts`
around lines 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.
Comment on lines
+114
to
+115
| const currentUrl = HelmChartRepositoryRes.spec?.connectionConfig?.url; | ||
| const upgradedUrl = await tryHttpsUpgrade(currentUrl); |
Contributor
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
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…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