EDM-2847: Implement checkOrigin for device terminal #451
Conversation
WalkthroughAdds an unexported checkOrigin(r *http.Request) bool in terminal.go and uses it for websocket.Upgrader.CheckOrigin, allowing empty Origin, origins matching BaseUiUrl (scheme+host, case-insensitive host), or same-origin requests; other Origins are rejected and logged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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: 1
🧹 Nitpick comments (1)
proxy/bridge/terminal.go (1)
103-131: Add logging for rejected origin attempts.Origin validation failures are rejected silently, which makes debugging connection issues and security monitoring difficult. Consider logging rejected origins with the reason for rejection.
🔎 Proposed enhancement to add rejection logging
func checkOrigin(r *http.Request) bool { origin := r.Header.Get("Origin") // If no Origin header is present, allow the request (same-origin from browsers) if origin == "" { return true } originURL, err := url.Parse(origin) if err != nil { + log.Warnf("Origin validation failed: invalid origin URL '%s': %v", origin, err) return false } // Allow if origin matches the configured BaseUiUrl baseURL, err := url.Parse(config.BaseUiUrl) if err != nil { log.Warnf("Failed to parse BaseUiUrl for origin check: %v", err) } else { if originURL.Scheme == baseURL.Scheme && originURL.Host == baseURL.Host { return true } } // Allow same-origin requests (Origin matches request Host) // This handles cases where the proxy is accessed directly if originURL.Host == r.Host { return true } + log.Warnf("Origin validation failed: origin '%s' not allowed (BaseUiUrl: %s, Host: %s)", origin, config.BaseUiUrl, r.Host) return false }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proxy/bridge/terminal.go
🧰 Additional context used
🧬 Code graph analysis (1)
proxy/bridge/terminal.go (1)
proxy/config/config.go (1)
BaseUiUrl(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration-tests
- GitHub Check: Build ocp plugin
- GitHub Check: Build
- GitHub Check: Lint
🔇 Additional comments (2)
proxy/bridge/terminal.go (2)
107-109: Verify the security implications of allowing requests without Origin headers.Allowing requests with no Origin header is standard for same-origin browser requests. However, this could potentially allow non-browser clients (e.g., scripts, curl) that don't send Origin headers to bypass origin validation. Please confirm this aligns with your security requirements.
If stricter validation is needed, consider requiring Origin headers and only allowing specific exceptions (e.g., authenticated service accounts). Would you like guidance on implementing stricter validation?
192-195: LGTM! Origin validation properly integrated.The
checkOriginfunction is correctly wired into the WebSocket upgrader, replacing the previous permissive behavior. This significantly improves security by preventing unauthorized cross-origin WebSocket connections.
celdrake
force-pushed
the
EDM-2847-implement-origin-check
branch
from
6e22e90 to
6d92bc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
proxy/bridge/terminal.go (1)
98-135: Well-implemented origin validation with past feedback incorporated.The implementation follows WebSocket security best practices:
- Uses explicit allow-list approach with exact matching
- Case-insensitive host comparison per RFC 3986
- Safe logging with
%qto prevent log injection- Logs BaseUiUrl parsing failures for debugging
One consideration for the same-origin check at line 128: it validates only the host, not the scheme. An
http://origin could pass if the host matches anhttps://server. For stricter validation, you could compare the origin scheme against the expected protocol (e.g., derived from BaseUiUrl orX-Forwarded-Proto). However, this is typically mitigated by HSTS and authentication requirements, so the current implementation is reasonable for most deployments.🔎 Optional: Add scheme validation to same-origin check
// Allow same-origin requests - if strings.EqualFold(originURL.Host, r.Host) { + // Compare scheme against BaseUiUrl's scheme (or default to https for production) + expectedScheme := "https" + if baseURL != nil { + expectedScheme = baseURL.Scheme + } + if originURL.Scheme == expectedScheme && strings.EqualFold(originURL.Host, r.Host) { return true }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proxy/bridge/terminal.go
🧰 Additional context used
🧬 Code graph analysis (1)
proxy/bridge/terminal.go (1)
proxy/config/config.go (1)
BaseUiUrl(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration-tests
- GitHub Check: Build
- GitHub Check: Build ocp plugin
- GitHub Check: Lint
🔇 Additional comments (1)
proxy/bridge/terminal.go (1)
196-199: LGTM!Correctly wires the new
checkOriginfunction into the WebSocket upgrader, replacing the previous permissive configuration with proper origin validation.
celdrake
force-pushed
the
EDM-2847-implement-origin-check
branch
from
6d92bc3 to
6e69f75
Compare
2 participants
Adds proper checks to allowed origins for terminal functionality.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.