Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 66 additions & 5 deletions npm/src/tools/vercel.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import { delegate } from '../delegate.js';
import { analyzeAll } from './analyzeAll.js';
import { searchSchema, querySchema, extractSchema, delegateSchema, analyzeAllSchema, searchDescription, queryDescription, extractDescription, delegateDescription, analyzeAllDescription, parseTargets, parseAndResolvePaths, resolveTargetPath } from './common.js';
import { existsSync } from 'fs';
import { formatErrorForAI } from '../utils/error-types.js';
import { annotateOutputWithHashes } from './hashline.js';

Expand Down Expand Up @@ -118,6 +119,18 @@
return normalizeTargets(fallbackTargetsFromText(trimmed));
}

function splitTargetSuffix(target) {
const searchStart = (target.length > 2 && target[1] === ':' && /[a-zA-Z]/.test(target[0])) ? 2 : 0;
const colonIdx = target.indexOf(':', searchStart);
const hashIdx = target.indexOf('#');
if (colonIdx !== -1 && (hashIdx === -1 || colonIdx < hashIdx)) {
return { filePart: target.substring(0, colonIdx), suffix: target.substring(colonIdx) };
} else if (hashIdx !== -1) {
return { filePart: target.substring(0, hashIdx), suffix: target.substring(hashIdx) };
}

Check notice on line 130 in npm/src/tools/vercel.js

View check run for this annotation

probelabs / Visor: security

architecture Issue

The splitTargetSuffix function is duplicated from resolveTargetPath in common.js. Both functions implement the same logic for splitting file paths from line numbers/symbols. This duplication could lead to maintenance issues where one is updated but not the other.
Raw output
Export splitTargetSuffix from common.js and import it in vercel.js instead of duplicating the logic. This ensures consistent path parsing behavior across the codebase.
return { filePart: target, suffix: '' };
}

function buildSearchDelegateTask({ searchQuery, searchPath, exact, language, allowTests }) {
return [
'You are a code-search subagent. Your job is to find ALL relevant code locations for the given query.',
Expand Down Expand Up @@ -283,16 +296,64 @@
if (options.fileTracker && typeof fallbackResult === 'string') {
options.fileTracker.trackFilesFromOutput(fallbackResult, options.cwd || '.').catch(() => {});
}
return fallbackResult;
}

// Resolve relative paths against the actual search directory, not the general cwd.
// The delegate returns paths relative to where the search was performed (searchPaths[0]),
// which may differ from options.cwd when the user specifies a path parameter.
// The delegate runs from workspace root (allowedFolders[0] or cwd), NOT from searchPaths[0].
// It returns paths relative to that workspace root. Resolve against the same base.
const delegateBase = options.allowedFolders?.[0] || options.cwd || '.';
const resolutionBase = searchPaths[0] || options.cwd || '.';
const resolvedTargets = targets.map(target => resolveTargetPath(target, resolutionBase));
const resolvedTargets = targets.map(target => resolveTargetPath(target, delegateBase));

// Auto-fix: detect and repair invalid paths (doubled segments, AI hallucinations)
const validatedTargets = [];
for (const target of resolvedTargets) {
const { filePart, suffix } = splitTargetSuffix(target);

// 1. Path exists as-is
if (existsSync(filePart)) {
validatedTargets.push(target);
continue;
}

// 2. Detect doubled directory segments: /ws/proj/proj/src → /ws/proj/src
let fixed = false;
const parts = filePart.split('/').filter(Boolean);
for (let i = 0; i < parts.length - 1; i++) {
if (parts[i] === parts[i + 1]) {
const candidate = '/' + [...parts.slice(0, i), ...parts.slice(i + 1)].join('/');
if (existsSync(candidate)) {
validatedTargets.push(candidate + suffix);
if (debug) console.error(`[search-delegate] Fixed doubled path segment: ${filePart} → ${candidate}`);
fixed = true;

Check notice on line 328 in npm/src/tools/vercel.js

View check run for this annotation

probelabs / Visor: security

security Issue

The doubled segment detection only checks for consecutive identical segments (parts[i] === parts[i + 1]). This misses cases where an attacker could craft paths like '/workspace/proj/PROJ/file' (case variation) or '/workspace/proj/proj2/file' (partial match) that might bypass the naive equality check while still causing path confusion issues.
Raw output
Consider normalizing paths to lowercase before comparison on case-insensitive filesystems, or document that this is a best-effort auto-fix for AI hallucinations rather than a security boundary.
break;
}
}
}
if (fixed) continue;

// 3. Try resolving against alternative bases (searchPaths[0], cwd)
for (const altBase of [resolutionBase, options.cwd].filter(Boolean)) {
if (altBase === delegateBase) continue;
const altResolved = resolveTargetPath(target, altBase);
const { filePart: altFile } = splitTargetSuffix(altResolved);
if (existsSync(altFile)) {
validatedTargets.push(altResolved);
if (debug) console.error(`[search-delegate] Resolved with alt base: ${filePart} → ${altFile}`);
fixed = true;
break;
}
}
if (fixed) continue;

Check warning on line 348 in npm/src/tools/vercel.js

View check run for this annotation

probelabs / Visor: security

security Issue

The new path resolution logic resolves paths against 'delegateBase' (workspace root) and then validates with existsSync, but there is no explicit check that the resolved path is within allowedFolders. If a malicious or compromised subagent returns a path like '/etc/passwd' or '../../../etc/passwd', the resolveTargetPath function will resolve it, and if the file happens to exist, it will be included in validatedTargets without verifying it's within the allowed workspace boundaries.
Raw output
Add explicit path boundary validation after resolution. Before adding to validatedTargets, verify the resolved filePart is within allowedFolders using the existing isPathAllowed pattern from edit.js or by checking that the resolved path starts with the allowed folder path + separator. Example: const resolvedAbs = resolve(delegateBase, filePart); if (!allowedFolders.some(f => resolvedAbs.startsWith(f + sep) || resolvedAbs === f)) { continue; }
// 4. Keep target anyway (probe binary will report the error)
// but log a warning
if (debug) console.error(`[search-delegate] Warning: target may not exist: ${filePart}`);
validatedTargets.push(target);
}

const extractOptions = {
files: resolvedTargets,
files: validatedTargets,
cwd: resolutionBase,
allowTests: allow_tests ?? true
};
Expand Down
23 changes: 13 additions & 10 deletions npm/tests/unit/search-delegate.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,22 +92,23 @@ describe('searchDelegate behavior', () => {
);
const extractArgs = mockExtract.mock.calls[0][0];
expect(extractArgs).toEqual(expect.objectContaining({ files: expect.any(Array) }));
// Paths should be resolved against the search path (/workspace/src), not cwd (/workspace)
// Paths should be resolved against delegateBase (allowedFolders[0] = /workspace),
// not searchPaths[0] (/workspace/src)
const normalizedFiles = extractArgs.files.map((file) =>
file.replace(/^[A-Za-z]:/, '').replace(/\\/g, '/')
);
expect(normalizedFiles).toEqual(expect.arrayContaining([
'/workspace/src/a.js#foo',
'/workspace/src/b.js:10-12'
'/workspace/a.js#foo',
'/workspace/b.js:10-12'
]));
expect(mockSearch).not.toHaveBeenCalled();
});

