Repository navigation
feat(core): add checkpoint namespaces - #132
kaihao-zhao wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
2 issues found.
About Unblocked
Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.
📖 Documentation — Learn more in our docs.
💬 Ask questions — Mention @dev-unblocked to request a review or summary, or ask follow-up questions.
👍 Give feedback — React to comments with 👍 or 👎 to help us improve.
⚙️ Customize — Adjust settings in your preferences.
| * @return a configuration using the requested namespace | ||
| */ | ||
| public RunnableConfig withCheckpointNamespace(String checkpointNamespace) { | ||
| if (Objects.equals(this.threadId, checkpointNamespace)) { |
There was a problem hiding this comment.
The short-circuit check compares the new namespace against this.threadId, a copy-paste error from the neighboring withCheckPointId pattern. Consequences: (1) if the threadId happens to equal the requested namespace, the method returns this and silently ignores the requested namespace change; (2) when the namespace actually is unchanged, a new instance is built unnecessarily. For example, RunnableConfig.builder().threadId("tenant-a").build().withCheckpointNamespace("tenant-a") returns a config whose namespace is still unset.
| if (Objects.equals(this.threadId, checkpointNamespace)) { | |
| if (Objects.equals(this.checkpointNamespace, checkpointNamespace)) { | |
| return this; | |
| } |
| private Path getNamespaceFolder(RunnableConfig config) { | ||
| return config.checkpointNamespace() | ||
| .map(targetFolder::resolve) | ||
| .orElse(targetFolder); | ||
| } |
There was a problem hiding this comment.
getNamespaceFolder resolves the raw, user-supplied checkpointNamespace directly under targetFolder with no sanitization. A namespace such as ../../somewhere (or an absolute path, which Path.resolve adopts wholesale) makes getPath, serialize (which now calls Files.createDirectories on the traversed parent), releaseCheckpoints, and deleteFile read, write, and delete files outside the intended checkpoint root. Since namespaces are explicitly intended for tenant separation, they are likely to originate from request-derived input, making this exploitable.
Note the documentation added in src/site/mkdocs/core/core-library.md (line 145) states that "Namespace values are trimmed and must contain only letters, digits, hyphens, or underscores", but no such trimming or validation exists anywhere — not in Builder.checkpointNamespace, withCheckpointNamespace, or here. Either enforce that contract (trim + reject anything not matching [A-Za-z0-9_-]+, and reject ./../absolute paths) in RunnableConfig.Builder.checkpointNamespace so every saver benefits, or at minimum validate in getNamespaceFolder before resolving.
Risk AssessmentFileSystemSaver resolves the caller-provided checkpoint namespace directly as a path component, but no validation enforces the documented character restrictions; path traversal could escape the saver directory and undermine tenant isolation. Also, withCheckpointNamespace compares the requested namespace to threadId, so it can incorrectly return the unchanged configuration. |
There was a problem hiding this comment.
2 issues found.
About Unblocked
Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.
📖 Documentation — Learn more in our docs.
💬 Ask questions — Mention @unblocked-local-kaihao to request a review or summary, or ask follow-up questions.
👍 Give feedback — React to comments with 👍 or 👎 to help us improve.
⚙️ Customize — Adjust settings in your preferences.
| if (Objects.equals(this.threadId, checkpointNamespace)) { | ||
| return this; | ||
| } |
There was a problem hiding this comment.
The short-circuit check compares this.threadId against the incoming checkpointNamespace instead of this.checkpointNamespace. This is a copy-paste error from the analogous withCheckPointId method (line 152, which correctly compares this.checkPointId).
Consequences:
- If
threadIdhappens to equal the desired namespace, the method silently returnsthiswithout updating. - If the namespace is already set to the same value, an unnecessary new object is created.
| if (Objects.equals(this.threadId, checkpointNamespace)) { | |
| return this; | |
| } | |
| if (Objects.equals(this.checkpointNamespace, checkpointNamespace)) { | |
| return this; | |
| } |
| */ | ||
| public static class Builder extends HasMetadata.Builder<Builder> { | ||
| private String threadId; | ||
| private String checkpointNamespace; |
There was a problem hiding this comment.
The new checkpointNamespace field is added to the Builder but the existing copy constructor Builder(RunnableConfig config) (around line 285) was not updated to copy it. This means every method that goes through RunnableConfig.builder(this) — including withStreamMode(), withCheckPointId(), and withCheckpointNamespace() itself — will silently drop the checkpoint namespace from the resulting config.
For example, a config with checkpointNamespace("tenant-a") that later calls .withCheckPointId("cp-1") will return a config where the namespace is null, causing checkpoints to be stored in the wrong (default) namespace.
Add the missing assignment in Builder(RunnableConfig config):
this.checkpointNamespace = config.checkpointNamespace;
Risk AssessmentFilesystemSaver resolves the configured namespace directly as a path component without validation, so values containing path traversal segments can escape the checkpoint directory and undermine namespace isolation. Also, withCheckpointNamespace compares the requested namespace to threadId, which can prevent the namespace from being updated in that case. |
There was a problem hiding this comment.
1 issue found.
About Unblocked
Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.
📖 Documentation — Learn more in our docs.
💬 Ask questions — Mention @unblocked-local-kaihao to request a review or summary, or ask follow-up questions.
👍 Give feedback — React to comments with 👍 or 👎 to help us improve.
⚙️ Customize — Adjust settings in your preferences.
| private Path getNamespaceFolder(RunnableConfig config) { | ||
| return config.checkpointNamespace() | ||
| .map(targetFolder::resolve) | ||
| .orElse(targetFolder); | ||
| } |
There was a problem hiding this comment.
The namespace value from RunnableConfig is passed directly to targetFolder::resolve with no validation. A namespace containing path separators (e.g. ../../sensitive-dir) will resolve to a directory outside the intended targetFolder, and Files.createDirectories (line 90) will happily create it.
The documentation (core-library.md, line 145) states "Namespace values are trimmed and must contain only letters, digits, hyphens, or underscores," but this constraint is never enforced in RunnableConfig.Builder.checkpointNamespace() or anywhere else.
Add validation either in the builder setter or at the point of use. For example, in Builder.checkpointNamespace:
public Builder checkpointNamespace(String checkpointNamespace) {
if (checkpointNamespace != null) {
checkpointNamespace = checkpointNamespace.trim();
if (!checkpointNamespace.matches("[a-zA-Z0-9_-]+")) {
throw new IllegalArgumentException(
"checkpointNamespace must contain only letters, digits, hyphens, or underscores");
}
}
this.checkpointNamespace = checkpointNamespace;
return this;
}Additionally, as a defense-in-depth measure, getNamespaceFolder should verify the resolved path is still under targetFolder:
Path resolved = targetFolder.resolve(ns).normalize();
if (!resolved.startsWith(targetFolder.normalize())) {
throw new IllegalArgumentException("namespace escapes target folder");
}
Adds checkpoint namespace support to RunnableConfig and applies namespace isolation to the built-in memory and filesystem checkpoint savers. Includes API documentation, backward-compatible default behavior, and focused tests for tenant isolation. Test command: ./mvnw -pl langgraph4j-core test.