fix(security): patch AppleScript injection and path traversal; add parser tests#175
Merged
Conversation
…rser tests
Escape double-quotes in macos_integration.py before interpolating
message/title/subtitle into the osascript string literal — a raw `"`
in any value could break out and inject arbitrary AppleScript commands.
Guard all three filter file operations in ui.py (save/load/delete) with
`path.resolve().is_relative_to(filters_dir.resolve())` — the previous
`replace("/","_")` left `..` sequences intact, allowing names like
`../../etc/passwd` to escape the filters directory.
Add 92 tests for versiontracker/version/parser.py covering every public
and internal function; parser branch coverage is now 93.45%.
Raise the CI coverage gate from 50% to 80% to match actual project
coverage (86%) and prevent silent regressions.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideFixes two security issues (AppleScript injection and filter path traversal), expands test coverage for the version parser with a comprehensive new test module, and tightens CI coverage thresholds to match current project coverage. Sequence diagram for escaped AppleScript notificationssequenceDiagram
participant App
participant MacOSIntegration
participant osascript
App->>MacOSIntegration: send_notification(title, message, subtitle)
MacOSIntegration->>MacOSIntegration: message.replace
MacOSIntegration->>MacOSIntegration: title.replace
alt subtitle provided
MacOSIntegration->>MacOSIntegration: subtitle.replace
end
MacOSIntegration->>osascript: subprocess.run(cmd)
osascript-->>MacOSIntegration: result
MacOSIntegration-->>App: bool
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🔒 Security Analysis ReportSecurity Analysis ReportGenerated: Fri May 29 11:12:19 UTC 2026 Bandit Security ScanSafety Check ResultsPip-Audit Results |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b9d548d72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| # Escape double-quotes so user-controlled values cannot break out of | ||
| # the AppleScript string literal and inject arbitrary commands. | ||
| safe_message = message.replace('"', '\\"') |
There was a problem hiding this comment.
When a user-controlled value contains a backslash immediately before a quote, this replacement still builds an unsafe AppleScript string: for example a message containing \" sound name "Funk" -- becomes \\" at the first quote, so AppleScript consumes the doubled backslash as a literal backslash and the quote can terminate the string. That leaves the injection issue reachable for message values (and the same escaping pattern is used for title/subtitle), so escape backslashes before quotes or avoid interpolating these values into AppleScript source.
Useful? React with 👍 / 👎.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The path traversal guard logic in
save_filter,load_filter, anddelete_filteris duplicated; consider extracting a small helper that takes a filter name and returns a validatedPath, so future changes to the validation rules stay consistent in one place. - The AppleScript string escaping currently only replaces
"; it would be safer and more maintainable to centralize this into an_escape_applescript_stringhelper that also handles backslashes and control characters, ensuring all osascript calls use the same robust escaping.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The path traversal guard logic in `save_filter`, `load_filter`, and `delete_filter` is duplicated; consider extracting a small helper that takes a filter name and returns a validated `Path`, so future changes to the validation rules stay consistent in one place.
- The AppleScript string escaping currently only replaces `"`; it would be safer and more maintainable to centralize this into an `_escape_applescript_string` helper that also handles backslashes and control characters, ensuring all osascript calls use the same robust escaping.
## Individual Comments
### Comment 1
<location path="versiontracker/ui.py" line_range="503-505" />
<code_context>
safe_name = name.replace(" ", "-").replace("/", "_").lower()
filter_path = self.filters_dir / f"{safe_name}.json"
+ # Guard against path traversal (e.g. "../../etc/passwd" survives the
+ # replace above because ".." contains no "/" or " ").
+ if not filter_path.resolve().is_relative_to(self.filters_dir.resolve()):
+ raise ValueError(f"Invalid filter name: {name!r}")
+
</code_context>
<issue_to_address>
**suggestion:** The path traversal guard logic is repeated across save/load/delete and could be factored into a helper.
The `safe_name` and `resolve().is_relative_to(...)` logic is repeated in `save_filter`, `load_filter`, and `delete_filter`. Consider extracting it into a helper (e.g. `_filter_path_for(name)` that returns a validated `Path` or raises) to keep the logic centralized and easier to evolve.
Suggested implementation:
```python
filter_path = self._filter_path_for(name)
# Write the filter data to a JSON file
with open(filter_path, "w") as f:
json.dump(filter_data, f, indent=2)
```
To fully implement the refactor across the file, you should:
1. **Add the `_filter_path_for` helper** as an instance method on the same class that owns `self.filters_dir`, for example:
```python
def _filter_path_for(self, name: str) -> Path:
"""Return a validated filter path for the given filter name.
This centralizes the safe name generation and path traversal guard.
"""
safe_name = name.replace(" ", "-").replace("/", "_").lower()
filter_path = self.filters_dir / f"{safe_name}.json"
# Guard against path traversal (e.g. "../../etc/passwd" survives the
# replace above because ".." contains no "/" or " ").
if not filter_path.resolve().is_relative_to(self.filters_dir.resolve()):
raise ValueError(f"Invalid filter name: {name!r}")
return filter_path
```
Place this method near the other filter-related methods to keep things organized.
2. **Update other call sites** (e.g. in `load_filter` and `delete_filter`) that currently duplicate:
```python
safe_name = name.replace(" ", "-").replace("/", "_").lower()
filter_path = self.filters_dir / f"{safe_name}.json"
# and the associated resolve().is_relative_to(...) guard
```
Replace those blocks with:
```python
filter_path = self._filter_path_for(name)
```
This will ensure all filter path computation and validation logic is centralized in `_filter_path_for`, making future changes (e.g. different sanitization rules) straightforward.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+503
to
+505
| # Guard against path traversal (e.g. "../../etc/passwd" survives the | ||
| # replace above because ".." contains no "/" or " "). | ||
| if not filter_path.resolve().is_relative_to(self.filters_dir.resolve()): |
Contributor
There was a problem hiding this comment.
suggestion: The path traversal guard logic is repeated across save/load/delete and could be factored into a helper.
The safe_name and resolve().is_relative_to(...) logic is repeated in save_filter, load_filter, and delete_filter. Consider extracting it into a helper (e.g. _filter_path_for(name) that returns a validated Path or raises) to keep the logic centralized and easier to evolve.
Suggested implementation:
filter_path = self._filter_path_for(name)
# Write the filter data to a JSON file
with open(filter_path, "w") as f:
json.dump(filter_data, f, indent=2)To fully implement the refactor across the file, you should:
-
Add the
_filter_path_forhelper as an instance method on the same class that ownsself.filters_dir, for example:def _filter_path_for(self, name: str) -> Path: """Return a validated filter path for the given filter name. This centralizes the safe name generation and path traversal guard. """ safe_name = name.replace(" ", "-").replace("/", "_").lower() filter_path = self.filters_dir / f"{safe_name}.json" # Guard against path traversal (e.g. "../../etc/passwd" survives the # replace above because ".." contains no "/" or " "). if not filter_path.resolve().is_relative_to(self.filters_dir.resolve()): raise ValueError(f"Invalid filter name: {name!r}") return filter_path
Place this method near the other filter-related methods to keep things organized.
-
Update other call sites (e.g. in
load_filteranddelete_filter) that currently duplicate:safe_name = name.replace(" ", "-").replace("/", "_").lower() filter_path = self.filters_dir / f"{safe_name}.json" # and the associated resolve().is_relative_to(...) guard
Replace those blocks with:
filter_path = self._filter_path_for(name)
This will ensure all filter path computation and validation logic is centralized in _filter_path_for, making future changes (e.g. different sanitization rules) straightforward.
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
1 participant
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.
Summary
macos_integration.py):message,title, andsubtitleare now escaped ("→\") before interpolation into the osascript string literal — a raw"in any value could previously break out of the string and inject arbitrary AppleScript commandsui.py): all three filter file operations (save_filter,load_filter,delete_filter) now assertpath.resolve().is_relative_to(filters_dir.resolve())— the previousreplace("/","_")left..sequences intact, allowing names like../../etc/passwdto escape the filters directoryversiontracker/version/parser.pycovering every public and internal function; parser branch coverage is now 93.45%coverage.ymlto match actual project coverage (86%) and prevent silent regressionsTest plan
pytest tests/test_version_parser.py— 92 passedpytest) — 2477 passed, 16 skipped🤖 Generated with Claude Code
Summary by Sourcery
Patch security vulnerabilities in macOS notifications and filter file handling, expand version parser test coverage, and tighten CI coverage enforcement.
Bug Fixes:
Enhancements:
CI: