Skip to content

feat(core): add checkpoint namespaces - #132

Open
kaihao-zhao wants to merge 1 commit into
mainfrom
codex/checkpoint-namespaces-review-fixture-v60
Open

kaihao-zhao wants to merge 1 commit into
mainfrom
codex/checkpoint-namespaces-review-fixture-v60

Conversation

@kaihao-zhao

Copy link
Copy Markdown

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.

@dev-unblocked dev-unblocked Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if (Objects.equals(this.threadId, checkpointNamespace)) {
if (Objects.equals(this.checkpointNamespace, checkpointNamespace)) {
return this;
}

Comment on lines +71 to +75
private Path getNamespaceFolder(RunnableConfig config) {
return config.checkpointNamespace()
.map(targetFolder::resolve)
.orElse(targetFolder);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dev-unblocked

dev-unblocked Bot commented Sep 27, 2026

Copy link
Copy Markdown

Risk Assessment

Risk: high

FileSystemSaver 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.

@unblocked-local-kaihao unblocked-local-kaihao Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +168 to +170
if (Objects.equals(this.threadId, checkpointNamespace)) {
return this;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 threadId happens to equal the desired namespace, the method silently returns this without updating.
  • If the namespace is already set to the same value, an unnecessary new object is created.
Suggested change
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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

@unblocked-local-kaihao

unblocked-local-kaihao Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Risk Assessment

Risk: high

FilesystemSaver 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.

@unblocked-local-kaihao unblocked-local-kaihao Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +71 to +75
private Path getNamespaceFolder(RunnableConfig config) {
return config.checkpointNamespace()
.map(targetFolder::resolve)
.orElse(targetFolder);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant