Support for custom scheme in connection URI. - #723
Conversation
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
📝 WalkthroughWalkthroughThe changes introduce a configurable URI scheme for the SSH landing page's "Open in VS Code/Code-OSS" link. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@build/scripts/code-sshd-page/server.js`:
- Around line 25-30: The uriScheme variable is assigned from unvalidated
command-line arguments and is subsequently rendered into HTML and JavaScript
contexts at multiple locations in the file. Add validation immediately after the
assignment of uriScheme from process.argv to restrict it to valid URI-scheme
characters only (alphanumeric characters, plus signs, periods, and hyphens per
RFC 3986). Implement this validation using a regular expression check (such as
/^[a-zA-Z][a-zA-Z0-9+.-]*$/) and either reject invalid schemes or fall back to
the default value, ensuring the malformed scheme cannot break out of HTML
attributes or JavaScript strings in the openDevspacesURI function call and href
attribute rendering.
- Around line 216-225: The timer in the getHostURL function is being armed
before environment variable validation and before proper request initialization,
causing the armed timer to fire after an early return, creating a temporal dead
zone where req is undefined and the callback gets called multiple times. Move
the setTimeout call to after the validation checks for consoleURL and
devspacesURL (after the early return condition), and only arm the timer when
both environment variables are defined and the request is properly initialized.
Additionally, ensure getFallbackURL is defined or imported before the getHostURL
function to avoid temporal dead zone issues with function references in the
timeout callback.
- Around line 242-250: Remove the `rejectUnauthorized: false` option from the
mod.get() call in the OAuth discovery request. This option disables TLS
certificate verification, creating a security vulnerability that allows
certificate spoofing. Instead, rely on the platform's proper CA configuration;
if a private CA is needed, deployers should configure NODE_EXTRA_CA_CERTS to add
it to Node's trust store.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c039b192-d4e3-4fbc-9315-a2707f88968d
📒 Files selected for processing (3)
build/scripts/code-sshd-page/page-utils.jsbuild/scripts/code-sshd-page/server.jsbuild/scripts/sshd.init
|
Click here to review and test in web IDE: |
&message=Dev%20cluster%20(for%20maintainers)&logo=eclipseche&color=525C86&labelColor=FDB940)
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-723-amd64 |
1 similar comment
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-723-amd64 |
|
|
I intend to merge this soon. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@build/scripts/code-sshd-page/server.js`:
- Around line 239-249: The OAuth discovery response body is never consumed,
which leaves sockets occupied indefinitely in Node.js 22. In the mod.get
callback function that handles the OAuth discovery response, after checking the
response headers and processing the redirect location, call res.resume() on the
response object to drain the body. This allows the socket to be properly
released back to the connection pool and should be called in both the successful
response branch (when statusCode is 302) and the fallback branch to ensure the
body is always consumed regardless of the response status.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6596b79c-2fa8-4601-b35d-3e2aa8d28973
📒 Files selected for processing (3)
build/scripts/code-sshd-page/page-utils.jsbuild/scripts/code-sshd-page/server.jsbuild/scripts/sshd.init
🚧 Files skipped from review as they are similar to previous changes (2)
- build/scripts/code-sshd-page/page-utils.js
- build/scripts/sshd.init
- Some forked editor definitions may choose a non-'vscode://' URI - Improvements to OAuth host for the 'logged in' link - Remove unnecessary -p flag from 'cp' to reduce warnings Signed-off-by: Roland Grunberg <rgrunber@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@build/scripts/code-sshd-page/server.js`:
- Line 224: Wrap both `new URL()` calls (at line 224 with `devspacesURL` and at
line 243 with the OAuth endpoint location) in try-catch blocks to prevent the
server from crashing on malformed URLs. In each catch block, log the error with
appropriate context and either exit the process gracefully or handle the error
appropriately by providing fallback values or returning an error response. This
ensures that malformed input from `CHE_DASHBOARD_URL` environment variable or
invalid redirect locations from OAuth responses will not cause unhandled
exceptions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 665f2795-b75e-48e6-bfbd-723c40e3ebf8
📒 Files selected for processing (3)
build/scripts/code-sshd-page/page-utils.jsbuild/scripts/code-sshd-page/server.jsbuild/scripts/sshd.init
🚧 Files skipped from review as they are similar to previous changes (2)
- build/scripts/code-sshd-page/page-utils.js
- build/scripts/sshd.init
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-723-amd64 |
2 participants
CC'ing @azatsarynnyy for a review as some of this code, particularly in sshd.init would be used by the Toolbox setup.
Summary by CodeRabbit
New Features
vscode).Bug Fixes