Merge pull request #459 from IlyaGulya/bugfix/false-positive-script-injection

Fix false-positive script injection warnings when using builtin functions with stable outputs

Author: rhysd

.gitignore

@@ -13,3 +13,4 @@
 /playground-dist
 /actionlint-workflow-ast
 /.git-hooks/.timestamp
+/.idea

expr_ast.go

@@ -5,83 +5,121 @@ package actionlint
 type ExprNode interface {
 	// Token returns the first token of the node. This method is useful to get position of this node.
 	Token() *Token
+	// Parent returns the parent node of this node.
+	Parent() ExprNode
 }
 
 // Variable
 
 // VariableNode is node for variable access.
 type VariableNode struct {
 	// Name is name of the variable
-	Name string
-	tok  *Token
+	Name   string
+	tok    *Token
+	parent ExprNode
 }
 
 // Token returns the first token of the node. This method is useful to get position of this node.
 func (n *VariableNode) Token() *Token {
 	return n.tok
 }
 
+// Parent returns the parent node of this node.
+func (n *VariableNode) Parent() ExprNode {
+	return n.parent
+}
+
 // Literals
 
 // NullNode is node for null literal.
 type NullNode struct {
-	tok *Token
+	tok    *Token
+	parent ExprNode
 }
 
 // Token returns the first token of the node. This method is useful to get position of this node.
 func (n *NullNode) Token() *Token {
 	return n.tok
 }
 
+// Parent returns the parent node of this node.
+func (n *NullNode) Parent() ExprNode {
+	return n.parent
+}
+
 // BoolNode is node for boolean literal, true or false.
 type BoolNode struct {
 	// Value is value of the boolean literal.
-	Value bool
-	tok   *Token
+	Value  bool
+	tok    *Token
+	parent ExprNode
 }
 
 // Token returns the first token of the node. This method is useful to get position of this node.
 func (n *BoolNode) Token() *Token {
 	return n.tok
 }
 
+// Parent returns the parent node of this node.
+func (n *BoolNode) Parent() ExprNode {
+	return n.parent
+}
+
 // IntNode is node for integer literal.
 type IntNode struct {
 	// Value is value of the integer literal.
-	Value int
-	tok   *Token
+	Value  int
+	tok    *Token
+	parent ExprNode
 }
 
 // Token returns the first token of the node. This method is useful to get position of this node.
 func (n *IntNode) Token() *Token {
 	return n.tok
 }
 
+// Parent returns the parent node of this node.
+func (n *IntNode) Parent() ExprNode {
+	return n.parent
+}
+
 // FloatNode is node for float literal.
 type FloatNode struct {
 	// Value is value of the float literal.
-	Value float64
-	tok   *Token
+	Value  float64
+	tok    *Token
+	parent ExprNode
 }
 
 // Token returns the first token of the node. This method is useful to get position of this node.
 func (n *FloatNode) Token() *Token {
 	return n.tok
 }
 
+// Parent returns the parent node of this node.
+func (n *FloatNode) Parent() ExprNode {
+	return n.parent
+}
+
 // StringNode is node for string literal.
 type StringNode struct {
 	// Value is value of the string literal. Escapes are resolved and quotes at both edges are
 	// removed.
-	Value string
-	tok   *Token
+	Value  string
+	tok    *Token
+	parent ExprNode
 }
 
 // Token returns the first token of the node. This method is useful to get position of this node.
 func (n *StringNode) Token() *Token {
 	return n.tok
 }
 
+// Parent returns the parent node of this node.
+func (n *StringNode) Parent() ExprNode {
+	return n.parent
+}
+
 // Operators
 
 // ObjectDerefNode represents property dereference of object like 'foo.bar'.
@@ -90,52 +128,76 @@ type ObjectDerefNode struct {
 	Receiver ExprNode
 	// Property is a name of property to access.
 	Property string
+	parent   ExprNode
 }
 
 // Token returns the first token of the node. This method is useful to get position of this node.
