header_rewrite: reject a modifier-only config line

plugins/header_rewrite/header_rewrite_test.cc

@@ -58,7 +58,7 @@ class ParserTest : public Parser
 public:
   ParserTest(const std::string &line) : res(true)
   {
-    Parser::parse_line(line);
+    parse_succeeded = Parser::parse_line(line);
     std::cout << "Finished parser test: " << line << std::endl;
   }
 
@@ -79,6 +79,7 @@ class ParserTest : public Parser
   }
 
   bool res;
+  bool parse_succeeded = false;
 };
 
 class SimpleTokenizerTest : public HRWSimpleTokenizer
@@ -436,6 +437,22 @@ test_parsing()
     END_TEST();
   }
 
+  { /* modifiers with no condition or operator to attach them to */
+    ParserTest p("[L]");
+
+    CHECK_EQ(p.parse_succeeded, false);
+
+    END_TEST();
+  }
+
+  { /* same, but long enough that the token is heap allocated rather than SSO */
+    ParserTest p("[AND,NOCASE,NOT,L,QSA,I,EXT,PRE]");
+
+    CHECK_EQ(p.parse_succeeded, false);
+
+    END_TEST();
+  }
+
   return errors;
 }
 

plugins/header_rewrite/parser.cc

@@ -171,11 +171,12 @@ Parser::preprocess(std::vector<std::string> tokens)
 {
   // The last token might be the "flags" section, lets consume it if it is
   if (tokens.size() > 0) {
-    std::string m = tokens[tokens.size() - 1];
+    const std::string flags = tokens[tokens.size() - 1];
+
+    if (!flags.empty() && (flags[0] == '[')) {
+      if (flags[flags.size() - 1] == ']') {
+        std::string m = flags.substr(1, flags.size() - 2);
 
-    if (!m.empty() && (m[0] == '[')) {
-      if (m[m.size() - 1] == ']') {
-        m = m.substr(1, m.size() - 2);
         if (m.find_first_of(',') != std::string::npos) {
           std::istringstream iss(m);
           std::string        t;
@@ -192,6 +193,11 @@ Parser::preprocess(std::vector<std::string> tokens)
           _mods.push_back(m);
         }
         tokens.pop_back(); // consume it, so we don't concatenate it into the value
+
+        if (tokens.empty()) {
+          TSError("[%s] modifiers must follow a condition or operator: %s", PLUGIN_NAME, flags.c_str());
+          return false;
+        }
       } else {
         TSError("[%s] mods have to be enclosed in []", PLUGIN_NAME);
         return false;

tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.replay.yaml

@@ -0,0 +1,89 @@
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you under the Apache License, Version 2.0 (the
+#  "License"); you may not use this file except in compliance
+#  with the License.  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+meta:
+  version: "1.0"
+
+autest:
+  description: 'Verify header_rewrite rejects a modifier-only config line'
+
+  server:
+    name: 'server'
+
+  client:
+    name: 'client'
+
+  ats:
+    name: 'ts'
+
+    process_config:
+      # The rejected config lines are reported with TSError, which lands in
+      # diags.log as an ERROR. That is the expected behavior here.
+      disable_log_checks: true
+
+    copy_to_config_dir:
+      - 'orphan_modifiers.conf'
+
+    records_config:
+      proxy.config.diags.debug.enabled: 1
+      proxy.config.diags.debug.tags: 'header_rewrite'
+
+    plugin_config:
+      - 'header_rewrite.so orphan_modifiers.conf'
+
+    remap_config:
+      - from: "http://www.example.com/"
+        to: "http://127.0.0.1:{SERVER_HTTP_PORT}/"
+
+    log_validation:
+      diags_log:
+        contains:
+          - expression: 'modifiers must follow a condition or operator: \[L\]'
+            description: 'header_rewrite must reject the short modifier-only line'
+          - expression: 'modifiers must follow a condition or operator: \[AND,NOCASE,NOT,L,QSA,I,EXT,PRE\]'
+            description: 'header_rewrite must reject the long modifier-only line'
+
+sessions:
+- transactions:
+
+  #############################################################################
+  # ATS came up despite the bad lines, and the valid rule that follows them
+  # still fires.
+  #############################################################################
+  - client-request:
+      method: "GET"
+      version: "1.1"
+      url: /orphan_modifiers/
+      headers:
+        fields:
+        - [ Host, www.example.com ]
+        - [ uuid, orphan-modifiers ]
+
+    server-response:
+      status: 200
+      reason: OK
+      headers:
+        fields:
+        - [ Content-Length, "3" ]
+      content:
+        encoding: plain
+        data: xxx
+
+    proxy-response:
+      status: 200
+      headers:
+        fields:
+        - [ X-Orphan-Modifiers-Survived, { value: "yes", as: equal } ]

tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.test.py

@@ -0,0 +1,28 @@
+'''
+Test that header_rewrite rejects a config line that is nothing but modifiers.
+'''
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you under the Apache License, Version 2.0 (the
+#  "License"); you may not use this file except in compliance
+#  with the License.  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+Test.Summary = '''
+A header_rewrite line holding only a modifier section, such as "[L]", leaves the
+parser with no tokens once the section is consumed. Verify that it is rejected
+rather than read out of bounds.
+'''
+
+Test.SkipUnless(Condition.PluginExists('header_rewrite.so'))
+
+Test.ATSReplayTest(replay_file="header_rewrite_orphan_modifiers.replay.yaml",)

tests/gold_tests/pluginTest/header_rewrite/orphan_modifiers.conf

@@ -0,0 +1,30 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A modifier section on a line of its own has no condition or operator to
+# attach to, so consuming it leaves the parser with an empty token list. Such a
+# line must be rejected and skipped.
+[L]
+
+# A longer one, so the token is heap allocated rather than living in the
+# std::string small-string buffer.
+[AND,NOCASE,NOT,L,QSA,I,EXT,PRE]
+
+# The surrounding rules must still load and fire, proving the bad line was
+# skipped rather than aborting the whole config.
+cond %{SEND_RESPONSE_HDR_HOOK}
+  set-header X-Orphan-Modifiers-Survived yes