Copilot commented on code in PR #13160:
URL: https://github.com/apache/trafficserver/pull/13160#discussion_r3289726642
##########
plugins/experimental/maxmind_acl/mmdb.cc:
##########
@@ -429,6 +434,42 @@ Acl::parseregex(const YAML::Node ®ex, bool allow)
}
}
+void
+Acl::loadbypass(const YAML::Node &bypassNode)
+{
+ if (!bypassNode) {
+ Dbg(dbg_ctl, "No bypass set");
+ return;
+ }
+ if (bypassNode.IsNull()) {
+ Dbg(dbg_ctl, "bypass node is NULL");
+ 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()` validates that `value` is present/non-empty, but it never
validates that the configured `header` string itself is non-empty. With
`header: ""` (or whitespace-only), config will appear accepted but
`check_bypass()` will never trigger (it short-circuits on
`_bypass_header.empty()`), leading to a confusing misconfiguration. Consider
rejecting empty/whitespace-only `header` (warn and leave bypass disabled) the
same way empty `value` is handled.
##########
plugins/experimental/maxmind_acl/mmdb.cc:
##########
@@ -429,6 +434,42 @@ Acl::parseregex(const YAML::Node ®ex, bool allow)
}
}
+void
+Acl::loadbypass(const YAML::Node &bypassNode)
+{
+ if (!bypassNode) {
+ Dbg(dbg_ctl, "No bypass set");
+ return;
+ }
+ if (bypassNode.IsNull()) {
+ Dbg(dbg_ctl, "bypass node is NULL");
+ 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 set to: %s", _bypass_header_value.c_str());
Review Comment:
The bypass header value is logged verbatim at debug level. If this value is
used as a shared secret/token between trusted components, emitting it to logs
can weaken the bypass’s security model. Consider avoiding logging the value (or
masking it) while still logging that bypass is configured.
--
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]
