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
WalkthroughAdded server-side Git SSH remote URL validation that extracts and resolves hosts, blocking internal/reserved addresses and emitting a new GIT_REMOTE_URL_HOST_BLOCKED error; wired into import/connect/fetch flows and 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 Result as Result
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->>Result: proceed with clone/connect/import
Result-->>GitService: success
GitService-->>Client: Success
end
else extraction failed / invalid URL
GitUtils-->>GitService: Mono.error(AppsmithException: INVALID_GIT_CONFIGURATION / INVALID_PARAMETER)
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
Comment on lines
+234
to
+236
| Mono<? extends Artifact> blankArtifactForImportMono = GitUtils.validateGitSshUrl( | ||
| gitConnectDTO.getRemoteUrl()) | ||
| .then(isRepositoryLimitReachedForWorkspaceMono) |
Contributor
There was a problem hiding this comment.
This guard only covers new connect/import requests, not legacy stored remotes.
Lines 234-236 and 1087-1088 validate inbound gitConnectDTO URLs, but existing artifacts can still execute remote SSH operations with persisted remoteUrl values. Consider validating before outbound SSH operations (fetch/pull/push paths) or adding a migration to sanitize blocked stored remotes.
Also applies to: 1087-1088
🤖 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 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.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java
Outdated
Show resolved
Hide resolved
Comment on lines
+436
to
+447
| @ParameterizedTest | ||
| @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", | ||
| }) | ||
| public void validateGitSshUrl_PublicHosts_Allowed(String allowedUrl) { | ||
| StepVerifier.create(GitUtils.validateGitSshUrl(allowedUrl)) | ||
| .verifyComplete(); | ||
| } |
Contributor
There was a problem hiding this comment.
Public-host allowlist test cases are DNS-environment dependent.
These cases can fail in CI environments with restricted DNS/network, even when code is correct. Prefer deterministic inputs (e.g., known public IP literals) or mock/stub host resolution in unit tests.
🤖 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 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.
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))); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect validateGitSshUrl implementation"
rg -n -C5 '\bvalidateGitSshUrl\s*\(|resolveIfAllowed|GIT_REMOTE_URL_HOST_BLOCKED' \
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java
echo
echo "Find all call sites for validateGitSshUrl"
rg -n -C6 '\bvalidateGitSshUrl\s*\(' app/server/appsmith-server/src/main/java/com/appsmith/server/git
echo
echo "Inspect whether downstream git operations use hostname-based remoteUrl after validation"
rg -n -C6 '\b(fetchRemoteRepository|connectArtifactToGit|importArtifactFromGit|setURI|setRemote|cloneRepository|lsRemote)\b|remoteUrl' \
app/server/appsmith-server/src/main/java/com/appsmith/server/gitRepository: appsmithorg/appsmith
Length of output: 50376
DNS rebinding gap in validateGitSshUrl: resolved address is validated but discarded, allowing DNS rebinding between check and connect.
The validation at lines 95-99 resolves the hostname and confirms it's allowed, but discards the InetAddress and returns only Mono<Void>. All downstream call sites (GitFSServiceCEImpl, CentralGitServiceCEImpl, CommonGitServiceCEImpl) receive the original remoteUrl string containing the hostname, not the validated resolved address. Git operations will perform fresh DNS resolution at connection time, creating a window for DNS rebinding attacks to bypass the guard.
Either pin the validated resolved address for the actual connection, or make resolution and SSH connection atomic in one guarded step.
🤖 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 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.
…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
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
Warning
Tests have not run on the HEAD 41e14ea yet
Wed, 01 Apr 2026 19:30:30 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit