Skip to content

Commit 7305c7d

Browse files
hbmartinclaude
hbmartin
and
claude
committed
Fix terminal shell injection and user-facing action bugs
Security: - PBTerminalUtil built the Terminal/iTerm doScript command by interpolating the repository directory path into a double-quoted shell string. Double quotes do not neutralize $(...) or backticks, so a repository whose path contained a command substitution executed arbitrary shell when opened in Terminal. Wrap the path in single quotes (fully literal) via a shellQuote helper. Added a regression test. UI: - PBGitWindowController revealInFinder: honored none of the file paths attached to the menu item and always revealed the repository root; resolve the selected files the same way openInTerminal: does, falling back to the root only when no files are selected. - PBGitXMessageSheet discarded the recovery suggestion because the stringByAppendingString: results were thrown away; assign them back. - Terminal preference text fields (initial command, custom executable/arguments) committed only on Return and discarded the edit on focus loss; set sendsActionOnEndEditing. Co-Authored-By: Claude Opus 4.8 <[email protected]>
1 parent b8fdff3 commit 7305c7d

6 files changed

Lines changed: 42 additions & 5 deletions

File tree

‎Classes/Controllers/ApplicationPreferencesUI.swift‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -444,20 +444,25 @@ final class SettingsViewFactory: NSObject { // swiftlint:disable:this unused_dec
444444
popup.action = #selector(SettingsPaneView.terminalChanged(_:))
445445
view.addRow("Default application:", control: popup, help: "GitX asks on first use if no default has been chosen.")
446446

447+
// sendsActionOnEndEditing so typing a value and clicking away (rather than pressing Return) still
448+
// commits the edit instead of silently discarding it.
447449
let command = NSTextField(string: ApplicationSettings.terminalInitialCommand)
448450
command.target = view
449451
command.action = #selector(SettingsPaneView.terminalCommandChanged(_:))
452+
command.cell?.sendsActionOnEndEditing = true
450453
command.widthAnchor.constraint(equalToConstant: 360).isActive = true
451454
view.addRow("Initial command:", control: command, help: "Leave empty to open only the repository directory.")
452455

453456
let executable = NSTextField(string: ApplicationSettings.customTerminalExecutable)
454457
executable.target = view
455458
executable.action = #selector(SettingsPaneView.customExecutableChanged(_:))
459+
executable.cell?.sendsActionOnEndEditing = true
456460
executable.widthAnchor.constraint(equalToConstant: 360).isActive = true
457461
view.addRow("Custom executable:", control: executable)
458462
let arguments = NSTextField(string: ApplicationSettings.customTerminalArguments)
459463
arguments.target = view
460464
arguments.action = #selector(SettingsPaneView.customArgumentsChanged(_:))
465+
arguments.cell?.sendsActionOnEndEditing = true
461466
arguments.widthAnchor.constraint(equalToConstant: 360).isActive = true
462467
view.addRow("Custom arguments:", control: arguments, help: "Use {directory} and optionally {command}. Arguments are launched directly, never through a shell.")
463468
return view

‎Classes/Controllers/PBGitWindowController.m‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -490,7 +490,15 @@ - (IBAction)openFiles:(id)sender
490490
}
491491
- (IBAction)revealInFinder:(id)sender
492492
{
493-
[self revealURLsInFinder:@[ self.repository.workingDirectoryURL ]];
493+
// Honor the file paths attached to a menu item (e.g. the history file tree's context menu) the same way
494+
// openInTerminal: does; fall back to the repository root when the sender carries no selection (e.g. a
495+
// toolbar/menu action with no represented files).
496+
NSArray<NSURL *> *urls = nil;
497+
if ([sender respondsToSelector:@selector(representedObject)])
498+
urls = [self selectedURLsFromSender:sender];
499+
if (urls.count == 0)
500+
urls = @[ self.repository.workingDirectoryURL ];
501+
[self revealURLsInFinder:urls];
494502
}
495503
- (IBAction)openInTerminal:(id)sender
496504
{

‎Classes/Util/PBTerminalUtil.m‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,19 @@ - (nullable id)eventDidFail:(const AppleEvent *)event withError:(NSError *)error
5757
return nil;
5858
}
5959

60+
// Wraps a string in single quotes for safe shell interpolation. Single-quoting is fully literal, so it
61+
// neutralizes command substitution ($(...)/backticks), variable expansion, and every other metacharacter —
62+
// unlike double quotes, which still evaluate $(...) and backticks. A repository whose path contains e.g.
63+
// "$(...)" must not be able to execute arbitrary commands when opened in Terminal/iTerm.
64+
+ (NSString *)shellQuote:(NSString *)string
65+
{
66+
NSString *escaped = [string stringByReplacingOccurrencesOfString:@"'" withString:@"'\\''"];
67+
return [NSString stringWithFormat:@"'%@'", escaped];
68+
}
69+
6070
- (BOOL)runTerminalCommand:(NSString *)command inDirectory:(NSURL *)directory
6171
{
62-
NSString *fullCommand = [NSString stringWithFormat:@"cd \"%@\"; clear; echo '# Opened by GitX'; %@", directory.path, command];
72+
NSString *fullCommand = [NSString stringWithFormat:@"cd %@; clear; echo '# Opened by GitX'; %@", [PBTerminalUtil shellQuote:directory.path], command];
6373

6474
TerminalApplication *term = [SBApplication applicationWithBundleIdentifier:@"com.apple.Terminal"];
6575
if (!term)
@@ -77,7 +87,7 @@ - (BOOL)runTerminalCommand:(NSString *)command inDirectory:(NSURL *)directory
7787

7888
- (BOOL)runiTerm2Command:(NSString *)command inDirectory:(NSURL *)directory
7989
{
80-
NSString *fullCommand = [NSString stringWithFormat:@"cd \"%@\"; clear; echo '# Opened by GitX'; %@", directory.path, command];
90+
NSString *fullCommand = [NSString stringWithFormat:@"cd %@; clear; echo '# Opened by GitX'; %@", [PBTerminalUtil shellQuote:directory.path], command];
8191

8292
iTerm2Application *term = [SBApplication applicationWithBundleIdentifier:@"com.googlecode.iterm2"];
8393
if (!term)

‎Classes/Views/PBGitXMessageSheet.m‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,8 @@ + (void)beginSheetWithError:(NSError *)error
7272

7373
if (error.localizedRecoverySuggestion) {
7474
NSString *message = NSLocalizedString(@"Maybe you could try the following:", @"PBGitXMessageSheet - localized recovery suggestion header");
75-
[message stringByAppendingString:@"\n"];
76-
[message stringByAppendingString:error.localizedRecoverySuggestion];
75+
message = [message stringByAppendingString:@"\n"];
76+
message = [message stringByAppendingString:error.localizedRecoverySuggestion];
7777
[messageParts addObject:message];
7878
}
7979

‎GitXTests/GitXSwiftFeatureTests.swift‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,16 @@ final class GitXSwiftFeatureTests: XCTestCase {
139139
XCTAssertEqual(PBRecentRepositoryKeyNavigation.nextRow(fromRow: -1, rowCount: 5, movingDown: true), 1)
140140
}
141141

142+
func testTerminalShellQuoteNeutralizesCommandSubstitution() {
143+
// A repository path containing $(...) or backticks must be fully literal inside single quotes so it
144+
// cannot execute arbitrary shell when the repo is opened in Terminal/iTerm.
145+
XCTAssertEqual(PBTerminalUtil.shellQuote("/repo/normal"), "'/repo/normal'")
146+
XCTAssertEqual(PBTerminalUtil.shellQuote("/repo/$(whoami)"), "'/repo/$(whoami)'")
147+
XCTAssertEqual(PBTerminalUtil.shellQuote("/repo/`id`"), "'/repo/`id`'")
148+
// An embedded single quote is escaped so it cannot terminate the quoting and break out.
149+
XCTAssertEqual(PBTerminalUtil.shellQuote("/repo/it's mine"), "'/repo/it'\\''s mine'")
150+
}
151+
142152
func testRecentRepositoryKeyNavigationReportsNoSelectableRowWhenEmpty() {
143153
XCTAssertEqual(PBRecentRepositoryKeyNavigation.nextRow(fromRow: 0, rowCount: 0, movingDown: true), -1)
144154
XCTAssertEqual(PBRecentRepositoryKeyNavigation.nextRow(fromRow: 0, rowCount: 0, movingDown: false), -1)

‎GitXTests/GitXTests-Bridging-Header.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,10 @@ typedef NS_ENUM(NSInteger, PBRecentRepositoryActivationAction) {
312312
NS_SWIFT_NAME(nextRow(fromRow:rowCount:movingDown:));
313313
@end
314314

315+
@interface PBTerminalUtil : NSObject
316+
+ (NSString *)shellQuote:(NSString *)string;
317+
@end
318+
315319
@interface PBRewindOverlayView : NSView
316320
- (instancetype)initWithFrame:(NSRect)frameRect;
317321
@end

0 commit comments

Comments
 (0)