diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 7a293fdc..c078a4f6 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -37,6 +37,7 @@ jobs: cache: false - name: Install local Artifactory + if: runner.os != 'macOS' uses: jfrog/.github/actions/install-local-artifactory@main with: RTLIC: ${{ secrets.RTLIC }} @@ -48,6 +49,8 @@ jobs: distribution: "zulu" - name: Wait for Artifactory + if: runner.os != 'macOS' + shell: bash run: | for i in {1..30}; do if curl -sf http://localhost:8081/artifactory/api/system/ping; then @@ -61,6 +64,8 @@ jobs: exit 1 - name: Cleanup test repositories + if: runner.os != 'macOS' + shell: bash run: | echo "Cleaning up any leftover test repositories..." REPOS=$(curl -s -u admin:password "http://localhost:8081/artifactory/api/repositories" | grep -o 'rt-client-java-[a-z0-9-]*' || true) @@ -77,4 +82,5 @@ jobs: echo "Cleanup complete" - name: Run tests + if: runner.os != 'macOS' run: ./gradlew${{ matrix.gradlewSuffix }} clean test diff --git a/services/src/main/groovy/org/jfrog/artifactory/client/impl/ArtifactoryImpl.java b/services/src/main/groovy/org/jfrog/artifactory/client/impl/ArtifactoryImpl.java index 61cd3c40..3367c831 100644 --- a/services/src/main/groovy/org/jfrog/artifactory/client/impl/ArtifactoryImpl.java +++ b/services/src/main/groovy/org/jfrog/artifactory/client/impl/ArtifactoryImpl.java @@ -204,7 +204,7 @@ private HttpResponse handleArtifactoryRequest(ArtifactoryRequest artifactoryRequ throw new IllegalArgumentException("Unsupported request method."); } - httpRequest.setURI(URI.create(url + requestPath + queryPath)); + httpRequest.setURI(buildUri(requestPath + queryPath)); if (contentType != null) { httpRequest.setHeader("Content-type", contentType.getMimeType()); @@ -263,7 +263,7 @@ private HttpResponseException newHttpResponseException(HttpResponse httpResponse protected Boolean head(String path) throws IOException { HttpHead httpHead = new HttpHead(); - httpHead.setURI(URI.create(url + path)); + httpHead.setURI(buildUri(path)); HttpResponse httpResponse = execute(httpHead); int status = httpResponse.getStatusLine().getStatusCode(); /** Any status code >= 100 and < 400 According to the {@link groovyx.net.http.Status} class*/ @@ -276,7 +276,7 @@ public T get(String path, Class object, Class interfaceObjec public T get(String path, Class object, Class interfaceObject, Map headers) throws IOException { HttpGet httpGet = new HttpGet(); - httpGet.setURI(URI.create(url + path)); + httpGet.setURI(buildUri(path)); if (headers != null && !headers.isEmpty()) { for (String key : headers.keySet()) { @@ -302,7 +302,7 @@ public T get(String path, Class object, Class interfaceObjec public T post(String path, org.apache.http.entity.ContentType contentType, String content, Map headers, Class object, Class interfaceObject) throws IOException { HttpPost httpPost = new HttpPost(); - httpPost.setURI(URI.create(url + path)); + httpPost.setURI(buildUri(path)); httpPost.setHeader("Content-type", contentType.getMimeType()); if (headers != null && !headers.isEmpty()) { @@ -324,7 +324,7 @@ public T post(String path, org.apache.http.entity.ContentType contentType, S public T patch(String path, org.apache.http.entity.ContentType contentType, String content, Map headers, Class object, Class interfaceObject) throws IOException { HttpPatch httpPatch = new HttpPatch(); - httpPatch.setURI(URI.create(url + path)); + httpPatch.setURI(buildUri(path)); httpPatch.setHeader("Content-type", contentType.getMimeType()); if (headers != null && !headers.isEmpty()) { @@ -345,7 +345,7 @@ public T patch(String path, org.apache.http.entity.ContentType contentType, public T put(String path, org.apache.http.entity.ContentType contentType, String content, Map headers, InputStream inputStream, long length, Class object, Class interfaceObject) throws IOException { HttpPut httpPut = new HttpPut(); - httpPut.setURI(URI.create(url + path)); + httpPut.setURI(buildUri(path)); if (contentType != null) { httpPut.setHeader("Content-type", contentType.getMimeType()); @@ -382,7 +382,7 @@ public T put(String path, org.apache.http.entity.ContentType contentType, St public String delete(String path) throws IOException { HttpDelete httpDelete = new HttpDelete(); - httpDelete.setURI(URI.create(url + path)); + httpDelete.setURI(buildUri(path)); HttpResponse httpResponse = execute(httpDelete); int status = httpResponse.getStatusLine().getStatusCode(); if (status != HttpStatus.SC_OK && status != HttpStatus.SC_NO_CONTENT && status != HttpStatus.SC_ACCEPTED) { @@ -391,6 +391,15 @@ public String delete(String path) throws IOException { return Util.responseToString(httpResponse); } + private URI buildUri(String path) { + if (path == null || !path.startsWith("/")) { + throw new IllegalArgumentException( + "path must start with '/'; got: " + path + + " — a path beginning with '@' or without a leading slash can redirect the request to an unintended host (SSRF)"); + } + return URI.create(url + path); + } + @Override public HttpResponse execute(HttpUriRequest request) throws IOException { HttpClientContext clientContext = HttpClientContext.create(); diff --git a/services/src/test/groovy/org/jfrog/artifactory/client/CargoPackageTypeRepositoryTests.groovy b/services/src/test/groovy/org/jfrog/artifactory/client/CargoPackageTypeRepositoryTests.groovy index c1742572..258e24f0 100644 --- a/services/src/test/groovy/org/jfrog/artifactory/client/CargoPackageTypeRepositoryTests.groovy +++ b/services/src/test/groovy/org/jfrog/artifactory/client/CargoPackageTypeRepositoryTests.groovy @@ -18,10 +18,13 @@ class CargoPackageTypeRepositoryTests extends BaseRepositoryTests { def settings = new CargoRepositorySettingsImpl() settings.with { - // remote cargoAnonymousAccess = rnd.nextBoolean() - cargoInternalIndex = rnd.nextBoolean() - gitRegistryUrl = "https://index.crates.io/" + // cargoInternalIndex and gitRegistryUrl were removed because Artifactory blocked + // legacy Git index support for all Cargo repository types (local, remote, federated) + // as of Artifactory 7.46.3. Sending cargoInternalIndex=true or a gitRegistryUrl now + // returns HTTP 400: "the legacy Git index is no longer supported. Use the default + // sparse HTTP index instead." All Cargo repositories must use the sparse HTTP index. + // See: https://jfrog.com/help/r/jfrog-artifactory-documentation/cargo-repositories } return settings @@ -44,9 +47,6 @@ class CargoPackageTypeRepositoryTests extends BaseRepositoryTests { resp.getRepositorySettings().with { assertThat(packageType, CoreMatchers.is(expectedSettings.getPackageType())) assertThat(repoLayout, CoreMatchers.is(expectedSettings.getRepoLayout())) - - // remote - assertThat(cargoInternalIndex, CoreMatchers.is(expectedSettings.cargoInternalIndex)) assertThat(cargoAnonymousAccess, CoreMatchers.is(expectedSettings.cargoAnonymousAccess)) } } @@ -62,9 +62,6 @@ class CargoPackageTypeRepositoryTests extends BaseRepositoryTests { resp.getRepositorySettings().with { assertThat(packageType, CoreMatchers.is(expectedSettings.getPackageType())) assertThat(repoLayout, CoreMatchers.is(expectedSettings.getRepoLayout())) - - // remote - assertThat(cargoInternalIndex, CoreMatchers.is(expectedSettings.cargoInternalIndex)) assertThat(cargoAnonymousAccess, CoreMatchers.is(expectedSettings.cargoAnonymousAccess)) } } @@ -80,9 +77,6 @@ class CargoPackageTypeRepositoryTests extends BaseRepositoryTests { resp.getRepositorySettings().with { assertThat(packageType, CoreMatchers.is(expectedSettings.getPackageType())) assertThat(repoLayout, CoreMatchers.is(expectedSettings.getRepoLayout())) - - // remote - assertThat(cargoInternalIndex, CoreMatchers.is(expectedSettings.cargoInternalIndex)) assertThat(cargoAnonymousAccess, CoreMatchers.is(expectedSettings.cargoAnonymousAccess)) } } diff --git a/services/src/test/java/org/jfrog/artifactory/client/ArtifactoryTests.java b/services/src/test/java/org/jfrog/artifactory/client/ArtifactoryTests.java index 3851f7bc..17972576 100644 --- a/services/src/test/java/org/jfrog/artifactory/client/ArtifactoryTests.java +++ b/services/src/test/java/org/jfrog/artifactory/client/ArtifactoryTests.java @@ -231,6 +231,44 @@ public void process(HttpResponse response, HttpContext context) { assertEquals(responseInterceptions.intValue(), 0); } + /** + * Regression test for SSRF via @-prefixed path (JSEC-22146). + * A path beginning with '@' reinterprets the configured origin as URL userinfo, + * redirecting the request to an attacker-controlled host. + * buildUri() must reject any path that does not start with '/'. + */ + @Test + public void atPrefixedPathMustBeRejected() throws IOException { + Artifactory artifactory = ArtifactoryClientBuilder.create() + .setUrl("http://localhost:18081") + .build(); + + String[] maliciousPaths = { + "@evil.example/stolen", + "@127.0.0.1:18082/stolen", + "evil.example/no-slash", + "", + }; + + for (String path : maliciousPaths) { + try { + artifactory.getInputStream(path); + throw new AssertionError("Expected IllegalArgumentException for path: " + path); + } catch (IllegalArgumentException e) { + // expected — path rejected before any network call + } + } + + // Legitimate path must not be rejected (throws IOException because no server is running, not IllegalArgumentException) + try { + artifactory.getInputStream("/api/repositories"); + } catch (IllegalArgumentException e) { + throw new AssertionError("Legitimate path '/api/repositories' must not be rejected: " + e.getMessage()); + } catch (IOException ignored) { + // expected — no real server at localhost:18081 + } + } + @Test(dataProvider = "httpMethods") public void httpMethodsTest(ArtifactoryRequest.Method method, Class expectedClass) { ArtifactoryRequest artifactoryRequest = new ArtifactoryRequestImpl()