fix(security): block SSRF via Git SSH connection — JGit code path bypasses WebClientUtils SSRF filter#41684
Open
fix(security): block SSRF via Git SSH connection — JGit code path bypasses WebClientUtils SSRF filter#41684
Conversation
…L hosts before JGit clone (GHSA-h7pq-hq98-554w) The Git integration used JGit SSH and native git to connect to user-supplied remote URLs with no IP/host validation, bypassing the WebClientUtils SSRF filter that protects HTTP paths. Any authenticated user could supply git@169.254.169.254:user/repo.git or ssh://git@127.0.0.1:5432/path and the server would initiate a TCP connection to the internal target. This commit adds SSRF host validation to all Git SSH connection paths by reusing WebClientUtils.resolveIfAllowed() to check extracted hostnames against loopback, link-local, multicast, cloud metadata, and ULA address ranges before any SSH connection is initiated. RFC 1918 private ranges remain allowed for self-hosted Git servers. Gated entry points: - CommonGitServiceCEImpl: connectArtifactToGit, importArtifactFromGit, repo recovery - CentralGitServiceCEImpl: connectArtifactToGit, importArtifactFromGit - GitFSServiceCEImpl: both fetchRemoteRepository overloads (defense-in-depth) Fixes APP-15043
subrata71
added
the
Security
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded SSH Git remote-host validation and a new GIT_REMOTE_URL_HOST_BLOCKED error; validation extracts hosts, resolves them via WebClientUtils, blocks internal/reserved hosts, and is invoked before clone/connect/fetch flows and SSH server key acceptance; covered by new unit tests. (≤50 words) Changes
sequenceDiagram
participant Client as Client
participant GitService as GitService
participant GitUtils as GitUtils
participant HostResolver as HostResolver
participant RepoAction as RepoAction
Client->>GitService: import/connect/fetch(remoteUrl)
GitService->>GitUtils: validateGitSshUrl(remoteUrl)
GitUtils->>GitUtils: extractHostFromGitUrl(remoteUrl)
alt host extracted
GitUtils->>HostResolver: resolveIfAllowed(host) (boundedElastic, rgba(0,128,0,0.5))
alt host blocked
HostResolver-->>GitUtils: blocked (error)
GitUtils-->>GitService: Mono.error(AppsmithException: GIT_REMOTE_URL_HOST_BLOCKED)
GitService-->>Client: 400 Error (host blocked)
else host allowed
HostResolver-->>GitUtils: allowed
GitUtils-->>GitService: Mono.empty()
GitService->>RepoAction: proceed with clone/connect/import
RepoAction-->>GitService: success
GitService-->>Client: Success
end
else extraction failed / invalid URL
GitUtils-->>GitService: Mono.error(AppsmithException: INVALID_PARAMETER / INVALID_GIT_CONFIGURATION)
GitService-->>Client: Error (invalid URL)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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 docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
166-190:⚠️ Potential issue | 🟠 MajorPreserve typed
AppsmithExceptionfrom URL validation.Both handlers remap upstream errors to generic clone failures. If
validateGitSshUrlemitsAppsmithException(e.g., blocked host), it should pass through unchanged.🔧 Proposed fix
.onErrorResume(error -> { + if (error instanceof AppsmithException) { + return Mono.error(error); + } log.error("Error in cloning the remote repo, {}", error.getMessage()); ... }).onErrorResume(error -> { + if (error instanceof AppsmithException) { + return Mono.error(error); + } log.error("Error while cloning the remote repo, {}", error.getMessage()); ... })Also applies to: 249-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java` around lines 166 - 190, The error handling in GitFSServiceCEImpl's clone flow currently remaps all errors to generic AppsmithExceptions, which swallows upstream AppsmithException from validateGitSshUrl; update the onErrorResume blocks (the one handling clone errors and the similar block later) to first check if (error instanceof AppsmithException) and if so return Mono.error(error) so AppsmithException is propagated unchanged, otherwise continue with the existing transport/invalidRemote/timeout branching and fallback AppsmithError.GIT_ACTION_FAILED mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java`:
- Line 151: The enum constant GIT_REMOTE_URL_HOST_BLOCKED in AppsmithErrorCode
currently reuses the code AE-GIT-5016 (which is already used by
GIT_GENERIC_ERROR); update GIT_REMOTE_URL_HOST_BLOCKED to a unique error code
(e.g., AE-GIT-5017 or the next unused AE-GIT-* value) so code-based handling is
unambiguous, and ensure the change is applied within the AppsmithErrorCode enum
where GIT_GENERIC_ERROR and GIT_REMOTE_URL_HOST_BLOCKED are declared.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java`:
- Around line 234-236: The current validation (GitUtils.validateGitSshUrl on
gitConnectDTO.getRemoteUrl) only protects new imports/connecting flows; you must
also validate persisted remote URLs before any outbound SSH operation. Add
GitUtils.validateGitSshUrl checks (or reject/normalize) in all outbound paths
that perform SSH network actions (fetch/pull/push methods in
CentralGitServiceCEImpl and any helper methods that use Artifact.getRemoteUrl),
and/or implement a one-time migration that scans stored Artifact.remoteUrl
values and sanitizes or disables blocked remotes so legacy artifacts cannot
trigger SSH operations with disallowed URLs.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java`:
- Around line 94-99: The call to WebClientUtils.resolveIfAllowed(host) is
performing blocking DNS (InetAddress.getAllByName) on the reactive thread; wrap
that call in Mono.fromCallable(() ->
WebClientUtils.resolveIfAllowed(host)).subscribeOn(Schedulers.boundedElastic())
and then flatMap the resulting Optional: if empty return Mono.error(new
AppsmithException(AppsmithError.GIT_REMOTE_URL_HOST_BLOCKED, host)) otherwise
continue with Mono.empty(); update the code around the current return paths in
the method containing resolveIfAllowed so all blocking resolution occurs on the
boundedElastic scheduler.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java`:
- Around line 436-447: The test validateGitSshUrl_PublicHosts_Allowed in
GitUtilsTest relies on real DNS resolution for hostnames and is flaky in CI;
update the test to be deterministic by either (a) replacing the hostname-based
inputs passed to GitUtils.validateGitSshUrl with equivalent public-host
IP-literal forms (e.g., use known-reachable IP literals) or (b) mock/stub
DNS/host resolution used by GitUtils.validateGitSshUrl (for example by mocking
InetAddress.getByName or the internal resolver method) so the validation
receives deterministic addresses; modify the parameterized inputs or add a mock
setup in the test to ensure consistent behavior across CI environments.
---
Outside diff comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java`:
- Around line 166-190: The error handling in GitFSServiceCEImpl's clone flow
currently remaps all errors to generic AppsmithExceptions, which swallows
upstream AppsmithException from validateGitSshUrl; update the onErrorResume
blocks (the one handling clone errors and the similar block later) to first
check if (error instanceof AppsmithException) and if so return Mono.error(error)
so AppsmithException is propagated unchanged, otherwise continue with the
existing transport/invalidRemote/timeout branching and fallback
AppsmithError.GIT_ACTION_FAILED mapping.
🪄 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: 28ef2b1e-daee-4f33-a86c-472cfd2f1398
📒 Files selected for processing (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java
app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java
Outdated
Show resolved
Hide resolved
...r/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java
Show resolved
Hide resolved
Failed server tests
|
…cking DNS, error preservation 1. Fix error code collision: AE-GIT-5016 was already used by GIT_GENERIC_ERROR, changed GIT_REMOTE_URL_HOST_BLOCKED to AE-GIT-5021. 2. Offload blocking DNS to boundedElastic scheduler: resolveIfAllowed() calls InetAddress.getAllByName() which blocks — now wrapped in Mono.fromCallable().subscribeOn(Schedulers.boundedElastic()). 3. Preserve AppsmithException in GitFSServiceCEImpl error handlers: the onErrorResume blocks would swallow the SSRF-blocked error and remap it to generic clone failures. Now AppsmithException passes through unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java`:
- Around line 95-99: The current validateGitSshUrl resolves the host but
discards the InetAddress, allowing a DNS rebinding race when callers
(GitFSServiceCEImpl, CentralGitServiceCEImpl, CommonGitServiceCEImpl) later open
the SSH connection by re-resolving the hostname; change validateGitSshUrl to
return the validated InetAddress (or a small tuple containing InetAddress +
original host) instead of Mono<Void>, or alternatively perform the SSH
connection inside the same guarded reactive chain so resolution and connect are
atomic; update the callers to use the returned InetAddress (or the atomic
connect method) when opening the SSH session so the connection is made to the
pre-validated IP rather than re-resolving the hostname.
🪄 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: 9cf496e4-6f52-4d91-9ddf-1760657bd866
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java
Comment on lines
+95
to
+99
| return Mono.fromCallable(() -> WebClientUtils.resolveIfAllowed(host)) | ||
| .subscribeOn(Schedulers.boundedElastic()) | ||
| .flatMap(resolved -> resolved.isPresent() | ||
| ? Mono.<Void>empty() | ||
| : Mono.error(new AppsmithException(AppsmithError.GIT_REMOTE_URL_HOST_BLOCKED, host))); |
Collaborator
Author
There was a problem hiding this comment.
@coderabbitai I think this is impractical because JGit’s SSH session factory resolves hostnames internally - there’s no API to pass a pre-resolved IP for SSH connections. The attack window is negligible (validation and connect happen in the same reactive chain), and SSH host key verification provides additional protection against rebinding. WDYT?
…igate DNS rebinding The pre-connection validation in validateGitSshUrl() resolves and checks the hostname, but JGit/MINA SSHD re-resolves at connection time, creating a theoretical DNS rebinding window. This adds a secondary check in AppsmithSshdSessionFactory.accept() which validates the actual connected IP address against the same blocklist, closing the TOCTOU gap for JGit SSH paths.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java (1)
437-448:⚠️ Potential issue | 🟡 MinorPublic-host allow tests are still DNS-environment dependent.
Line 440-Line 447 can fail in CI environments with restricted DNS/network. Prefer deterministic IP-literal inputs (or a resolver stub) for stable unit tests.
Deterministic test input example
`@ValueSource`( strings = { - "git@github.com:user/repo.git", - "git@gitlab.com:user/repo.git", - "git@bitbucket.org:user/repo.git", - "ssh://git@github.com/user/repo.git", - "git@ssh.dev.azure.com:v3/org/project/repo.git", + "git@1.1.1.1:user/repo.git", + "git@8.8.8.8:user/repo.git", + "git@9.9.9.9:user/repo.git", + "ssh://git@1.0.0.1/user/repo.git", + "git@208.67.222.222:v3/org/project/repo.git", })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java` around lines 437 - 448, The test validateGitSshUrl_PublicHosts_Allowed is DNS-dependent; replace hostname-based inputs with deterministic IP-literal variants so CI won't rely on network/DNS. Update the ValueSource strings passed to GitUtils.validateGitSshUrl to use numeric IP literals (e.g., "git@192.0.2.1:user/repo.git", "ssh://git@192.0.2.1/user/repo.git", "git@198.51.100.2:user/repo.git", etc.) or alternatively stub out the resolver used by GitUtils.validateGitSshUrl, but keep references to the same test method and GitUtils.validateGitSshUrl so the test logic is unchanged.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java (1)
93-97:⚠️ Potential issue | 🟠 MajorDNS-rebinding TOCTOU still possible because validated address is discarded.
At Line 95-Line 97, the validated
InetAddressis not propagated, and downstream git connections still use the original hostname string and can re-resolve later. That keeps a rebinding window open. Please return/pin the resolved address through this API and use it at connect time.Suggested direction
-public static Mono<Void> validateGitSshUrl(String remoteUrl) { +public static Mono<InetAddress> validateGitSshUrl(String remoteUrl) { ... - return Mono.fromCallable(() -> WebClientUtils.resolveIfAllowed(host)) + return Mono.fromCallable(() -> WebClientUtils.resolveIfAllowed(host)) .subscribeOn(Schedulers.boundedElastic()) - .flatMap(resolved -> resolved.isPresent() - ? Mono.<Void>empty() - : Mono.error(new AppsmithException(AppsmithError.GIT_REMOTE_URL_HOST_BLOCKED, host))); + .flatMap(resolved -> resolved + .map(Mono::just) + .orElseGet(() -> Mono.error(new AppsmithException(AppsmithError.GIT_REMOTE_URL_HOST_BLOCKED, host)))); }Then ensure callers connect using this resolved address (not a fresh DNS lookup from hostname).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java` around lines 93 - 97, The current GitUtils method discards the resolved InetAddress (calling WebClientUtils.resolveIfAllowed(host)) and returns Mono<Void>, leaving callers to re-resolve the hostname and enabling a DNS-rebinding TOCTOU; change the API to return (or emit) the pinned InetAddress (e.g., Mono<InetAddress> or a small value object with host+InetAddress) instead of Mono<Void>, propagate the resolved value from WebClientUtils.resolveIfAllowed(host) through this method, and then update downstream callers that perform git connections to use the provided/pinned InetAddress (not the hostname) when opening connections so no new DNS lookup occurs at connect time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java`:
- Around line 93-97: The current GitUtils method discards the resolved
InetAddress (calling WebClientUtils.resolveIfAllowed(host)) and returns
Mono<Void>, leaving callers to re-resolve the hostname and enabling a
DNS-rebinding TOCTOU; change the API to return (or emit) the pinned InetAddress
(e.g., Mono<InetAddress> or a small value object with host+InetAddress) instead
of Mono<Void>, propagate the resolved value from
WebClientUtils.resolveIfAllowed(host) through this method, and then update
downstream callers that perform git connections to use the provided/pinned
InetAddress (not the hostname) when opening connections so no new DNS lookup
occurs at connect time.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java`:
- Around line 437-448: The test validateGitSshUrl_PublicHosts_Allowed is
DNS-dependent; replace hostname-based inputs with deterministic IP-literal
variants so CI won't rely on network/DNS. Update the ValueSource strings passed
to GitUtils.validateGitSshUrl to use numeric IP literals (e.g.,
"git@192.0.2.1:user/repo.git", "ssh://git@192.0.2.1/user/repo.git",
"git@198.51.100.2:user/repo.git", etc.) or alternatively stub out the resolver
used by GitUtils.validateGitSshUrl, but keep references to the same test method
and GitUtils.validateGitSshUrl so the test logic is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac87d62a-3429-4b0b-b5e4-9d339dfd7468
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
… SSH URLs Address CodeRabbit review comments: - Add validatePersistedGitSshUrl() for lenient validation of stored remote URLs - Gate all 8 unprotected fetchRemoteReferences/fetchRemoteRepository paths in CentralGitServiceCEImpl with SSH URL validation - Replace DNS-dependent test inputs with deterministic IP literals for CI stability - Preserve AppsmithException in checkoutRemoteReference error handler Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
subrata71
commented
Apr 7, 2026
Comment on lines
+95
to
+99
| return Mono.fromCallable(() -> WebClientUtils.resolveIfAllowed(host)) | ||
| .subscribeOn(Schedulers.boundedElastic()) | ||
| .flatMap(resolved -> resolved.isPresent() | ||
| ? Mono.<Void>empty() | ||
| : Mono.error(new AppsmithException(AppsmithError.GIT_REMOTE_URL_HOST_BLOCKED, host))); |
Collaborator
Author
There was a problem hiding this comment.
@coderabbitai I think this is impractical because JGit’s SSH session factory resolves hostnames internally - there’s no API to pass a pre-resolved IP for SSH connections. The attack window is negligible (validation and connect happen in the same reactive chain), and SSH host key verification provides additional protection against rebinding. WDYT?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java (1)
414-425:metadata.google.internalmakes this blocked-host test ambiguous.If that hostname does not resolve in CI, this case only exercises the DNS-miss path, so the suite still passes when unresolved public hosts are incorrectly reported as
GIT_REMOTE_URL_HOST_BLOCKED. Prefer deterministic blocked literals here, or add a separate test that distinguishes unresolved hosts from blocked ones.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java` around lines 414 - 425, The parameterized test in GitUtilsTest that supplies hosts (the ValueSource in the parameterized test) includes "metadata.google.internal", which can be unresolved in CI and makes the blocked-host assertion ambiguous; replace that entry with a deterministic blocked literal (e.g., a known blocked hostname string such as "blocked-host.internal" or another explicit blocked domain) so the test always exercises the blocked-host path, and if you need to cover DNS-miss behavior add a separate parameterized case or a new test method that uses an obviously non-resolving host (e.g., "nonexistent-domain-for-test.invalid") to assert the DNS-miss path; update the ValueSource array in GitUtilsTest accordingly and/or add the new test method to distinguish blocked vs unresolved hosts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java`:
- Around line 761-762: In createReference(), the fetchRemoteMono (produced by
GitUtils.validatePersistedGitSshUrl(...).then(...)) can throw an
AppsmithException with code GIT_REMOTE_URL_HOST_BLOCKED but the downstream
onErrorResume unconditionally maps all errors to GIT_ACTION_FAILED; update the
onErrorResume that handles fetchRemoteMono so it preserves and re-throws
existing AppsmithException instances (specifically when error.getErrorCode() ==
GIT_REMOTE_URL_HOST_BLOCKED) instead of wrapping them—i.e., in the onErrorResume
for fetchRemoteMono check if the throwable is an AppsmithException and return
Mono.error(throwable) (or rethrow with the same code/message) otherwise map to a
new AppsmithException with GIT_ACTION_FAILED; reference fetchRemoteMono,
createReference(), and the onErrorResume handler to locate the change.
- Around line 3041-3046: The validator errors from
GitUtils.validatePersistedGitSshUrl are being swallowed because the downstream
error handlers in mergeBranch() and isBranchMergable() call Mono.error(error)
without returning it; update those reactive chains (the handlers that currently
invoke Mono.error(error)) to return Mono.error(error) (i.e., use "return
Mono.error(error)" or directly "Mono.error(error)" as the last expression in the
lambda) so the error is propagated, and apply the same fix to the similar
fetch/fetchingRemoteMono blocks referenced around the other handler (the
identical pattern at the other fetch-related block). Ensure each
flatMap/handle/otherwise lambda returns the Mono.error instead of just invoking
it so blocked-host/AppsmithException flows reach the caller.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java`:
- Around line 93-97: The code treats Optional.empty() from
WebClientUtils.resolveIfAllowed(...) as "host blocked" but that same return
value can mean DNS resolution failed; change the logic to first attempt explicit
DNS resolution and only then check allowlist: call a resolution API (e.g.,
WebClientUtils.resolve(host) or add a resolve-only variant) and if resolution
fails return a retryable/network error (not
AppsmithError.GIT_REMOTE_URL_HOST_BLOCKED), otherwise if resolved but not
allowed throw AppsmithException(AppsmithError.GIT_REMOTE_URL_HOST_BLOCKED,
host); apply the same change to both occurrences that use resolveIfAllowed (the
snippet around WebClientUtils.resolveIfAllowed and the later block at 119-123)
so typos/transient resolver failures are not turned into client 400 errors.
---
Nitpick comments:
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java`:
- Around line 414-425: The parameterized test in GitUtilsTest that supplies
hosts (the ValueSource in the parameterized test) includes
"metadata.google.internal", which can be unresolved in CI and makes the
blocked-host assertion ambiguous; replace that entry with a deterministic
blocked literal (e.g., a known blocked hostname string such as
"blocked-host.internal" or another explicit blocked domain) so the test always
exercises the blocked-host path, and if you need to cover DNS-miss behavior add
a separate parameterized case or a new test method that uses an obviously
non-resolving host (e.g., "nonexistent-domain-for-test.invalid") to assert the
DNS-miss path; update the ValueSource array in GitUtilsTest accordingly and/or
add the new test method to distinguish blocked vs unresolved hosts.
🪄 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: fb6151bb-be99-4047-ace4-79b02a11073d
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java
Comment on lines
+761
to
+762
| Mono<String> fetchRemoteMono = GitUtils.validatePersistedGitSshUrl(baseGitMetadata.getRemoteUrl()) | ||
| .then(gitHandlingService.fetchRemoteReferences(baseRefTransformationDTO, fetchRemoteDTO, baseGitAuth)); |
Contributor
There was a problem hiding this comment.
Preserve GIT_REMOTE_URL_HOST_BLOCKED in createReference().
fetchRemoteMono can now fail with AppsmithException, but the downstream onErrorResume at Line 767 rewrites every failure as GIT_ACTION_FAILED. The branch creation flow still aborts, but clients lose the specific 400 and message introduced by this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java`
around lines 761 - 762, In createReference(), the fetchRemoteMono (produced by
GitUtils.validatePersistedGitSshUrl(...).then(...)) can throw an
AppsmithException with code GIT_REMOTE_URL_HOST_BLOCKED but the downstream
onErrorResume unconditionally maps all errors to GIT_ACTION_FAILED; update the
onErrorResume that handles fetchRemoteMono so it preserves and re-throws
existing AppsmithException instances (specifically when error.getErrorCode() ==
GIT_REMOTE_URL_HOST_BLOCKED) instead of wrapping them—i.e., in the onErrorResume
for fetchRemoteMono check if the throwable is an AppsmithException and return
Mono.error(throwable) (or rethrow with the same code/message) otherwise map to a
new AppsmithException with GIT_ACTION_FAILED; reference fetchRemoteMono,
createReference(), and the onErrorResume handler to locate the change.
Comment on lines
+3041
to
+3046
| Mono<String> fetchingRemoteMono = | ||
| GitUtils.validatePersistedGitSshUrl(baseGitMetadata.getRemoteUrl()) | ||
| .then(gitHandlingService.fetchRemoteReferences( | ||
| jsonTransformationDTO, | ||
| fetchRemoteDTO, | ||
| baseGitMetadata.getGitAuth())); |
Contributor
There was a problem hiding this comment.
Merge-related status paths still swallow validator failures.
These fetch gates now emit AppsmithException, but the downstream handlers at Line 3107 and Line 3354 call Mono.error(error) without returning it. A blocked host is therefore rewritten as a generic status failure in mergeBranch() and as mergeAble=false in isBranchMergable().
Also applies to: 3263-3268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java`
around lines 3041 - 3046, The validator errors from
GitUtils.validatePersistedGitSshUrl are being swallowed because the downstream
error handlers in mergeBranch() and isBranchMergable() call Mono.error(error)
without returning it; update those reactive chains (the handlers that currently
invoke Mono.error(error)) to return Mono.error(error) (i.e., use "return
Mono.error(error)" or directly "Mono.error(error)" as the last expression in the
lambda) so the error is propagated, and apply the same fix to the similar
fetch/fetchingRemoteMono blocks referenced around the other handler (the
identical pattern at the other fetch-related block). Ensure each
flatMap/handle/otherwise lambda returns the Mono.error instead of just invoking
it so blocked-host/AppsmithException flows reach the caller.
Comment on lines
+93
to
+97
| return Mono.fromCallable(() -> WebClientUtils.resolveIfAllowed(host)) | ||
| .subscribeOn(Schedulers.boundedElastic()) | ||
| .flatMap(resolved -> resolved.isPresent() | ||
| ? Mono.<Void>empty() | ||
| : Mono.error(new AppsmithException(AppsmithError.GIT_REMOTE_URL_HOST_BLOCKED, host))); |
Contributor
There was a problem hiding this comment.
Separate DNS misses from blocked hosts.
WebClientUtils.resolveIfAllowed() also returns Optional.empty() when the host cannot be resolved, so these branches turn typos or transient resolver failures on public remotes into GIT_REMOTE_URL_HOST_BLOCKED. That changes a retryable/network failure into a 400 client error.
Also applies to: 119-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java`
around lines 93 - 97, The code treats Optional.empty() from
WebClientUtils.resolveIfAllowed(...) as "host blocked" but that same return
value can mean DNS resolution failed; change the logic to first attempt explicit
DNS resolution and only then check allowlist: call a resolution API (e.g.,
WebClientUtils.resolve(host) or add a resolve-only variant) and if resolution
fails return a retryable/network error (not
AppsmithError.GIT_REMOTE_URL_HOST_BLOCKED), otherwise if resolved but not
allowed throw AppsmithException(AppsmithError.GIT_REMOTE_URL_HOST_BLOCKED,
host); apply the same change to both occurrences that use resolveIfAllowed (the
snippet around WebClientUtils.resolveIfAllowed and the later block at 119-123)
so typos/transient resolver failures are not turned into client 400 errors.
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
1 participant
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.
Description
TL;DR: The Git integration (JGit SSH + native
git) bypassed the existingWebClientUtilsSSRF filter, allowing any authenticated user to initiate SSH connections to internal/reserved IPs (loopback, cloud metadata, link-local). This fix adds host validation to all Git SSH connection paths using the existingresolveIfAllowed()mechanism.Root Cause
Appsmith has two outbound connection paths:
WebClientUtils.IP_CHECK_FILTERand customNameResolver.When a user submitted
git@169.254.169.254:user/repo.git, theisRepoPrivate()HTTP probe was correctly blocked byWebClientUtils, but the SSH clone viacloneRemoteIntoArtifactRepo()proceeded without any check.Fix
Added
GitUtils.validateGitSshUrl()which:WebClientUtils.resolveIfAllowed(host)to resolve DNS and check against loopback, link-local, multicast, cloud metadata, and ULA address rangesMono.error(AppsmithException)if the host is blockedGated all Git SSH connection entry points:
CommonGitServiceCEImpl:connectArtifactToGit,importArtifactFromGit, repo recovery cloneCentralGitServiceCEImpl:connectArtifactToGit,importArtifactFromGitGitFSServiceCEImpl: bothfetchRemoteRepositoryoverloads (defense-in-depth)Regression Safety
convertSshUrlToBrowserSupportedUrl()is unchanged — existing URL display/conversion works as beforeGitUtilsTesttests continue to pass, plus 7 new tests addedFixes https://linear.app/appsmith/issue/APP-15043/security-high-ssrf-via-git-ssh-connection-jgit-code-path-bypasses
Fixes https://github.com/appsmithorg/appsmith/security/advisories/GHSA-h7pq-hq98-554w
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/24100712256
Commit: 80e6b7c
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 07 Apr 2026 20:57:36 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit