Copilot commented on code in PR #917:
URL: https://github.com/apache/fesod/pull/917#discussion_r3240906248
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/util/FileTypeUtils.java:
##########
@@ -30,21 +31,25 @@
*/
public class FileTypeUtils {
- private static final char[] DIGITS = {'0', '1', '2', '3', '4', '5', '6',
'7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f'
- };
- private static final int IMAGE_TYPE_MARK_LENGTH = 28;
+ private static final int IMAGE_TYPE_MARK_MIN_LENGTH = 3;
- private static final Map<String, ImageData.ImageType> FILE_TYPE_MAP;
+ private static final byte[] JPEG_SIGNATURE = {(byte) 0xFF, (byte) 0xD8,
(byte) 0xFF};
+ private static final byte[] PNG_SIGNATURE = {(byte) 0x89, 0x50, 0x4E,
0x47};
+
+ private static final Map<FileMagic, ImageData.ImageType> FILE_TYPE_MAP;
/**
* Default image type
*/
public static ImageData.ImageType defaultImageType =
ImageData.ImageType.PICTURE_TYPE_PNG;
static {
- FILE_TYPE_MAP = new HashMap<>();
- FILE_TYPE_MAP.put("ffd8ff", ImageData.ImageType.PICTURE_TYPE_JPEG);
- FILE_TYPE_MAP.put("89504e47", ImageData.ImageType.PICTURE_TYPE_PNG);
+ FILE_TYPE_MAP = new EnumMap<>(FileMagic.class);
+ FILE_TYPE_MAP.put(FileMagic.JPEG,
ImageData.ImageType.PICTURE_TYPE_JPEG);
+ FILE_TYPE_MAP.put(FileMagic.PNG, ImageData.ImageType.PICTURE_TYPE_PNG);
+ FILE_TYPE_MAP.put(FileMagic.WMF, ImageData.ImageType.PICTURE_TYPE_WMF);
+ FILE_TYPE_MAP.put(FileMagic.EMF, ImageData.ImageType.PICTURE_TYPE_EMF);
+ FILE_TYPE_MAP.put(FileMagic.BMP, ImageData.ImageType.PICTURE_TYPE_DIB);
Review Comment:
`FileMagic.BMP` detects a full BMP file (with the BMP file header), but the
returned POI type is DIB and the bytes are passed through unchanged to
`workbook.addPicture`. Since DIB data is the bitmap data without the BMP file
header, this can embed BMP URL/file inputs under the wrong format and produce
unreadable images unless the header is stripped or BMP is rejected.
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/url/UrlImageConverter.java:
##########
@@ -38,6 +45,23 @@ public class UrlImageConverter implements Converter<URL> {
public static int urlConnectTimeout = 1000;
public static int urlReadTimeout = 5000;
+ private static volatile UrlImageFetchPolicy fetchPolicy =
UrlImageFetchPolicy.defaultPolicy();
+
+ public static UrlImageFetchPolicy getFetchPolicy() {
+ return fetchPolicy;
+ }
+
+ public static void setFetchPolicy(UrlImageFetchPolicy fetchPolicy) {
+ if (fetchPolicy == null) {
+ throw new IllegalArgumentException("Fetch policy can not be null");
+ }
+ UrlImageConverter.fetchPolicy = fetchPolicy;
+ }
+
+ public static void resetFetchPolicy() {
+ fetchPolicy = UrlImageFetchPolicy.defaultPolicy();
+ }
+
Review Comment:
This stores the fetch policy as JVM-wide mutable state. In a server handling
concurrent exports, a call that temporarily relaxes schemes/private-host
allowlists changes the policy for all other conversions using the default
converter, which can inadvertently weaken another request's security; consider
scoping the policy to the writer/configuration or a converter instance instead.
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/url/CidrBlock.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converters.url;
+
+import java.math.BigInteger;
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.util.Arrays;
+import lombok.EqualsAndHashCode;
+
+/**
+ * CIDR block matcher for URL image fetch allowlists.
+ */
+@EqualsAndHashCode
+public final class CidrBlock {
+
+ private final String value;
+ private final byte[] networkAddress;
+ private final int prefixLength;
+
+ private CidrBlock(String value, byte[] networkAddress, int prefixLength) {
+ this.value = value;
+ this.networkAddress = Arrays.copyOf(networkAddress,
networkAddress.length);
+ this.prefixLength = prefixLength;
+ }
+
+ public String getValue() {
+ return value;
+ }
+
+ public byte[] getNetworkAddress() {
+ return Arrays.copyOf(networkAddress, networkAddress.length);
+ }
+
+ public int getPrefixLength() {
+ return prefixLength;
+ }
+
+ public static CidrBlock parse(String value) {
+ if (value == null) {
+ throw new IllegalArgumentException("CIDR block can not be null");
+ }
+ String[] parts = value.trim().split("/", -1);
+ if (parts.length != 2) {
+ throw new IllegalArgumentException("CIDR block must use
address/prefix format");
+ }
+
+ try {
+ InetAddress address = InetAddress.getByName(parts[0]);
Review Comment:
`InetAddress.getByName` accepts hostnames (and some local aliases), so
`CidrBlock.parse` can perform DNS and turn a non-literal value like
`example.com/24` into whatever address happens to resolve at parse time. CIDR
allowlists should reject non-literal addresses to avoid DNS-dependent or
surprising private-network grants.
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/url/UrlImageFetchPolicy.java:
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converters.url;
+
+import java.net.IDN;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+
+/**
+ * Security policy for fetching images from URL values.
+ */
+@Getter
+@EqualsAndHashCode
+public final class UrlImageFetchPolicy {
+
+ public static final int DEFAULT_MAX_REDIRECTS = 3;
+ public static final int DEFAULT_MAX_IMAGE_BYTES = 10 * 1024 * 1024;
+
+ private static final UrlImageFetchPolicy DEFAULT = builder().build();
+
+ private final boolean allowPrivateNetwork;
+ private final Set<String> allowedPrivateHosts;
+ private final List<CidrBlock> allowedPrivateCidrs;
+ private final Set<String> allowedSchemes;
+ private final int maxRedirects;
+ private final int maxImageBytes;
+
+ private UrlImageFetchPolicy(Builder builder) {
+ this.allowPrivateNetwork = builder.allowPrivateNetwork;
+ this.allowedPrivateHosts =
Collections.unmodifiableSet(normalizeHosts(builder.allowedPrivateHosts));
+ this.allowedPrivateCidrs = Collections.unmodifiableList(new
ArrayList<>(builder.allowedPrivateCidrs));
+ this.allowedSchemes = Collections.unmodifiableSet(new
HashSet<>(builder.schemePolicy.getSchemes()));
+ this.maxRedirects = builder.maxRedirects;
+ this.maxImageBytes = builder.maxImageBytes;
+ }
+
+ public static UrlImageFetchPolicy defaultPolicy() {
+ return DEFAULT;
+ }
+
+ public static Builder builder() {
+ return new Builder();
+ }
+
+ private static Set<String> normalizeHosts(Collection<String> hosts) {
+ Set<String> result = new HashSet<>();
+ for (String host : hosts) {
+ if (host == null) {
+ continue;
+ }
+ String normalized = normalizeHost(host);
+ if (!normalized.isEmpty()) {
+ result.add(normalized);
+ }
+ }
+ return result;
+ }
+
+ static String normalizeHost(String host) {
+ String normalized = host.trim().toLowerCase(Locale.ROOT);
+ while (normalized.endsWith(".")) {
+ normalized = normalized.substring(0, normalized.length() - 1);
+ }
+ if (normalized.isEmpty()) {
+ return normalized;
+ }
+ return IDN.toASCII(normalized);
+ }
+
+ public static final class Builder {
+ private boolean allowPrivateNetwork;
+ private Set<String> allowedPrivateHosts = Collections.emptySet();
+ private List<CidrBlock> allowedPrivateCidrs = Collections.emptyList();
+ private SchemePolicy schemePolicy = SchemePolicy.HTTP_OR_HTTPS;
+ private int maxRedirects = DEFAULT_MAX_REDIRECTS;
+ private int maxImageBytes = DEFAULT_MAX_IMAGE_BYTES;
+
+ private Builder() {}
+
+ public Builder allowPrivateNetwork(boolean allowPrivateNetwork) {
+ this.allowPrivateNetwork = allowPrivateNetwork;
+ return this;
+ }
+
+ public Builder allowedPrivateHosts(Collection<String>
allowedPrivateHosts) {
+ if (allowedPrivateHosts == null) {
+ this.allowedPrivateHosts = Collections.emptySet();
+ } else {
+ this.allowedPrivateHosts = new HashSet<>(allowedPrivateHosts);
+ }
+ return this;
+ }
+
+ public Builder allowedPrivateCidrs(Collection<CidrBlock>
allowedPrivateCidrs) {
+ if (allowedPrivateCidrs == null) {
+ this.allowedPrivateCidrs = Collections.emptyList();
+ } else {
+ this.allowedPrivateCidrs = new
ArrayList<>(allowedPrivateCidrs);
Review Comment:
A collection containing a null CIDR entry builds successfully here, but
`isAllowedPrivateAddress` later iterates the list and calls
`cidrBlock.contains(address)`, causing a NullPointerException during URL
conversion. Validate and reject or filter null entries when building the policy
so configuration errors fail deterministically.
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/url/UrlImageConverter.java:
##########
@@ -47,18 +71,167 @@ public Class<?> supportJavaTypeKey() {
public WriteCellData<?> convertToExcelData(
URL value, ExcelContentProperty contentProperty,
GlobalConfiguration globalConfiguration)
throws IOException {
- InputStream inputStream = null;
+ byte[] bytes = readImage(value, fetchPolicy);
+ ImageData.ImageType imageType = FileTypeUtils.getImageType(bytes);
+ if (imageType == null) {
+ throw new IOException("URL image data is not a supported image
type");
+ }
+ return new WriteCellData<>(bytes);
+ }
+
+ private byte[] readImage(URL value, UrlImageFetchPolicy policy) throws
IOException {
+ URL currentUrl = value;
+ for (int redirectCount = 0; redirectCount <= policy.getMaxRedirects();
redirectCount++) {
+ validateUrl(currentUrl, policy);
+ HttpURLConnection connection = openConnection(currentUrl);
+ try {
+ int responseCode = connection.getResponseCode();
+ if (isRedirect(responseCode)) {
+ if (redirectCount == policy.getMaxRedirects()) {
+ throw new IOException("URL image request exceeded
redirect limit");
+ }
+ currentUrl = resolveRedirect(currentUrl,
connection.getHeaderField("Location"));
+ continue;
+ }
+ if (responseCode < HttpURLConnection.HTTP_OK || responseCode
>= HttpURLConnection.HTTP_MULT_CHOICE) {
+ throw new IOException("URL image request failed with HTTP
status " + responseCode);
+ }
+ int contentLength = connection.getContentLength();
+ if (contentLength > policy.getMaxImageBytes()) {
+ throw new IOException("URL image data exceeds maximum
size");
+ }
+ try (InputStream inputStream = connection.getInputStream()) {
+ return readLimited(inputStream, policy.getMaxImageBytes());
+ }
+ } finally {
+ connection.disconnect();
+ }
+ }
+ throw new IOException("URL image request exceeded redirect limit");
+ }
+
+ private HttpURLConnection openConnection(URL value) throws IOException {
+ HttpURLConnection connection = (HttpURLConnection)
value.openConnection();
+ connection.setConnectTimeout(urlConnectTimeout);
+ connection.setReadTimeout(urlReadTimeout);
+ connection.setInstanceFollowRedirects(false);
+ return connection;
+ }
+
+ private void validateUrl(URL value, UrlImageFetchPolicy policy) throws
IOException {
+ String protocol = value.getProtocol();
+ if (protocol == null ||
!policy.getAllowedSchemes().contains(protocol.toLowerCase(Locale.ROOT))) {
+ throw new IOException("URL image protocol is not allowed");
+ }
+ String host = value.getHost();
+ if (host == null || host.trim().isEmpty()) {
+ throw new IOException("URL image host is required");
+ }
+
+ String normalizedHost;
+ try {
+ normalizedHost = UrlImageFetchPolicy.normalizeHost(host);
+ } catch (IllegalArgumentException e) {
+ throw new IOException("URL image host is invalid", e);
+ }
+
+ InetAddress[] addresses = InetAddress.getAllByName(normalizedHost);
+ if (addresses.length == 0) {
+ throw new IOException("URL image host can not be resolved");
+ }
+ for (InetAddress address : addresses) {
Review Comment:
The DNS check is only a preflight: `openConnection(currentUrl)` performs its
own DNS lookup after this validation, so a rebinding or changing DNS record can
pass validation with a public address and then connect to a restricted/private
address. For this security policy to be enforceable, the connection needs to
use the validated address (or otherwise verify the actual remote address)
rather than resolving the hostname again.
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/url/UrlImageConverter.java:
##########
@@ -47,18 +71,167 @@ public Class<?> supportJavaTypeKey() {
public WriteCellData<?> convertToExcelData(
URL value, ExcelContentProperty contentProperty,
GlobalConfiguration globalConfiguration)
throws IOException {
- InputStream inputStream = null;
+ byte[] bytes = readImage(value, fetchPolicy);
+ ImageData.ImageType imageType = FileTypeUtils.getImageType(bytes);
+ if (imageType == null) {
+ throw new IOException("URL image data is not a supported image
type");
+ }
+ return new WriteCellData<>(bytes);
Review Comment:
`FileTypeUtils.getImageType` only checks short magic signatures, so a
truncated payload containing just the JPEG/PNG header bytes is accepted and
then embedded as image data. Since this converter now uses the result as
validation for remote responses, malformed responses can still produce corrupt
workbooks; validate that the bytes form a complete supported image before
returning them.
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/url/UrlImageConverter.java:
##########
@@ -47,18 +71,167 @@ public Class<?> supportJavaTypeKey() {
public WriteCellData<?> convertToExcelData(
URL value, ExcelContentProperty contentProperty,
GlobalConfiguration globalConfiguration)
throws IOException {
- InputStream inputStream = null;
+ byte[] bytes = readImage(value, fetchPolicy);
+ ImageData.ImageType imageType = FileTypeUtils.getImageType(bytes);
+ if (imageType == null) {
+ throw new IOException("URL image data is not a supported image
type");
+ }
+ return new WriteCellData<>(bytes);
+ }
+
+ private byte[] readImage(URL value, UrlImageFetchPolicy policy) throws
IOException {
+ URL currentUrl = value;
+ for (int redirectCount = 0; redirectCount <= policy.getMaxRedirects();
redirectCount++) {
+ validateUrl(currentUrl, policy);
+ HttpURLConnection connection = openConnection(currentUrl);
+ try {
+ int responseCode = connection.getResponseCode();
+ if (isRedirect(responseCode)) {
+ if (redirectCount == policy.getMaxRedirects()) {
+ throw new IOException("URL image request exceeded
redirect limit");
+ }
+ currentUrl = resolveRedirect(currentUrl,
connection.getHeaderField("Location"));
+ continue;
+ }
+ if (responseCode < HttpURLConnection.HTTP_OK || responseCode
>= HttpURLConnection.HTTP_MULT_CHOICE) {
+ throw new IOException("URL image request failed with HTTP
status " + responseCode);
+ }
+ int contentLength = connection.getContentLength();
+ if (contentLength > policy.getMaxImageBytes()) {
+ throw new IOException("URL image data exceeds maximum
size");
+ }
+ try (InputStream inputStream = connection.getInputStream()) {
+ return readLimited(inputStream, policy.getMaxImageBytes());
+ }
+ } finally {
+ connection.disconnect();
+ }
+ }
+ throw new IOException("URL image request exceeded redirect limit");
+ }
+
+ private HttpURLConnection openConnection(URL value) throws IOException {
+ HttpURLConnection connection = (HttpURLConnection)
value.openConnection();
+ connection.setConnectTimeout(urlConnectTimeout);
+ connection.setReadTimeout(urlReadTimeout);
+ connection.setInstanceFollowRedirects(false);
+ return connection;
+ }
+
+ private void validateUrl(URL value, UrlImageFetchPolicy policy) throws
IOException {
+ String protocol = value.getProtocol();
+ if (protocol == null ||
!policy.getAllowedSchemes().contains(protocol.toLowerCase(Locale.ROOT))) {
+ throw new IOException("URL image protocol is not allowed");
+ }
+ String host = value.getHost();
+ if (host == null || host.trim().isEmpty()) {
+ throw new IOException("URL image host is required");
+ }
+
+ String normalizedHost;
+ try {
+ normalizedHost = UrlImageFetchPolicy.normalizeHost(host);
+ } catch (IllegalArgumentException e) {
+ throw new IOException("URL image host is invalid", e);
+ }
+
+ InetAddress[] addresses = InetAddress.getAllByName(normalizedHost);
+ if (addresses.length == 0) {
+ throw new IOException("URL image host can not be resolved");
+ }
+ for (InetAddress address : addresses) {
+ if (isRestrictedAddress(address) &&
!isAllowedPrivateAddress(normalizedHost, address, policy)) {
+ throw new IOException("URL image host resolves to a restricted
address");
+ }
+ }
+ }
+
+ private boolean isAllowedPrivateAddress(String normalizedHost, InetAddress
address, UrlImageFetchPolicy policy) {
+ if (!policy.isAllowPrivateNetwork()) {
+ return false;
+ }
+ if (policy.getAllowedPrivateHosts().contains(normalizedHost)) {
+ return true;
+ }
+ for (CidrBlock cidrBlock : policy.getAllowedPrivateCidrs()) {
+ if (cidrBlock.contains(address)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private boolean isRestrictedAddress(InetAddress address) {
+ return address.isAnyLocalAddress()
+ || address.isLoopbackAddress()
+ || address.isLinkLocalAddress()
+ || address.isSiteLocalAddress()
+ || address.isMulticastAddress()
+ || isRestrictedIpv4Address(address)
+ || isRestrictedIpv6Address(address);
+ }
+
+ private boolean isRestrictedIpv4Address(InetAddress address) {
+ if (!(address instanceof Inet4Address)) {
+ return false;
+ }
+ byte[] bytes = address.getAddress();
+ int first = bytes[0] & 0xFF;
+ int second = bytes[1] & 0xFF;
+ return first == 0
+ || first == 10
+ || first == 127
+ || (first == 100 && second >= 64 && second <= 127)
+ || (first == 169 && second == 254)
+ || (first == 172 && second >= 16 && second <= 31)
+ || (first == 192 && second == 168)
Review Comment:
This IPv4 restriction list misses special-use non-public ranges such as
198.18.0.0/15 (benchmarking) and 192.0.0.0/24. Those ranges are commonly not
globally reachable and can be used on internal networks, so the default SSRF
protection can still allow requests to restricted infrastructure unless all
non-global ranges are covered.
##########
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/UrlImageConverterTest.java:
##########
@@ -0,0 +1,222 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converter;
+
+import com.sun.net.httpserver.HttpExchange;
+import com.sun.net.httpserver.HttpServer;
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.URL;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.fesod.sheet.converters.url.CidrBlock;
+import org.apache.fesod.sheet.converters.url.SchemePolicy;
+import org.apache.fesod.sheet.converters.url.UrlImageConverter;
+import org.apache.fesod.sheet.converters.url.UrlImageFetchPolicy;
+import org.apache.fesod.sheet.metadata.GlobalConfiguration;
+import org.apache.fesod.sheet.metadata.data.WriteCellData;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests {@link UrlImageConverter}.
+ */
+class UrlImageConverterTest {
+
+ private static final byte[] PNG_BYTES =
+ new byte[] {(byte) 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A,
0x00, 0x00, 0x00, 0x0D};
+
+ private final UrlImageConverter converter = new UrlImageConverter();
+ private HttpServer server;
+ private AtomicInteger requestCount;
+
+ @BeforeEach
+ void beforeEach() {
+ UrlImageConverter.resetFetchPolicy();
+ requestCount = new AtomicInteger();
+ }
+
+ @AfterEach
+ void afterEach() {
+ UrlImageConverter.resetFetchPolicy();
+ if (server != null) {
+ server.stop(0);
+ }
+ }
+
+ @Test
+ void test_rejectFileProtocol() {
+ IOException exception =
+ Assertions.assertThrows(IOException.class, () -> convert(new
URL("file:///etc/passwd")));
+
+ Assertions.assertTrue(exception.getMessage().contains("protocol"));
+ }
+
+ @Test
+ void test_rejectNullSchemePolicy() {
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
UrlImageFetchPolicy.builder().allowedSchemes(null).build());
+ }
+
+ @Test
+ void test_httpsOnlyPolicyRejectsHttpUrl() throws Exception {
+ URL url = startServer(HttpStatus.OK, PNG_BYTES, "image/png");
+ UrlImageConverter.setFetchPolicy(
+
UrlImageFetchPolicy.builder().allowedSchemes(SchemePolicy.HTTPS).build());
+
+ IOException exception = Assertions.assertThrows(IOException.class, ()
-> convert(url));
+
+ Assertions.assertTrue(exception.getMessage().contains("protocol"));
+ Assertions.assertEquals(0, requestCount.get());
+ }
+
+ @Test
+ void test_rejectLoopbackByDefault() throws Exception {
+ URL url = startServer(HttpStatus.OK, PNG_BYTES, "image/png");
+
+ IOException exception = Assertions.assertThrows(IOException.class, ()
-> convert(url));
+
+ Assertions.assertTrue(exception.getMessage().contains("restricted
address"));
+ Assertions.assertEquals(0, requestCount.get());
+ }
+
+ @Test
+ void test_allowPrivateHostWhenExplicitlyAllowlisted() throws Exception {
+ URL url = startServer(HttpStatus.OK, PNG_BYTES, "image/png");
+ UrlImageConverter.setFetchPolicy(UrlImageFetchPolicy.builder()
+ .allowPrivateNetwork(true)
+ .allowedPrivateHosts(Collections.singleton("127.0.0.1"))
+ .build());
+
+ WriteCellData<?> cellData = convert(url);
+
+ Assertions.assertArrayEquals(
+ PNG_BYTES, cellData.getImageDataList().get(0).getImage());
+ Assertions.assertEquals(1, requestCount.get());
+ }
+
+ @Test
+ void test_allowPrivateCidrWhenExplicitlyAllowlisted() throws Exception {
+ URL url = startServer(HttpStatus.OK, PNG_BYTES, "image/png");
+ UrlImageConverter.setFetchPolicy(UrlImageFetchPolicy.builder()
+ .allowPrivateNetwork(true)
+
.allowedPrivateCidrs(Collections.singleton(CidrBlock.parse("127.0.0.0/8")))
+ .build());
+
+ WriteCellData<?> cellData = convert(url);
+
+ Assertions.assertArrayEquals(
+ PNG_BYTES, cellData.getImageDataList().get(0).getImage());
+ }
+
+ @Test
+ void test_rejectNonImageResponse() throws Exception {
+ URL url = startServer(HttpStatus.OK, "root:x:0:0".getBytes("UTF-8"),
"text/plain");
+ UrlImageConverter.setFetchPolicy(allowLoopbackPolicy());
+
+ IOException exception = Assertions.assertThrows(IOException.class, ()
-> convert(url));
+
+ Assertions.assertTrue(exception.getMessage().contains("supported image
type"));
+ }
+
+ @Test
+ void test_rejectRedirectToNonAllowlistedPrivateHost() throws Exception {
+ URL url = startRedirectServer("http://localhost:8080/image.png";);
+ UrlImageConverter.setFetchPolicy(allowLoopbackPolicy());
+
+ IOException exception = Assertions.assertThrows(IOException.class, ()
-> convert(url));
+
+ Assertions.assertTrue(exception.getMessage().contains("restricted
address"));
+ Assertions.assertEquals(1, requestCount.get());
+ }
+
+ @Test
+ void test_rejectImageLargerThanPolicyLimit() throws Exception {
+ byte[] body = Arrays.copyOf(PNG_BYTES, PNG_BYTES.length + 20);
+ URL url = startServer(HttpStatus.OK, body, "image/png");
+ UrlImageConverter.setFetchPolicy(UrlImageFetchPolicy.builder()
+ .allowPrivateNetwork(true)
+ .allowedPrivateHosts(Collections.singleton("127.0.0.1"))
+ .maxImageBytes(PNG_BYTES.length)
+ .build());
+
+ IOException exception = Assertions.assertThrows(IOException.class, ()
-> convert(url));
+
+ Assertions.assertTrue(exception.getMessage().contains("maximum size"));
+ }
+
+ private UrlImageFetchPolicy allowLoopbackPolicy() {
+ return UrlImageFetchPolicy.builder()
+ .allowPrivateNetwork(true)
+ .allowedPrivateHosts(Collections.singleton("127.0.0.1"))
+ .build();
+ }
+
+ private WriteCellData<?> convert(URL url) throws IOException {
+ return converter.convertToExcelData(url, null, new
GlobalConfiguration());
+ }
+
+ private URL startServer(int status, byte[] body, String contentType)
throws IOException {
+ server = HttpServer.create(new
InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0);
+ server.createContext("/", exchange -> {
+ requestCount.incrementAndGet();
+ send(exchange, status, body, contentType);
+ });
+ server.start();
+ return serverUrl("/");
+ }
+
+ private URL startRedirectServer(String location) throws IOException {
+ server = HttpServer.create(new
InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0);
+ server.createContext("/", exchange -> {
Review Comment:
This repeats the same IPv4/IPv6 mismatch as the normal test server: the
server may bind to IPv6 `::1` via `getLoopbackAddress()`, while `serverUrl`
constructs an IPv4 `127.0.0.1` URL. Use the same explicit loopback address for
binding and URL generation to avoid environment-dependent test failures.
##########
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/UrlImageConverterTest.java:
##########
@@ -0,0 +1,222 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converter;
+
+import com.sun.net.httpserver.HttpExchange;
+import com.sun.net.httpserver.HttpServer;
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.URL;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.fesod.sheet.converters.url.CidrBlock;
+import org.apache.fesod.sheet.converters.url.SchemePolicy;
+import org.apache.fesod.sheet.converters.url.UrlImageConverter;
+import org.apache.fesod.sheet.converters.url.UrlImageFetchPolicy;
+import org.apache.fesod.sheet.metadata.GlobalConfiguration;
+import org.apache.fesod.sheet.metadata.data.WriteCellData;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests {@link UrlImageConverter}.
+ */
+class UrlImageConverterTest {
+
+ private static final byte[] PNG_BYTES =
+ new byte[] {(byte) 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A,
0x00, 0x00, 0x00, 0x0D};
+
+ private final UrlImageConverter converter = new UrlImageConverter();
+ private HttpServer server;
+ private AtomicInteger requestCount;
+
+ @BeforeEach
+ void beforeEach() {
+ UrlImageConverter.resetFetchPolicy();
+ requestCount = new AtomicInteger();
+ }
+
+ @AfterEach
+ void afterEach() {
+ UrlImageConverter.resetFetchPolicy();
+ if (server != null) {
+ server.stop(0);
+ }
+ }
+
+ @Test
+ void test_rejectFileProtocol() {
+ IOException exception =
+ Assertions.assertThrows(IOException.class, () -> convert(new
URL("file:///etc/passwd")));
+
+ Assertions.assertTrue(exception.getMessage().contains("protocol"));
+ }
+
+ @Test
+ void test_rejectNullSchemePolicy() {
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
UrlImageFetchPolicy.builder().allowedSchemes(null).build());
+ }
+
+ @Test
+ void test_httpsOnlyPolicyRejectsHttpUrl() throws Exception {
+ URL url = startServer(HttpStatus.OK, PNG_BYTES, "image/png");
+ UrlImageConverter.setFetchPolicy(
+
UrlImageFetchPolicy.builder().allowedSchemes(SchemePolicy.HTTPS).build());
+
+ IOException exception = Assertions.assertThrows(IOException.class, ()
-> convert(url));
+
+ Assertions.assertTrue(exception.getMessage().contains("protocol"));
+ Assertions.assertEquals(0, requestCount.get());
+ }
+
+ @Test
+ void test_rejectLoopbackByDefault() throws Exception {
+ URL url = startServer(HttpStatus.OK, PNG_BYTES, "image/png");
+
+ IOException exception = Assertions.assertThrows(IOException.class, ()
-> convert(url));
+
+ Assertions.assertTrue(exception.getMessage().contains("restricted
address"));
+ Assertions.assertEquals(0, requestCount.get());
+ }
+
+ @Test
+ void test_allowPrivateHostWhenExplicitlyAllowlisted() throws Exception {
+ URL url = startServer(HttpStatus.OK, PNG_BYTES, "image/png");
+ UrlImageConverter.setFetchPolicy(UrlImageFetchPolicy.builder()
+ .allowPrivateNetwork(true)
+ .allowedPrivateHosts(Collections.singleton("127.0.0.1"))
+ .build());
+
+ WriteCellData<?> cellData = convert(url);
+
+ Assertions.assertArrayEquals(
+ PNG_BYTES, cellData.getImageDataList().get(0).getImage());
+ Assertions.assertEquals(1, requestCount.get());
+ }
+
+ @Test
+ void test_allowPrivateCidrWhenExplicitlyAllowlisted() throws Exception {
+ URL url = startServer(HttpStatus.OK, PNG_BYTES, "image/png");
+ UrlImageConverter.setFetchPolicy(UrlImageFetchPolicy.builder()
+ .allowPrivateNetwork(true)
+
.allowedPrivateCidrs(Collections.singleton(CidrBlock.parse("127.0.0.0/8")))
+ .build());
+
+ WriteCellData<?> cellData = convert(url);
+
+ Assertions.assertArrayEquals(
+ PNG_BYTES, cellData.getImageDataList().get(0).getImage());
+ }
+
+ @Test
+ void test_rejectNonImageResponse() throws Exception {
+ URL url = startServer(HttpStatus.OK, "root:x:0:0".getBytes("UTF-8"),
"text/plain");
+ UrlImageConverter.setFetchPolicy(allowLoopbackPolicy());
+
+ IOException exception = Assertions.assertThrows(IOException.class, ()
-> convert(url));
+
+ Assertions.assertTrue(exception.getMessage().contains("supported image
type"));
+ }
+
+ @Test
+ void test_rejectRedirectToNonAllowlistedPrivateHost() throws Exception {
+ URL url = startRedirectServer("http://localhost:8080/image.png";);
+ UrlImageConverter.setFetchPolicy(allowLoopbackPolicy());
+
+ IOException exception = Assertions.assertThrows(IOException.class, ()
-> convert(url));
+
+ Assertions.assertTrue(exception.getMessage().contains("restricted
address"));
+ Assertions.assertEquals(1, requestCount.get());
+ }
+
+ @Test
+ void test_rejectImageLargerThanPolicyLimit() throws Exception {
+ byte[] body = Arrays.copyOf(PNG_BYTES, PNG_BYTES.length + 20);
+ URL url = startServer(HttpStatus.OK, body, "image/png");
+ UrlImageConverter.setFetchPolicy(UrlImageFetchPolicy.builder()
+ .allowPrivateNetwork(true)
+ .allowedPrivateHosts(Collections.singleton("127.0.0.1"))
+ .maxImageBytes(PNG_BYTES.length)
+ .build());
+
+ IOException exception = Assertions.assertThrows(IOException.class, ()
-> convert(url));
+
+ Assertions.assertTrue(exception.getMessage().contains("maximum size"));
+ }
+
+ private UrlImageFetchPolicy allowLoopbackPolicy() {
+ return UrlImageFetchPolicy.builder()
+ .allowPrivateNetwork(true)
+ .allowedPrivateHosts(Collections.singleton("127.0.0.1"))
+ .build();
+ }
+
+ private WriteCellData<?> convert(URL url) throws IOException {
+ return converter.convertToExcelData(url, null, new
GlobalConfiguration());
+ }
+
+ private URL startServer(int status, byte[] body, String contentType)
throws IOException {
+ server = HttpServer.create(new
InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0);
+ server.createContext("/", exchange -> {
Review Comment:
The server is bound to `InetAddress.getLoopbackAddress()`, but `serverUrl`
always returns `127.0.0.1`. On JVMs or CI hosts where the loopback address
resolves to IPv6 `::1`, the server may listen only on IPv6 while the client
connects to IPv4, making these tests fail intermittently; bind and build the
URL with the same explicit address.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
![http://localhost:8080/image.png"](http://localhost:8080/image.png"){kind=link}