Skip to content

[MINOR] Reject traversal segments in note and folder paths#5227

Draft
jongyoul wants to merge 2 commits intoapache:masterfrom
jongyoul:ZEPPELIN-fs-notebook-path
Draft

[MINOR] Reject traversal segments in note and folder paths#5227
jongyoul wants to merge 2 commits intoapache:masterfrom
jongyoul:ZEPPELIN-fs-notebook-path

Conversation

@jongyoul
Copy link
Copy Markdown
Member

@jongyoul jongyoul commented May 6, 2026

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

jongyoul and others added 2 commits May 6, 2026 20:57
NotebookRepo.buildNoteFileName composes a filesystem path or object-store
key from a user-supplied note path. The previous implementation only
required a leading "/" but otherwise concatenated the value verbatim, so
paths 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).

Changes:

- NotebookRepo: add a default rejectTraversalSegments helper that
  URL-decodes the path repeatedly (capped at five layers) and refuses
  any "." or ".." segment. buildNoteFileName now calls it, so every
  NotebookRepo implementation is hardened in one place.
- FileSystemNotebookRepo: the folder-level move and remove APIs build
  the path directly from the input rather than via buildNoteFileName,
  so apply rejectTraversalSegments to those inputs as well.
- NotebookService.renameNote: normalize the rebuilt note path through
  normalizeNotePath, matching what other rename / move entry points
  already do (createNote, cloneNote, moveFolder).

Tests:

- NotebookRepoPathValidationTest (27): rejects "..", ".", URL-encoded
  variants such as "%2e%2e" and double-encoded variants, and the
  excessive-encoding-layer case. Accepts realistic note names including
  Korean characters and trailing dots inside a segment ("..." stays
  valid, only exact "..", "." segments are rejected).
- Existing FileSystemNotebookRepoTest (2) and NotebookServiceTest (5)
  continue to pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address reviewer feedback (simplify pass) on the previous commit:

- Move rejectTraversalSegments and decodeRepeatedly out of NotebookRepo
  default methods into a new final NotebookPathValidator class. Default
  methods on a public interface can be overridden, which would let an
  implementation accidentally — or intentionally — bypass the check; a
  static helper on a final class makes that impossible.
- Drop the duplicate decodeRepeatedly that lived as a private static in
  NotebookService and route normalizeNotePath through the same helper.
- Cache the segment-split Pattern as a static final field so the regex
  is compiled once instead of on every save / get / move call.
- Drop the InMemoryStub boilerplate from the test and exercise the
  utility class directly; the stub existed only to instantiate an
  interface that no longer carries the helpers.
- Keep the existing "Exceeded maximum decode attempts" error message so
  existing assertion tests in NotebookServiceTest continue to pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant