Add wildcard and test sensitivity to trafic annotation auditor.

Traffic annotation auditor checks that test annotationa are only used in
test files with whitelisted keywords in file path.
File path white list checking is updated to accept wildcards.

Bug: 690323
Change-Id: I5ccde0bb3d449fa75667bf1b293bd533fdfcd2c0
Reviewed-on: https://chromium-review.googlesource.com/947945Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543399}

net/traffic_annotation/network_traffic_annotation_test_helper.h

@@ -7,7 +7,7 @@
 
 #include "net/traffic_annotation/network_traffic_annotation.h"
 
-// Macro for unit tests traffic annotations.
+// Macros for tests traffic annotations.
 #define TRAFFIC_ANNOTATION_FOR_TESTS   \
   net::DefineNetworkTrafficAnnotation( \
       "test", "Traffic annotation for unit, browser and other tests")

tools/traffic_annotation/auditor/BUILD.gn

@@ -69,6 +69,7 @@ source_set("auditor_sources") {
     "//net:traffic_annotation",
     "//third_party/libxml",
     "//third_party/protobuf:protobuf_full",
+    "//third_party/re2",
   ]
 }
 

tools/traffic_annotation/auditor/DEPS

@@ -2,4 +2,5 @@ include_rules = [
   "+components/policy/proto",
   "+third_party/libxml",
   "+third_party/protobuf/src/google",
+  "+third_party/re2",
 ]

tools/traffic_annotation/auditor/auditor_result.cc

@@ -28,7 +28,8 @@ AuditorResult::AuditorResult(Type type,
          type == AuditorResult::Type::ERROR_MISSING_TAG_USED ||
          type == AuditorResult::Type::ERROR_NO_ANNOTATION ||
          type == AuditorResult::Type::ERROR_MISSING_SECOND_ID ||
-         type == AuditorResult::Type::ERROR_DIRECT_ASSIGNMENT);
+         type == AuditorResult::Type::ERROR_DIRECT_ASSIGNMENT ||
+         type == AuditorResult::Type::ERROR_TEST_ANNOTATION);
   if (!message.empty())
     details_.push_back(message);
 };
@@ -163,6 +164,10 @@ std::string AuditorResult::ToText() const {
           "files (like jumbo), rerun the auditor using --all-files switch.",
           details_[0].c_str());
 
+    case AuditorResult::Type::ERROR_TEST_ANNOTATION:
+      return base::StringPrintf("Annotation for tests is used in '%s:%i'.",
+                                file_path_.c_str(), line_);
+
     default:
       return std::string();
   }

tools/traffic_annotation/auditor/auditor_result.h

@@ -43,7 +43,8 @@ class AuditorResult {
     ERROR_DIRECT_ASSIGNMENT,       // A value is directly assigned to a mutable
                                    // annotation or annotation instialized with
                                    // list expresssion.
-    ERROR_ANNOTATIONS_XML_UPDATE   // Annotations XML requires update.
+    ERROR_ANNOTATIONS_XML_UPDATE,  // Annotations XML requires update.
+    ERROR_TEST_ANNOTATION,         // Annotation for tests is used.
   };
 
   static const int kNoCodeLineSpecified;

tools/traffic_annotation/auditor/instance.cc

