[CALCITE-7740] Harden ModelHandler against content-dependent errors - #5221
[CALCITE-7740] Harden ModelHandler against content-dependent errors#5221rubenada wants to merge 1 commit into
Conversation
|
xuzifu666
left a comment
xuzifu666
left a comment
There was a problem hiding this comment.
LGTM, only two comments regarding exception handling were left.
| 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" |
There was a problem hiding this 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.
| root = mapper.readValue(new File(uri), JsonRoot.class); | ||
| try { | ||
| root = mapper.readValue(modelFile(uri), JsonRoot.class); | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
The catch block only covers IOException. In rare instances, mapper.readValue might throw unchecked exceptions containing payload data (such as RuntimeExceptions 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.
2 participants
Jira Link
CALCITE-7740
Changes Proposed
Harden ModelHandler against content-dependent errors: sanitize parse-error text for non-inline models, add optional calcite.model.baseDirectory system property, add some clarifications in this regard to the threat model.