rubenada commented on code in PR #5334: URL: https://github.com/apache/calcite/pull/5334#discussion_r4400512741
########## 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: tests refactored ########## 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: done -- 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]
