refactor(sloppak): one containment guard for manifest-relative paths#1039
Merged
Conversation
Every manifest key that names a file carried its own copy of the same traversal guard: resolve, prove containment under source_dir, warn and skip on ValueError, warn and skip on OSError. Seven copies — original_audio, drum_tab, arrangement, notation, song_timeline, lyrics, keys — which is seven chances for the next side-file to get a security check subtly wrong by copying the wrong neighbour. Route them all through `_resolve_pack_path(source_dir, rel, label)`. Deliberately preserved, because each was load-bearing: - Both exception branches, with their different messages. ValueError means the path resolved outside the pack (a crafted or broken manifest); OSError means it could not be resolved at all (symlink loop, permissions). They send an operator to different places. - Per-call-site control flow. The helper returns `Path | None` and says nothing about what to do next, so the two sites that return, the one that continues, and the four that fall through to an `is not None` test each keep the shape they had. - The existence-check asymmetry. Some sites test `.exists()` (or `.is_file()`) after resolving and some do not, which is intentional — a missing optional side-file is silent, a missing arrangement skips an entry — so existence stays out of the helper entirely. Pure refactor: no behaviour change and no new validation. Log output is byte-identical (the hardcoded labels become a `%s` argument rendering to the same text). Full suite is unchanged at 2774 passed / 4 skipped before and after, and the five loader-level traversal tests that used to cover five separate copies of the guard now all exercise the same function. Signed-off-by: gionnibgud <gionnibgud@gmail.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds centralized safe resolution for sloppak manifest-relative paths and applies it across full-mix, drum-tab, arrangement, notation, timeline, lyrics, and keys loading. ChangesSloppak path safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
2 participants
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
Pure refactor of
lib/sloppak.py: seven copies of the manifest-path containment guard become one_resolve_pack_path(source_dir, rel, label)helper.Every manifest key that names a file (
original_audio,drum_tab, arrangementfile,notation,song_timeline,lyrics,keys) carried its own copy of the same block — resolve, prove containment undersource_dir, warn and skip onValueError, warn and skip onOSError. That's seven chances for the next side-file to get a security check subtly wrong by copying the wrong neighbour, and the guard is the thing standing between a crafted manifest and an arbitrary file read.No behaviour change, no new validation.
Deliberately preserved
Each of these looked like something to unify and isn't:
ValueErrormeans the path resolved outside the pack (crafted or broken manifest);OSErrormeans it couldn't be resolved at all (symlink loop, permissions). Reading "escapes source_dir" and reading "path resolution failed" send an operator to very different places.Path | Noneand says nothing about what to do next, so the two sites thatreturn, the one thatcontinues, and the four that fall through to anis not Nonetest each keep the shape they had..exists()(or.is_file()) after resolving and some don't — deliberate, since a missing optional side-file is silent while a missing arrangement skips an entry. Existence stays out of the helper entirely.Verification
%sargument that renders to the same text. Verified by triggering the guard for each label and diffing the rendered records. (Worth noting since the rawLogRecord.msg/argsdo change, which would matter to structured-log consumers; nothing in-tree reads them.)feedpak surface
Checklist
CHANGELOG.md[Unreleased]updated (user-visible changes) — n/a, internal refactor with no user-visible changegit commit -s)Summary by CodeRabbit