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 f05a0183c87..9e201a4ded2 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 void move(String noteId, @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 void remove(String noteId, String notePath, AuthenticationInfo subject) @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 00000000000..40f754076aa --- /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 b55067c7848..021a6b860d7 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 @@ default String buildNoteFileName(String noteId, String notePath) throws IOExcept 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 fbc1507f79f..554b85f4de2 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 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.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 @@ String normalizeNotePath(String notePath) throws IOException { 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 void renameNote(String noteId, } } try { + newNotePathReal = normalizeNotePath(newNotePathReal); notebook.moveNote(noteId, newNotePathReal, context.getAutheInfo()); callback.onSuccess(readNote, context); } catch (NotePathAlreadyExistsException e) { @@ -1567,20 +1567,4 @@ private boolean checkPermission(String noteId, } } - 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 00000000000..58ead149dfd --- /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")); + } +}