[MINOR] Reject traversal segments in note and folder paths#5227
Draft
jongyoul wants to merge 2 commits intoapache:masterfrom
Draft
[MINOR] Reject traversal segments in note and folder paths#5227jongyoul wants to merge 2 commits intoapache:masterfrom
jongyoul wants to merge 2 commits intoapache:masterfrom
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR for?
NotebookRepo.buildNoteFileNamecomposes 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/foowould 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
NotebookRepointerface level so every backend gets the same defence, fixes one folder-level path that bypassedbuildNoteFileName, and adds the missingnormalizeNotePathcall inNotebookService.renameNote.What type of PR is it?
Improvement
Todos
NotebookRepo.rejectTraversalSegments(URL-decoded, recursive, capped at 5 layers).rejectTraversalSegmentsfrom the defaultbuildNoteFileName, so every implementation is covered.FileSystemNotebookRepo's folder-levelmove/remove, which build the path without going throughbuildNoteFileName.normalizeNotePathinNotebookService.renameNoteto matchcreateNote,cloneNote, andmoveFolder.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?
All test sets pass locally.
Questions
IOExceptioninstead of silently composing to an out-of-root location.