Copilot commented on code in PR #13024:
URL: https://github.com/apache/trafficserver/pull/13024#discussion_r3010491260
##########
plugins/header_rewrite/matcher.h:
##########
@@ -209,7 +209,7 @@ template <class T> class Matchers : public Matcher
field.ltrim_if(&isspace).rtrim_if(&isspace);
values.insert(convert(std::string(field)));
- start = ++cur + skip_quotes;
+ start = ++cur;
skip_quotes = 0;
Review Comment:
Changing `start = ++cur` fixes the truncation for adjacent quoted items
("foo","bar"), but it breaks common inputs where there is whitespace after the
comma before the next quote (e.g. `"foo", "bar"`). With a space present,
`skip_quotes` ends up skipping the space instead of the opening quote, and the
last-field extraction can drop the closing quote, yielding a wrong set entry.
Consider reworking MATCH_SET parsing to (1) split on commas outside quotes, (2)
trim whitespace on each field, then (3) strip surrounding quotes if present; or
explicitly skip whitespace after the comma before applying quote-skipping logic.
##########
plugins/header_rewrite/matcher_tests.cc:
##########
@@ -106,3 +117,16 @@ TEST_CASE("MatcherSet", "[plugins][header_rewrite]")
foo.set("foo, bar, baz", CondModifiers::MOD_NOCASE);
REQUIRE(foo.test("FOO", res) == true);
}
+
+TEST_CASE("MatcherSetQuoted", "[plugins][header_rewrite]")
+{
+ Matchers<std::string> foo(MATCH_SET);
+ TSHttpTxn txn = nullptr;
+ TSCont c = nullptr;
+ Resources res(txn, c);
+
+ foo.set("\"foo\",\"bar\"", CondModifiers::MOD_NOCASE);
+ REQUIRE(foo.test("FOO", res) == true);
+ REQUIRE(foo.test("BAR", res) == true);
+ REQUIRE(foo.test("BAZ", res) == false);
Review Comment:
The new quoted-set test only covers the no-whitespace form (`"foo","bar"`).
Since config files commonly allow spaces after commas (e.g. `"foo", "bar"`) and
mixed quoted/unquoted entries, it would be good to add assertions covering
those forms as well so the MATCH_SET parser behavior is locked in (and to catch
regressions like whitespace handling).
```suggestion
// No-whitespace quoted set.
foo.set("\"foo\",\"bar\"", CondModifiers::MOD_NOCASE);
REQUIRE(foo.test("FOO", res) == true);
REQUIRE(foo.test("BAR", res) == true);
REQUIRE(foo.test("BAZ", res) == false);
// Quoted set with whitespace after commas.
Matchers<std::string> foo_ws(MATCH_SET);
foo_ws.set("\"foo\", \"bar\"", CondModifiers::MOD_NOCASE);
REQUIRE(foo_ws.test("FOO", res) == true);
REQUIRE(foo_ws.test("BAR", res) == true);
REQUIRE(foo_ws.test("BAZ", res) == false);
// Mixed quoted and unquoted entries with whitespace.
Matchers<std::string> foo_mixed(MATCH_SET);
foo_mixed.set("\"foo\", bar, \"baz\"", CondModifiers::MOD_NOCASE);
REQUIRE(foo_mixed.test("FOO", res) == true);
REQUIRE(foo_mixed.test("BAR", res) == true);
REQUIRE(foo_mixed.test("BAZ", res) == true);
REQUIRE(foo_mixed.test("QUX", res) == false);
```
##########
plugins/header_rewrite/CMakeLists.txt:
##########
@@ -99,11 +99,15 @@ if(BUILD_TESTING)
endif()
endif()
- # This test has linker issue when cripts is enabled, so its commented for now
- # add_executable(test_matcher matcher_tests.cc matcher.cc lulu.cc
regex_helper.cc resources.cc)
- # add_catch2_test(NAME test_matcher COMMAND $<TARGET_FILE:test_matcher>)
+ # add_executable(test_matcher matcher_tests.cc matcher.cc lulu.cc
regex_helper.cc)
+ # add_catch2_test(NAME test_matcher COMMAND $<TARGET_FILE:test_matcher>)
#
- # target_link_libraries(test_matcher PRIVATE Catch2::Catch2WithMain
ts::tscore libswoc::libswoc PkgConfig::PCRE2)
+ # target_link_libraries(test_matcher PRIVATE Catch2::Catch2WithMain
ts::tscore libswoc::libswoc PkgConfig::PCRE2)
+ #
+ # if(ENABLE_CRIPTS)
+ # target_link_libraries(test_matcher PRIVATE ts::cripts)
+ # target_compile_definitions(test_matcher PRIVATE TS_HAS_CRIPTS=1)
+ # endif()
Review Comment:
The PR description says the `test_matcher` build target is being enabled,
but in this CMakeLists the target is still entirely commented out (only the
commented text was edited). If the intent is to actually enable it under
BUILD_TESTING, these lines should be uncommented and any required link
libraries/compile defs applied (including the ENABLE_CRIPTS handling).
##########
tests/gold_tests/pluginTest/header_rewrite/rules/rule_client.conf:
##########
@@ -39,3 +39,9 @@ elif
set-header X-Testing "elif"
else
set-header X-Testing "No"
+
+cond %{SEND_RESPONSE_HDR_HOOK}
+cond %{CLIENT-HEADER:X-Quoted-Set} ("foo","bar")
Review Comment:
The new quoted-set condition uses the no-whitespace form `( "foo","bar" )`.
To exercise the more typical config formatting, consider adding coverage for
whitespace after commas as well (e.g. `("foo", "bar")`), either by adjusting
this rule or adding an additional rule, so the parsing fix is validated against
real-world formatting.
```suggestion
cond %{CLIENT-HEADER:X-Quoted-Set} ("foo", "bar")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
