skip insecure checks if inside functions which output considered safe regardless of the arguments

Author: IlyaGulya

expr_ast.go

@@ -366,3 +366,18 @@ func visitExprNode(n, p ExprNode, f VisitExprNodeFunc) {
 func VisitExprNode(n ExprNode, f VisitExprNodeFunc) {
 	visitExprNode(n, nil, f)
 }
+
+// FindParent applies predicate to each parent of this node until predicate returns true.
+// Then it returns result of predicate. If no parent found, returns nil, false.
+func FindParent[T ExprNode](n ExprNode, predicate func(n ExprNode) (T, bool)) (T, bool) {
+	parent := n.Parent()
+	for parent != nil {
+		t, ok := predicate(parent)
+		if ok {
+			return t, true
+		}
+		parent = parent.Parent()
+	}
+	var zero T
+	return zero, false
+}

expr_insecure.go

@@ -135,6 +135,15 @@ var BuiltinUntrustedInputs = UntrustedInputSearchRoots{
 	),
 }
 
+// Contains functions which are considered to be safe.
+// No script injection is possible if unsafe expression is used inside these functions
+// because they have stable determined output
+var safeFunctions = map[string]bool{
+	"contains":   true,
+	"startswith": true,
+	"endswith":   true,
+}
+
 // UntrustedInputChecker is a checker to detect untrusted inputs in an expression syntax tree.
 // This checker checks object property accesses, array index accesses, and object filters. And
 // detects paths to untrusted inputs. Found errors are stored in this instance and can be get via
@@ -292,8 +301,27 @@ func (u *UntrustedInputChecker) end() {
 	u.reset()
 }
 
+// IsFunctionSafe Checks if this function is safe in terms of script injection possibility when using unsafe args
+func (u *UntrustedInputChecker) IsFunctionSafe(name string) bool {
+	return safeFunctions[strings.ToLower(name)]
+}
+
 // OnVisitNodeLeave is a callback which should be called on visiting node after visiting its children.
 func (u *UntrustedInputChecker) OnVisitNodeLeave(n ExprNode) {
+	_, isInsideSafeFunctionCall := FindParent(n, func(n ExprNode) (*FuncCallNode, bool) {
+		if funcCall, ok := n.(*FuncCallNode); ok {
+			if u.IsFunctionSafe(funcCall.Callee) {
+				return funcCall, true
+			}
+		}
+		return nil, false
+	})
+
+	// Skip unsafe checks if we are inside of safe function call expression
+	if isInsideSafeFunctionCall {
+		return
+	}
+
 	switch n := n.(type) {
 	case *VariableNode:
 		u.end()

expr_insecure_test.go

@@ -192,20 +192,6 @@ func TestExprInsecureDetectUntrustedValue(t *testing.T) {
 				"github.event.pages.*.page_name",
 			},
 		},
-		testCase{
-			"contains(github.event.pages.*.page_name, github.event.issue.title)",
-			[]string{
-				"github.event.pages.*.page_name",
-				"github.event.issue.title",
-			},
-		},
-		testCase{
-			"contains(github.event.*.body, github.event.*.*)",
-			[]string{
-				"github.event.",
-				"github.event.",
-			},
-		},
 	)
 
 	for _, tc := range tests {
@@ -310,6 +296,14 @@ func TestExprInsecureNoUntrustedValue(t *testing.T) {
 		"matrix.foo[github.event.pages].page_name",
 		"github.event.issue.body.foo.bar",
 		"github.event.issue.body[0]",
+
+		"contains(github.event.issue.title, 'bug')",
+		"contains(github.event.issue.body, github.event.issue.title)",
+		"startsWith(github.event.comment.body, 'LGTM')",
+		"endsWith(github.event.pull_request.title, github.event.issue.title)",
+		"contains(github.event.*.body, 'sensitive')",
+		"startsWith(github.event.*.body[0], 'Important')",
+		"endsWith(github.event.commits[0].message, github.event.pull_request.title)",
 	}
 
 	for _, input := range inputs {