diff --git a/plugins/header_rewrite/header_rewrite_test.cc b/plugins/header_rewrite/header_rewrite_test.cc index 7ec83230a36..f742baeb060 100644 --- a/plugins/header_rewrite/header_rewrite_test.cc +++ b/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; } diff --git a/plugins/header_rewrite/parser.cc b/plugins/header_rewrite/parser.cc index c1f467d39a8..6bf81b793f3 100644 --- a/plugins/header_rewrite/parser.cc +++ b/plugins/header_rewrite/parser.cc @@ -171,11 +171,12 @@ Parser::preprocess(std::vector 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 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; diff --git a/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.replay.yaml b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.replay.yaml new file mode 100644 index 00000000000..bd0b95968b0 --- /dev/null +++ b/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 } ] diff --git a/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.test.py b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_orphan_modifiers.test.py new file mode 100644 index 00000000000..7f2c7073c3d --- /dev/null +++ b/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",) diff --git a/tests/gold_tests/pluginTest/header_rewrite/orphan_modifiers.conf b/tests/gold_tests/pluginTest/header_rewrite/orphan_modifiers.conf new file mode 100644 index 00000000000..90a72ec3a02 --- /dev/null +++ b/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