[MINOR] Reject traversal segments in note and folder paths#5248
Merged
Conversation
`NotebookRepo.buildNoteFileName` composes a filesystem path or object-store key from a user-supplied note path. The previous implementation required only a leading `/` but otherwise concatenated the value verbatim, so a value such as `/../etc/foo` would compose to a location outside the configured notebook root for backends that perform a raw filesystem operation (`FileSystemNotebookRepo`) or build an object key directly from the string (S3 / Azure / GCS). This PR centralises a small validation helper at the `NotebookRepo` interface level so every backend gets the same defence, fixes one folder-level path that bypassed `buildNoteFileName`, and adds the missing `normalizeNotePath` call in `NotebookService.renameNote`. Improvement * [x] Add `NotebookRepo.rejectTraversalSegments` (URL-decoded, recursive, capped at 5 layers). * [x] Call `rejectTraversalSegments` from the default `buildNoteFileName`, so every implementation is covered. * [x] Apply it to `FileSystemNotebookRepo`'s folder-level `move` / `remove`, which build the path without going through `buildNoteFileName`. * [x] Add the missing `normalizeNotePath` in `NotebookService.renameNote` to match `createNote`, `cloneNote`, and `moveFolder`. * [x] `NotebookRepoPathValidationTest` (27): `..`, `.`, URL-encoded variants (`%2e%2e`, `%2E%2E`), double-encoded variants, and an excessive-encoding-layer payload. Realistic note names including Korean characters and `...` inside a segment are accepted. N/A — `[MINOR]` change. ``` ./mvnw install -pl zeppelin-server -am -DskipTests ./mvnw test -pl zeppelin-server -Dtest='NotebookRepoPathValidationTest,NotebookServiceTest' -DfailIfNoTests=false ./mvnw test -pl zeppelin-plugins/notebookrepo/filesystem ``` All test sets pass locally. * Does the license files need to update? No * Is there breaking changes for older versions? No — existing valid note paths render to byte-identical filenames; only paths containing literal traversal segments now produce a clear `IOException` instead of silently composing to an out-of-root location. * Does this needs documentation? No Closes apache#5227 from jongyoul/ZEPPELIN-fs-notebook-path. Signed-off-by: Jongyoul Lee <jongyoul@gmail.com>
There was a problem hiding this comment.
Pull request overview
Backport of PR #5227 to branch-0.12, adding path traversal protection for note and folder paths. Due to module differences in 0.12, the validator and tests live in zeppelin-zengine instead of zeppelin-server.
Changes:
- Introduces
NotebookPathValidatorwithrejectTraversalSegmentsanddecodeRepeatedly(URL-decoded, capped at 5 layers). - Wires the validator into
NotebookRepo.buildNoteFileName,FileSystemNotebookRepofolder-levelmove/remove, andNotebookService.renameNote(which now also callsnormalizeNotePath). - Adds
NotebookRepoPathValidationTestcovering traversal, URL-encoded, and excessive-encoding payloads as well as accepted realistic names.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookPathValidator.java | New shared validator with traversal-segment rejection and bounded repeated URL decoding. |
| zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepo.java | Calls validator from default buildNoteFileName so all backends are covered. |
| zeppelin-plugins/notebookrepo/filesystem/src/main/java/org/apache/zeppelin/notebook/repo/FileSystemNotebookRepo.java | Validates folder paths in move/remove that don't go through buildNoteFileName. |
| zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java | Replaces local decodeRepeatedly with shared validator and adds missing normalizeNotePath in renameNote. |
| zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoPathValidationTest.java | New tests for traversal rejection, encoded variants, and accepted paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ParkGyeongTae
approved these changes
May 14, 2026
Member
ParkGyeongTae
left a comment
ParkGyeongTae
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the backport to branch-0.12.
The validation is placed at the right level so notebook repo implementations get consistent protection, and the extra checks for filesystem folder move / remove paths cover the bypass cases. The test coverage for raw, encoded, double-encoded traversal segments and valid note names looks good to me.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR for?
This is a backport of PR #5227 to branch-0.12. It adds path traversal protection by rejecting traversal segments in note and folder paths. Due to module structure differences between master and branch-0.12, the files were moved from zeppelin-server to zeppelin-zengine.
What type of PR is it?
Improvement
Todos
What is the Jira issue?
How should this be tested?