Skip to content

Commit 1a21e15

Browse files
amoscickiclaude
amoscicki
and
claude
committed
fix: address PR review comments for auto-update feature
Addresses the following review comments: Critical (security): - #2649468854: Add URL validation in check.ts - #2649468856: Add URL validation in pull.ts - #2649468858: Strengthen isValidGitUrl to block shell metacharacters Medium (robustness): - #2649468862: Use random temp remote name in check.ts - #2649468863: Refactor check.ts to use try...finally for cleanup - #2649468864: Use DEFAULT_AUTO_UPDATE_SETTINGS in info.ts - #2649468865: Use random temp remote name in pull.ts - #2649468866: Refactor pull.ts to use try...finally for cleanup - #2649468868: Extract getRepoDisplayName to shared utility - #2649468870: Fix docs default interval (5->15 minutes) Additional fixes from CodeRabbit: - Stabilize onCheck callback in use-update-polling.ts - Guard process.env.HOME in common.ts - Fix markdown lint issues in docs - Update "Later" button behavior comment - Remove unused currentBranch variable in pull.ts Skipped (acceptable as-is): - #2649468861: Fragile project root resolution 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
1 parent e9b1b19 commit 1a21e15

File tree

9 files changed

+91
-96
lines changed

9 files changed

+91
-96
lines changed

‎apps/server/src/routes/updates/common.ts‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,12 @@ if (process.platform === 'win32') {
3939
additionalPaths.push(
4040
'/opt/homebrew/bin', // Homebrew on Apple Silicon
4141
'/usr/local/bin', // Homebrew on Intel Mac, common Linux location
42-
'/home/linuxbrew/.linuxbrew/bin', // Linuxbrew
43-
`${process.env.HOME}/.local/bin` // pipx, other user installs
42+
'/home/linuxbrew/.linuxbrew/bin' // Linuxbrew
4443
);
44+
// pipx, other user installs - only add if HOME is defined
45+
if (process.env.HOME) {
46+
additionalPaths.push(`${process.env.HOME}/.local/bin`);
47+
}
4548
}
4649

4750
const extendedPath = [process.env.PATH, ...additionalPaths.filter(Boolean)]
@@ -124,14 +127,19 @@ export async function hasLocalChanges(repoPath: string): Promise<boolean> {
124127
}
125128

