diff --git a/release-notes/opensearch-geospatial.release-notes-3.2.0.0.md b/release-notes/opensearch-geospatial.release-notes-3.2.0.0.md index e8107a8c..2d5e034f 100644 --- a/release-notes/opensearch-geospatial.release-notes-3.2.0.0.md +++ b/release-notes/opensearch-geospatial.release-notes-3.2.0.0.md @@ -2,6 +2,9 @@ Compatible with OpenSearch and OpenSearch Dashboards version 3.2.0 +### Bug Fixes +* Block redirect in IP2Geo and move validation to transport action ([#782](https://github.com/opensearch-project/geospatial/pull/782)) + ### Maintenance * Upgrade gradle to 8.14.3 and run CI checks with JDK24 ([#776](https://github.com/opensearch-project/geospatial/pull/776)) diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequest.java b/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequest.java index 42072d6c..26a17a58 100644 --- a/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequest.java +++ b/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequest.java @@ -10,7 +10,6 @@ import java.net.URISyntaxException; import java.net.URL; import java.util.List; -import java.util.Locale; import org.opensearch.action.ActionRequest; import org.opensearch.action.ActionRequestValidationException; @@ -19,7 +18,6 @@ import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.core.xcontent.ObjectParser; -import org.opensearch.geospatial.ip2geo.common.DatasourceManifest; import org.opensearch.geospatial.ip2geo.common.ParameterValidator; import lombok.Getter; @@ -106,7 +104,6 @@ public ActionRequestValidationException validate() { /** * Conduct following validation on endpoint * 1. endpoint format complies with RFC-2396 - * 2. validate manifest file from the endpoint * * @param errors the errors to add error messages */ @@ -114,52 +111,12 @@ private void validateEndpoint(final ActionRequestValidationException errors) { try { URL url = new URL(endpoint); url.toURI(); // Validate URL complies with RFC-2396 - validateManifestFile(url, errors); } catch (MalformedURLException | URISyntaxException e) { log.info("Invalid URL[{}] is provided", endpoint, e); errors.addValidationError("Invalid URL format is provided"); } } - /** - * Conduct following validation on url - * 1. can read manifest file from the endpoint - * 2. the url in the manifest file complies with RFC-2396 - * 3. updateInterval is less than validForInDays value in the manifest file - * - * @param url the url to validate - * @param errors the errors to add error messages - */ - private void validateManifestFile(final URL url, final ActionRequestValidationException errors) { - DatasourceManifest manifest; - try { - manifest = DatasourceManifest.Builder.build(url); - } catch (Exception e) { - log.info("Error occurred while reading a file from {}", url, e); - errors.addValidationError(String.format(Locale.ROOT, "Error occurred while reading a file from %s: %s", url, e.getMessage())); - return; - } - - try { - new URL(manifest.getUrl()).toURI(); // Validate URL complies with RFC-2396 - } catch (MalformedURLException | URISyntaxException e) { - log.info("Invalid URL[{}] is provided for url field in the manifest file", manifest.getUrl(), e); - errors.addValidationError("Invalid URL format is provided for url field in the manifest file"); - return; - } - - if (manifest.getValidForInDays() != null && updateInterval.days() >= manifest.getValidForInDays()) { - errors.addValidationError( - String.format( - Locale.ROOT, - "updateInterval %d should be smaller than %d", - updateInterval.days(), - manifest.getValidForInDays() - ) - ); - } - } - /** * Validate updateInterval is equal or larger than 1 * diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceTransportAction.java b/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceTransportAction.java index e943e3ac..6df1c7f5 100644 --- a/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceTransportAction.java +++ b/src/main/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceTransportAction.java @@ -7,7 +7,11 @@ import static org.opensearch.geospatial.ip2geo.common.Ip2GeoLockService.LOCK_DURATION_IN_SECONDS; +import java.net.MalformedURLException; +import java.net.URISyntaxException; +import java.net.URL; import java.time.Instant; +import java.util.Locale; import java.util.concurrent.atomic.AtomicReference; import org.opensearch.ResourceAlreadyExistsException; @@ -20,6 +24,7 @@ import org.opensearch.core.action.ActionListener; import org.opensearch.geospatial.annotation.VisibleForTesting; import org.opensearch.geospatial.exceptions.ConcurrentModificationException; +import org.opensearch.geospatial.ip2geo.common.DatasourceManifest; import org.opensearch.geospatial.ip2geo.common.DatasourceState; import org.opensearch.geospatial.ip2geo.common.Ip2GeoLockService; import org.opensearch.geospatial.ip2geo.dao.DatasourceDao; @@ -70,6 +75,7 @@ public PutDatasourceTransportAction( @Override protected void doExecute(final Task task, final PutDatasourceRequest request, final ActionListener listener) { + validateManifestFile(request); lockService.acquireLock(request.getName(), LOCK_DURATION_IN_SECONDS, ActionListener.wrap(lock -> { if (lock == null) { listener.onFailure( @@ -170,4 +176,43 @@ private void markDatasourceAsCreateFailed(final Datasource datasource) { log.error("Failed to mark datasource state as CREATE_FAILED for {}", datasource.getName(), e); } } + + /** + * Conduct the following validation on request + * 1. can read the manifest file from the endpoint + * 2. the url in the manifest file complies with RFC-2396 + * 3. updateInterval is less than validForInDays value in the manifest file + * + * @param request the request to validate + */ + private void validateManifestFile(final PutDatasourceRequest request) { + DatasourceManifest manifest; + try { + URL url = new URL(request.getEndpoint()); + manifest = DatasourceManifest.Builder.build(url); + } catch (Exception e) { + log.info("Error occurred while reading a file from {}", request.getEndpoint(), e); + throw new IllegalArgumentException( + String.format(Locale.ROOT, "Error occurred while reading a file from %s: %s", request.getEndpoint(), e.getMessage()) + ); + } + + try { + new URL(manifest.getUrl()).toURI(); // Validate URL complies with RFC-2396 + } catch (MalformedURLException | URISyntaxException e) { + log.info("Invalid URL[{}] is provided for url field in the manifest file", manifest.getUrl(), e); + throw new IllegalArgumentException("Invalid URL format is provided for url field in the manifest file"); + } + + if (manifest.getValidForInDays() != null && request.getUpdateInterval().days() >= manifest.getValidForInDays()) { + throw new IllegalArgumentException( + String.format( + Locale.ROOT, + "updateInterval %d should be smaller than %d", + request.getUpdateInterval().days(), + manifest.getValidForInDays() + ) + ); + } + } } diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceRequest.java b/src/main/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceRequest.java index 64d3deb0..17f335a8 100644 --- a/src/main/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceRequest.java +++ b/src/main/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceRequest.java @@ -9,7 +9,6 @@ import java.net.MalformedURLException; import java.net.URISyntaxException; import java.net.URL; -import java.util.Locale; import org.opensearch.action.ActionRequest; import org.opensearch.action.ActionRequestValidationException; @@ -18,7 +17,6 @@ import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.core.xcontent.ObjectParser; -import org.opensearch.geospatial.ip2geo.common.DatasourceManifest; import org.opensearch.geospatial.ip2geo.common.ParameterValidator; import lombok.EqualsAndHashCode; @@ -124,39 +122,12 @@ private void validateEndpoint(final ActionRequestValidationException errors) { try { URL url = new URL(endpoint); url.toURI(); // Validate URL complies with RFC-2396 - validateManifestFile(url, errors); } catch (MalformedURLException | URISyntaxException e) { log.info("Invalid URL[{}] is provided", endpoint, e); errors.addValidationError("Invalid URL format is provided"); } } - /** - * Conduct following validation on url - * 1. can read manifest file from the endpoint - * 2. the url in the manifest file complies with RFC-2396 - * - * @param url the url to validate - * @param errors the errors to add error messages - */ - private void validateManifestFile(final URL url, final ActionRequestValidationException errors) { - DatasourceManifest manifest; - try { - manifest = DatasourceManifest.Builder.build(url); - } catch (Exception e) { - log.info("Error occurred while reading a file from {}", url, e); - errors.addValidationError(String.format(Locale.ROOT, "Error occurred while reading a file from %s: %s", url, e.getMessage())); - return; - } - - try { - new URL(manifest.getUrl()).toURI(); // Validate URL complies with RFC-2396 - } catch (MalformedURLException | URISyntaxException e) { - log.info("Invalid URL[{}] is provided for url field in the manifest file", manifest.getUrl(), e); - errors.addValidationError("Invalid URL format is provided for url field in the manifest file"); - } - } - /** * Validate updateInterval is equal or larger than 1 * diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportAction.java b/src/main/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportAction.java index 98c87eb4..7103e791 100644 --- a/src/main/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportAction.java +++ b/src/main/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportAction.java @@ -6,6 +6,8 @@ package org.opensearch.geospatial.ip2geo.action; import java.io.IOException; +import java.net.MalformedURLException; +import java.net.URISyntaxException; import java.net.URL; import java.security.InvalidParameterException; import java.time.Instant; @@ -80,6 +82,7 @@ public UpdateDatasourceTransportAction( */ @Override protected void doExecute(final Task task, final UpdateDatasourceRequest request, final ActionListener listener) { + validateManifestFile(request); lockService.acquireLock(request.getName(), LOCK_DURATION_IN_SECONDS, ActionListener.wrap(lock -> { if (lock == null) { listener.onFailure( @@ -219,4 +222,35 @@ private boolean isEndpointChanged(final UpdateDatasourceRequest request, final D private boolean isUpdateIntervalChanged(final UpdateDatasourceRequest request) { return request.getUpdateInterval() != null; } + + /** + * Conduct the following validation on url + * 1. can read the manifest file from the endpoint + * 2. the url in the manifest file complies with RFC-2396 + * + * @param request the request to validate + */ + private void validateManifestFile(final UpdateDatasourceRequest request) { + if (request.getEndpoint() == null) { + return; + } + + DatasourceManifest manifest; + try { + URL url = new URL(request.getEndpoint()); + manifest = DatasourceManifest.Builder.build(url); + } catch (Exception e) { + log.info("Error occurred while reading a file from {}", request.getEndpoint(), e); + throw new IllegalArgumentException( + String.format(Locale.ROOT, "Error occurred while reading a file from %s: %s", request.getEndpoint(), e.getMessage()) + ); + } + + try { + new URL(manifest.getUrl()).toURI(); // Validate URL complies with RFC-2396 + } catch (MalformedURLException | URISyntaxException e) { + log.info("Invalid URL[{}] is provided for url field in the manifest file", manifest.getUrl(), e); + throw new IllegalArgumentException("Invalid URL format is provided for url field in the manifest file"); + } + } } diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/common/DatasourceManifest.java b/src/main/java/org/opensearch/geospatial/ip2geo/common/DatasourceManifest.java index 2c67e26e..975d330b 100644 --- a/src/main/java/org/opensearch/geospatial/ip2geo/common/DatasourceManifest.java +++ b/src/main/java/org/opensearch/geospatial/ip2geo/common/DatasourceManifest.java @@ -8,6 +8,7 @@ import java.io.BufferedReader; import java.io.IOException; import java.io.InputStreamReader; +import java.net.HttpURLConnection; import java.net.URL; import java.net.URLConnection; import java.nio.CharBuffer; @@ -129,6 +130,9 @@ public static DatasourceManifest build(final URL url) { @SuppressForbidden(reason = "Need to connect to http endpoint to read manifest file") protected static DatasourceManifest internalBuild(final URLConnection connection) throws IOException { connection.addRequestProperty(Constants.USER_AGENT_KEY, Constants.USER_AGENT_VALUE); + if (connection instanceof HttpURLConnection) { + HttpRedirectValidator.validateNoRedirects((HttpURLConnection) connection); + } InputStreamReader inputStreamReader = new InputStreamReader(connection.getInputStream()); try (BufferedReader reader = new BufferedReader(inputStreamReader)) { CharBuffer charBuffer = CharBuffer.allocate(MANIFEST_FILE_MAX_BYTES); diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/common/HttpRedirectValidator.java b/src/main/java/org/opensearch/geospatial/ip2geo/common/HttpRedirectValidator.java new file mode 100644 index 00000000..b1f3968e --- /dev/null +++ b/src/main/java/org/opensearch/geospatial/ip2geo/common/HttpRedirectValidator.java @@ -0,0 +1,50 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.geospatial.ip2geo.common; + +import java.io.IOException; +import java.net.HttpURLConnection; +import java.util.Locale; + +import lombok.extern.log4j.Log4j2; + +/** + * Utility class for validating HTTP connections no redirects + */ +@Log4j2 +public class HttpRedirectValidator { + private static final int HTTP_REDIRECT_STATUS_MIN = 300; + private static final int HTTP_REDIRECT_STATUS_MAX = 400; + private static final String LOCATION_HEADER = "Location"; + + // Private constructor to prevent instantiation + private HttpRedirectValidator() {} + + /** + * Validates that an HTTP connection does not attempt to redirect + * + * @param httpConnection the HTTP connection to validate + * @throws IOException if an I/O error occurs + * @throws IllegalArgumentException if a redirect attempt is detected + */ + public static void validateNoRedirects(final HttpURLConnection httpConnection) throws IOException { + httpConnection.setInstanceFollowRedirects(false); + + final int responseCode = httpConnection.getResponseCode(); + if (responseCode >= HTTP_REDIRECT_STATUS_MIN && responseCode < HTTP_REDIRECT_STATUS_MAX) { + final String redirectLocation = httpConnection.getHeaderField(LOCATION_HEADER); + throw new IllegalArgumentException( + String.format( + Locale.ROOT, + "HTTP redirects are not allowed. URL [%s] attempted to redirect to [%s] with status code [%d]", + httpConnection.getURL().toString(), + redirectLocation != null ? redirectLocation : "unknown", + responseCode + ) + ); + } + } +} diff --git a/src/main/java/org/opensearch/geospatial/ip2geo/dao/GeoIpDataDao.java b/src/main/java/org/opensearch/geospatial/ip2geo/dao/GeoIpDataDao.java index 65937a55..967b2d24 100644 --- a/src/main/java/org/opensearch/geospatial/ip2geo/dao/GeoIpDataDao.java +++ b/src/main/java/org/opensearch/geospatial/ip2geo/dao/GeoIpDataDao.java @@ -11,6 +11,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.net.HttpURLConnection; import java.net.URL; import java.net.URLConnection; import java.nio.charset.StandardCharsets; @@ -53,6 +54,7 @@ import org.opensearch.geospatial.annotation.VisibleForTesting; import org.opensearch.geospatial.constants.IndexSetting; import org.opensearch.geospatial.ip2geo.common.DatasourceManifest; +import org.opensearch.geospatial.ip2geo.common.HttpRedirectValidator; import org.opensearch.geospatial.ip2geo.common.Ip2GeoSettings; import org.opensearch.geospatial.ip2geo.common.URLDenyListChecker; import org.opensearch.geospatial.shared.Constants; @@ -169,7 +171,8 @@ public CSVParser getDatabaseReader(final DatasourceManifest manifest) { return AccessController.doPrivileged(() -> { try { URL zipUrl = urlDenyListChecker.toUrlIfNotInDenyList(manifest.getUrl()); - return internalGetDatabaseReader(manifest, zipUrl.openConnection()); + URLConnection connection = zipUrl.openConnection(); + return internalGetDatabaseReader(manifest, connection); } catch (IOException e) { throw new OpenSearchException("failed to read geoip data from {}", manifest.getUrl(), e); } @@ -180,6 +183,9 @@ public CSVParser getDatabaseReader(final DatasourceManifest manifest) { @SuppressForbidden(reason = "Need to connect to http endpoint to read GeoIP database file") protected CSVParser internalGetDatabaseReader(final DatasourceManifest manifest, final URLConnection connection) throws IOException { connection.addRequestProperty(Constants.USER_AGENT_KEY, Constants.USER_AGENT_VALUE); + if (connection instanceof HttpURLConnection) { + HttpRedirectValidator.validateNoRedirects((HttpURLConnection) connection); + } ZipInputStream zipIn = new ZipInputStream(connection.getInputStream()); ZipEntry zipEntry = zipIn.getNextEntry(); while (zipEntry != null) { diff --git a/src/test/java/org/opensearch/geospatial/ip2geo/Ip2GeoTestCase.java b/src/test/java/org/opensearch/geospatial/ip2geo/Ip2GeoTestCase.java index acb54ba1..68133aef 100644 --- a/src/test/java/org/opensearch/geospatial/ip2geo/Ip2GeoTestCase.java +++ b/src/test/java/org/opensearch/geospatial/ip2geo/Ip2GeoTestCase.java @@ -180,8 +180,18 @@ protected String sampleManifestUrl() { return Paths.get(this.getClass().getClassLoader().getResource("ip2geo/manifest.json").toURI()).toUri().toURL().toExternalForm(); } + @SneakyThrows + @SuppressForbidden(reason = "unit test") + protected String anotherSampleManifestUrl() { + return Paths.get(this.getClass().getClassLoader().getResource("ip2geo/another_manifest.json").toURI()) + .toUri() + .toURL() + .toExternalForm(); + } + + @SneakyThrows @SuppressForbidden(reason = "unit test") - protected String sampleManifestUrlWithInvalidUrl() throws Exception { + protected String sampleManifestUrlWithInvalidUrl() { return Paths.get(this.getClass().getClassLoader().getResource("ip2geo/manifest_invalid_url.json").toURI()) .toUri() .toURL() @@ -222,7 +232,7 @@ protected Datasource randomDatasource(final Instant updateStartTime) { datasource.setState(randomState()); datasource.setCurrentIndex(datasource.newIndexName(UUID.randomUUID().toString())); datasource.setIndices(Arrays.asList(GeospatialTestHelper.randomLowerCaseString(), GeospatialTestHelper.randomLowerCaseString())); - datasource.setEndpoint(String.format(Locale.ROOT, "https://%s.com/manifest.json", GeospatialTestHelper.randomLowerCaseString())); + datasource.setEndpoint(sampleManifestUrl()); datasource.getDatabase() .setFields(Arrays.asList(GeospatialTestHelper.randomLowerCaseString(), GeospatialTestHelper.randomLowerCaseString())); datasource.getDatabase().setProvider(GeospatialTestHelper.randomLowerCaseString()); diff --git a/src/test/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequestTests.java b/src/test/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequestTests.java index b06651f2..4d4fc8c2 100644 --- a/src/test/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequestTests.java +++ b/src/test/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceRequestTests.java @@ -27,21 +27,6 @@ public void testValidate_whenInvalidUrl_thenFails() { assertEquals("Invalid URL format is provided", exception.validationErrors().get(0)); } - public void testValidate_whenInvalidManifestFile_thenFails() { - String datasourceName = GeospatialTestHelper.randomLowerCaseString(); - String domain = GeospatialTestHelper.randomLowerCaseString(); - PutDatasourceRequest request = new PutDatasourceRequest(datasourceName); - request.setEndpoint(String.format(Locale.ROOT, "https://%s.com", domain)); - request.setUpdateInterval(TimeValue.timeValueDays(1)); - - // Run - ActionRequestValidationException exception = request.validate(); - - // Verify - assertEquals(1, exception.validationErrors().size()); - assertTrue(exception.validationErrors().get(0).contains("Error occurred while reading a file")); - } - public void testValidate_whenValidInput_thenSucceed() { String datasourceName = GeospatialTestHelper.randomLowerCaseString(); PutDatasourceRequest request = new PutDatasourceRequest(datasourceName); @@ -82,34 +67,6 @@ public void testValidate_whenZeroUpdateInterval_thenFails() throws Exception { ); } - public void testValidate_whenLargeUpdateInterval_thenFail() throws Exception { - String datasourceName = GeospatialTestHelper.randomLowerCaseString(); - PutDatasourceRequest request = new PutDatasourceRequest(datasourceName); - request.setEndpoint(sampleManifestUrl()); - request.setUpdateInterval(TimeValue.timeValueDays(30)); - - // Run - ActionRequestValidationException exception = request.validate(); - - // Verify - assertEquals(1, exception.validationErrors().size()); - assertTrue(exception.validationErrors().get(0).contains("should be smaller")); - } - - public void testValidate_whenInvalidUrlInsideManifest_thenFail() throws Exception { - String datasourceName = GeospatialTestHelper.randomLowerCaseString(); - PutDatasourceRequest request = new PutDatasourceRequest(datasourceName); - request.setEndpoint(sampleManifestUrlWithInvalidUrl()); - request.setUpdateInterval(TimeValue.timeValueDays(1)); - - // Run - ActionRequestValidationException exception = request.validate(); - - // Verify - assertEquals(1, exception.validationErrors().size()); - assertTrue(exception.validationErrors().get(0).contains("Invalid URL format")); - } - public void testStreamInOut_whenValidInput_thenSucceed() throws Exception { String datasourceName = GeospatialTestHelper.randomLowerCaseString(); String domain = GeospatialTestHelper.randomLowerCaseString(); diff --git a/src/test/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceTransportActionTests.java b/src/test/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceTransportActionTests.java index ef8a8a35..4037f54f 100644 --- a/src/test/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceTransportActionTests.java +++ b/src/test/java/org/opensearch/geospatial/ip2geo/action/PutDatasourceTransportActionTests.java @@ -14,6 +14,7 @@ import static org.mockito.Mockito.verify; import java.io.IOException; +import java.util.Locale; import org.junit.Before; import org.mockito.ArgumentCaptor; @@ -72,6 +73,8 @@ private void validateDoExecute(final LockModel lockModel, final Exception before Task task = mock(Task.class); Datasource datasource = randomDatasource(); PutDatasourceRequest request = new PutDatasourceRequest(datasource.getName()); + request.setEndpoint(sampleManifestUrl()); + request.setUpdateInterval(TimeValue.timeValueDays(1)); ActionListener listener = mock(ActionListener.class); if (after != null) { doThrow(after).when(datasourceDao).createIndexIfNotExists(any(StepListener.class)); @@ -106,6 +109,53 @@ private void validateDoExecute(final LockModel lockModel, final Exception before } } + @SneakyThrows + public void testDoExecute_whenLargeUpdateInterval_thenFail() { + String datasourceName = GeospatialTestHelper.randomLowerCaseString(); + Task task = mock(Task.class); + PutDatasourceRequest request = new PutDatasourceRequest(datasourceName); + request.setEndpoint(sampleManifestUrl()); + request.setUpdateInterval(TimeValue.timeValueDays(30)); + ActionListener listener = mock(ActionListener.class); + + // Run + Throwable throwable = expectThrows(IllegalArgumentException.class, () -> action.doExecute(task, request, listener)); + + // Verify + assertTrue(throwable.getMessage().contains("should be smaller")); + } + + @SneakyThrows + public void testDoExecute_whenInvalidUrlInsideManifest_thenFail() { + String datasourceName = GeospatialTestHelper.randomLowerCaseString(); + Task task = mock(Task.class); + PutDatasourceRequest request = new PutDatasourceRequest(datasourceName); + request.setEndpoint(sampleManifestUrlWithInvalidUrl()); + ActionListener listener = mock(ActionListener.class); + + // Run + Throwable throwable = expectThrows(IllegalArgumentException.class, () -> action.doExecute(task, request, listener)); + + // Verify + assertTrue(throwable.getMessage().contains("Invalid URL format")); + } + + @SneakyThrows + public void testDoExecute_whenInvalidManifestFile_thenFails() { + String domain = GeospatialTestHelper.randomLowerCaseString(); + String datasourceName = GeospatialTestHelper.randomLowerCaseString(); + Task task = mock(Task.class); + PutDatasourceRequest request = new PutDatasourceRequest(datasourceName); + request.setEndpoint(String.format(Locale.ROOT, "https://%s.com", domain)); + ActionListener listener = mock(ActionListener.class); + + // Run + Throwable throwable = expectThrows(IllegalArgumentException.class, () -> action.doExecute(task, request, listener)); + + // Verify + assertTrue(throwable.getMessage().contains("Error occurred while reading a file")); + } + @SneakyThrows public void testInternalDoExecute_whenValidInput_thenSucceed() { PutDatasourceRequest request = new PutDatasourceRequest(GeospatialTestHelper.randomLowerCaseString()); diff --git a/src/test/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceRequestTests.java b/src/test/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceRequestTests.java index 4c346d4c..6d8c7463 100644 --- a/src/test/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceRequestTests.java +++ b/src/test/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceRequestTests.java @@ -44,20 +44,6 @@ public void testValidate_whenInvalidUrl_thenFails() { assertEquals("Invalid URL format is provided", exception.validationErrors().get(0)); } - public void testValidate_whenInvalidManifestFile_thenFails() { - String datasourceName = GeospatialTestHelper.randomLowerCaseString(); - String domain = GeospatialTestHelper.randomLowerCaseString(); - UpdateDatasourceRequest request = new UpdateDatasourceRequest(datasourceName); - request.setEndpoint(String.format(Locale.ROOT, "https://%s.com", domain)); - - // Run - ActionRequestValidationException exception = request.validate(); - - // Verify - assertEquals(1, exception.validationErrors().size()); - assertTrue(exception.validationErrors().get(0).contains("Error occurred while reading a file")); - } - @SneakyThrows public void testValidate_whenValidInput_thenSucceed() { String datasourceName = GeospatialTestHelper.randomLowerCaseString(); @@ -100,21 +86,6 @@ public void testValidate_whenZeroUpdateInterval_thenFails() { ); } - @SneakyThrows - public void testValidate_whenInvalidUrlInsideManifest_thenFail() { - String datasourceName = GeospatialTestHelper.randomLowerCaseString(); - UpdateDatasourceRequest request = new UpdateDatasourceRequest(datasourceName); - request.setEndpoint(sampleManifestUrlWithInvalidUrl()); - request.setUpdateInterval(TimeValue.timeValueDays(1)); - - // Run - ActionRequestValidationException exception = request.validate(); - - // Verify - assertEquals(1, exception.validationErrors().size()); - assertTrue(exception.validationErrors().get(0).contains("Invalid URL format")); - } - @SneakyThrows public void testStreamInOut_whenValidInput_thenSucceed() { String datasourceName = GeospatialTestHelper.randomLowerCaseString(); diff --git a/src/test/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportActionTests.java b/src/test/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportActionTests.java index 25ea7705..e5eab372 100644 --- a/src/test/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportActionTests.java +++ b/src/test/java/org/opensearch/geospatial/ip2geo/action/UpdateDatasourceTransportActionTests.java @@ -18,6 +18,7 @@ import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.List; +import java.util.Locale; import org.junit.Before; import org.mockito.ArgumentCaptor; @@ -26,6 +27,7 @@ import org.opensearch.action.support.clustermanager.AcknowledgedResponse; import org.opensearch.common.unit.TimeValue; import org.opensearch.core.action.ActionListener; +import org.opensearch.geospatial.GeospatialTestHelper; import org.opensearch.geospatial.exceptions.IncompatibleDatasourceException; import org.opensearch.geospatial.ip2geo.Ip2GeoTestCase; import org.opensearch.geospatial.ip2geo.common.DatasourceState; @@ -92,7 +94,7 @@ public void testDoExecute_whenValidInput_thenUpdate() { datasource.setTask(DatasourceTask.DELETE_UNUSED_INDICES); Instant originalStartTime = datasource.getSchedule().getStartTime(); UpdateDatasourceRequest request = new UpdateDatasourceRequest(datasource.getName()); - request.setEndpoint(sampleManifestUrl()); + request.setEndpoint(anotherSampleManifestUrl()); request.setUpdateInterval(TimeValue.timeValueDays(datasource.getSchedule().getInterval())); Task task = mock(Task.class); @@ -185,7 +187,6 @@ public void testDoExecute_whenNotInAvailableState_thenError() { Datasource datasource = randomDatasource(); datasource.setState(DatasourceState.CREATE_FAILED); UpdateDatasourceRequest request = new UpdateDatasourceRequest(datasource.getName()); - request.setEndpoint(datasource.getEndpoint()); Task task = mock(Task.class); when(datasourceDao.getDatasource(datasource.getName())).thenReturn(datasource); @@ -215,7 +216,7 @@ public void testDoExecute_whenIncompatibleFields_thenError() { Datasource datasource = randomDatasource(); datasource.setState(DatasourceState.AVAILABLE); UpdateDatasourceRequest request = new UpdateDatasourceRequest(datasource.getName()); - request.setEndpoint(sampleManifestUrl()); + request.setEndpoint(anotherSampleManifestUrl()); Task task = mock(Task.class); when(datasourceDao.getDatasource(datasource.getName())).thenReturn(datasource); @@ -302,4 +303,43 @@ public void testDoExecute_whenExpireWithNewUpdateInterval_thenError() { exceptionCaptor.getValue().getMessage().contains("will expire"); verify(ip2GeoLockService).releaseLock(eq(lockModel)); } + + @SneakyThrows + public void testDoExecute_whenInvalidUrlInsideManifest_thenFail() { + Datasource datasource = randomDatasource(); + datasource.getUpdateStats().setLastSkippedAt(null); + datasource.getUpdateStats().setLastSucceededAt(Instant.now().minus(datasource.getDatabase().getValidForInDays(), ChronoUnit.DAYS)); + UpdateDatasourceRequest request = new UpdateDatasourceRequest(datasource.getName()); + request.setEndpoint(sampleManifestUrlWithInvalidUrl()); + + Task task = mock(Task.class); + when(datasourceDao.getDatasource(datasource.getName())).thenReturn(datasource); + ActionListener listener = mock(ActionListener.class); + + // Run + Throwable throwable = expectThrows(IllegalArgumentException.class, () -> action.doExecute(task, request, listener)); + + // Verify + assertTrue(throwable.getMessage().contains("Invalid URL format is provided for url field in the manifest file")); + } + + @SneakyThrows + public void testDoExecute_whenInvalidManifestFile_thenFails() { + String domain = GeospatialTestHelper.randomLowerCaseString(); + Datasource datasource = randomDatasource(); + datasource.getUpdateStats().setLastSkippedAt(null); + datasource.getUpdateStats().setLastSucceededAt(Instant.now().minus(datasource.getDatabase().getValidForInDays(), ChronoUnit.DAYS)); + UpdateDatasourceRequest request = new UpdateDatasourceRequest(datasource.getName()); + request.setEndpoint(String.format(Locale.ROOT, "https://%s.com", domain)); + + Task task = mock(Task.class); + when(datasourceDao.getDatasource(datasource.getName())).thenReturn(datasource); + ActionListener listener = mock(ActionListener.class); + + // Run + Throwable throwable = expectThrows(RuntimeException.class, () -> action.doExecute(task, request, listener)); + + // Verify + assertTrue(throwable.getMessage().contains("Error occurred while reading a file from")); + } } diff --git a/src/test/java/org/opensearch/geospatial/ip2geo/common/HttpRedirectValidatorTests.java b/src/test/java/org/opensearch/geospatial/ip2geo/common/HttpRedirectValidatorTests.java new file mode 100644 index 00000000..2385cada --- /dev/null +++ b/src/test/java/org/opensearch/geospatial/ip2geo/common/HttpRedirectValidatorTests.java @@ -0,0 +1,70 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.geospatial.ip2geo.common; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.net.HttpURLConnection; + +import org.opensearch.test.OpenSearchTestCase; + +import lombok.SneakyThrows; + +/** + * Unit tests for HttpRedirectValidator + */ +public class HttpRedirectValidatorTests extends OpenSearchTestCase { + + /** + * Test that non-redirect status codes (200, 404, etc.) are allowed + */ + @SneakyThrows + public void testValidateNoRedirects_whenNonRedirectStatus_thenAllowed() { + int[] nonRedirectCodes = { 200, 201, 400, 404, 500 }; + + for (int statusCode : nonRedirectCodes) { + HttpURLConnection connection = mock(HttpURLConnection.class); + when(connection.getResponseCode()).thenReturn(statusCode); + + // Should not throw any exception + HttpRedirectValidator.validateNoRedirects(connection); + + // Verify that redirects are disabled + verify(connection).setInstanceFollowRedirects(false); + } + } + + /** + * Test that redirect status codes (3xx) are blocked + */ + @SneakyThrows + public void testValidateNoRedirects_whenRedirectStatus_thenBlocked() { + int[] redirectCodes = { 300, 301, 302, 303, 307, 308 }; + + for (int statusCode : redirectCodes) { + HttpURLConnection connection = mock(HttpURLConnection.class); + when(connection.getResponseCode()).thenReturn(statusCode); + when(connection.getHeaderField("Location")).thenReturn("http://redirect-target.com"); + when(connection.getURL()).thenReturn(new java.net.URL("https://example.com/test")); + + IllegalArgumentException exception = expectThrows( + IllegalArgumentException.class, + () -> HttpRedirectValidator.validateNoRedirects(connection) + ); + + // Verify error message + assertTrue( + "Error message should mention redirects are not allowed", + exception.getMessage().contains("HTTP redirects are not allowed") + ); + assertTrue("Error message should contain status code", exception.getMessage().contains(String.valueOf(statusCode))); + assertTrue("Error message should contain original URL", exception.getMessage().contains("https://example.com/test")); + assertTrue("Error message should contain redirect target", exception.getMessage().contains("http://redirect-target.com")); + } + } +} diff --git a/src/test/resources/ip2geo/another_manifest.json b/src/test/resources/ip2geo/another_manifest.json new file mode 100644 index 00000000..86a76e47 --- /dev/null +++ b/src/test/resources/ip2geo/another_manifest.json @@ -0,0 +1,8 @@ +{ + "url": "https://test.com/db.zip", + "db_name": "sample_valid.csv", + "sha256_hash": "safasdfaskkkesadfasdf", + "valid_for_in_days": 30, + "updated_at_in_epoch_milli": 3134012341236, + "provider": "sample_provider" +} \ No newline at end of file diff --git a/src/test/resources/ip2geo/server/city/manifest.json b/src/test/resources/ip2geo/server/city/manifest.json index ac903a18..0bb5dc34 100644 --- a/src/test/resources/ip2geo/server/city/manifest.json +++ b/src/test/resources/ip2geo/server/city/manifest.json @@ -1,5 +1,5 @@ { - "url": "https://github.com/opensearch-project/geospatial/raw/main/src/test/resources/ip2geo/server/city/city.zip", + "url": "https://raw.githubusercontent.com/opensearch-project/geospatial/main/src/test/resources/ip2geo/server/city/city.zip", "db_name": "data.csv", "sha256_hash": "oDPgEv+9+kNov7bdQQiLrhr8jQeEPdLnuJ22Hz5npvk=", "valid_for_in_days": 30,