This is an automated email from the ASF dual-hosted git repository.
jongyoul 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 c30052a907 [MINOR] Reject traversal segments in note and folder paths
c30052a907 is described below
commit c30052a90793437988252fa7c6c2b128661a4dc7
Author: Jongyoul Lee <[email protected]>
AuthorDate: Thu May 14 13:55:54 2026 +0900
[MINOR] Reject traversal segments in note and folder paths
## 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
* [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.
## 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
Closes #5227 from jongyoul/ZEPPELIN-fs-notebook-path.
Signed-off-by: Jongyoul Lee <[email protected]>
---
.../notebook/repo/FileSystemNotebookRepo.java | 3 +
.../notebook/repo/NotebookPathValidator.java | 87 ++++++++++++++++
.../zeppelin/notebook/repo/NotebookRepo.java | 1 +
.../apache/zeppelin/service/NotebookService.java | 22 +---
.../repo/NotebookRepoPathValidationTest.java | 113 +++++++++++++++++++++
5 files changed, 207 insertions(+), 19 deletions(-)
diff --git
a/zeppelin-plugins/notebookrepo/filesystem/src/main/java/org/apache/zeppelin/notebook/repo/FileSystemNotebookRepo.java
b/zeppelin-plugins/notebookrepo/filesystem/src/main/java/org/apache/zeppelin/notebook/repo/FileSystemNotebookRepo.java
index f05a0183c8..9e201a4ded 100644
---
a/zeppelin-plugins/notebookrepo/filesystem/src/main/java/org/apache/zeppelin/notebook/repo/FileSystemNotebookRepo.java
+++
b/zeppelin-plugins/notebookrepo/filesystem/src/main/java/org/apache/zeppelin/notebook/repo/FileSystemNotebookRepo.java
@@ -103,6 +103,8 @@ public class FileSystemNotebookRepo extends
AbstractNotebookRepo {
@Override
public void move(String folderPath, String newFolderPath, AuthenticationInfo
subject)
throws IOException {
+ NotebookPathValidator.rejectTraversalSegments(folderPath);
+ NotebookPathValidator.rejectTraversalSegments(newFolderPath);
// [ZEPPELIN-4195] newFolderPath parent path maybe not exist
this.fs.tryMkDir(new Path(notebookDir,
folderPath.substring(1)).getParent());
this.fs.move(new Path(notebookDir, folderPath.substring(1)),
@@ -119,6 +121,7 @@ public class FileSystemNotebookRepo extends
AbstractNotebookRepo {
@Override
public void remove(String folderPath, AuthenticationInfo subject) throws
IOException {
+ NotebookPathValidator.rejectTraversalSegments(folderPath);
if (!this.fs.delete(new Path(notebookDir, folderPath.substring(1)))) {
LOGGER.warn("Fail to remove folder: {}", folderPath);
}
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
new file mode 100644
index 0000000000..40f754076a
--- /dev/null
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookPathValidator.java
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zeppelin.notebook.repo;
+
+import java.io.IOException;
+import java.net.URLDecoder;
+import java.nio.charset.StandardCharsets;
+import java.util.regex.Pattern;
+
+/**
+ * Note-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
+ * checks and bypassing them.
+ */
+public final class NotebookPathValidator {
+
+ private static final Pattern PATH_SEGMENT_SPLIT = Pattern.compile("/+");
+ private static final String PARENT_SEGMENT = "..";
+ private static final String CURRENT_SEGMENT = ".";
+ private static final int MAX_DECODE_LAYERS = 5;
+
+ private NotebookPathValidator() {
+ }
+
+ /**
+ * Refuses any {@code ..} or {@code .} segment in {@code notePath}. The
+ * input is URL-decoded repeatedly first so that variants such as
+ * {@code %2e%2e} or {@code %252e%252e} cannot bypass the check.
+ *
+ * @throws IOException if the path is null, contains a traversal segment,
+ * or has more URL-encoding layers than {@value #MAX_DECODE_LAYERS}
+ */
+ public static void rejectTraversalSegments(String notePath) throws
IOException {
+ if (notePath == null) {
+ throw new IOException("Path must not be null");
+ }
+ String decoded = decodeRepeatedly(notePath);
+ String stripped = decoded.startsWith("/") ? decoded.substring(1) : decoded;
+ for (String segment : PATH_SEGMENT_SPLIT.split(stripped)) {
+ if (PARENT_SEGMENT.equals(segment) || CURRENT_SEGMENT.equals(segment)) {
+ throw new IOException("Path traversal segments are not allowed: " +
notePath);
+ }
+ }
+ }
+
+ /**
+ * Repeatedly URL-decodes {@code encoded} until it stabilises, capped at
+ * {@value #MAX_DECODE_LAYERS} layers. Detecting stability after {@code N}
+ * layers of encoding requires {@code N + 1} decode passes — the final pass
+ * confirms the result is unchanged — so the loop runs one extra iteration.
+ *
+ * @throws IOException if the input is malformed or has more URL-encoding
+ * layers than {@value #MAX_DECODE_LAYERS}
+ */
+ public static String decodeRepeatedly(String encoded) throws IOException {
+ String previous = encoded;
+ for (int pass = 0; pass <= MAX_DECODE_LAYERS; pass++) {
+ String decoded;
+ try {
+ decoded = URLDecoder.decode(previous, StandardCharsets.UTF_8);
+ } catch (IllegalArgumentException e) {
+ throw new IOException("Malformed URL-encoded path: " + encoded, e);
+ }
+ if (decoded.equals(previous)) {
+ return decoded;
+ }
+ previous = decoded;
+ }
+ throw new IOException("Exceeded maximum decode attempts. Possible
malicious input.");
+ }
+}
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepo.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepo.java
index b55067c784..021a6b860d 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepo.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepo.java
@@ -146,6 +146,7 @@ public interface NotebookRepo extends Closeable {
if (!notePath.startsWith("/")) {
throw new IOException("Invalid notePath: " + notePath);
}
+ NotebookPathValidator.rejectTraversalSegments(notePath);
return (notePath + "_" + noteId + ".zpln").substring(1);
}
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 fbc1507f79..554b85f4de 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
@@ -25,8 +25,6 @@ import static org.apache.zeppelin.scheduler.Job.Status.ABORT;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
-import java.net.URLDecoder;
-import java.nio.charset.StandardCharsets;
import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.time.Instant;
@@ -57,6 +55,7 @@ import org.apache.zeppelin.notebook.Notebook.NoteProcessor;
import org.apache.zeppelin.notebook.AuthorizationService;
import org.apache.zeppelin.notebook.exception.CorruptedNoteException;
import org.apache.zeppelin.notebook.exception.NotePathAlreadyExistsException;
+import org.apache.zeppelin.notebook.repo.NotebookPathValidator;
import org.apache.zeppelin.notebook.repo.NotebookRepoWithVersionControl;
import org.apache.zeppelin.notebook.scheduler.SchedulerService;
import org.apache.zeppelin.common.Message;
@@ -240,7 +239,7 @@ public class NotebookService {
notePath = notePath.replace("\r", " ").replace("\n", " ");
- notePath = decodeRepeatedly(notePath);
+ notePath = NotebookPathValidator.decodeRepeatedly(notePath);
if (notePath.endsWith("/")) {
throw new IOException("Note name shouldn't end with '/'");
}
@@ -315,6 +314,7 @@ public class NotebookService {
}
}
try {
+ newNotePathReal = normalizeNotePath(newNotePathReal);
notebook.moveNote(noteId, newNotePathReal, context.getAutheInfo());
callback.onSuccess(readNote, context);
} catch (NotePathAlreadyExistsException e) {
@@ -1567,20 +1567,4 @@ public class NotebookService {
}
}
- private static String decodeRepeatedly(final String encoded) throws
IOException {
- String previous = encoded;
- int maxDecodeAttempts = 5;
- int attempts = 0;
-
- while (attempts < maxDecodeAttempts) {
- String decoded = URLDecoder.decode(previous, StandardCharsets.UTF_8);
- attempts++;
- if (decoded.equals(previous)) {
- return decoded;
- }
- previous = decoded;
- }
-
- throw new IOException("Exceeded maximum decode attempts. Possible
malicious input.");
- }
}
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
new file mode 100644
index 0000000000..58ead149df
--- /dev/null
+++
b/zeppelin-server/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoPathValidationTest.java
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zeppelin.notebook.repo;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.io.IOException;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+class NotebookRepoPathValidationTest {
+
+ @ParameterizedTest
+ @ValueSource(strings = {
+ "/../etc/passwd",
+ "/foo/../../etc/passwd",
+ "/foo/../bar",
+ "/foo/./bar",
+ "/./bar",
+ "/foo/..",
+ "/..",
+ "/.",
+ // URL-encoded variants must be rejected after decoding.
+ "/%2e%2e/etc/passwd",
+ "/foo/%2E%2E/bar",
+ "/%2e/foo",
+ // Double-encoded.
+ "/%252e%252e/etc/passwd",
+ })
+ void rejectTraversalSegments_rejects_traversal(String malicious) {
+ assertThrows(IOException.class,
+ () -> NotebookPathValidator.rejectTraversalSegments(malicious),
+ "expected rejection for: " + malicious);
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {
+ "/MyNote",
+ "/folder/MyNote",
+ "/folder/sub-folder/My Note With Spaces",
+ "/한글노트",
+ "/foo.bar.baz",
+ // ".." and "." are rejected only as exact segments — names containing
+ // dots remain valid.
+ "/...",
+ "/foo/...",
+ "/foo..bar",
+ })
+ void rejectTraversalSegments_accepts_normal_paths(String safe) throws
IOException {
+ NotebookPathValidator.rejectTraversalSegments(safe);
+ }
+
+ @Test
+ void rejectTraversalSegments_rejects_null() {
+ assertThrows(IOException.class, () ->
NotebookPathValidator.rejectTraversalSegments(null));
+ }
+
+ @Test
+ void rejectTraversalSegments_rejects_excessive_encoding_layers() {
+ // Six layers of "..", past the 5-layer decode cap.
+ String payload = "/%252525252e%252525252e/etc";
+ assertThrows(IOException.class, () ->
NotebookPathValidator.rejectTraversalSegments(payload));
+ }
+
+ @Test
+ void decodeRepeatedly_returns_input_when_already_decoded() throws
IOException {
+ assertEquals("/foo bar", NotebookPathValidator.decodeRepeatedly("/foo
bar"));
+ }
+
+ @Test
+ void decodeRepeatedly_decodes_until_stable() throws IOException {
+ assertEquals("/..", NotebookPathValidator.decodeRepeatedly("/%252e%252e"));
+ }
+
+ @Test
+ void decodeRepeatedly_throws_on_too_many_layers() {
+ String payload = "/%2525252525252525252e";
+ assertThrows(IOException.class, () ->
NotebookPathValidator.decodeRepeatedly(payload));
+ }
+
+ @Test
+ void decodeRepeatedly_wraps_malformed_percent_encoding_as_io_exception() {
+ // Trailing "%" without two hex digits makes URLDecoder.decode raise
+ // IllegalArgumentException; the validator must convert it to IOException
+ // so callers get a single, declared failure mode.
+ assertThrows(IOException.class, () ->
NotebookPathValidator.decodeRepeatedly("/foo%"));
+ assertThrows(IOException.class, () ->
NotebookPathValidator.decodeRepeatedly("/foo%ZZ"));
+ }
+
+ @Test
+ void decodeRepeatedly_accepts_max_decode_layers() throws IOException {
+ // Exactly five layers of "%2e" wrapping ("%252525252e") must decode
+ // cleanly; the constant means *layers*, not raw loop iterations.
+ assertEquals("/..",
NotebookPathValidator.decodeRepeatedly("/%252525252e%252525252e"));
+ }
+}