126129
/**
127-
* Validate that a URL looks like a valid git remote URL
130+
* Validate that a URL looks like a valid git remote URL.
131+
* Also blocks shell metacharacters to prevent command injection.
128132
*/
129133
export function isValidGitUrl(url: string): boolean {
130-
// Allow HTTPS and SSH git URLs
131-
return (
134+
// Allow HTTPS, SSH, and git protocols
135+
const startsWithValidProtocol =
132136
url.startsWith('https://') ||
133137
url.startsWith('git@') ||
134138
url.startsWith('git://') ||
135-
url.startsWith('ssh://')
136-
);
139+
url.startsWith('ssh://');
140+
141+
// Block shell metacharacters to prevent command injection
142+
const hasShellChars = /[;`|&<>()$!\\[\] ]/.test(url);
143+
144+
return startsWithValidProtocol && !hasShellChars;
137145
}

‎apps/server/src/routes/updates/routes/check.ts‎

Lines changed: 23 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import type { Request, Response } from 'express';
88
import type { SettingsService } from '../../../services/settings-service.js';
99
import type { UpdateCheckResult } from '@automaker/types';
10+
import crypto from 'crypto';
1011
import {
1112
execAsync,
1213
execEnv,
@@ -15,6 +16,7 @@ import {
1516
getShortCommit,
1617
isGitRepo,
1718
isGitAvailable,
19+
isValidGitUrl,
1820
getErrorMessage,
1921
logError,
2022
} from '../common.js';
@@ -47,24 +49,23 @@ export function createCheckHandler(settingsService: SettingsService) {
4749
const sourceUrl =
4850
settings.autoUpdate?.upstreamUrl || 'https://github.com/AutoMaker-Org/automaker.git';
4951

52+
// Validate URL to prevent command injection
53+
if (!isValidGitUrl(sourceUrl)) {
54+
res.status(400).json({
55+
success: false,
56+
error: 'Invalid upstream URL format',
57+
});
58+
return;
59+
}
60+
5061
// Get local version
5162
const localVersion = await getCurrentCommit(installPath);
5263
const localVersionShort = await getShortCommit(installPath);
5364

54-
// Fetch from upstream (use a temporary remote name to avoid conflicts)
55-
const tempRemoteName = 'automaker-update-check';
65+
// Use a random remote name to avoid conflicts with concurrent checks
66+
const tempRemoteName = `automaker-update-check-${crypto.randomBytes(8).toString('hex')}`;
5667

5768
try {
58-
// Remove temp remote if it exists (ignore errors)
59-
try {
60-
await execAsync(`git remote remove ${tempRemoteName}`, {
61-
cwd: installPath,
62-
env: execEnv,
63-
});
64-
} catch {
65-
// Remote doesn't exist, that's fine
66-
}
67-
6869
// Add temporary remote
6970
await execAsync(`git remote add ${tempRemoteName} "${sourceUrl}"`, {
7071
cwd: installPath,
@@ -91,12 +92,6 @@ export function createCheckHandler(settingsService: SettingsService) {
9192
);
9293
const remoteVersionShort = remoteVersionShortOutput.trim();
9394

94-
// Clean up temp remote
95-
await execAsync(`git remote remove ${tempRemoteName}`, {
96-
cwd: installPath,
97-
env: execEnv,
98-
});
99-
10095
// Check if remote is ahead of local (update available)
10196
// git merge-base --is-ancestor <commit1> <commit2> returns 0 if commit1 is ancestor of commit2
10297
let updateAvailable = false;
@@ -132,16 +127,6 @@ export function createCheckHandler(settingsService: SettingsService) {
132127
result,
133128
});
134129
} catch (fetchError) {
135-
// Clean up temp remote on error
136-
try {
137-
await execAsync(`git remote remove ${tempRemoteName}`, {
138-
cwd: installPath,
139-
env: execEnv,
140-
});
141-
} catch {
142-
// Ignore cleanup errors
143-
}
144-
145130
const errorMsg = getErrorMessage(fetchError);
146131
logError(fetchError, 'Failed to fetch from upstream');
147132

@@ -158,6 +143,16 @@ export function createCheckHandler(settingsService: SettingsService) {
158143
error: `Could not fetch from upstream: ${errorMsg}`,
159144
} satisfies UpdateCheckResult,
160145
});
146+
} finally {
147+
// Always clean up temp remote
148+
try {
149+
await execAsync(`git remote remove ${tempRemoteName}`, {
150+
cwd: installPath,
151+
env: execEnv,
152+
});
153+
} catch {
154+
// Ignore cleanup errors
155+
}
161156
}
162157
} catch (error) {
163158
logError(error, 'Update check failed');

‎apps/server/src/routes/updates/routes/info.ts‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
import type { Request, Response } from 'express';
88
import type { SettingsService } from '../../../services/settings-service.js';
9-
import type { UpdateInfo } from '@automaker/types';
9+
import { DEFAULT_AUTO_UPDATE_SETTINGS, type UpdateInfo } from '@automaker/types';
1010
import {
1111
execAsync,
1212
execEnv,
@@ -27,11 +27,7 @@ export function createInfoHandler(settingsService: SettingsService) {
2727

2828
// Get settings
2929
const settings = await settingsService.getGlobalSettings();
30-
const autoUpdateSettings = settings.autoUpdate || {
31-
enabled: true,
32-
checkIntervalMinutes: 15,
33-
upstreamUrl: 'https://github.com/AutoMaker-Org/automaker.git',
34-
};
30+
const autoUpdateSettings = settings.autoUpdate || DEFAULT_AUTO_UPDATE_SETTINGS;
3531

3632
// Check if git is available
3733
const gitAvailable = await isGitAvailable();

‎apps/server/src/routes/updates/routes/pull.ts‎

Lines changed: 23 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import type { Request, Response } from 'express';
88
import type { SettingsService } from '../../../services/settings-service.js';
99
import type { UpdatePullResult } from '@automaker/types';
10+
import crypto from 'crypto';
1011
import {
1112
execAsync,
1213
execEnv,
@@ -15,6 +16,7 @@ import {
1516
getShortCommit,
1617
isGitRepo,
1718
isGitAvailable,
19+
isValidGitUrl,
1820
hasLocalChanges,
1921
getErrorMessage,
2022
logError,
@@ -57,24 +59,23 @@ export function createPullHandler(settingsService: SettingsService) {
5759
const sourceUrl =
5860
settings.autoUpdate?.upstreamUrl || 'https://github.com/AutoMaker-Org/automaker.git';
5961

62+
// Validate URL to prevent command injection
63+
if (!isValidGitUrl(sourceUrl)) {
64+
res.status(400).json({
65+
success: false,
66+
error: 'Invalid upstream URL format',
67+
});
68+
return;
69+
}
70+
6071
// Get current version before pull
6172
const previousVersion = await getCurrentCommit(installPath);
6273
const previousVersionShort = await getShortCommit(installPath);
6374

64-
// Use a temporary remote to pull from
65-
const tempRemoteName = 'automaker-update-pull';
75+
// Use a random remote name to avoid conflicts with concurrent pulls
76+
const tempRemoteName = `automaker-update-pull-${crypto.randomBytes(8).toString('hex')}`;
6677

6778
try {
68-
// Remove temp remote if it exists (ignore errors)
69-
try {
70-
await execAsync(`git remote remove ${tempRemoteName}`, {
71-
cwd: installPath,
72-
env: execEnv,
73-
});
74-
} catch {
75-
// Remote doesn't exist, that's fine
76-
}
77-
7879
// Add temporary remote
7980
await execAsync(`git remote add ${tempRemoteName} "${sourceUrl}"`, {
8081
cwd: installPath,
@@ -87,25 +88,12 @@ export function createPullHandler(settingsService: SettingsService) {
8788
env: execEnv,
8889
});
8990

90-
// Get current branch
91-
const { stdout: branchOutput } = await execAsync('git rev-parse --abbrev-ref HEAD', {
92-
cwd: installPath,
93-
env: execEnv,
94-
});
95-
const currentBranch = branchOutput.trim();
96-
9791
// Merge the fetched changes
9892
const { stdout: mergeOutput } = await execAsync(
9993
`git merge ${tempRemoteName}/main --ff-only`,
10094
{ cwd: installPath, env: execEnv }
10195
);
10296

103-
// Clean up temp remote
104-
await execAsync(`git remote remove ${tempRemoteName}`, {
105-
cwd: installPath,
106-
env: execEnv,
107-
});
108-
10997
// Get new version after merge
11098
const newVersion = await getCurrentCommit(installPath);
11199
const newVersionShort = await getShortCommit(installPath);
@@ -130,16 +118,6 @@ export function createPullHandler(settingsService: SettingsService) {
130118
result,
131119
});
132120
} catch (pullError) {
133-
// Clean up temp remote on error
134-
try {
135-
await execAsync(`git remote remove ${tempRemoteName}`, {
136-
cwd: installPath,
137-
env: execEnv,
138-
});
139-
} catch {
140-
// Ignore cleanup errors
141-
}
142-
143121
const errorMsg = getErrorMessage(pullError);
144122
logError(pullError, 'Failed to pull updates');
145123

@@ -165,6 +143,16 @@ export function createPullHandler(settingsService: SettingsService) {
165143
success: false,
166144
error: `Failed to pull updates: ${errorMsg}`,
167145
});
146+
} finally {
147+
// Always clean up temp remote
148+
try {
149+
await execAsync(`git remote remove ${tempRemoteName}`, {
150+
cwd: installPath,
151+
env: execEnv,
152+
});
153+
} catch {
154+
// Ignore cleanup errors
155+
}
168156
}
169157
} catch (error) {
170158
logError(error, 'Update pull failed');

‎apps/ui/src/components/updates/update-notifier.tsx‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { toast } from 'sonner';
1313
import { useUpdatesStore } from '@/store/updates-store';
1414
import { useUpdatePolling } from '@/hooks/use-update-polling';
1515
import { useAppStore } from '@/store/app-store';
16+
import { getRepoDisplayName } from '@/lib/utils';
1617

1718
// ============================================================================
1819
// Types
@@ -117,9 +118,7 @@ export function UpdateNotifier({ onUpdateAvailable, onUpdateInstalled }: UpdateN
117118
}
118119

119120
// Extract repo name for display
120-
const upstreamUrl = autoUpdate.upstreamUrl;
121-
const repoMatch = upstreamUrl.match(/github\.com[/:]([^/]+\/[^/.]+)/);
122-
const repoName = repoMatch ? repoMatch[1] : 'upstream';
121+
const repoName = getRepoDisplayName(autoUpdate.upstreamUrl);
123122

124123
// Show persistent toast with update button
125124
toastIdRef.current = toast.info('Update Available', {
@@ -132,7 +131,7 @@ export function UpdateNotifier({ onUpdateAvailable, onUpdateInstalled }: UpdateN
132131
cancel: {
133132
label: 'Later',
134133
onClick: () => {
135-
// Dismiss toast, will show again on next check if update still available
134+
// Dismiss toast - won't show again for this version until a new version appears
136135
shownToastForCommitRef.current = remoteVersionShort;
137136
},
138137
},

‎apps/ui/src/components/views/settings-view/updates/updates-section.tsx‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import {
1818
CheckCircle2,
1919
AlertCircle,
2020
} from 'lucide-react';
21-
import { cn } from '@/lib/utils';
21+
import { cn, getRepoDisplayName } from '@/lib/utils';
2222
import { toast } from 'sonner';
2323
import { useUpdatesStore } from '@/store/updates-store';
2424
import type { AutoUpdateSettings } from '@automaker/types';
@@ -104,12 +104,6 @@ export function UpdatesSection({ autoUpdate, onAutoUpdateChange }: UpdatesSectio
104104
}
105105
};
106106

107-
// Extract repo name from URL for display
108-
const getRepoDisplayName = (url: string) => {
109-
const match = url.match(/github\.com[/:]([^/]+\/[^/.]+)/);
110-
return match ? match[1] : url;
111-
};
112-
113107
const isLoading = isChecking || isPulling || isLoadingInfo;
114108

115109
return (

‎apps/ui/src/hooks/use-update-polling.ts‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
* The actual check logic lives in the updates-store.
99
*/
1010

11-
import { useEffect, useRef } from 'react';
11+
import { useEffect, useRef, useCallback } from 'react';
1212
import { useAppStore } from '@/store/app-store';
1313
import { useUpdatesStore } from '@/store/updates-store';
1414

@@ -55,7 +55,12 @@ export function useUpdatePolling(options: UseUpdatePollingOptions = {}): UseUpda
5555
// Allow overrides for testing
5656
const isEnabled = options.enabled ?? autoUpdate.enabled;
5757
const intervalMinutes = options.intervalMinutes ?? autoUpdate.checkIntervalMinutes;
58-
const onCheck = options.onCheck ?? checkForUpdates;
58+
59+
// Stabilize the check function reference to prevent interval resets
60+
const onCheckRef = useRef(options.onCheck ?? checkForUpdates);
61+
onCheckRef.current = options.onCheck ?? checkForUpdates;
62+
63+
const stableOnCheck = useCallback(() => onCheckRef.current(), []);
5964

6065
const intervalRef = useRef<NodeJS.Timeout | null>(null);
6166

@@ -72,23 +77,23 @@ export function useUpdatePolling(options: UseUpdatePollingOptions = {}): UseUpda
7277
}
7378

7479
// Check immediately on enable
75-
onCheck();
80+
stableOnCheck();
7681

7782
// Set up interval
7883
const intervalMs = intervalMinutes * 60 * 1000;
79-
intervalRef.current = setInterval(onCheck, intervalMs);
84+
intervalRef.current = setInterval(stableOnCheck, intervalMs);
8085

8186
return () => {
8287
if (intervalRef.current) {
8388
clearInterval(intervalRef.current);
8489
intervalRef.current = null;
8590
}
8691
};
87-
}, [isEnabled, intervalMinutes, onCheck]);
92+
}, [isEnabled, intervalMinutes, stableOnCheck]);
8893

8994
return {
9095
isPollingActive: isEnabled,
91-
checkNow: onCheck,
96+
checkNow: stableOnCheck,
9297
lastChecked,
9398
};
9499
}

‎apps/ui/src/lib/utils.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,3 +63,13 @@ export const isMac =
6363
: typeof navigator !== 'undefined' &&
6464
(/Mac/.test(navigator.userAgent) ||
6565
(navigator.platform ? navigator.platform.toLowerCase().includes('mac') : false));
66+
67+
/**
68+
* Extract a display name from a git repository URL.
69+
* Handles GitHub URLs and returns the owner/repo format.
70+
* Falls back to 'upstream' for unrecognized URLs.
71+
*/
72+
export function getRepoDisplayName(url: string): string {
73+
const match = url.match(/github\.com[/:]([^/]+\/[^/.]+)/);
74+
return match ? match[1] : 'upstream';
75+
}

0 commit comments

Comments
 (0)