xuzifu666 commented on code in PR #5221:
URL: https://github.com/apache/calcite/pull/5221#discussion_r3859102538
##########
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:
Using `SecurityException` to signal an "escape" rejection is somewhat
unconventional. In JVM semantics, `SecurityException` is typically associated
with the `SecurityManager`; using it here to indicate an "out-of-bounds path"
could mislead operations staff when they examine the stack trace. Wouldn't it
be more readable to use a dedicated `RuntimeException` (such as
`ModelBaseDirectoryException`)?
That would also avoid confusion with potential `SecurityManager` behavior.
Functionally, however, there are no issues.
##########
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) {
Review Comment:
The `catch` block only covers `IOException`. In rare instances,
`mapper.readValue` might throw unchecked exceptions containing payload data
(such as `RuntimeException`s from custom deserializers), which would propagate
directly to the client. Given that the vast majority of Jackson parsing or
mapping errors are subclasses of `IOException`, the primary attack surface is
already covered and the risk is low; however, for a stricter approach, one
could add a catch-all for `RuntimeException` to suppress those as well.
--
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]