@@ -164,7 +164,8 @@ AuditorResult AnnotationInstance::Deserialize(
   if (unique_id_hash_code == TRAFFIC_ANNOTATION_FOR_TESTS.unique_id_hash_code ||
       unique_id_hash_code ==
           PARTIAL_TRAFFIC_ANNOTATION_FOR_TESTS.unique_id_hash_code) {
-    return AuditorResult(AuditorResult::Type::RESULT_IGNORE);
+    return AuditorResult(AuditorResult::Type::ERROR_TEST_ANNOTATION, "",
+                         file_path, line_number);
   }
 
   // Process undefined tags.

tools/traffic_annotation/auditor/safe_list.txt

@@ -2,7 +2,9 @@
 # anntotation auditor. Refer to AuditorException::ExceptionType in
 # 'tools/traffic_annotation/auditor/traffic_annotation_auditor.h' for types of
 # exceptions.
-all,tools
+# Use * as wildcard for zero or more characters.
+all,tools/*
 missing,net/url_request/url_fetcher.cc
 missing,net/url_request/url_request_context.cc
-direct_assignment,download::ProtoConversions::EntryFromProto@components/download/internal/background_service/proto_conversions.cc
+direct_assignment,download::ProtoConversions::EntryFromProto@components/download/internal/background_service/proto_conversions.cc
\ No newline at end of file
+test_annotation,*test*,*fuzzer*,*mock*,*fake*,net/quic/chromium/quic_chromium_client_session_peer.cc
\ No newline at end of file

tools/traffic_annotation/auditor/traffic_annotation_auditor.cc

@@ -18,6 +18,7 @@
 #include "build/build_config.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "third_party/re2/src/re2/re2.h"
 #include "tools/traffic_annotation/auditor/traffic_annotation_file_filter.h"
 #include "tools/traffic_annotation/auditor/traffic_annotation_id_checker.h"
 
@@ -307,8 +308,8 @@ bool TrafficAnnotationAuditor::IsSafeListed(
   const std::vector<std::string>& safe_list =
       safe_list_[static_cast<int>(exception_type)];
 
-  for (const std::string& ignore_path : safe_list) {
-    if (!strncmp(file_path.c_str(), ignore_path.c_str(), ignore_path.length()))
+  for (const std::string& ignore_pattern : safe_list) {
+    if (re2::RE2::FullMatch(file_path, ignore_pattern))
       return true;
   }
 
@@ -365,12 +366,24 @@ bool TrafficAnnotationAuditor::ParseClangToolRawOutput() {
     if (block_type == "ANNOTATION") {
       AnnotationInstance new_annotation;
       result = new_annotation.Deserialize(lines, current, end_line);
-      if (result.IsOK()) {
-        extracted_annotations_.push_back(new_annotation);
-      } else if (result.type() == AuditorResult::Type::ERROR_MISSING_TAG_USED &&
-                 IsSafeListed(result.file_path(),
-                              AuditorException::ExceptionType::MISSING)) {
-        result = AuditorResult(AuditorResult::Type::RESULT_IGNORE);
+      switch (result.type()) {
+        case AuditorResult::Type::RESULT_OK:
+          extracted_annotations_.push_back(new_annotation);
+          break;
+        case AuditorResult::Type::ERROR_MISSING_TAG_USED:
+          if (IsSafeListed(result.file_path(),
+                           AuditorException::ExceptionType::MISSING)) {
+            result = AuditorResult(AuditorResult::Type::RESULT_IGNORE);
+          }
+          break;
+        case AuditorResult::Type::ERROR_TEST_ANNOTATION:
+          if (IsSafeListed(result.file_path(),
+                           AuditorException::ExceptionType::TEST_ANNOTATION)) {
+            result = AuditorResult(AuditorResult::Type::RESULT_IGNORE);
+          }
+          break;
+        default:
+          break;
       }
     } else if (block_type == "CALL") {
       CallInstance new_call;
@@ -418,37 +431,56 @@ bool TrafficAnnotationAuditor::ParseClangToolRawOutput() {
 bool TrafficAnnotationAuditor::LoadSafeList() {
   base::FilePath safe_list_file =
       base::MakeAbsoluteFilePath(source_path_.Append(kSafeListPath));
+
   std::string file_content;
-  if (base::ReadFileToString(safe_list_file, &file_content)) {
-    base::RemoveChars(file_content, "\r", &file_content);
-    std::vector<std::string> lines = base::SplitString(
-        file_content, "\n", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
-    for (const std::string& line : lines) {
-      // Ignore comments and empty lines.
-      if (!line.length() || line[0] == '#')
-        continue;
-      size_t comma = line.find(',');
-      if (comma == std::string::npos) {
-        LOG(ERROR) << "Unexpected syntax in safe_list.txt, line: " << line;
-        return false;
-      }
+  if (!base::ReadFileToString(safe_list_file, &file_content)) {
+    LOG(ERROR) << "Could not read " << kSafeListPath.MaybeAsASCII();
+    return false;
+  }
 
-      AuditorException::ExceptionType exception_type;
-      if (AuditorException::TypeFromString(line.substr(0, comma),
-                                           &exception_type)) {
-        safe_list_[static_cast<int>(exception_type)].push_back(
-            line.substr(comma + 1, line.length() - comma - 1));
-      } else {
-        LOG(ERROR) << "Unexpected type in safe_list.txt line: " << line;
+  base::RemoveChars(file_content, "\r", &file_content);
+  std::vector<std::string> lines = base::SplitString(
+      file_content, "\n", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
+
+  for (const std::string& line : lines) {
+    // Ignore comments and empty lines.
+    if (!line.length() || line[0] == '#')
+      continue;
+
+    std::vector<std::string> tokens = base::SplitString(
+        line, ",", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
+
+    // Expect a type and at least one value in each line.
+    if (tokens.size() < 2) {
+      LOG(ERROR) << "Unexpected syntax in safe_list.txt, line: " << line;
+      return false;
+    }
+
+    AuditorException::ExceptionType exception_type;
+    if (!AuditorException::TypeFromString(tokens[0], &exception_type)) {
+      LOG(ERROR) << "Unexpected type in safe_list.txt line: " << line;
+      return false;
+    }
+    for (unsigned i = 1; i < tokens.size(); i++) {
+      // Convert the rest of the line into re2 patterns, making dots as fixed
+      // characters and asterisks as wildcards.
+      // Note that all file paths are converted to Linux style before checking.
+      if (!base::ContainsOnlyChars(
+              base::ToLowerASCII(tokens[i]),
+              "0123456789_abcdefghijklmnopqrstuvwxyz.*/:@")) {
+        LOG(ERROR) << "Unexpected character in safe_list.txt token: "
+                   << tokens[i];
         return false;
       }
