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