Repository: VKCOM/noverify
Pull request #928 (2021-02-19 11:15:40)

linter: add stripTags checker
Reports suspicious usage of `string_tags()` function.
More precisely, it checks that `$allowed_tags` argument is valid.

There are many ways to use this function incorrectly.
This checker tries to find the most common mistakes.


Patch:
diff --git a/src/linter/block_linter.go b/src/linter/block_linter.go
index 6e40520..95d7573 100644
--- a/src/linter/block_linter.go
+++ b/src/linter/block_linter.go
@@ -790,6 +790,11 @@ func (b *blockLinter) checkFunctionCall(e *ir.FunctionCallExpr) {
 	}
 
 	switch fqName {
+	case `\strip_tags`:
+		if len(e.Args) < 2 {
+			break
+		}
+		b.checkStripTags(e)
 	case `\preg_match`, `\preg_match_all`, `\preg_replace`, `\preg_split`:
 		if len(e.Args) < 1 {
 			break
@@ -804,6 +809,74 @@ func (b *blockLinter) checkFunctionCall(e *ir.FunctionCallExpr) {
 	}
 }
 
+func (b *blockLinter) checkStripTags(e *ir.FunctionCallExpr) {
+	reportArg := func(n ir.Node, format string, args ...interface{}) {
+		message := fmt.Sprintf(format, args...)
+		b.report(n, LevelWarning, "stripTags", "$allowed_tags argument: "+message)
+	}
+
+	normalizeTag := func(s string) string {
+		s = strings.ReplaceAll(s, " ", "")
+		if strings.HasSuffix(s, "/>") {
+			s = strings.TrimSuffix(s, "/>") + ">"
+		}
+		s = strings.ToLower(s)
+		return s
+	}
+
+	set := make(map[string]string)
+	addTag := func(n ir.Node, tag string) {
+		normalized := normalizeTag(tag)
+		if prev := set[normalized]; prev != "" {
+			if prev == tag {
+				reportArg(n, "tag '%s' is duplicated", tag)
+			} else {
+				reportArg(n, "tag '%s' is duplicated, previously spelled as '%s'", tag, prev)
+			}
+		} else {
+			set[normalized] = tag
+		}
+	}
+
+	switch allowed := e.Arg(1).Expr.(type) {
+	case *ir.ArrayExpr:
+		for _, item := range allowed.Items {
+			literal, ok := item.Val.(*ir.String)
+			if !ok {
+				continue
+			}
+			s := strings.TrimSpace(literal.Value)
+			if strings.HasPrefix(s, "<") {
+				reportArg(item.Val, "'<' and '>' are not needed for tags when using array argument")
+			}
+			addTag(literal, literal.Value)
+		}
+	case *ir.String:
+		s := allowed.Value
+		if strings.ContainsAny(s, `'"`) {
+			reportArg(allowed, "using values/attrs is an error; they make matching always fail")
+			break
+		}
+		for {
+			s = strings.TrimLeft(s, " \n\r\t")
+			end := strings.IndexByte(s, '>')
+			if end == -1 {
+				break
+			}
+			tag := s[:end+1]
+			if strings.HasSuffix(tag, "/>") {
+				fixed := strings.TrimSuffix(tag, "/>") + ">"
+				reportArg(allowed, "'%s' should be written as '%s'", tag, fixed)
+			}
+			if strings.Contains(tag, " ") {
+				reportArg(allowed, "tag '%s' should not contain spaces", tag)
+			}
+			addTag(allowed, tag)
+			s = s[end+1:]
+		}
+	}
+}
+
 func (b *blockLinter) checkMethodCall(e *ir.MethodCallExpr) {
 	parseState := b.classParseState()
 
diff --git a/src/linter/report.go b/src/linter/report.go
index e6a3db2..e2f8b51 100644
--- a/src/linter/report.go
+++ b/src/linter/report.go
@@ -16,6 +16,15 @@ const (
 
 func addBuiltinCheckers(reg *CheckersRegistry) {
 	allChecks := []CheckerInfo{
+		{
+			Name:     "stripTags",
+			Default:  true,
+			Quickfix: false,
+			Comment:  `Report invalid strip_tags function usage.`,
+			Before:   `$s = strip_tags($s, '<br/>')`,
+			After:    `$s = strip_tags($s, '<br>')`,
+		},
+
 		{
 			Name:     "emptyStmt",
 			Default:  true,


Test patch:
diff --git a/src/tests/checkers/stripTags_test.go b/src/tests/checkers/stripTags_test.go
new file mode 100644
index 0000000..3ad6a78
--- /dev/null
+++ b/src/tests/checkers/stripTags_test.go
@@ -0,0 +1,71 @@
+package checkers_test
+
+import (
+	"testing"
+
+	"github.com/VKCOM/noverify/src/linttest"
+)
+
+func TestStripTagsGood(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.LoadStubs = []string{"stubs/phpstorm-stubs/standard/standard_1.php"}
+	test.AddFile(`<?php
+function f(string $s) {
+  $_ = strip_tags($s, ['br', 'a']);
+  $_ = strip_tags($s, ['BR']);
+
+  $_ = strip_tags($s, ' <a><br><html> ');
+  $_ = strip_tags($s, '<br>');
+}
+`)
+	test.RunAndMatch()
+}
+
+func TestStripTagsArray(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.LoadStubs = []string{"stubs/phpstorm-stubs/standard/standard_1.php"}
+	test.AddFile(`<?php
+function f(string $s) {
+  return strip_tags($s, ['<br>', 'a', 'A']);
+}
+`)
+	test.Expect = []string{
+		`$allowed_tags argument: '<' and '>' are not needed for tags when using array argument`,
+		`$allowed_tags argument: tag 'A' is duplicated, previously spelled as 'a'`,
+	}
+	test.RunAndMatch()
+}
+
+func TestStripTagsString1(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.LoadStubs = []string{"stubs/phpstorm-stubs/standard/standard_1.php"}
+	test.AddFile(`<?php
+function f1(string $s) {
+  return strip_tags($s, '<a href="#">');
+}
+function f2(string $s) {
+  return strip_tags($s, "<a href='#'>");
+}
+`)
+	test.Expect = []string{
+		`$allowed_tags argument: using values/attrs is an error; they make matching always fail`,
+		`$allowed_tags argument: using values/attrs is an error; they make matching always fail`,
+	}
+	test.RunAndMatch()
+}
+
+func TestStripTagsString2(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.LoadStubs = []string{"stubs/phpstorm-stubs/standard/standard_1.php"}
+	test.AddFile(`<?php
+function f(string $s) {
+  return strip_tags($s, '<a ><br/><br>');
+}
+`)
+	test.Expect = []string{
+		`$allowed_tags argument: tag '<a >' should not contain spaces`,
+		`$allowed_tags argument: '<br/>' should be written as '<br>'`,
+		`$allowed_tags argument: tag '<br>' is duplicated, previously spelled as '<br/>'`,
+	}
+	test.RunAndMatch()
+}