dbeaver/pro#10559 Fix URL redirection from remote source - #4670
dgr-dbeaver wants to merge 9 commits into
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 8 complexity
Metric Results Complexity 8
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Host allowlisting currently relies on exact host[:port] string equality, which can incorrectly reject equivalent default-port configurations (e.g., example.com:443 vs https://example.com).
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
This PR hardens CloudBeaver CE request-host handling to prevent unsafe HTTPS redirections by only redirecting/allowing hosts explicitly configured in supportedHosts, and by validating redirect authorities/paths.
Changes:
- Tighten
RequestHostFilterredirect/host validation and construct redirects via validatedURIbuilding. - Extend
ServletAppUtils.getOriginFromRequestto accept an explicitServletApplication(used byRequestHostFilter) and refactor origin normalization. - Add platform regression tests for trusted/untrusted redirect scenarios and include them in the CE test suite.
| File | Description |
|---|---|
| server/test/io.cloudbeaver.test.platform/src/io/cloudbeaver/test/platform/CEServerTestSuite.java | Adds the new Jetty host filter regression tests to the CE test suite. |
| server/test/io.cloudbeaver.test.platform/src/io/cloudbeaver/server/jetty/RequestHostFilterTest.java | Introduces regression tests for HTTPS redirect behavior and unsafe redirect inputs. |
| server/bundles/io.cloudbeaver.server.ce/src/io/cloudbeaver/server/jetty/RequestHostFilter.java | Implements safer host allowlisting + redirect URI construction/validation. |
| server/bundles/io.cloudbeaver.server.ce/META-INF/MANIFEST.MF | Exports io.cloudbeaver.server.jetty so tests/consumers can access the filter package. |
| server/bundles/io.cloudbeaver.model/src/io/cloudbeaver/utils/ServletAppUtils.java | Adds an overload to compute origin using a provided application instance; refactors origin derivation helpers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
There are security- and runtime-impacting edge cases (fail-open behavior on origin parsing errors, potential OSGi split-package export, and uncaught forwarded-host parsing exceptions) that should be addressed before merge.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
Resolved since last review (1)
| Export-Package: io.cloudbeaver, | ||
| io.cloudbeaver.model, | ||
| io.cloudbeaver.model.config, | ||
| io.cloudbeaver.server, | ||
| io.cloudbeaver.server.jetty, | ||
| io.cloudbeaver.service, | ||
| io.cloudbeaver.service.core, | ||
| io.cloudbeaver.service.session |
| URI uri = URI.create(origin); | ||
| int port = uri.getPort(); | ||
| if (CommonUtils.isNotEmpty(request.getHeader(HEADER_FORWARDED_PORT))) { | ||
| try { | ||
| port = Integer.parseInt(request.getHeader(HEADER_FORWARDED_PORT)); | ||
| } catch (NumberFormatException e) { | ||
| log.error("Failed to parse port from header: " + request.getHeader(HEADER_FORWARDED_PORT), e); | ||
| } | ||
| } | ||
|
|
||
| URI forwardedHostUri = parseForwardedHost(forwardedHost); | ||
| int forwardedHostPort = forwardedHostUri == null ? -1 : forwardedHostUri.getPort(); | ||
| int port = getForwardedPort(request, forwardedHostPort > -1 ? forwardedHostPort : uri.getPort()); |
Closes dbeaver/pro#10559
Summary
supportedHostsTesting
mvn verify -f server/product/aggregate/pom.xml -Dtest=RequestHostFilterTest -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false -rf :io.cloudbeaver.model