rubenada commented on code in PR #5221:
URL: https://github.com/apache/calcite/pull/5221#discussion_r3861171876
##########
core/src/main/java/org/apache/calcite/model/ModelHandler.java:
##########
@@ -114,12 +124,48 @@ public ModelHandler(SchemaPlus rootSchema, String uri,
root = mapper.readValue(inline, JsonRoot.class);
} else {
mapper = uri.endsWith(".yaml") || uri.endsWith(".yml") ? YAML_MAPPER :
JSON_MAPPER;
- root = mapper.readValue(new File(uri), JsonRoot.class);
+ try {
+ root = mapper.readValue(modelFile(uri), JsonRoot.class);
+ } catch (IOException e) {
+ // The client-facing message for a non-inline model must not depend
+ // on the file's contents or on whether the file exists. Collapse
everything
+ // to one path-only summary; keep the detail in the operator log.
+ LOGGER.warn("Unable to read model file '{}'", uri, e);
+ throw new IOException("Unable to read model file '" + uri
+ + "'; see log of " + ModelHandler.class.getName()
+ + " for details");
+ }
}
visit(root);
this.defaultSchemaName = root.defaultSchema;
}
+ /** Resolves a non-{@code inline:} model URI to a file, enforcing
+ * {@link CalciteSystemProperty#MODEL_BASE_DIRECTORY} when it is set. */
+ private static File modelFile(String uri) {
+ return modelFile(CalciteSystemProperty.MODEL_BASE_DIRECTORY.value(), uri);
+ }
+
+ /** As {@link #modelFile(String)}, but with an explicit base directory;
+ * package-private for tests.
+ *
+ * <p>An empty base directory means "no restriction": the URI is used as
+ * given. Otherwise a relative URI resolves under the base directory,
+ * and any URI that resolves (lexically) outside it is rejected. */
+ static File modelFile(String baseDirectory, String uri) {
+ if (baseDirectory.isEmpty()) {
+ return new File(uri);
+ }
+ final Path basePath =
Paths.get(baseDirectory).toAbsolutePath().normalize();
+ final Path path = basePath.resolve(uri).normalize();
+ if (!path.startsWith(basePath)) {
+ throw new SecurityException("Model file '" + uri + "' resolves"
Review Comment:
I see your point, but `SecurityException` is already used in Calcite for
this type of cases. See e.g. `ClassNameFilter` checks, and the corresponding
tests that exist already in `ModelHandlerTest` expecting a `SecurityException`
--
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]