+      std::string pattern;
+      base::ReplaceChars(tokens[i], ".", "[.]", &pattern);
+      base::ReplaceChars(pattern, "*", ".*", &pattern);
+      safe_list_[static_cast<int>(exception_type)].push_back(pattern);
     }
-    safe_list_loaded_ = true;
-    return true;
   }
 
-  LOG(ERROR) << "Could not read " << kSafeListPath.MaybeAsASCII();
-  return false;
+  safe_list_loaded_ = true;
+  return true;
 }
 
 // static

tools/traffic_annotation/auditor/traffic_annotation_auditor.h

@@ -20,7 +20,8 @@ struct AuditorException {
     ALL,                // Ignore all errors (doesn't check the files at all).
     MISSING,            // Ignore missing annotations.
     DIRECT_ASSIGNMENT,  // Ignore direct assignment of annotation value.
-    EXCEPTION_TYPE_LAST = DIRECT_ASSIGNMENT
+    TEST_ANNOTATION,    // Ignore usages of annotation for tests.
+    EXCEPTION_TYPE_LAST = TEST_ANNOTATION
   } type;
 
   static bool TypeFromString(const std::string& type_string,
@@ -31,6 +32,8 @@ struct AuditorException {
       *type_value = ExceptionType::MISSING;
     } else if (type_string == "direct_assignment") {
       *type_value = ExceptionType::DIRECT_ASSIGNMENT;
+    } else if (type_string == "test_annotation") {
+      *type_value = ExceptionType::TEST_ANNOTATION;
     } else {
       return false;
     }

tools/traffic_annotation/auditor/traffic_annotation_auditor_unittest.cc

@@ -272,6 +272,18 @@ TEST_F(TrafficAnnotationAuditorTest, IsSafeListed) {
                                      AuditorException::ExceptionType::MISSING));
   EXPECT_TRUE(auditor().IsSafeListed("net/url_request/url_request_context.cc",
                                      AuditorException::ExceptionType::MISSING));
+
+  // Files having the word test in their full path can have annotation for
+  // tests.
+  EXPECT_FALSE(
+      auditor().IsSafeListed("net/url_request/url_fetcher.cc",
+                             AuditorException::ExceptionType::TEST_ANNOTATION));
+  EXPECT_TRUE(
+      auditor().IsSafeListed("chrome/browser/test_something.cc",
+                             AuditorException::ExceptionType::TEST_ANNOTATION));
+  EXPECT_TRUE(
+      auditor().IsSafeListed("test/send_something.cc",
+                             AuditorException::ExceptionType::TEST_ANNOTATION));
 }
 
 // Tests if annotation instances are corrrectly deserialized.
@@ -292,7 +304,7 @@ TEST_F(TrafficAnnotationAuditorTest, AnnotationDeserialization) {
        AnnotationInstance::Type::ANNOTATION_COMPLETING},
       {"good_partial_annotation.txt", AuditorResult::Type::RESULT_OK,
        AnnotationInstance::Type::ANNOTATION_PARTIAL},
-      {"good_test_annotation.txt", AuditorResult::Type::RESULT_IGNORE},
+      {"good_test_annotation.txt", AuditorResult::Type::ERROR_TEST_ANNOTATION},
       {"missing_annotation.txt", AuditorResult::Type::ERROR_MISSING_TAG_USED},
       {"no_annotation.txt", AuditorResult::Type::ERROR_NO_ANNOTATION},
       {"fatal_annotation1.txt", AuditorResult::Type::ERROR_FATAL},
@@ -309,7 +321,7 @@ TEST_F(TrafficAnnotationAuditorTest, AnnotationDeserialization) {
     AnnotationInstance annotation;
     AuditorResult::Type result_type =
         Deserialize(test_case.file_name, &annotation);
-    EXPECT_EQ(result_type, test_case.result_type);
+    EXPECT_EQ(result_type, test_case.result_type) << test_case.file_name;
 
     if (result_type == AuditorResult::Type::RESULT_OK)
       EXPECT_EQ(annotation.type, test_case.type);

tools/traffic_annotation/auditor/traffic_annotation_file_filter.cc

@@ -16,6 +16,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
+#include "third_party/re2/src/re2/re2.h"
 
 namespace {
 
@@ -142,9 +143,8 @@ void TrafficAnnotationFileFilter::GetRelevantFiles(
   for (const std::string& file_path : git_files_) {
     if (!strncmp(file_path.c_str(), directory_name.c_str(), name_length)) {
       bool ignore = false;
-      for (const std::string& ignore_path : ignore_list) {
-        if (!strncmp(file_path.c_str(), ignore_path.c_str(),
-                     ignore_path.length())) {
+      for (const std::string& ignore_pattern : ignore_list) {
+        if (re2::RE2::FullMatch(file_path.c_str(), ignore_pattern)) {
           ignore = true;
           break;
         }