Copilot commented on code in PR #13160:
URL: https://github.com/apache/trafficserver/pull/13160#discussion_r3304824093
##########
plugins/experimental/maxmind_acl/mmdb.cc:
##########
@@ -429,6 +434,38 @@ Acl::parseregex(const YAML::Node ®ex, bool allow)
}
}
+void
+Acl::loadbypass(const YAML::Node &bypassNode)
+{
+ if (bypassNode.IsNull()) {
+ Dbg(dbg_ctl, "No bypass set");
+ return;
+ }
+
+ try {
+ if (bypassNode["header"]) {
+ if (!bypassNode["value"]) {
+ TSWarning("[%s] bypass 'header' set without 'value' — bypass disabled;
both are required", PLUGIN_NAME);
+ return;
+ }
+ _bypass_header_value = bypassNode["value"].as<std::string>();
+ if (_bypass_header_value.empty()) {
+ TSWarning("[%s] bypass 'value' is empty — bypass disabled; a non-empty
value is required", PLUGIN_NAME);
+ return;
+ }
+ _bypass_header = bypassNode["header"].as<std::string>();
Review Comment:
`loadbypass()` treats YAML null values as valid scalars (e.g. `value: ~`
becomes the string "null" in yaml-cpp). That would enable bypass matching on
the literal value "null", which contradicts the doc requirement that both
header and value be non-empty and can lead to surprising/unsafe bypass behavior
on misconfiguration. Consider explicitly rejecting `header`/`value` nodes that
are null or non-scalar (warn and leave bypass disabled).
##########
plugins/experimental/maxmind_acl/mmdb.cc:
##########
@@ -429,6 +434,38 @@ Acl::parseregex(const YAML::Node ®ex, bool allow)
}
}
+void
+Acl::loadbypass(const YAML::Node &bypassNode)
+{
+ if (bypassNode.IsNull()) {
+ Dbg(dbg_ctl, "No bypass set");
+ return;
+ }
+
+ try {
+ if (bypassNode["header"]) {
+ if (!bypassNode["value"]) {
+ TSWarning("[%s] bypass 'header' set without 'value' — bypass disabled;
both are required", PLUGIN_NAME);
+ return;
+ }
+ _bypass_header_value = bypassNode["value"].as<std::string>();
+ if (_bypass_header_value.empty()) {
+ TSWarning("[%s] bypass 'value' is empty — bypass disabled; a non-empty
value is required", PLUGIN_NAME);
+ return;
+ }
+ _bypass_header = bypassNode["header"].as<std::string>();
+ Dbg(dbg_ctl, "bypass header set to: %s", _bypass_header.c_str());
+ Dbg(dbg_ctl, "bypass value is configured");
+ } else {
+ Dbg(dbg_ctl, "bypass missing 'header' key");
+ return;
+ }
Review Comment:
`loadbypass()` only logs with `Dbg()` when the `bypass` node exists but is
malformed (e.g. `bypass: {}` / missing `header`), which means misconfiguration
may be silent unless debug is enabled. Since this bypass disables all ACL
enforcement, consider using `TSWarning` consistently for any invalid bypass
configuration (missing/empty header, missing/empty value, wrong node types) so
operators see it in the error log.
##########
plugins/experimental/maxmind_acl/mmdb.cc:
##########
@@ -503,6 +540,41 @@ Acl::loaddb(const YAML::Node &dbNode)
return true;
}
+bool
+Acl::check_bypass(TSHttpTxn txnp) const
+{
+ if (_bypass_header.empty()) {
+ return false;
+ }
+
+ TSMBuffer mbuf;
+ TSMLoc hdr_loc;
+ if (TS_SUCCESS != TSHttpTxnClientReqGet(txnp, &mbuf, &hdr_loc)) {
+ Dbg(dbg_ctl, "check_bypass: failed to get client request headers");
+ return false;
+ }
+
+ TSMLoc field_loc = TSMimeHdrFieldFind(mbuf, hdr_loc, _bypass_header.c_str(),
static_cast<int>(_bypass_header.size()));
+ if (TS_NULL_MLOC == field_loc) {
+ TSHandleMLocRelease(mbuf, TS_NULL_MLOC, hdr_loc);
+ return false;
+ }
+
+ bool bypassed = false;
+ int val_len = 0;
+ const char *val = TSMimeHdrFieldValueStringGet(mbuf, hdr_loc,
field_loc, -1, &val_len);
+ if (val != nullptr && 0 < val_len && std::string_view(val, val_len) ==
_bypass_header_value) {
+ Dbg(dbg_ctl, "check_bypass: bypass triggered");
Review Comment:
`check_bypass()` uses `std::string_view` but this translation unit/header
doesn’t include `<string_view>` explicitly. Relying on transitive includes can
break builds on some standard libraries; add the appropriate include where
`std::string_view` is used.
--
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]