test('resolves delegate paths against search path, not cwd, when they differ', async () => {
// Simulate the bug case: cwd differs from search path
// Delegate returns paths relative to the search directory
test('resolves delegate paths against workspace root when subagent returns workspace-relative paths', async () => {
// Real scenario: subagent runs from /tmp/workspace (workspace root)
// and returns paths relative to that root, including the project dir name
mockDelegate.mockResolvedValue(JSON.stringify({
targets: ['dashboard/api.go#Handler', 'dashboard/model.go:10-20']
targets: ['tyk-analytics/dashboard/api.go#Handler', 'tyk-analytics/dashboard/model.go:10-20']
}));
mockExtract.mockResolvedValue('EXTRACTED');

Expand All @@ -127,13 +128,15 @@ describe('searchDelegate behavior', () => {
const normalizedFiles = extractArgs.files.map((file) =>
file.replace(/^[A-Za-z]:/, '').replace(/\\/g, '/')
);
// Paths must resolve against the search path (/tmp/workspace/tyk-analytics),
// NOT against cwd (/tmp/workspace)
// Paths should resolve against delegateBase (/tmp/workspace), NOT searchPaths[0] (/tmp/workspace/tyk-analytics)
// This prevents doubled path: /tmp/workspace/tyk-analytics/tyk-analytics/dashboard/api.go
expect(normalizedFiles).toEqual(expect.arrayContaining([
'/tmp/workspace/tyk-analytics/dashboard/api.go#Handler',
'/tmp/workspace/tyk-analytics/dashboard/model.go:10-20'
]));
// Extract cwd should also be the search path
// Should NOT have doubled paths
expect(normalizedFiles.some(f => f.includes('tyk-analytics/tyk-analytics'))).toBe(false);
// Extract cwd should be the search path (resolutionBase)
expect(extractArgs.cwd).toBe('/tmp/workspace/tyk-analytics');
});

Expand Down
Loading