zabetak commented on code in PR #5334:
URL: https://github.com/apache/calcite/pull/5334#discussion_r4389825991
##########
core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java:
##########
@@ -532,6 +532,39 @@ public final class CalciteSystemProperty<T> {
public static final CalciteSystemProperty<String> MODEL_BASE_DIRECTORY =
stringProperty("calcite.model.baseDirectory", "");
+ /**
+ * Regular expression that a remote URL in a file adapter table {@code url}
+ * operand must match for the adapter to dereference it (for example
+ * {@code -Dcalcite.file.urls.allowed=https://data\.example\.com/.*}).
+ * Empty by default: only local {@code file:} sources are dereferenced.
+ *
+ * <p>The expression uses {@link java.util.regex.Pattern} syntax and must
+ * match the whole URL, as written in the operand. Use {@code |} to allow
+ * several sites, for example
+ * {@code https://data\.example\.com/.*|http://data\.example\.com:8080/.*}.
+ * Matching is case-sensitive (use {@code (?i)} to ignore case), and the URL
+ * is not normalized first: default ports, {@code .} and {@code ..} segments
+ * and percent-encoding are compared as written. Escape each {@code .} in a
+ * host name, and end the host with {@code /} (or {@code :port/}), so that
+ * {@code https://data\.example\.com/.*} does not match
+ * {@code https://data.example.com.other.org/} nor
+ * {@code https://[email protected]/}.
Review Comment:
Saying that we support `java.util.regex.Pattern` syntax is enough. Let's
avoid adding too many details about the supported patterns.
##########
file/src/main/java/org/apache/calcite/adapter/file/FileSchema.java:
##########
@@ -119,10 +130,60 @@ private static String trim(String s, String suffix) {
return builder.build();
}
+ /** Checks that a model-supplied table URL may be dereferenced by this
+ * adapter: a local {@code file} URL always, a remote one (any other
+ * protocol, or a {@code file} URL naming a host, such as a Windows UNC path)
+ * only when it matches the regular expression
+ * {@link CalciteSystemProperty#FILE_URLS_ALLOWED}. */
+ static void checkUrlAllowed(String url) {
+ checkUrlAllowed(url, CalciteSystemProperty.FILE_URLS_ALLOWED.value());
+ }
+
+ static void checkUrlAllowed(String url, String allowedUrls) {
+ final URI uri;
+ try {
+ uri = new URI(url);
+ uri.toURL();
+ } catch (URISyntaxException | MalformedURLException |
IllegalArgumentException e) {
+ // Malformed; Sources.url parses the same way and will report the error
+ return;
+ }
+ // A "file" URL is local unless it names a host, as an authority
+ // ("file://host/share") or, as Windows resolves it to a UNC path, as a
+ // path starting with two slashes or backslashes ("file:////host/share",
+ // "file:%5C%5Chost%5Cshare")
+ final String path =
+ Util.first(uri.isOpaque() ? uri.getSchemeSpecificPart() :
uri.getPath(), "");
+ if ("file".equalsIgnoreCase(uri.getScheme())
+ && Util.isNullOrEmpty(uri.getRawAuthority())
+ && !path.replace('\\', '/').startsWith("//")) {
+ return;
+ }
+ if (!allowedUrls.isEmpty()) {
+ final Pattern pattern;
+ try {
+ pattern = Pattern.compile(allowedUrls);
+ } catch (PatternSyntaxException e) {
+ throw new IllegalArgumentException("Invalid regular expression '"
+ + allowedUrls + "' in system property
\"calcite.file.urls.allowed\"",
+ e);
+ }
Review Comment:
Since the pattern is not gonna change after the JVM startup maybe we should
consider a `static final` to avoid the perf overhead of compiling the pattern
every single time.
##########
core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java:
##########
@@ -532,6 +532,39 @@ public final class CalciteSystemProperty<T> {
public static final CalciteSystemProperty<String> MODEL_BASE_DIRECTORY =
stringProperty("calcite.model.baseDirectory", "");
+ /**
+ * Regular expression that a remote URL in a file adapter table {@code url}
+ * operand must match for the adapter to dereference it (for example
+ * {@code -Dcalcite.file.urls.allowed=https://data\.example\.com/.*}).
+ * Empty by default: only local {@code file:} sources are dereferenced.
+ *
+ * <p>The expression uses {@link java.util.regex.Pattern} syntax and must
+ * match the whole URL, as written in the operand. Use {@code |} to allow
+ * several sites, for example
+ * {@code https://data\.example\.com/.*|http://data\.example\.com:8080/.*}.
+ * Matching is case-sensitive (use {@code (?i)} to ignore case), and the URL
+ * is not normalized first: default ports, {@code .} and {@code ..} segments
+ * and percent-encoding are compared as written. Escape each {@code .} in a
+ * host name, and end the host with {@code /} (or {@code :port/}), so that
+ * {@code https://data\.example\.com/.*} does not match
+ * {@code https://data.example.com.other.org/} nor
+ * {@code https://[email protected]/}.
+ *
+ * <p>A {@code file:} URL that names a host (for example
+ * {@code file://host/share/t.csv}, which Windows resolves to a UNC path) is
+ * remote, and is dereferenced only if the expression matches it.
+ * {@code jar:} URLs are considered remote even when they wrap a local
+ * {@code file:} URL; the expression is matched against the whole
+ * {@code jar:} URL (for example {@code jar:file:/opt/app/lib/.*}).
Review Comment:
Instead of mentioning every possible scheme, which in any case is gonna be
incomplete, we can just highlight what URLs are considered local and bypass
this check.
##########
file/src/test/java/org/apache/calcite/adapter/file/FileSchemaProtocolTest.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.calcite.adapter.file;
+
+import org.apache.calcite.config.CalciteSystemProperty;
+import org.apache.calcite.schema.Table;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
+
+/**
+ * Tests that {@link FileSchema} dereferences only local {@code file} sources
by
+ * default, and requires an explicit operator opt-in (the
+ * {@code calcite.file.urls.allowed} system property) for remote URL table
+ * operands.
+ */
+class FileSchemaProtocolTest {
+
+ private static Map<String, Object> table(String name, String url) {
+ final Map<String, Object> tableDef = new HashMap<>();
+ tableDef.put("name", name);
+ tableDef.put("url", url);
+ return tableDef;
+ }
+
+ /** Skips a test that relies on the default allowlist, if the test JVM was
+ * started with {@code calcite.file.urls.allowed} set. */
+ private static void assumeDefaultAllowlist() {
+ assumeTrue(CalciteSystemProperty.FILE_URLS_ALLOWED.value().isEmpty(),
+ "calcite.file.urls.allowed is set");
+ }
+
+ private static void assertAllowed(String url, String allowedUrls) {
+ assertDoesNotThrow(() -> FileSchema.checkUrlAllowed(url, allowedUrls),
+ () -> url + " should be allowed by " + allowedUrls);
+ }
+
+ private static void assertRejected(String url, String allowedUrls) {
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class,
+ () -> FileSchema.checkUrlAllowed(url, allowedUrls),
+ () -> url + " should be rejected by " + allowedUrls);
+ assertThat(url, e.getMessage(),
+ containsString("Remote URL '" + url + "' is not allowed"));
+ assertThat(url, e.getMessage(),
+ containsString("calcite.file.urls.allowed"));
+ }
+
+ @Test void testRemoteUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "http://localhost/table.csv";)));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL 'http://localhost/table.csv' is not
allowed"));
+ assertThat(e.getMessage(),
+ containsString("calcite.file.urls.allowed"));
+ }
+
+ @Test void testFtpUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "ftp://localhost/table.csv";)));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL 'ftp://localhost/table.csv' is not
allowed"));
+ }
Review Comment:
It's not necessary to test every different protocol. This only adds coverage
in pattern matching which is out of scope for the PR.
##########
file/src/main/java/org/apache/calcite/adapter/file/FileSchema.java:
##########
@@ -119,10 +130,60 @@ private static String trim(String s, String suffix) {
return builder.build();
}
+ /** Checks that a model-supplied table URL may be dereferenced by this
+ * adapter: a local {@code file} URL always, a remote one (any other
+ * protocol, or a {@code file} URL naming a host, such as a Windows UNC path)
+ * only when it matches the regular expression
+ * {@link CalciteSystemProperty#FILE_URLS_ALLOWED}. */
+ static void checkUrlAllowed(String url) {
+ checkUrlAllowed(url, CalciteSystemProperty.FILE_URLS_ALLOWED.value());
+ }
+
+ static void checkUrlAllowed(String url, String allowedUrls) {
+ final URI uri;
+ try {
+ uri = new URI(url);
+ uri.toURL();
+ } catch (URISyntaxException | MalformedURLException |
IllegalArgumentException e) {
+ // Malformed; Sources.url parses the same way and will report the error
+ return;
+ }
+ // A "file" URL is local unless it names a host, as an authority
+ // ("file://host/share") or, as Windows resolves it to a UNC path, as a
+ // path starting with two slashes or backslashes ("file:////host/share",
+ // "file:%5C%5Chost%5Cshare")
+ final String path =
+ Util.first(uri.isOpaque() ? uri.getSchemeSpecificPart() :
uri.getPath(), "");
+ if ("file".equalsIgnoreCase(uri.getScheme())
+ && Util.isNullOrEmpty(uri.getRawAuthority())
+ && !path.replace('\\', '/').startsWith("//")) {
+ return;
+ }
Review Comment:
Maybe extract this into a small method `boolean isLocalUri` and turn the
comment into Javadoc.
##########
file/src/test/java/org/apache/calcite/adapter/file/FileSchemaProtocolTest.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.calcite.adapter.file;
+
+import org.apache.calcite.config.CalciteSystemProperty;
+import org.apache.calcite.schema.Table;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
+
+/**
+ * Tests that {@link FileSchema} dereferences only local {@code file} sources
by
+ * default, and requires an explicit operator opt-in (the
+ * {@code calcite.file.urls.allowed} system property) for remote URL table
+ * operands.
+ */
+class FileSchemaProtocolTest {
+
+ private static Map<String, Object> table(String name, String url) {
+ final Map<String, Object> tableDef = new HashMap<>();
+ tableDef.put("name", name);
+ tableDef.put("url", url);
+ return tableDef;
+ }
+
+ /** Skips a test that relies on the default allowlist, if the test JVM was
+ * started with {@code calcite.file.urls.allowed} set. */
+ private static void assumeDefaultAllowlist() {
+ assumeTrue(CalciteSystemProperty.FILE_URLS_ALLOWED.value().isEmpty(),
+ "calcite.file.urls.allowed is set");
+ }
+
+ private static void assertAllowed(String url, String allowedUrls) {
+ assertDoesNotThrow(() -> FileSchema.checkUrlAllowed(url, allowedUrls),
+ () -> url + " should be allowed by " + allowedUrls);
+ }
+
+ private static void assertRejected(String url, String allowedUrls) {
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class,
+ () -> FileSchema.checkUrlAllowed(url, allowedUrls),
+ () -> url + " should be rejected by " + allowedUrls);
+ assertThat(url, e.getMessage(),
+ containsString("Remote URL '" + url + "' is not allowed"));
+ assertThat(url, e.getMessage(),
+ containsString("calcite.file.urls.allowed"));
+ }
+
+ @Test void testRemoteUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "http://localhost/table.csv";)));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL 'http://localhost/table.csv' is not
allowed"));
+ assertThat(e.getMessage(),
+ containsString("calcite.file.urls.allowed"));
+ }
+
+ @Test void testFtpUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "ftp://localhost/table.csv";)));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL 'ftp://localhost/table.csv' is not
allowed"));
+ }
+
+ @Test void testFileUrlAllowedByDefault() {
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "file:foo.csv")));
+ final Map<String, Table> tableMap = schema.getTableMap();
+ assertThat(tableMap.containsKey("T"), is(true));
+ }
+
+ @Test void testRemoteFileUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(
+ table("T", "file://fileserver.example.com/share/table.csv")));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL
'file://fileserver.example.com/share/table.csv'"
+ + " is not allowed"));
+ }
+
+ /** Exercises the check with an explicit regular expression rather than by
+ * mutating a JVM-wide system property; {@link
+ * org.apache.calcite.config.CalciteSystemProperty} caches at class-load, so
+ * mid-run {@code System.setProperty} would not be observed by the read
+ * inside {@link FileSchema#checkUrlAllowed(String)}. */
+ @Test void testRegex() {
+ final String allowed = "https://data\\.example\\.com/.*";;
+ assertAllowed("https://data.example.com/table.csv";, allowed);
+ assertAllowed("https://data.example.com/a/b/table.csv?x=1";, allowed);
+ assertRejected("http://data.example.com/table.csv";, allowed);
+ assertRejected("https://other.example.com/table.csv";, allowed);
+ assertRejected("https://data.example.com:8443/table.csv";, allowed);
+ assertRejected("ftp://data.example.com/table.csv";, allowed);
+ }
+
+ @Test void testEmptyRegexRejectsRemote() {
+ assertRejected("https://data.example.com/table.csv";, "");
+ }
Review Comment:
Since the default value of the property is empty we could make the test
based on FileSchema instead of just calling the check method.
##########
file/src/test/java/org/apache/calcite/adapter/file/FileSchemaProtocolTest.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.calcite.adapter.file;
+
+import org.apache.calcite.config.CalciteSystemProperty;
+import org.apache.calcite.schema.Table;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
+
+/**
+ * Tests that {@link FileSchema} dereferences only local {@code file} sources
by
+ * default, and requires an explicit operator opt-in (the
+ * {@code calcite.file.urls.allowed} system property) for remote URL table
+ * operands.
+ */
+class FileSchemaProtocolTest {
+
+ private static Map<String, Object> table(String name, String url) {
+ final Map<String, Object> tableDef = new HashMap<>();
+ tableDef.put("name", name);
+ tableDef.put("url", url);
+ return tableDef;
+ }
+
+ /** Skips a test that relies on the default allowlist, if the test JVM was
+ * started with {@code calcite.file.urls.allowed} set. */
+ private static void assumeDefaultAllowlist() {
+ assumeTrue(CalciteSystemProperty.FILE_URLS_ALLOWED.value().isEmpty(),
+ "calcite.file.urls.allowed is set");
+ }
+
+ private static void assertAllowed(String url, String allowedUrls) {
+ assertDoesNotThrow(() -> FileSchema.checkUrlAllowed(url, allowedUrls),
+ () -> url + " should be allowed by " + allowedUrls);
+ }
+
+ private static void assertRejected(String url, String allowedUrls) {
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class,
+ () -> FileSchema.checkUrlAllowed(url, allowedUrls),
+ () -> url + " should be rejected by " + allowedUrls);
+ assertThat(url, e.getMessage(),
+ containsString("Remote URL '" + url + "' is not allowed"));
+ assertThat(url, e.getMessage(),
+ containsString("calcite.file.urls.allowed"));
+ }
+
+ @Test void testRemoteUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "http://localhost/table.csv";)));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL 'http://localhost/table.csv' is not
allowed"));
+ assertThat(e.getMessage(),
+ containsString("calcite.file.urls.allowed"));
+ }
+
+ @Test void testFtpUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "ftp://localhost/table.csv";)));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL 'ftp://localhost/table.csv' is not
allowed"));
+ }
+
+ @Test void testFileUrlAllowedByDefault() {
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "file:foo.csv")));
+ final Map<String, Table> tableMap = schema.getTableMap();
+ assertThat(tableMap.containsKey("T"), is(true));
+ }
+
+ @Test void testRemoteFileUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(
+ table("T", "file://fileserver.example.com/share/table.csv")));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL
'file://fileserver.example.com/share/table.csv'"
+ + " is not allowed"));
+ }
+
+ /** Exercises the check with an explicit regular expression rather than by
+ * mutating a JVM-wide system property; {@link
+ * org.apache.calcite.config.CalciteSystemProperty} caches at class-load, so
+ * mid-run {@code System.setProperty} would not be observed by the read
+ * inside {@link FileSchema#checkUrlAllowed(String)}. */
+ @Test void testRegex() {
+ final String allowed = "https://data\\.example\\.com/.*";;
+ assertAllowed("https://data.example.com/table.csv";, allowed);
+ assertAllowed("https://data.example.com/a/b/table.csv?x=1";, allowed);
+ assertRejected("http://data.example.com/table.csv";, allowed);
+ assertRejected("https://other.example.com/table.csv";, allowed);
+ assertRejected("https://data.example.com:8443/table.csv";, allowed);
+ assertRejected("ftp://data.example.com/table.csv";, allowed);
+ }
Review Comment:
This test mainly checks Java pattern matching and specification. It doesn't
add test coverage specific to calcite so can be dropped.
##########
file/src/test/java/org/apache/calcite/adapter/file/FileSchemaProtocolTest.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.calcite.adapter.file;
+
+import org.apache.calcite.config.CalciteSystemProperty;
+import org.apache.calcite.schema.Table;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
+
+/**
+ * Tests that {@link FileSchema} dereferences only local {@code file} sources
by
+ * default, and requires an explicit operator opt-in (the
+ * {@code calcite.file.urls.allowed} system property) for remote URL table
+ * operands.
+ */
+class FileSchemaProtocolTest {
+
+ private static Map<String, Object> table(String name, String url) {
+ final Map<String, Object> tableDef = new HashMap<>();
+ tableDef.put("name", name);
+ tableDef.put("url", url);
+ return tableDef;
+ }
+
+ /** Skips a test that relies on the default allowlist, if the test JVM was
+ * started with {@code calcite.file.urls.allowed} set. */
+ private static void assumeDefaultAllowlist() {
+ assumeTrue(CalciteSystemProperty.FILE_URLS_ALLOWED.value().isEmpty(),
+ "calcite.file.urls.allowed is set");
+ }
+
+ private static void assertAllowed(String url, String allowedUrls) {
+ assertDoesNotThrow(() -> FileSchema.checkUrlAllowed(url, allowedUrls),
+ () -> url + " should be allowed by " + allowedUrls);
+ }
+
+ private static void assertRejected(String url, String allowedUrls) {
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class,
+ () -> FileSchema.checkUrlAllowed(url, allowedUrls),
+ () -> url + " should be rejected by " + allowedUrls);
+ assertThat(url, e.getMessage(),
+ containsString("Remote URL '" + url + "' is not allowed"));
+ assertThat(url, e.getMessage(),
+ containsString("calcite.file.urls.allowed"));
+ }
+
+ @Test void testRemoteUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "http://localhost/table.csv";)));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL 'http://localhost/table.csv' is not
allowed"));
+ assertThat(e.getMessage(),
+ containsString("calcite.file.urls.allowed"));
+ }
+
+ @Test void testFtpUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "ftp://localhost/table.csv";)));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL 'ftp://localhost/table.csv' is not
allowed"));
+ }
+
+ @Test void testFileUrlAllowedByDefault() {
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "file:foo.csv")));
+ final Map<String, Table> tableMap = schema.getTableMap();
+ assertThat(tableMap.containsKey("T"), is(true));
+ }
+
+ @Test void testRemoteFileUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(
+ table("T", "file://fileserver.example.com/share/table.csv")));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL
'file://fileserver.example.com/share/table.csv'"
+ + " is not allowed"));
+ }
+
+ /** Exercises the check with an explicit regular expression rather than by
+ * mutating a JVM-wide system property; {@link
+ * org.apache.calcite.config.CalciteSystemProperty} caches at class-load, so
+ * mid-run {@code System.setProperty} would not be observed by the read
+ * inside {@link FileSchema#checkUrlAllowed(String)}. */
+ @Test void testRegex() {
+ final String allowed = "https://data\\.example\\.com/.*";;
+ assertAllowed("https://data.example.com/table.csv";, allowed);
+ assertAllowed("https://data.example.com/a/b/table.csv?x=1";, allowed);
+ assertRejected("http://data.example.com/table.csv";, allowed);
+ assertRejected("https://other.example.com/table.csv";, allowed);
+ assertRejected("https://data.example.com:8443/table.csv";, allowed);
+ assertRejected("ftp://data.example.com/table.csv";, allowed);
+ }
+
+ @Test void testEmptyRegexRejectsRemote() {
+ assertRejected("https://data.example.com/table.csv";, "");
+ }
+
+ /** Tests that the regular expression must match the whole URL, not just a
+ * part of it. */
+ @Test void testRegexMatchesWholeUrl() {
+ assertRejected("https://data.example.com/table.csv";,
+ "https://data\\.example\\.com/";);
+ assertRejected("https://data.example.com/table.csv";,
"data\\.example\\.com");
+
assertRejected("https://other.example.com/?u=https://data.example.com/t.csv";,
+ "https://data\\.example\\.com/.*";);
+ }
+
+ @Test void testRegexAlternation() {
+ final String allowed =
+
"https://.*|http://data\\.example\\.com:8080/.*|ftp://ftp\\.example\\.com/.*";;
+ assertAllowed("https://any.example.com:8443/x/table.csv";, allowed);
+ assertAllowed("http://data.example.com:8080/table.csv";, allowed);
+ assertAllowed("ftp://ftp.example.com/pub/table.csv";, allowed);
+ assertRejected("http://data.example.com/table.csv";, allowed);
+ assertRejected("ftp://other.example.com/pub/table.csv";, allowed);
+ }
+
+ /** Tests that a host that is followed by "/" in the regular expression is
+ * matched exactly: hosts that start or end with it, or that have it as user
+ * info, do not match; and shows that without the "/" they would. */
+ @Test void testRegexHostBoundary() {
+ final String allowed = "https://example\\.com/.*";;
+ assertRejected("https://example.com.other.org/table.csv";, allowed);
+ assertRejected("https://[email protected]/table.csv";, allowed);
+ assertRejected("https://my-example.com/table.csv";, allowed);
+ assertRejected("https://example.comx/table.csv";, allowed);
+ assertRejected("https://www.example.com/table.csv";, allowed);
+
+ final String noSlash = "https://example\\.com.*";;
+ assertAllowed("https://example.com.other.org/table.csv";, noSlash);
+ assertAllowed("https://[email protected]/table.csv";, noSlash);
+ final String unescaped = "https://example.com/.*";;
+ assertAllowed("https://examplexcom/table.csv";, unescaped);
+ }
Review Comment:
This test as well does not seem to add much additional coverage apart from
pattern variations.
##########
file/src/main/java/org/apache/calcite/adapter/file/FileSchema.java:
##########
@@ -119,10 +130,60 @@ private static String trim(String s, String suffix) {
return builder.build();
}
+ /** Checks that a model-supplied table URL may be dereferenced by this
+ * adapter: a local {@code file} URL always, a remote one (any other
+ * protocol, or a {@code file} URL naming a host, such as a Windows UNC path)
+ * only when it matches the regular expression
+ * {@link CalciteSystemProperty#FILE_URLS_ALLOWED}. */
+ static void checkUrlAllowed(String url) {
+ checkUrlAllowed(url, CalciteSystemProperty.FILE_URLS_ALLOWED.value());
+ }
+
+ static void checkUrlAllowed(String url, String allowedUrls) {
+ final URI uri;
+ try {
+ uri = new URI(url);
+ uri.toURL();
+ } catch (URISyntaxException | MalformedURLException |
IllegalArgumentException e) {
+ // Malformed; Sources.url parses the same way and will report the error
+ return;
+ }
+ // A "file" URL is local unless it names a host, as an authority
+ // ("file://host/share") or, as Windows resolves it to a UNC path, as a
+ // path starting with two slashes or backslashes ("file:////host/share",
+ // "file:%5C%5Chost%5Cshare")
+ final String path =
+ Util.first(uri.isOpaque() ? uri.getSchemeSpecificPart() :
uri.getPath(), "");
+ if ("file".equalsIgnoreCase(uri.getScheme())
+ && Util.isNullOrEmpty(uri.getRawAuthority())
+ && !path.replace('\\', '/').startsWith("//")) {
+ return;
+ }
+ if (!allowedUrls.isEmpty()) {
+ final Pattern pattern;
+ try {
+ pattern = Pattern.compile(allowedUrls);
+ } catch (PatternSyntaxException e) {
+ throw new IllegalArgumentException("Invalid regular expression '"
+ + allowedUrls + "' in system property
\"calcite.file.urls.allowed\"",
+ e);
+ }
+ if (pattern.matcher(url).matches()) {
+ return;
+ }
+ }
+ throw new IllegalArgumentException("Remote URL '" + url
+ + "' is not allowed in file adapter table operands (only local 'file'"
+ + " URLs are allowed by default); to allow it, set the system property"
+ + " \"calcite.file.urls.allowed\" to a regular expression that matches"
+ + " it");
Review Comment:
The exception message is a bit verbose. I think it would be just fine to say
`URL http://abc is not allowed` or `URL http://abc does not match
'calcite.file.urls.allowed' pattern`. The fact that we are dealing with the
file adapter is clear in the stacktrace and the peculiarities about local vs
remote are present in the documentation of the property.
##########
file/src/test/java/org/apache/calcite/adapter/file/FileSchemaProtocolTest.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.calcite.adapter.file;
+
+import org.apache.calcite.config.CalciteSystemProperty;
+import org.apache.calcite.schema.Table;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
+
+/**
+ * Tests that {@link FileSchema} dereferences only local {@code file} sources
by
+ * default, and requires an explicit operator opt-in (the
+ * {@code calcite.file.urls.allowed} system property) for remote URL table
+ * operands.
+ */
+class FileSchemaProtocolTest {
+
+ private static Map<String, Object> table(String name, String url) {
+ final Map<String, Object> tableDef = new HashMap<>();
+ tableDef.put("name", name);
+ tableDef.put("url", url);
+ return tableDef;
+ }
+
+ /** Skips a test that relies on the default allowlist, if the test JVM was
+ * started with {@code calcite.file.urls.allowed} set. */
+ private static void assumeDefaultAllowlist() {
+ assumeTrue(CalciteSystemProperty.FILE_URLS_ALLOWED.value().isEmpty(),
+ "calcite.file.urls.allowed is set");
+ }
+
+ private static void assertAllowed(String url, String allowedUrls) {
+ assertDoesNotThrow(() -> FileSchema.checkUrlAllowed(url, allowedUrls),
+ () -> url + " should be allowed by " + allowedUrls);
+ }
+
+ private static void assertRejected(String url, String allowedUrls) {
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class,
+ () -> FileSchema.checkUrlAllowed(url, allowedUrls),
+ () -> url + " should be rejected by " + allowedUrls);
+ assertThat(url, e.getMessage(),
+ containsString("Remote URL '" + url + "' is not allowed"));
+ assertThat(url, e.getMessage(),
+ containsString("calcite.file.urls.allowed"));
+ }
+
+ @Test void testRemoteUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "http://localhost/table.csv";)));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL 'http://localhost/table.csv' is not
allowed"));
+ assertThat(e.getMessage(),
+ containsString("calcite.file.urls.allowed"));
+ }
+
+ @Test void testFtpUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "ftp://localhost/table.csv";)));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL 'ftp://localhost/table.csv' is not
allowed"));
+ }
+
+ @Test void testFileUrlAllowedByDefault() {
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "file:foo.csv")));
+ final Map<String, Table> tableMap = schema.getTableMap();
+ assertThat(tableMap.containsKey("T"), is(true));
+ }
+
+ @Test void testRemoteFileUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(
+ table("T", "file://fileserver.example.com/share/table.csv")));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL
'file://fileserver.example.com/share/table.csv'"
+ + " is not allowed"));
+ }
+
+ /** Exercises the check with an explicit regular expression rather than by
+ * mutating a JVM-wide system property; {@link
+ * org.apache.calcite.config.CalciteSystemProperty} caches at class-load, so
+ * mid-run {@code System.setProperty} would not be observed by the read
+ * inside {@link FileSchema#checkUrlAllowed(String)}. */
+ @Test void testRegex() {
+ final String allowed = "https://data\\.example\\.com/.*";;
+ assertAllowed("https://data.example.com/table.csv";, allowed);
+ assertAllowed("https://data.example.com/a/b/table.csv?x=1";, allowed);
+ assertRejected("http://data.example.com/table.csv";, allowed);
+ assertRejected("https://other.example.com/table.csv";, allowed);
+ assertRejected("https://data.example.com:8443/table.csv";, allowed);
+ assertRejected("ftp://data.example.com/table.csv";, allowed);
+ }
+
+ @Test void testEmptyRegexRejectsRemote() {
+ assertRejected("https://data.example.com/table.csv";, "");
+ }
+
+ /** Tests that the regular expression must match the whole URL, not just a
+ * part of it. */
+ @Test void testRegexMatchesWholeUrl() {
+ assertRejected("https://data.example.com/table.csv";,
+ "https://data\\.example\\.com/";);
+ assertRejected("https://data.example.com/table.csv";,
"data\\.example\\.com");
+
assertRejected("https://other.example.com/?u=https://data.example.com/t.csv";,
+ "https://data\\.example\\.com/.*";);
+ }
+
+ @Test void testRegexAlternation() {
+ final String allowed =
+
"https://.*|http://data\\.example\\.com:8080/.*|ftp://ftp\\.example\\.com/.*";;
+ assertAllowed("https://any.example.com:8443/x/table.csv";, allowed);
+ assertAllowed("http://data.example.com:8080/table.csv";, allowed);
+ assertAllowed("ftp://ftp.example.com/pub/table.csv";, allowed);
+ assertRejected("http://data.example.com/table.csv";, allowed);
+ assertRejected("ftp://other.example.com/pub/table.csv";, allowed);
+ }
+
+ /** Tests that a host that is followed by "/" in the regular expression is
+ * matched exactly: hosts that start or end with it, or that have it as user
+ * info, do not match; and shows that without the "/" they would. */
+ @Test void testRegexHostBoundary() {
+ final String allowed = "https://example\\.com/.*";;
+ assertRejected("https://example.com.other.org/table.csv";, allowed);
+ assertRejected("https://[email protected]/table.csv";, allowed);
+ assertRejected("https://my-example.com/table.csv";, allowed);
+ assertRejected("https://example.comx/table.csv";, allowed);
+ assertRejected("https://www.example.com/table.csv";, allowed);
+
+ final String noSlash = "https://example\\.com.*";;
+ assertAllowed("https://example.com.other.org/table.csv";, noSlash);
+ assertAllowed("https://[email protected]/table.csv";, noSlash);
+ final String unescaped = "https://example.com/.*";;
+ assertAllowed("https://examplexcom/table.csv";, unescaped);
+ }
+
+ @Test void testInvalidRegex() {
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, () ->
+ FileSchema.checkUrlAllowed("https://data.example.com/t.csv";,
+ "https://(data"));
+ assertThat(e.getMessage(),
+ containsString("Invalid regular expression 'https://(data' in system"
+ + " property \"calcite.file.urls.allowed\""));
+ // The regular expression is not compiled while checking a local file URL
+ assertAllowed("file:/tmp/table.csv", "https://(data");
+ }
+
+ /** Tests that every form of {@code file} URL that Windows resolves to a UNC
+ * path counts as remote, as does a {@code file} URL with any authority. */
+ @Test void testRemoteFileUrlForms() {
+ final String[] urls = {
+ "file://fileserver.example.com/share/table.csv",
+ "file:////fileserver.example.com/share/table.csv",
+ "file://///fileserver.example.com/share/table.csv",
+ "file:%5C%5Cfileserver.example.com%5Cshare%5Ctable.csv",
+ "file:/%5C%5Cfileserver.example.com/share/table.csv",
+ };
+ for (String url : urls) {
+ assertRejected(url, "");
+ assertRejected(url, "https://.*";);
+ assertRejected(url, "file://other\\.example\\.com/.*");
+ assertAllowed(url, "file:.*");
+ }
+ // The expression is matched against the URL as written, so it must allow
+ // each form that is used
+ assertAllowed(urls[0], "file://fileserver\\.example\\.com/share/.*");
+ assertRejected(urls[1], "file://fileserver\\.example\\.com/share/.*");
+ assertRejected("file://localhost/tmp/table.csv", "");
+ }
+
+ @Test void testLocalFileUrlForms() {
+ final String[] urls = {
+ "file:table.csv",
+ "file:/tmp/table.csv",
+ "file:///tmp/table.csv",
+ "file:///C:/data/table.csv",
+ "file:/C:/data/table.csv",
+ };
+ for (String url : urls) {
+ assertAllowed(url, "");
+ assertAllowed(url, "https://data\\.example\\.com/.*";);
+ }
+ }
Review Comment:
Both tests focus on what is considered a local URL so if we add a separate
method `isLocalURL` in `FileSchema` we would be able to test the behavior
explicitly.
##########
site/_docs/file_adapter.md:
##########
@@ -344,6 +353,55 @@ sqlline> select distinct deptno from depts;
3 rows selected (0.985 seconds)
{% endhighlight %}
+## Enabling remote fetches
Review Comment:
There is a lot of overlap with the Javadoc of the property. How about
removing the section and put a link to the Javadoc whenever necessary. We could
also keep the section but trim most of it and just add a pointer to the Javadoc.
##########
file/src/test/java/org/apache/calcite/adapter/file/FileSchemaProtocolTest.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.calcite.adapter.file;
+
+import org.apache.calcite.config.CalciteSystemProperty;
+import org.apache.calcite.schema.Table;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
+
+/**
+ * Tests that {@link FileSchema} dereferences only local {@code file} sources
by
+ * default, and requires an explicit operator opt-in (the
+ * {@code calcite.file.urls.allowed} system property) for remote URL table
+ * operands.
+ */
+class FileSchemaProtocolTest {
+
+ private static Map<String, Object> table(String name, String url) {
+ final Map<String, Object> tableDef = new HashMap<>();
+ tableDef.put("name", name);
+ tableDef.put("url", url);
+ return tableDef;
+ }
+
+ /** Skips a test that relies on the default allowlist, if the test JVM was
+ * started with {@code calcite.file.urls.allowed} set. */
+ private static void assumeDefaultAllowlist() {
+ assumeTrue(CalciteSystemProperty.FILE_URLS_ALLOWED.value().isEmpty(),
+ "calcite.file.urls.allowed is set");
+ }
+
+ private static void assertAllowed(String url, String allowedUrls) {
+ assertDoesNotThrow(() -> FileSchema.checkUrlAllowed(url, allowedUrls),
+ () -> url + " should be allowed by " + allowedUrls);
+ }
+
+ private static void assertRejected(String url, String allowedUrls) {
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class,
+ () -> FileSchema.checkUrlAllowed(url, allowedUrls),
+ () -> url + " should be rejected by " + allowedUrls);
+ assertThat(url, e.getMessage(),
+ containsString("Remote URL '" + url + "' is not allowed"));
+ assertThat(url, e.getMessage(),
+ containsString("calcite.file.urls.allowed"));
+ }
+
+ @Test void testRemoteUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "http://localhost/table.csv";)));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL 'http://localhost/table.csv' is not
allowed"));
+ assertThat(e.getMessage(),
+ containsString("calcite.file.urls.allowed"));
+ }
+
+ @Test void testFtpUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "ftp://localhost/table.csv";)));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL 'ftp://localhost/table.csv' is not
allowed"));
+ }
+
+ @Test void testFileUrlAllowedByDefault() {
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(table("T", "file:foo.csv")));
+ final Map<String, Table> tableMap = schema.getTableMap();
+ assertThat(tableMap.containsKey("T"), is(true));
+ }
+
+ @Test void testRemoteFileUrlRejectedByDefault() {
+ assumeDefaultAllowlist();
+ final FileSchema schema =
+ new FileSchema(null, "S", null,
+ ImmutableList.of(
+ table("T", "file://fileserver.example.com/share/table.csv")));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, schema::getTableMap);
+ assertThat(e.getMessage(),
+ containsString("Remote URL
'file://fileserver.example.com/share/table.csv'"
+ + " is not allowed"));
+ }
+
+ /** Exercises the check with an explicit regular expression rather than by
+ * mutating a JVM-wide system property; {@link
+ * org.apache.calcite.config.CalciteSystemProperty} caches at class-load, so
+ * mid-run {@code System.setProperty} would not be observed by the read
+ * inside {@link FileSchema#checkUrlAllowed(String)}. */
+ @Test void testRegex() {
+ final String allowed = "https://data\\.example\\.com/.*";;
+ assertAllowed("https://data.example.com/table.csv";, allowed);
+ assertAllowed("https://data.example.com/a/b/table.csv?x=1";, allowed);
+ assertRejected("http://data.example.com/table.csv";, allowed);
+ assertRejected("https://other.example.com/table.csv";, allowed);
+ assertRejected("https://data.example.com:8443/table.csv";, allowed);
+ assertRejected("ftp://data.example.com/table.csv";, allowed);
+ }
+
+ @Test void testEmptyRegexRejectsRemote() {
+ assertRejected("https://data.example.com/table.csv";, "");
+ }
+
+ /** Tests that the regular expression must match the whole URL, not just a
+ * part of it. */
+ @Test void testRegexMatchesWholeUrl() {
+ assertRejected("https://data.example.com/table.csv";,
+ "https://data\\.example\\.com/";);
+ assertRejected("https://data.example.com/table.csv";,
"data\\.example\\.com");
+
assertRejected("https://other.example.com/?u=https://data.example.com/t.csv";,
+ "https://data\\.example\\.com/.*";);
+ }
+
+ @Test void testRegexAlternation() {
+ final String allowed =
+
"https://.*|http://data\\.example\\.com:8080/.*|ftp://ftp\\.example\\.com/.*";;
+ assertAllowed("https://any.example.com:8443/x/table.csv";, allowed);
+ assertAllowed("http://data.example.com:8080/table.csv";, allowed);
+ assertAllowed("ftp://ftp.example.com/pub/table.csv";, allowed);
+ assertRejected("http://data.example.com/table.csv";, allowed);
+ assertRejected("ftp://other.example.com/pub/table.csv";, allowed);
+ }
+
+ /** Tests that a host that is followed by "/" in the regular expression is
+ * matched exactly: hosts that start or end with it, or that have it as user
+ * info, do not match; and shows that without the "/" they would. */
+ @Test void testRegexHostBoundary() {
+ final String allowed = "https://example\\.com/.*";;
+ assertRejected("https://example.com.other.org/table.csv";, allowed);
+ assertRejected("https://[email protected]/table.csv";, allowed);
+ assertRejected("https://my-example.com/table.csv";, allowed);
+ assertRejected("https://example.comx/table.csv";, allowed);
+ assertRejected("https://www.example.com/table.csv";, allowed);
+
+ final String noSlash = "https://example\\.com.*";;
+ assertAllowed("https://example.com.other.org/table.csv";, noSlash);
+ assertAllowed("https://[email protected]/table.csv";, noSlash);
+ final String unescaped = "https://example.com/.*";;
+ assertAllowed("https://examplexcom/table.csv";, unescaped);
+ }
+
+ @Test void testInvalidRegex() {
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, () ->
+ FileSchema.checkUrlAllowed("https://data.example.com/t.csv";,
+ "https://(data"));
+ assertThat(e.getMessage(),
+ containsString("Invalid regular expression 'https://(data' in system"
+ + " property \"calcite.file.urls.allowed\""));
+ // The regular expression is not compiled while checking a local file URL
+ assertAllowed("file:/tmp/table.csv", "https://(data");
+ }
+
+ /** Tests that every form of {@code file} URL that Windows resolves to a UNC
+ * path counts as remote, as does a {@code file} URL with any authority. */
+ @Test void testRemoteFileUrlForms() {
+ final String[] urls = {
+ "file://fileserver.example.com/share/table.csv",
+ "file:////fileserver.example.com/share/table.csv",
+ "file://///fileserver.example.com/share/table.csv",
+ "file:%5C%5Cfileserver.example.com%5Cshare%5Ctable.csv",
+ "file:/%5C%5Cfileserver.example.com/share/table.csv",
+ };
+ for (String url : urls) {
+ assertRejected(url, "");
+ assertRejected(url, "https://.*";);
+ assertRejected(url, "file://other\\.example\\.com/.*");
+ assertAllowed(url, "file:.*");
+ }
+ // The expression is matched against the URL as written, so it must allow
+ // each form that is used
+ assertAllowed(urls[0], "file://fileserver\\.example\\.com/share/.*");
+ assertRejected(urls[1], "file://fileserver\\.example\\.com/share/.*");
+ assertRejected("file://localhost/tmp/table.csv", "");
+ }
+
+ @Test void testLocalFileUrlForms() {
+ final String[] urls = {
+ "file:table.csv",
+ "file:/tmp/table.csv",
+ "file:///tmp/table.csv",
+ "file:///C:/data/table.csv",
+ "file:/C:/data/table.csv",
+ };
+ for (String url : urls) {
+ assertAllowed(url, "");
+ assertAllowed(url, "https://data\\.example\\.com/.*";);
+ }
+ }
+
+ /** Tests that {@code jar} URLs are remote, even when they wrap a local file,
+ * and that the regular expression is matched against the whole {@code jar}
+ * URL. */
+ @Test void testJarUrl() {
+ final String local = "jar:file:/opt/app/lib/data.jar!/table.csv";
+ final String remote = "jar:https://files.example.com/data.jar!/table.csv";;
+ assertRejected(local, "");
+ assertRejected(local, "file:.*");
+ assertAllowed(local, "jar:.*");
+ assertAllowed(remote, "jar:.*");
+
+ final String lib = "jar:file:/opt/app/lib/[^/]*\\.jar!/.*";
+ assertAllowed(local, lib);
+ assertRejected("jar:file:/opt/app/other/data.jar!/table.csv", lib);
+ assertRejected(remote, lib);
+
+ assertAllowed(remote, "jar:https://files\\.example\\.com/.*";);
+ assertRejected(remote, "jar:https://data\\.example\\.com/.*";);
+ // A regular expression for the archive's URL does not allow "jar" URLs
+ // that wrap it
+ assertRejected(remote, "https://files\\.example\\.com/.*";);
+ }
Review Comment:
This is also a test about what is a local URL or not so the jar protocol
does not change much. It can be unified with the rest of isLocal tests above
and simplified as we don't need to pass `expectedUrls`.
##########
site/_docs/history.md:
##########
@@ -114,6 +114,19 @@ check the width of each group, and turned `1-2-3-4-5` into
requires exactly 16 bytes; a longer value used to be truncated. Blanks are not
trimmed.
+* [<a
href="https://issues.apache.org/jira/browse/CALCITE-7860";>CALCITE-7860</a>]
+The file adapter now dereferences only local `file:` sources by default. Table
+`url` operands naming remote protocols (`http`, `https`, `ftp`, ...; including
+`jar:`, even when it wraps a local file) are rejected with
+`IllegalArgumentException` unless they match the new
+`calcite.file.urls.allowed` system property, a regular expression that must
+match the whole URL, for example
+`-Dcalcite.file.urls.allowed=https://data\.example\.com/.*`. A
+`file:` URL that names a host (`file://host/...`, or a Windows UNC path) is
+remote too, and also requires a match; this includes
+`file://localhost/...`. See
+[Enabling remote fetches]({{ site.baseurl
}}/docs/file_adapter.html#enabling-remote-fetches).
Review Comment:
Let's keep the release notes brief. Interested users can check the Jira,
doc, code, or Javadoc. No need to include examples and configuration
suggestions here.
```suggestion
The file adapter now dereferences only local `file:` sources by default.
Table `url` operands naming remote protocols (`http`, `https`, `ftp`, etc.) are
rejected with `IllegalArgumentException` unless they match the new
[calcite.file.urls.allowed system property](link to Javadoc or section)`.
```
--
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]
