fix: AppleScript injection hardening (supersedes #10)#11
Merged
Conversation
#5) - Bug 1 & 2: Add _applescript_escape() helper in actions.py that escapes backslashes, double-quotes, and strips newlines before embedding user input into AppleScript do script / keystroke calls - Bug 3: Gate --dangerously-skip-permissions behind JARVIS_SKIP_PERMISSIONS env var across actions.py, qa.py, work_mode.py, and server.py; document the new var in .env.example - Bug 4: Fix escape order in mail_access.py search_mail() and read_message() so backslashes are replaced before double-quotes, preventing \" → \\"
Builds on the AppleScript injection fix from PR #10. While reviewing, found additional unescaped interpolation sites that should also use the shared escape helper: - _focus_terminal_window: was using the same incomplete quote-only escape pattern that PR #10 fixed elsewhere in actions.py - _run_task: {work_dir} interpolated raw into do script - handle_build: {path} interpolated raw into do script - "open last project" fallback: {last["path"]} interpolated raw into a Finder open POSIX file call - api_fix_self: {jarvis_dir} interpolated raw (low risk since derived from __file__, but escape for consistency) Also: - Renamed actions._applescript_escape -> actions.applescript_escape (drop underscore) so server.py can import it as a shared helper instead of duplicating the implementation - Added tests/test_applescript_escape.py covering backslash, quote, newline/CR, ordering, and a representative injection payload Note on upgrade behavior: PR #10 flips the default for --dangerously-skip-permissions from "always on" to "off unless JARVIS_SKIP_PERMISSIONS=true". Existing users who upgrade without setting that env var will see Claude permission prompts on each subprocess (which may appear to hang in JARVIS's voice-driven flow). Set JARVIS_SKIP_PERMISSIONS=true to preserve the previous behavior. Co-Authored-By: Nandana Dileep <nandanadileep29@gmail.com>
4 tasks
3 tasks
pull Bot
pushed a commit
to RealShocky/jarvis
that referenced
this pull request
May 15, 2026
PR ethanplusai#11 (the AppleScript injection hardening) flipped the default for --dangerously-skip-permissions from "always on" to "off unless JARVIS_SKIP_PERMISSIONS=true is set." That was a defense-in-depth addition bundled with the injection fixes, but it's an awful default for a voice-driven assistant: Claude's interactive permission prompts have no UI surface in JARVIS, so subprocess calls silently hang waiting for input nobody can give. This commit keeps the env var as an opt-out toggle but flips its default back to "skip permissions = true", so JARVIS works out of the box. All the injection escapes from ethanplusai#11 are preserved — the actual security win (proper string escaping in AppleScript interpolation) is unaffected. Truth table: unset -> skip = True (default, works OOTB) JARVIS_SKIP_PERMISSIONS=true -> skip = True JARVIS_SKIP_PERMISSIONS=1 -> skip = True JARVIS_SKIP_PERMISSIONS=false-> skip = False (opt-in to prompts) JARVIS_SKIP_PERMISSIONS=0 -> skip = False JARVIS_SKIP_PERMISSIONS=no -> skip = False Updated .env.example comment to reflect the new default.
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.
Summary
Takes over and extends @nandanadileep's PR #10, which fixes the four AppleScript command-injection bugs reported in #5 by @consigcody94.
The original commit is preserved with @nandanadileep as author. A follow-up commit adds:
server.pythat the original PR didn't cover:_focus_terminal_window— was using the same incomplete quote-only escape pattern PR fix: resolve AppleScript command injection and escape order bugs (#5) #10 fixed elsewhere_run_task—{work_dir}was interpolated raw intodo scripthandle_build—{path}was interpolated raw intodo script{last["path"]}was interpolated rawapi_fix_self—{jarvis_dir}(low risk, derived from__file__, but escaped for consistency)actions._applescript_escape→actions.applescript_escape(dropped underscore) soserver.pycan import the same implementation instead of duplicating it.tests/test_applescript_escape.pycovers backslash, quote, newline/CR, escape ordering, and a representative injection payload (8 tests, all passing).PR #10 flipped the default for
--dangerously-skip-permissionsfrom always on to off unlessJARVIS_SKIP_PERMISSIONS=trueis set. Existing users who upgrade without setting that env var will see Claude permission prompts on each subprocess invocation — and in JARVIS's voice-driven flow those prompts can appear to silently hang.To preserve the previous unrestricted behavior:
```bash
echo "JARVIS_SKIP_PERMISSIONS=true" >> .env
```
This is the safer default for new installs but worth flagging in release notes.
Test plan
Closes #5. Supersedes #10.
🤖 Generated with Claude Code