Clear auth#301
Conversation
The findRelayPtyBinary() logic was duplicated in both spawner.ts and relay-pty-orchestrator.ts, with the wrapper version not having the global npm install fixes that were added to spawner.ts. This change: - Creates shared utility in @agent-relay/utils/relay-pty-path - Both spawner.ts and relay-pty-orchestrator.ts now use the shared utility - Fixes global npm install binary resolution for wrapper package When installed globally via npm -g, the binary search now correctly handles nested @agent-relay/* packages by using a non-greedy regex to find the root node_modules directory. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Updated remaining files to use @agent-relay/utils/relay-pty-path: - packages/daemon/src/cli-auth.ts - packages/cloud/src/api/cli-pty-runner.ts - scripts/test-cli-auth/test-oauth-flow.ts - scripts/test-cli-auth/ci-test-runner.ts All six locations that needed to find the relay-pty binary now use the same shared utility, ensuring consistent behavior across: - Global npm installs (npm i -g agent-relay) - Local installs (npm i agent-relay) - Development (cargo build) - npx usage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 My Senior Dev — Analysis Complete👤 For @khaliqgant📁 Expert in View your contributor analytics → 📊 22 files reviewed • 6 high risk • 6 need attention 🚨 High Risk:
🚀 Open Interactive Review →The full interface unlocks features not available in GitHub:
💬 Chat here: 📖 View all 12 personas & slash commandsYou can interact with me by mentioning In PR comments or on any line of code:
Slash commands:
AI Personas (mention to get their perspective):
For the best experience, view this PR on myseniordev.com — includes AI chat, file annotations, and interactive reviews. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (provider === 'gemini' || provider === 'google') { | ||
| pathsToDelete.push(path.join(userHome, '.gemini', '.env')); | ||
| } | ||
|
|
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
| pathsToDelete.push(path.join(userHome, '.gemini', '.env')); | ||
| } | ||
|
|
||
| // Handle anthropic special case (both .credentials.json and credentials.json) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
In general, to fix uncontrolled-path issues you must ensure that any filesystem path derived from user input is normalized and verified to lie under a trusted root directory before passing it to functions like fs.mkdirSync, fs.readFileSync, etc. This usually involves using path.resolve/fs.realpathSync and checking that the resulting absolute path starts with the trusted base path.
In this codebase, the safest minimal fix—without changing existing behavior—is to harden ensureDirectory so it only creates directories within the UserDirectoryService’s root (this.baseDir), and optionally log/throw if it is asked to operate outside that root. We already validate userId to keep paths under this.usersDir, but by adding a guard in ensureDirectory we ensure all future uses of this helper are also constrained. Because ensureDirectory is an instance method, it can access this.baseDir. The exact change is:
- Modify
ensureDirectory(dirPath: string)inpackages/user-directory/src/user-directory.tsto:- Resolve
dirPathto an absolute path. - Resolve
this.baseDirto an absolute path. - Check
resolvedDirPath.startsWith(resolvedBaseDir + path.sep)or equality. - If the check fails, log a warning and throw an error instead of creating the directory.
- Only then call
fs.existsSyncandfs.mkdirSync.
- Resolve
This preserves behavior for all legitimate paths (which are already under baseDir) while preventing accidental or malicious attempts to create directories outside the data root. It also satisfies CodeQL by adding a containment check directly at the sink that was being flagged.
| @@ -354,11 +354,24 @@ | ||
|
|
||
| /** | ||
| * Ensure a directory exists, creating it recursively if needed. | ||
| * Ensures the directory is contained within the service's base directory. | ||
| */ | ||
| private ensureDirectory(dirPath: string): void { | ||
| if (!fs.existsSync(dirPath)) { | ||
| fs.mkdirSync(dirPath, { recursive: true }); | ||
| const resolvedBaseDir = path.resolve(this.baseDir); | ||
| const resolvedDirPath = path.resolve(dirPath); | ||
|
|
||
| // Prevent creating directories outside the configured base directory | ||
| if ( | ||
| resolvedDirPath !== resolvedBaseDir && | ||
| !resolvedDirPath.startsWith(resolvedBaseDir + path.sep) | ||
| ) { | ||
| logger.warn(`Attempt to create directory outside baseDir: ${resolvedDirPath}`); | ||
| throw new Error('Directory path escapes base directory'); | ||
| } | ||
|
|
||
| if (!fs.existsSync(resolvedDirPath)) { | ||
| fs.mkdirSync(resolvedDirPath, { recursive: true }); | ||
| } | ||
| } | ||
| } | ||
|
|
| // Delete each file if it exists | ||
| for (const credPath of pathsToDelete) { | ||
| try { | ||
| if (fs.existsSync(credPath)) { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
In general, this kind of problem is fixed by ensuring that any user-controlled data used in a filesystem path is constrained to a safe format and/or checked to ensure the resulting path stays within an expected root. In this codebase, the intended design is /data/users/{userId}/..., so the simplest, least-invasive fix is to strictly validate userId so it cannot contain path separators or traversal sequences, effectively turning it into a simple identifier rather than an arbitrary path.
The best fix here is to (1) implement a strong validateUserId method that enforces a tight allowlist (for example, 1–64 characters of [a-zA-Z0-9_-]), and (2) ensure that all code paths using userId for path construction go through this validation. The code already calls this.validateUserId(userId) in getUserHome and deleteProviderCredentials, so adding a robust implementation of validateUserId inside UserDirectoryService is sufficient; we do not need to change call sites or the behavior of directory layout. This will satisfy CodeQL by breaking taint propagation and will prevent directory traversal or arbitrary path injection while preserving existing functionality for legitimate user IDs that meet the new constraints.
Concretely:
- In
packages/user-directory/src/user-directory.ts, insideUserDirectoryService, add a private methodvalidateUserId(userId: string): void(or strengthen it if it already exists but is not shown). This method should:- Check that
userIdis a non-empty string. - Enforce a safe regex like
/^[a-zA-Z0-9_-]{1,64}$/. - Throw a descriptive
Errorif validation fails.
- Check that
- Optionally, include a short comment explaining that this is to prevent path traversal, which also helps future maintainers understand why the constraints are strict.
No changes are needed in packages/dashboard/src/server.ts because that code already depends on the UserDirectoryService validations, and we are not modifying the public API or the semantics beyond rejecting obviously malicious userId formats.
| @@ -87,6 +87,28 @@ | ||
| private usersDir: string; | ||
|
|
||
| /** | ||
| * Validate that a userId is safe to use as a directory name. | ||
| * | ||
| * Restricts user IDs to a simple, non-path format to prevent directory | ||
| * traversal or writing outside the configured users directory. | ||
| * | ||
| * Allowed characters: letters, digits, underscore, hyphen. | ||
| * Length: 1 to 64 characters. | ||
| * | ||
| * @param userId - User ID supplied by the caller | ||
| * @throws Error if the userId is invalid or unsafe | ||
| */ | ||
| private validateUserId(userId: string): void { | ||
| if (typeof userId !== 'string' || userId.length === 0) { | ||
| throw new Error('Invalid userId: must be a non-empty string'); | ||
| } | ||
| const USER_ID_REGEX = /^[a-zA-Z0-9_-]{1,64}$/; | ||
| if (!USER_ID_REGEX.test(userId)) { | ||
| throw new Error('Invalid userId: contains unsafe characters'); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Create a new UserDirectoryService. | ||
| * @param baseDir - Base data directory (e.g., /data) | ||
| */ |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Undefined path variable used instead of imported join function
The script uses path.join() on line 84 but the path module is not imported - only join and dirname are imported from node:path.
Click to expand
Import Statement (line 19)
import { join, dirname } from 'node:path';Usage (line 84)
const mockCliPath = path.join(__dirname, 'mock-cli.sh');This will cause a ReferenceError: path is not defined at runtime when the test script is executed.
Expected
Should use the imported join function directly:
const mockCliPath = join(__dirname, 'mock-cli.sh');(Refers to line 84)
Recommendation: Change path.join(__dirname, 'mock-cli.sh') to join(__dirname, 'mock-cli.sh') to use the imported function.
Was this helpful? React with 👍 or 👎 to provide feedback.
Addresses GitHub Advanced Security feedback: 1. Added provider whitelist validation in user-directory - New VALID_PROVIDERS set with known providers and aliases - validateProvider() method throws on invalid providers - Called at start of deleteProviderCredentials() 2. Sanitized log messages to prevent log injection - Strip newlines and limit length before logging - Use structured logging format with objects instead of string interpolation - Affected files: dashboard/server.ts, cloud/providers.ts 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| const VALID_PROVIDERS = new Set([ | ||
| ...ALL_PROVIDERS, | ||
| 'anthropic', // alias for claude | ||
| 'google', // alias for gemini | ||
| ]); |
There was a problem hiding this comment.
🟡 Missing provider aliases in VALID_PROVIDERS causes credential deletion to fail
The VALID_PROVIDERS set in user-directory.ts does not include 'openai' or 'factory' as valid provider names, but ALL_CREDENTIAL_PROVIDERS in provider-env.ts includes both of these aliases.
Click to expand
VALID_PROVIDERS in user-directory.ts (lines 75-79):
const VALID_PROVIDERS = new Set([
...ALL_PROVIDERS, // claude, codex, gemini, opencode, droid, cursor
'anthropic', // alias for claude
'google', // alias for gemini
]);ALL_CREDENTIAL_PROVIDERS in provider-env.ts (lines 13-20):
const ALL_CREDENTIAL_PROVIDERS = [
'anthropic', 'claude',
'codex', 'openai', // <-- 'openai' not in VALID_PROVIDERS
'google', 'gemini',
'opencode',
'droid', 'factory', // <-- 'factory' not in VALID_PROVIDERS
'cursor',
];If clearProviderCredentials() is called with 'openai' or 'factory', the validateProvider() check in deleteProviderCredentials() will throw an error: Invalid provider: openai.
The clearProviderCredentials function in provider-env.ts:144 calls userDirService.deleteProviderCredentials(userId, provider) which validates the provider name.
Impact: While the current frontend uses codex and droid as provider IDs, any code path that passes openai or factory will fail to delete credentials.
Recommendation: Add 'openai' and 'factory' to the VALID_PROVIDERS set as aliases: const VALID_PROVIDERS = new Set([...ALL_PROVIDERS, 'anthropic', 'google', 'openai', 'factory']);
Was this helpful? React with 👍 or 👎 to provide feedback.
| for (const credPath of pathsToDelete) { | ||
| try { | ||
| if (fs.existsSync(credPath)) { | ||
| fs.unlinkSync(credPath); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
In general, to fix uncontrolled path usage based on user input, you should both (a) validate the incoming identifier (here userId) and (b) enforce that any constructed path stays within an expected root directory by resolving and checking it. Normalizing with path.resolve and verifying the result still lies inside the designated base directory prevents path traversal even if the identifier validation is later weakened or bypassed.
The best targeted fix here is to harden getUserHome so that it never returns a path outside this.usersDir, regardless of userId. We can do this by:
- Keeping the existing
validateUserId(userId)call. - Constructing
userHomewithpath.join(this.usersDir, userId)as before. - Resolving it with
path.resolveand verifying that the resolved path starts withthis.usersDiras a proper path prefix. If it doesn’t, throw an error. - Using the resolved, validated
userHomefor directory creation and as the return value.
This change is local to packages/user-directory/src/user-directory.ts inside getUserHome. It doesn’t change any external API or valid behavior: for legitimate userId values, the resolved path will equal the original path.join result, and everything works as before. No new imports are needed because path is already imported. All later uses of userHome (such as in deleteProviderCredentials) automatically benefit from this containment guarantee.
| @@ -110,7 +110,16 @@ | ||
| getUserHome(userId: string): string { | ||
| this.validateUserId(userId); | ||
|
|
||
| const userHome = path.join(this.usersDir, userId); | ||
| // Construct the user home directory under the users base directory | ||
| const joinedPath = path.join(this.usersDir, userId); | ||
| const userHome = path.resolve(joinedPath); | ||
|
|
||
| // Ensure the resolved path is still within the users directory to prevent path traversal | ||
| const usersDirResolved = path.resolve(this.usersDir); | ||
| if (!userHome.startsWith(usersDirResolved + path.sep) && userHome !== usersDirResolved) { | ||
| throw new Error('Invalid userId: resolved path is outside of users directory'); | ||
| } | ||
|
|
||
| this.ensureDirectory(userHome); | ||
|
|
||
| return userHome; |
| export function hasRelayPtyBinary(callerDirname: string): boolean { | ||
| if (!cacheChecked) { | ||
| cachedBinaryPath = findRelayPtyBinary(callerDirname); | ||
| cacheChecked = true; | ||
| } | ||
| return cachedBinaryPath !== null; |
There was a problem hiding this comment.
🔴 hasRelayPtyBinary caches result using only first caller's __dirname, ignoring subsequent callers
The hasRelayPtyBinary function caches the binary path using module-level variables, but ignores the callerDirname parameter after the first call. This means if different modules call this function with different __dirname values, all callers after the first will receive results based on the first caller's path, which may be incorrect.
Click to expand
How it breaks
The function at packages/utils/src/relay-pty-path.ts:113-118:
export function hasRelayPtyBinary(callerDirname: string): boolean {
if (!cacheChecked) {
cachedBinaryPath = findRelayPtyBinary(callerDirname);
cacheChecked = true;
}
return cachedBinaryPath !== null;
}Once cacheChecked is set to true, subsequent calls with different callerDirname values are completely ignored. The findRelayPtyBinary function relies heavily on callerDirname to compute relative paths (see lines 45-56), so using the wrong path can result in:
- Binary found with first caller but not found for second caller (returns incorrect
true) - Binary not found with first caller but would be found for second caller (returns incorrect
false)
Impact
If the binary is incorrectly reported as not found, agent spawning and PTY orchestration will fail with "relay-pty binary not found" errors.
Recommendation: Either remove the caching from hasRelayPtyBinary (call findRelayPtyBinary each time), or use a Map keyed by callerDirname to cache results per-caller. Example fix:
const cacheByDir = new Map<string, string | null>();
export function hasRelayPtyBinary(callerDirname: string): boolean {
if (!cacheByDir.has(callerDirname)) {
cacheByDir.set(callerDirname, findRelayPtyBinary(callerDirname));
}
return cacheByDir.get(callerDirname) !== null;
}
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Uh oh!
There was an error while loading. Please reload this page.