Skip to content

Commit c8a463e

Browse files
icecrasher321icecrasher321
committed
fix(tools): fail closed on opaque URL helpers
1 parent 0b9dbcc commit c8a463e

2 files changed

Lines changed: 327 additions & 7 deletions

File tree

‎scripts/check-tool-request-boundary.test.ts‎

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -611,7 +611,15 @@ describe('tool self-hop audit', () => {
611611
})
612612

613613
it('does not mistake a provider-relative path argument for a Sim API route', () => {
614-
const audit = auditRequest("url: (params) => providerUrl('/api/messages', params.host)")
614+
const audit = auditToolSelfHops(`
615+
function providerUrl(path, host) {
616+
return new URL(path, host).toString()
617+
}
618+
const tool = {
619+
id: 'test_tool',
620+
request: { url: (params) => providerUrl('/api/messages', params.host), method: 'GET' },
621+
}
622+
`)
615623

616624
expect(audit.violations).toEqual([])
617625
})
@@ -798,6 +806,53 @@ describe('tool self-hop audit', () => {
798806
])
799807
})
800808

809+
it('rejects a request URL returned by an uninspectable helper', () => {
810+
const audit = auditToolSelfHops(`
811+
const tool = {
812+
id: 'test_tool',
813+
request: { url: () => unknownUrlHelper(), method: 'GET' },
814+
}
815+
`)
816+
817+
expect(audit.violations).toEqual([
818+
expect.objectContaining({
819+
toolId: 'test_tool',
820+
reason: 'unresolved-request-policy',
821+
}),
822+
])
823+
})
824+
825+
it('rejects an uninspectable URL helper combined with the Sim origin', () => {
826+
const audit = auditToolSelfHops(`
827+
import { getBaseUrl } from '@/lib/core/utils/urls'
828+
const tool = {
829+
id: 'test_tool',
830+
request: { url: () => getBaseUrl() + unknownPathHelper(), method: 'GET' },
831+
}
832+
`)
833+
834+
expect(audit.violations).toEqual([
835+
expect.objectContaining({
836+
toolId: 'test_tool',
837+
reason: 'unresolved-request-policy',
838+
}),
839+
])
840+
})
841+
842+
it('allows an uninspectable path helper after an explicit external origin', () => {
843+
const audit = auditToolSelfHops(`
844+
const tool = {
845+
id: 'test_tool',
846+
request: {
847+
url: (params) => 'https://provider.example.com/' + unknownPathHelper(params.id),
848+
method: 'GET',
849+
},
850+
}
851+
`)
852+
853+
expect(audit.violations).toEqual([])
854+
})
855+
801856
it('rejects a direct-id tool whose request may come from an unresolved spread', () => {
802857
const audit = auditToolSelfHops(`
803858
const tool = { id: 'test_tool', ...unknownBase }

‎scripts/check-tool-request-boundary.ts‎

Lines changed: 271 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ const FUNCTION_NODE_TYPES = new Set([
2727
'FunctionDeclaration',
2828
'ObjectMethod',
2929
])
30+
const URL_VALUE_WRAPPER_CALLS = new Set(['String', 'encodeURI', 'encodeURIComponent'])
3031

3132
interface Violation {
3233
file: string
@@ -636,6 +637,264 @@ function isInternalUrlConstruction(node: SyntaxNode, resolver: SelfHopResolver):
636637
)
637638
}
638639

640+
function hasExplicitExternalUrlPrefix(
641+
expression: SyntaxNode,
642+
resolver: SelfHopResolver,
643+
seen = new Set<string>()
644+
): boolean {
645+
const current = unwrapExpression(expression)
646+
const staticValue = getStaticString(current)
647+
if (staticValue !== undefined) return /^https?:\/\//.test(staticValue)
648+
const prefix = getStringPrefix(current)
649+
if (prefix !== undefined && /^https?:\/\//.test(prefix)) return true
650+
if (current.type === 'Identifier' && typeof current.name === 'string') {
651+
const key = `${resolver.file}:external-origin:${current.name}`
652+
if (seen.has(key)) return false
653+
const binding = resolveScopedIdentifier(current.name, resolver)
654+
if (!binding) return false
655+
const nextSeen = new Set(seen)
656+
nextSeen.add(key)
657+
return hasExplicitExternalUrlPrefix(binding.expression, binding.resolver, nextSeen)
658+
}
659+
if (
660+
current.type === 'BinaryExpression' &&
661+
current.operator === '+' &&
662+
isSyntaxNode(current.left)
663+
) {
664+
return hasExplicitExternalUrlPrefix(current.left, resolver, new Set(seen))
665+
}
666+
if (current.type === 'ConditionalExpression') {
667+
return (
668+
isSyntaxNode(current.consequent) &&
669+
isSyntaxNode(current.alternate) &&
670+
hasExplicitExternalUrlPrefix(current.consequent, resolver, new Set(seen)) &&
671+
hasExplicitExternalUrlPrefix(current.alternate, resolver, new Set(seen))
672+
)
673+
}
674+
if (current.type === 'LogicalExpression') {
675+
return (
676+
isSyntaxNode(current.left) &&
677+
isSyntaxNode(current.right) &&
678+
hasExplicitExternalUrlPrefix(current.left, resolver, new Set(seen)) &&
679+
hasExplicitExternalUrlPrefix(current.right, resolver, new Set(seen))
680+
)
681+
}
682+
if (
683+
current.type === 'TemplateLiteral' &&
684+
Array.isArray(current.expressions) &&
685+
Array.isArray(current.quasis) &&
686+
current.expressions.length > 0 &&
687+
current.quasis.length === current.expressions.length + 1 &&
688+
current.expressions.every(isSyntaxNode) &&
689+
current.quasis.every(isSyntaxNode) &&
690+
getTemplateQuasiValue(current.quasis[0]) === '' &&
691+
hasExplicitExternalUrlPrefix(current.expressions[0], resolver, new Set(seen))
692+
) {
693+
const suffix = getTemplateQuasiValue(current.quasis[1])
694+
return (
695+
suffix !== undefined &&
696+
(suffix === '' ? current.expressions.length === 1 : /^[/?#]/.test(suffix))
697+
)
698+
}
699+
return false
700+
}
701+
702+
function expressionContainsUnresolvedUrlHelper(
703+
expression: SyntaxNode,
704+
resolver: SelfHopResolver,
705+
seen = new Set<string>()
706+
): boolean {
707+
const current = unwrapExpression(expression)
708+
if (hasExplicitExternalUrlPrefix(current, resolver)) return false
709+
710+
if (current.type === 'Identifier' && typeof current.name === 'string') {
711+
const key = `${resolver.file}:unresolved-url:${current.name}`
712+
if (seen.has(key)) return false
713+
const binding = resolveScopedIdentifier(current.name, resolver)
714+
if (!binding) return false
715+
const nextSeen = new Set(seen)
716+
nextSeen.add(key)
717+
return expressionContainsUnresolvedUrlHelper(binding.expression, binding.resolver, nextSeen)
718+
}
719+
720+
if (current.type === 'ConditionalExpression') {
721+
return (
722+
(isSyntaxNode(current.consequent) &&
723+
expressionContainsUnresolvedUrlHelper(current.consequent, resolver, new Set(seen))) ||
724+
(isSyntaxNode(current.alternate) &&
725+
expressionContainsUnresolvedUrlHelper(current.alternate, resolver, new Set(seen)))
726+
)
727+
}
728+
if (current.type === 'LogicalExpression') {
729+
return (
730+
(isSyntaxNode(current.left) &&
731+
expressionContainsUnresolvedUrlHelper(current.left, resolver, new Set(seen))) ||
732+
(isSyntaxNode(current.right) &&
733+
expressionContainsUnresolvedUrlHelper(current.right, resolver, new Set(seen)))
734+
)
735+
}
736+
if (
737+
current.type === 'BinaryExpression' &&
738+
current.operator === '+' &&
739+
isSyntaxNode(current.left)
740+
) {
741+
if (isSimOriginExpression(current.left, resolver) && isSyntaxNode(current.right)) {
742+
return expressionContainsUnresolvedUrlHelper(current.right, resolver, new Set(seen))
743+
}
744+
return expressionContainsUnresolvedUrlHelper(current.left, resolver, new Set(seen))
745+
}
746+
if (
747+
current.type === 'TemplateLiteral' &&
748+
Array.isArray(current.expressions) &&
749+
Array.isArray(current.quasis) &&
750+
current.expressions.every(isSyntaxNode) &&
751+
current.quasis.every(isSyntaxNode) &&
752+
current.expressions.length > 0 &&
753+
current.quasis.length === current.expressions.length + 1
754+
) {
755+
const leading = getTemplateQuasiValue(current.quasis[0])
756+
if (leading !== '') return false
757+
const origin = current.expressions[0]
758+
if (isSimOriginExpression(origin, resolver)) {
759+
const following = getTemplateQuasiValue(current.quasis[1])
760+
return (
761+
following === '' &&
762+
current.expressions.length > 1 &&
763+
expressionContainsUnresolvedUrlHelper(current.expressions[1], resolver, new Set(seen))
764+
)
765+
}
766+
return expressionContainsUnresolvedUrlHelper(origin, resolver, new Set(seen))
767+
}
768+
769+
if (current.type === 'CallExpression' || current.type === 'OptionalCallExpression') {
770+
if (!isSyntaxNode(current.callee)) return true
771+
const callee = unwrapExpression(current.callee)
772+
if (callee.type === 'Identifier' && typeof callee.name === 'string') {
773+
if (
774+
resolver.simOriginBindings.has(callee.name) ||
775+
resolver.simUrlBuilderBindings.has(callee.name)
776+
) {
777+
return false
778+
}
779+
if (URL_VALUE_WRAPPER_CALLS.has(callee.name)) {
780+
const firstArgument = Array.isArray(current.arguments)
781+
? current.arguments.find(isSyntaxNode)
782+
: undefined
783+
return firstArgument
784+
? expressionContainsUnresolvedUrlHelper(firstArgument, resolver, new Set(seen))
785+
: false
786+
}
787+
const key = `${resolver.file}:unresolved-url-call:${callee.name}`
788+
if (seen.has(key)) return false
789+
const binding = resolveScopedIdentifier(callee.name, resolver)
790+
if (!binding || !FUNCTION_NODE_TYPES.has(unwrapExpression(binding.expression).type)) {
791+
return true
792+
}
793+
const nextSeen = new Set(seen)
794+
nextSeen.add(key)
795+
const argumentsList = Array.isArray(current.arguments)
796+
? current.arguments
797+
.filter(isSyntaxNode)
798+
.map((argument) => resolveScopedArgument(argument, resolver))
799+
: []
800+
return functionContainsUnresolvedUrlHelper(
801+
binding.expression,
802+
binding.resolver,
803+
nextSeen,
804+
argumentsList
805+
)
806+
}
807+
const access = getStaticMemberAccess(callee)
808+
if (!access) return true
809+
return expressionContainsUnresolvedUrlHelper(access.target, resolver, new Set(seen))
810+
}
811+
812+
if (current.type === 'NewExpression') {
813+
if (
814+
isSyntaxNode(current.callee) &&
815+
current.callee.type === 'Identifier' &&
816+
current.callee.name === 'URL' &&
817+
Array.isArray(current.arguments)
818+
) {
819+
if (
820+
current.arguments.length > 1 &&
821+
isSyntaxNode(current.arguments[1]) &&
822+
hasExplicitExternalUrlPrefix(current.arguments[1], resolver)
823+
) {
824+
return false
825+
}
826+
const path = current.arguments.find(isSyntaxNode)
827+
const base = current.arguments.length > 1 ? current.arguments[1] : undefined
828+
if (isSyntaxNode(base)) {
829+
return isSimOriginExpression(base, resolver)
830+
? Boolean(path && expressionContainsUnresolvedUrlHelper(path, resolver, new Set(seen)))
831+
: expressionContainsUnresolvedUrlHelper(base, resolver, new Set(seen))
832+
}
833+
return path ? expressionContainsUnresolvedUrlHelper(path, resolver, new Set(seen)) : false
834+
}
835+
return true
836+
}
837+
838+
if (current.type === 'MemberExpression' || current.type === 'OptionalMemberExpression') {
839+
const access = getStaticMemberAccess(current)
840+
return access
841+
? expressionContainsUnresolvedUrlHelper(access.target, resolver, new Set(seen))
842+
: false
843+
}
844+
if (FUNCTION_NODE_TYPES.has(current.type)) {
845+
return functionContainsUnresolvedUrlHelper(current, resolver, seen)
846+
}
847+
return false
848+
}
849+
850+
function functionContainsUnresolvedUrlHelper(
851+
fn: SyntaxNode,
852+
resolver: SelfHopResolver,
853+
seen: ReadonlySet<string>,
854+
argumentsList: readonly ScopedExpression[] = []
855+
): boolean {
856+
const current = unwrapExpression(fn)
857+
if (!FUNCTION_NODE_TYPES.has(current.type)) return true
858+
const locals = new Map(resolver.locals)
859+
const scopedLocals = new Map(resolver.scopedLocals)
860+
const parameters = Array.isArray(current.params) ? current.params : []
861+
for (const [index, parameter] of parameters.entries()) {
862+
if (
863+
isSyntaxNode(parameter) &&
864+
parameter.type === 'Identifier' &&
865+
typeof parameter.name === 'string' &&
866+
argumentsList[index]
867+
) {
868+
const argument = argumentsList[index]
869+
scopedLocals.set(parameter.name, argument)
870+
if (argument.resolver === resolver) locals.set(parameter.name, argument.expression)
871+
}
872+
}
873+
collectFunctionLocalBindings(current, locals)
874+
const localResolver = { ...resolver, locals, scopedLocals }
875+
if (current.type === 'ArrowFunctionExpression' && isSyntaxNode(current.body)) {
876+
const body = unwrapExpression(current.body)
877+
if (body.type !== 'BlockStatement') {
878+
return expressionContainsUnresolvedUrlHelper(body, localResolver, new Set(seen))
879+
}
880+
}
881+
let unresolved = false
882+
const visit = (node: SyntaxNode) => {
883+
if (unresolved || (node !== current && FUNCTION_NODE_TYPES.has(node.type))) return
884+
if (
885+
node.type === 'ReturnStatement' &&
886+
isSyntaxNode(node.argument) &&
887+
expressionContainsUnresolvedUrlHelper(node.argument, localResolver, new Set(seen))
888+
) {
889+
unresolved = true
890+
return
891+
}
892+
for (const child of getChildNodes(node)) visit(child)
893+
}
894+
visit(current)
895+
return unresolved
896+
}
897+
639898
function collectImportedBindings(program: SyntaxNode): {
640899
importedBindings: Map<string, ImportedBinding>
641900
simOriginBindings: Set<string>
@@ -1263,12 +1522,14 @@ export function auditToolSelfHops(source: string, file = 'source.ts'): ToolSelfH
12631522
for (const request of requests.requests) {
12641523
const urlProperties = getResolvedObjectProperties(request, 'url')
12651524
const internalProperties = getResolvedObjectProperties(request, 'internal')
1525+
let hasLegacyInternalPolicy = false
12661526
if (!urlProperties.complete || !internalProperties.complete) {
12671527
reportUnresolved(requestProperty.loc?.start.line ?? 1)
12681528
}
12691529
for (const resolvedInternal of internalProperties.properties) {
12701530
if (!resolvedInternal) continue
12711531
const internalProperty = resolvedInternal.property
1532+
hasLegacyInternalPolicy = true
12721533
legacyInternalPolicies += 1
12731534
violations.push({
12741535
file,
@@ -1287,19 +1548,23 @@ export function auditToolSelfHops(source: string, file = 'source.ts'): ToolSelfH
12871548
? urlProperty.value
12881549
: undefined
12891550
if (!urlExpression) continue
1290-
if (
1291-
expressionContainsInternalRoute(
1292-
unwrapExpression(urlExpression),
1293-
requestObjectResolver(urlRequest)
1294-
)
1295-
) {
1551+
const currentUrl = unwrapExpression(urlExpression)
1552+
const urlResolver = requestObjectResolver(urlRequest)
1553+
if (expressionContainsInternalRoute(currentUrl, urlResolver)) {
12961554
detectedSelfHops += 1
12971555
violations.push({
12981556
file,
12991557
line: urlProperty.loc?.start.line ?? requestProperty.loc?.start.line ?? 1,
13001558
toolId,
13011559
reason: 'same-origin-tool-request',
13021560
})
1561+
} else if (
1562+
!hasLegacyInternalPolicy &&
1563+
expressionContainsUnresolvedUrlHelper(currentUrl, urlResolver)
1564+
) {
1565+
reportUnresolved(
1566+
urlProperty.loc?.start.line ?? requestProperty.loc?.start.line ?? 1
1567+
)
13031568
}
13041569
}
13051570
}

0 commit comments

Comments
 (0)