[MINOR] Reject traversal segments in note and folder paths

[jongyoul]

What is this PR for?

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.

What type of PR is it?

Improvement

Todos

• Add NotebookRepo.rejectTraversalSegments (URL-decoded, recursive, capped at 5 layers).
• Call rejectTraversalSegments from the default buildNoteFileName, so every implementation is covered.
• Apply it to FileSystemNotebookRepo's folder-level move / remove, which build the path without going through buildNoteFileName.
• Add the missing normalizeNotePath in NotebookService.renameNote to match createNote, cloneNote, and moveFolder.
• 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.

What is the Jira issue?

N/A — [MINOR] change.

How should this be tested?

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

Questions

• 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

[Copilot]

Pull request overview

This PR hardens Zeppelin’s notebook path handling against path traversal by rejecting .. / . path segments (including URL-encoded forms) when composing note storage paths/keys, and aligns service-layer rename behavior with existing normalization.

Changes:

• Introduces NotebookPathValidator with shared helpers to repeatedly URL-decode note paths (capped) and reject traversal segments.
• Applies traversal-segment rejection in NotebookRepo.buildNoteFileName(...) so all backends using it get consistent protection.
• Fixes missed normalization in NotebookService.renameNote(...) and adds traversal checks for folder move/remove in FileSystemNotebookRepo.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookPathValidator.java	Adds centralized URL-decoding and traversal-segment rejection utilities.
zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepo.java	Enforces traversal-segment rejection in the default note filename/key builder.
zeppelin-plugins/notebookrepo/filesystem/src/main/java/org/apache/zeppelin/notebook/repo/FileSystemNotebookRepo.java	Validates folder paths for traversal segments before folder move/remove operations.
zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java	Uses the shared decoder and normalizes rename paths before moving notes.
zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoPathValidationTest.java	Adds test coverage for traversal rejection and repeated-decoding behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jongyoul]

@Copilot You are right — the mkDir line targets the wrong parent. However, that line was not introduced by this PR (the PR only added the two rejectTraversalSegments calls just above it; the buggy tryMkDir(folderPath...) predates this change in master). Since this PR is a focused security fix for path traversal, I would prefer to keep its scope tight and address the move(folderPath, newFolderPath, ...) mkdir bug in a separate follow-up.

Filed as ZEPPELIN-6415.

[jongyoul]

@Copilot Good catch on both. Fixed in dd53762:

1. URLDecoder.decode is now wrapped in a try/catch that re-throws IllegalArgumentException as IOException — keeps the declared failure mode consistent.
2. The loop now runs MAX_DECODE_LAYERS + 1 iterations (pass <= MAX_DECODE_LAYERS), so the constant truly caps the number of accepted encoding layers: detecting stability after N layers needs N decodes plus one confirmation pass. Updated the Javadoc and added two tests — one for the malformed-encoding wrap, one for the exact 5-layer boundary.

[tbonelee]

LGTM, approving. A couple of questions that might be worth a follow-up:

1. The folder-level move(folderPath, ...) and remove(folderPath, ...) in S3NotebookRepo, OSSNotebookRepo, AzureNotebookRepo, etc. don't go through buildNoteFileName either, so they look like they'd benefit from the same rejectTraversalSegments call you added to FileSystemNotebookRepo. Was leaving them out intentional, or worth a follow-up?
2. The segment splitter is /+, so backslash variants like /foo/..\bar pass through. I'm not sure how realistic this is, but on Windows + LocalFileSystem I wonder if java.io.File could end up interpreting \ as a separator and resolving outside the notebook dir? Might be worth considering [\\/]+, or it may not be a case we need to worry about.