feat: rfe-7592: Add support for custom login server URL in console#16125
feat: rfe-7592: Add support for custom login server URL in console#16125sandert-k8s wants to merge 2 commits intoopenshift:mainfrom
Conversation
Signed-off-by: sandert-k8s <sandert98@gmail.com>
|
@sandert-k8s: This pull request references rfe-7592 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature request to target the "4.22.0" version, but no target version was set. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sandert-k8s The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @sandert-k8s. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
@sandert-k8s: This pull request references rfe-7592 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature request to target the "4.22.0" version, but no target version was set. 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. |
|
@sandert-k8s: This pull request references rfe-7592 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature request to target the "4.22.0" version, but no target version was set. 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. |
📝 WalkthroughWalkthroughThis pull request introduces support for customizing the login server URL displayed in OpenShift Console's copy login commands feature. Changes span the Go backend, configuration layer, and TypeScript frontend. The backend adds a 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/server/server.go (1)
842-857: Consider URL sanitization before command string concatenation.The
loginServerURLis concatenated directly into a shell command string without any sanitization. While the operator API enforces HTTPS URL format via regex, shell metacharacters in the URL path or query parameters (e.g.,$,`,$(...)) could theoretically cause issues when users copy-paste the generated command.Given that:
- The API validation restricts to HTTPS URLs
- The URL validation suggested for
cmd/bridge/main.gowould further constrain input- Users are expected to review commands before execution
This is low risk but worth documenting the trust boundary. The function assumes valid, well-formed URLs.
📝 Optional: Add defensive comment documenting the trust assumption
// applyLoginServerURL substitutes (or appends) the --server= flag in an oc // login command string with loginServerURL. Returns cmd unchanged when either // argument is empty. +// +// IMPORTANT: loginServerURL is expected to be a validated HTTPS URL from either +// the operator API (regex-validated) or CLI flag (URL-validated). No additional +// sanitization is performed here. func applyLoginServerURL(cmd, loginServerURL string) string {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/server.go` around lines 842 - 857, The function applyLoginServerURL concatenates loginServerURL directly into a shell command (references: applyLoginServerURL, loginServerURL, cmd, serverFlagRe) which can allow shell metacharacters in the URL to be interpreted if the command is copy-pasted; fix by sanitizing/escaping the URL before concatenation — e.g., URL-encode the path/query or wrap the value in a shell-safe form (escape internal single quotes and enclose in single quotes) and use that escaped value when building serverFlag, or alternatively add a clear defensive comment in applyLoginServerURL documenting the trust boundary and that the value must be validated/HTTPS-only upstream if you choose not to escape here.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/bridge/main.go`:
- Line 131: The fCustomLoginServerURL flag value is not validated; add the same
URL validation used for other flags by calling flags.ValidateFlagIsURL on
*fCustomLoginServerURL (with the flag name "custom-login-server-url") before
assigning to srv.LoginServerURL; if validation returns an error,
propagate/return it so malformed URLs are rejected early (mirror the pattern
used for
alermanager-public-url/grafana-public-url/prometheus-public-url/thanos-public-url).
---
Nitpick comments:
In `@pkg/server/server.go`:
- Around line 842-857: The function applyLoginServerURL concatenates
loginServerURL directly into a shell command (references: applyLoginServerURL,
loginServerURL, cmd, serverFlagRe) which can allow shell metacharacters in the
URL to be interpreted if the command is copy-pasted; fix by sanitizing/escaping
the URL before concatenation — e.g., URL-encode the path/query or wrap the value
in a shell-safe form (escape internal single quotes and enclose in single
quotes) and use that escaped value when building serverFlag, or alternatively
add a clear defensive comment in applyLoginServerURL documenting the trust
boundary and that the value must be validated/HTTPS-only upstream if you choose
not to escape here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 702897d7-376d-4b80-9f4d-07ac082a8d06
📒 Files selected for processing (7)
cmd/bridge/main.gofrontend/@types/console/window.d.tsfrontend/packages/console-shared/src/hooks/useCopyLoginCommands.tspkg/server/server.gopkg/serverconfig/config.gopkg/serverconfig/types.govendor/github.com/openshift/api/operator/v1/types_console.go
Signed-off-by: sandert-k8s <sandert98@gmail.com>
325eca8 to
19560ae
Compare
Community contribution.
Adds RFE-7592. Adds the possibility to use the Capsule Proxy as a Server Address. It's only a optical change, it doesn't change anything to the api itself.
Should be reviewed/implemented together with openshift/api#2754
Summary by CodeRabbit
New Features