-func (n ObjectDerefNode) Token() *Token {
+func (n *ObjectDerefNode) Token() *Token {
 	return n.Receiver.Token()
 }
 
+// Parent returns the parent node of this node.
+func (n *ObjectDerefNode) Parent() ExprNode {
+	return n.parent
+}
+
 // ArrayDerefNode represents elements dereference of arrays like '*' in 'foo.bar.*.piyo'.
 type ArrayDerefNode struct {
 	// Receiver is an expression at receiver of array element dereference.
 	Receiver ExprNode
+	parent   ExprNode
 }
 
 // Token returns the first token of the node. This method is useful to get position of this node.
-func (n ArrayDerefNode) Token() *Token {
+func (n *ArrayDerefNode) Token() *Token {
 	return n.Receiver.Token()
 }
 
+// Parent returns the parent node of this node.
+func (n *ArrayDerefNode) Parent() ExprNode {
+	return n.parent
+}
+
 // IndexAccessNode is node for index access, which represents dynamic object property access or
 // array index access.
 type IndexAccessNode struct {
 	// Operand is an expression at operand of index access, which should be array or object.
 	Operand ExprNode
 	// Index is an expression at index, which should be integer or string.
-	Index ExprNode
+	Index  ExprNode
+	parent ExprNode
 }
 
 // Token returns the first token of the node. This method is useful to get position of this node.
 func (n *IndexAccessNode) Token() *Token {
 	return n.Operand.Token()
 }
 
+// Parent returns the parent node of this node.
+func (n *IndexAccessNode) Parent() ExprNode {
+	return n.parent
+}
+
 // Note: Currently only ! is a logical unary operator
 
 // NotOpNode is node for unary ! operator.
 type NotOpNode struct {
 	// Operand is an expression at operand of ! operator.
 	Operand ExprNode
 	tok     *Token
+	parent  ExprNode
 }
 
 // Token returns the first token of the node. This method is useful to get position of this node.
 func (n *NotOpNode) Token() *Token {
 	return n.tok
 }
 
+// Parent returns the parent node of this node.
+func (n *NotOpNode) Parent() ExprNode {
+	return n.parent
+}
+
 // CompareOpNodeKind is a kind of compare operators; ==, !=, <, <=, >, >=.
 type CompareOpNodeKind int
 
@@ -187,14 +249,20 @@ type CompareOpNode struct {
 	// Left is an expression for left hand side of the binary operator.
 	Left ExprNode
 	// Right is an expression for right hand side of the binary operator.
-	Right ExprNode
+	Right  ExprNode
+	parent ExprNode
 }
 
 // Token returns the first token of the node. This method is useful to get position of this node.
 func (n *CompareOpNode) Token() *Token {
 	return n.Left.Token()
 }
 
+// Parent returns the parent node of this node.
+func (n *CompareOpNode) Parent() ExprNode {
+	return n.parent
+}
+
 // LogicalOpNodeKind is a kind of logical operators; && and ||.
 type LogicalOpNodeKind int
 
@@ -225,30 +293,42 @@ type LogicalOpNode struct {
 	// Left is an expression for left hand side of the binary operator.
 	Left ExprNode
 	// Right is an expression for right hand side of the binary operator.
-	Right ExprNode
+	Right  ExprNode
+	parent ExprNode
 }
 
 // Token returns the first token of the node. This method is useful to get position of this node.
 func (n *LogicalOpNode) Token() *Token {
 	return n.Left.Token()
 }
 
+// Parent returns the parent node of this node.
+func (n *LogicalOpNode) Parent() ExprNode {
+	return n.parent
+}
+
 // FuncCallNode represents function call in expression.
 // Note that currently only calling builtin functions is supported.
 type FuncCallNode struct {
 	// Callee is a name of called function. This is string value because currently only built-in
 	// functions can be called.
 	Callee string
 	// Args is arguments of the function call.
-	Args []ExprNode
-	tok  *Token
+	Args   []ExprNode
+	tok    *Token
+	parent ExprNode
 }
 
 // Token returns the first token of the node. This method is useful to get position of this node.
 func (n *FuncCallNode) Token() *Token {
 	return n.tok
 }
 
+// Parent returns the parent node of this node.
+func (n *FuncCallNode) Parent() ExprNode {
+	return n.parent
+}
+
 // VisitExprNodeFunc is a visitor function for VisitExprNode(). The entering argument is set to
 // true when it is called before visiting children. It is set to false when it is called after
 // visiting children. It means that this function is called twice for the same node. The parent
@@ -275,8 +355,8 @@ func visitExprNode(n, p ExprNode, f VisitExprNodeFunc) {
 		visitExprNode(n.Left, n, f)
 		visitExprNode(n.Right, n, f)
 	case *FuncCallNode:
-		for _, a := range n.Args {
-			visitExprNode(a, n, f)
+		for i := range n.Args {
+			visitExprNode(n.Args[i], n, f)
 		}
 	}
 	f(n, p, false)
@@ -286,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,25 @@ 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 && 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

@@ -193,14 +193,14 @@ func TestExprInsecureDetectUntrustedValue(t *testing.T) {
 			},
 		},
 		testCase{
-			"contains(github.event.pages.*.page_name, github.event.issue.title)",
+			"fromJson(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.*.*)",
+			"fromJson(github.event.*.body, github.event.*.*)",
 			[]string{
 				"github.event.",
 				"github.event.",
@@ -310,6 +310,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 {