This is an automated email from the ASF dual-hosted git repository.
voidmatcha pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new 38b3e9687c [ZEPPELIN-6692] centralize folder path handling
38b3e9687c is described below
commit 38b3e9687c8e39bb64ec62ee44f5221aeca19a79
Author: gyowoo1113 <[email protected]>
AuthorDate: Sat Sep 12 23:13:25 2026 +0900
[ZEPPELIN-6692] centralize folder path handling
### What is this PR for?
Folder paths are currently handled differently depending on the operation.
Some call sites add a leading `/`, while others pass the path unchanged. Some
lower layers also assume a particular path form without checking it.
This can cause the same folder path to be interpreted differently depending
on how it reaches the repository. For example, `VFSNotebookRepo` uses
`substring(1)` assuming that the path starts with `/`, so a relative path can
silently lose its first character.
This PR centralizes folder path normalization in `NotebookService`. Rename,
move-to-trash, restore, and remove now use the same normalization so paths with
or without a leading `/` are passed to lower layers in the same absolute form.
The operation-specific leading-slash handling in `NotebookServer` is removed
accordingly.
`NotebookService` is used as the normalization point because all four
affected folder operations pass through it before reaching `NoteManager`. This
also keeps the shared normalization out of the individual `NotebookServer`
handlers and lets the service apply the same rule to all four operations.
`VFSNotebookRepo` also checks for the leading slash before using
`substring(1)`, so an unexpected relative path is rejected instead of being
silently truncated. The validation is currently applied to VFS, where the
leading-slash assumption directly affects this path handling. It can be
extended to other `NotebookRepo` implementations if needed as part of this PR.
### What type of PR is it?
Bug Fix
### Todos
- [x] Centralize folder path normalization in `NotebookService`
- [x] Remove operation-specific folder path compensation from
`NotebookServer`
- [x] Apply the shared folder path convention to rename, trash, restore,
and remove
- [x] Reject non-absolute folder paths in `VFSNotebookRepo`
- [x] Add regression tests for folder path normalization and VFS validation
### What is the Jira issue?
[[ZEPPELIN-6692]](https://issues.apache.org/jira/browse/ZEPPELIN-6692)
### How should this be tested?
`./mvnw test -pl zeppelin-server
-Dtest='NotebookServiceTest,NotebookServerTest,VFSNotebookRepoTest'`
`./mvnw test -pl zeppelin-server -Dtest='NotebookRepoPathValidationTest'`
### Screenshots (if appropriate)
N/A
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
Closes #5459 from gyowoo1113/ZEPPELIN-6692-centralize-folder-path-handling.
Signed-off-by: YONGJAE LEE <[email protected]>
---
.../notebook/repo/NotebookPathValidator.java | 42 ++++++++++++++-
.../zeppelin/notebook/repo/VFSNotebookRepo.java | 5 ++
.../apache/zeppelin/service/NotebookService.java | 36 ++++++++-----
.../org/apache/zeppelin/socket/NotebookServer.java | 2 -
.../repo/NotebookRepoPathValidationTest.java | 62 ++++++++++++++++++++++
.../notebook/repo/VFSNotebookRepoTest.java | 15 ++++++
.../zeppelin/service/NotebookServiceTest.java | 16 ++++--
7 files changed, 159 insertions(+), 19 deletions(-)
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookPathValidator.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookPathValidator.java
index 40f754076a..5a4aabf483 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookPathValidator.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookPathValidator.java
@@ -22,7 +22,7 @@ import java.nio.charset.StandardCharsets;
import java.util.regex.Pattern;
/**
- * Note-path validation helpers shared by {@link NotebookRepo} implementations
+ * Notebook path validation helpers shared by {@link NotebookRepo}
implementations
* and the service layer. A {@code final} class with {@code static} methods
* (rather than {@link NotebookRepo} default methods) prevents an
* implementation from accidentally — or intentionally — overriding the
@@ -84,4 +84,44 @@ public final class NotebookPathValidator {
}
throw new IOException("Exceeded maximum decode attempts. Possible
malicious input.");
}
+
+ /**
+ * Normalizes a path using the rules shared by note and folder paths.
+ *
+ * @param path the path to normalize
+ * @return the normalized path
+ * @throws IOException if the path cannot be normalized
+ */
+ public static String normalizePath(String path) throws IOException {
+ if (path == null) {
+ throw new IOException("Path must not be null");
+ }
+
+ if (!path.startsWith("/")) {
+ path = "/" + path;
+ }
+
+ path = decodeRepeatedly(path);
+
+ if (path.contains("..")) {
+ throw new IOException("Path can not contain '..'");
+ }
+
+ return path;
+ }
+
+ /**
+ * Requires {@code folderPath} to use the canonical absolute folder-path
form.
+ *
+ * @throws IOException if the path is null or does not start with {@code /}
+ */
+ public static void requireAbsoluteFolderPath(String folderPath) throws
IOException {
+ if (folderPath == null) {
+ throw new IOException("Folder path must not be null");
+ }
+
+ if (!folderPath.startsWith("/")) {
+ throw new IOException("Folder path must start with '/'");
+ }
+ }
}
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/VFSNotebookRepo.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/VFSNotebookRepo.java
index 32e433fff8..420a1c148a 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/VFSNotebookRepo.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/VFSNotebookRepo.java
@@ -197,6 +197,9 @@ public class VFSNotebookRepo extends AbstractNotebookRepo {
@Override
public void move(String folderPath, String newFolderPath,
AuthenticationInfo subject) throws IOException{
+ NotebookPathValidator.requireAbsoluteFolderPath(folderPath);
+ NotebookPathValidator.requireAbsoluteFolderPath(newFolderPath);
+
LOGGER.info("Move folder from {} to {}", folderPath, newFolderPath);
FileObject fileObject = rootNotebookFileObject.resolveFile(
folderPath.substring(1), NameScope.DESCENDENT);
@@ -218,6 +221,8 @@ public class VFSNotebookRepo extends AbstractNotebookRepo {
@Override
public void remove(String folderPath, AuthenticationInfo subject) throws
IOException {
+ NotebookPathValidator.requireAbsoluteFolderPath(folderPath);
+
LOGGER.info("Remove folder: {}", folderPath);
FileObject folderObject = rootNotebookFileObject.resolveFile(
folderPath.substring(1), NameScope.DESCENDENT);
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java
index dd3c6ac7ca..0e1cdfcdab 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java
@@ -234,13 +234,11 @@ public class NotebookService {
if (StringUtils.isBlank(notePath)) {
notePath = "/Untitled Note";
}
- if (!notePath.startsWith("/")) {
- notePath = "/" + notePath;
- }
notePath = notePath.replace("\r", " ").replace("\n", " ");
- notePath = NotebookPathValidator.decodeRepeatedly(notePath);
+ notePath = NotebookPathValidator.normalizePath(notePath);
+
if (notePath.endsWith("/")) {
throw new IOException("Note name shouldn't end with '/'");
}
@@ -250,12 +248,21 @@ public class NotebookService {
throw new IOException("Note name must be less than 255");
}
- if (notePath.contains("..")) {
- throw new IOException("Note name can not contain '..'");
- }
return notePath;
}
+ /**
+ * Normalizes a folder path to the canonical absolute form used by folder
operations.
+ * Accepts paths with or without a leading slash.
+ *
+ * @param folderPath
+ * @return
+ * @throws IOException
+ */
+ String normalizeFolderPath(String folderPath) throws IOException {
+ return NotebookPathValidator.normalizePath(folderPath);
+ }
+
public void removeNote(String noteId,
ServiceContext context,
ServiceCallback<String> callback) throws IOException {
@@ -745,6 +752,8 @@ public class NotebookService {
ServiceContext context,
ServiceCallback<Void> callback) throws IOException
{
+ folderPath = normalizeFolderPath(folderPath);
+
if (!folderPath.startsWith("/" + NoteManager.TRASH_FOLDER)) {
callback.onFailure(new IOException("Can not restore this folder: " +
folderPath +
" as it is not in trash folder"), context);
@@ -1302,16 +1311,17 @@ public class NotebookService {
ServiceCallback<Void> callback) throws
IOException {
//TODO(zjffdu) folder permission check
- //TODO(zjffdu) folderPath is relative path, need to fix it in frontend
LOGGER.info("Move folder {} to trash", folderPath);
- String destFolderPath = "/" + NoteManager.TRASH_FOLDER + "/" + folderPath;
+ folderPath = normalizeFolderPath(folderPath);
+
+ String destFolderPath = "/" + NoteManager.TRASH_FOLDER + folderPath;
if (notebook.containsNote(destFolderPath)) {
destFolderPath = destFolderPath + " " +
TRASH_CONFLICT_TIMESTAMP_FORMATTER.format(Instant.now());
}
- notebook.moveFolder("/" + folderPath, destFolderPath,
context.getAutheInfo());
+ notebook.moveFolder(folderPath, destFolderPath, context.getAutheInfo());
callback.onSuccess(null, context);
}
@@ -1331,7 +1341,7 @@ public class NotebookService {
ServiceContext context,
ServiceCallback<List<NoteInfo>> callback) throws
IOException {
try {
- notebook.removeFolder(folderPath, context.getAutheInfo());
+ notebook.removeFolder(normalizeFolderPath(folderPath),
context.getAutheInfo());
List<NoteInfo> notesInfo = notebook.getNotesInfo(
noteId -> authorizationService.isReader(noteId,
context.getUserAndRoles()));
callback.onSuccess(notesInfo, context);
@@ -1349,8 +1359,8 @@ public class NotebookService {
//TODO(zjffdu) folder permission check
try {
- notebook.moveFolder(normalizeNotePath(folderPath),
- normalizeNotePath(newFolderPath), context.getAutheInfo());
+ notebook.moveFolder(normalizeFolderPath(folderPath),
+ normalizeFolderPath(newFolderPath), context.getAutheInfo());
List<NoteInfo> notesInfo = notebook.getNotesInfo(
noteId -> authorizationService.isReader(noteId,
context.getUserAndRoles()));
callback.onSuccess(notesInfo, context);
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
index fcf76a8105..000c565eea 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
@@ -1061,7 +1061,6 @@ public class NotebookServer implements
AngularObjectRegistryListener,
private void removeFolder(NotebookSocket conn, ServiceContext context,
Message fromMessage) throws IOException {
String folderPath = (String) fromMessage.get("id");
- folderPath = "/" + folderPath;
getNotebookService().removeFolder(folderPath, context,
new WebSocketServiceCallback<List<NoteInfo>>(conn) {
@Override
@@ -1121,7 +1120,6 @@ public class NotebookServer implements
AngularObjectRegistryListener,
ServiceContext context,
Message fromMessage) throws IOException {
String folderPath = (String) fromMessage.get("id");
- folderPath = "/" + folderPath;
getNotebookService().restoreFolder(folderPath, context,
new WebSocketServiceCallback<Void>(conn) {
@Override
diff --git
a/zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoPathValidationTest.java
b/zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoPathValidationTest.java
index 58ead149df..f3184c88fc 100644
---
a/zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoPathValidationTest.java
+++
b/zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoPathValidationTest.java
@@ -16,6 +16,7 @@
*/
package org.apache.zeppelin.notebook.repo;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -110,4 +111,65 @@ class NotebookRepoPathValidationTest {
// cleanly; the constant means *layers*, not raw loop iterations.
assertEquals("/..",
NotebookPathValidator.decodeRepeatedly("/%252525252e%252525252e"));
}
+
+ @Test
+ void normalizePath_adds_leading_slash() throws IOException {
+ assertEquals("/folder/note",
NotebookPathValidator.normalizePath("folder/note"));
+ }
+
+ @Test
+ void normalizePath_keeps_existing_leading_slash() throws IOException {
+ assertEquals("/folder/note",
NotebookPathValidator.normalizePath("/folder/note"));
+ }
+
+ @Test
+ void normalizePath_decodes_url_encoding() throws IOException {
+ assertEquals("/folder/My Note",
NotebookPathValidator.normalizePath("/folder/My%20Note"));
+ }
+
+ @Test
+ void normalizePath_decodes_repeated_url_encoding() throws IOException {
+ assertEquals("/folder/My Note",
NotebookPathValidator.normalizePath("/folder/My%2520Note"));
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {
+ "/foo/../bar",
+ "/foo..bar",
+ "/...",
+ "/%2e%2e/bar",
+ "/%252e%252e/bar"
+ })
+ void normalizePath_rejects_double_dot(String path) {
+ assertThrows(IOException.class, () ->
NotebookPathValidator.normalizePath(path));
+ }
+
+ @Test
+ void normalizePath_rejects_null() {
+ assertThrows(IOException.class, () ->
NotebookPathValidator.normalizePath(null));
+ }
+
+ @Test
+ void requireAbsoluteFolderPath_accepts_absolute_path() {
+ assertDoesNotThrow(
+ () ->
NotebookPathValidator.requireAbsoluteFolderPath("/folder/subfolder"));
+ }
+
+ @Test
+ void requireAbsoluteFolderPath_accepts_root_path() {
+ assertDoesNotThrow(
+ () -> NotebookPathValidator.requireAbsoluteFolderPath("/"));
+ }
+
+ @Test
+ void requireAbsoluteFolderPath_rejects_relative_path() {
+ assertThrows(IOException.class,
+ () ->
NotebookPathValidator.requireAbsoluteFolderPath("folder/subfolder"));
+ }
+
+ @Test
+ void requireAbsoluteFolderPath_rejects_null() {
+ assertThrows(IOException.class,
+ () -> NotebookPathValidator.requireAbsoluteFolderPath(null));
+ }
}
diff --git
a/zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/VFSNotebookRepoTest.java
b/zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/VFSNotebookRepoTest.java
index 9bf02c0b3d..c7dcc9bdc4 100644
---
a/zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/VFSNotebookRepoTest.java
+++
b/zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/VFSNotebookRepoTest.java
@@ -39,6 +39,7 @@ import java.util.List;
import java.util.Map;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
class VFSNotebookRepoTest {
@@ -177,4 +178,18 @@ class VFSNotebookRepoTest {
File dir = new File(notebookRepo.rootNotebookFolder + "/" + dirName);
dir.mkdir();
}
+
+ @Test
+ void testMoveFolderRequiresAbsolutePath() {
+ assertThrows(IOException.class,
+ () -> notebookRepo.move("my_project", "/new_project",
AuthenticationInfo.ANONYMOUS));
+ assertThrows(IOException.class,
+ () -> notebookRepo.move("/my_project", "new_project",
AuthenticationInfo.ANONYMOUS));
+ }
+
+ @Test
+ void testRemoveFolderRequiresAbsolutePath(){
+ assertThrows(IOException.class,
+ () -> notebookRepo.remove("my_project", AuthenticationInfo.ANONYMOUS));
+ }
}
diff --git
a/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java
b/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java
index dd40f8af83..e5d6407b0f 100644
---
a/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java
+++
b/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java
@@ -837,20 +837,20 @@ class NotebookServiceTest {
notebookService.normalizeNotePath("my..note");
fail("Should fail");
} catch (IOException e) {
- assertEquals("Note name can not contain '..'", e.getMessage());
+ assertEquals("Path can not contain '..'", e.getMessage());
}
try {
notebookService.normalizeNotePath("%2e%2e/%2e%2e/tmp/test222");
fail("Should fail");
} catch (IOException e) {
- assertEquals("Note name can not contain '..'", e.getMessage());
+ assertEquals("Path can not contain '..'", e.getMessage());
}
try {
// Double URL encoding of ".."
notebookService.normalizeNotePath("%252e%252e/%252e%252e/tmp/test333");
fail("Should fail");
} catch (IOException e) {
- assertEquals("Note name can not contain '..'", e.getMessage());
+ assertEquals("Path can not contain '..'", e.getMessage());
}
try {
notebookService.normalizeNotePath("%25252525252e%25252525252e/tmp/test444");
@@ -865,4 +865,14 @@ class NotebookServiceTest {
assertEquals("Note name shouldn't end with '/'", e.getMessage());
}
}
+
+ @Test
+ void testNormalizeFolderPath() throws IOException {
+ assertEquals("/folder", notebookService.normalizeFolderPath("folder"));
+ assertEquals("/folder", notebookService.normalizeFolderPath("/folder"));
+ assertEquals("/folder/subfolder",
notebookService.normalizeFolderPath("folder/subfolder"));
+ assertEquals("/folder/subfolder",
notebookService.normalizeFolderPath("/folder/subfolder"));
+
+ assertThrows(IOException.class, () ->
notebookService.normalizeFolderPath(null));
+ }
}
