Copilot commented on code in PR #13160:
URL: https://github.com/apache/trafficserver/pull/13160#discussion_r3289211874
##########
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;
Review Comment:
`loadbypass()` currently disables bypass unless *both* `header` and a
non-empty `value` are provided (and warns if `value` is missing/empty). This
conflicts with the PR description (and earlier intent) that `value` is optional
and that presence of the header alone can trigger bypass. Either update the
implementation to support header-only bypass (treat missing/empty `value` as
presence-only) or update the PR description and docs to state `value` is
required.
##########
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 configured bypass value is logged verbatim (`Dbg` prints the full
value). Since the bypass token effectively acts as an authorization secret to
skip all ACL checks, logging it can leak the bypass credential into debug logs.
Consider omitting the value entirely from logs, or logging only that a value
was configured / its length / a redacted form.
##########
plugins/experimental/maxmind_acl/mmdb.cc:
##########
@@ -503,6 +544,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 header '%s' matched value '%s'",
_bypass_header.c_str(), _bypass_header_value.c_str());
Review Comment:
`check_bypass()` logs the bypass header name and configured value when a
request matches. Even at debug level, emitting the full bypass credential can
unintentionally expose it via logs. Consider redacting the value (or not
logging it) and logging only that bypass was triggered.
##########
doc/admin-guide/plugins/maxmind_acl.en.rst:
##########
@@ -113,4 +113,47 @@ The plugin also supports optional fields from GeoGuard
databases which includes:
``vpn_datacenter``
``relay_proxy``
``proxy_over_vpn``
-``smart_dns_proxy``
\ No newline at end of file
+``smart_dns_proxy``
+
+Bypass
+======
+
+An optional ``bypass`` field allows a request to skip all geo checks entirely
and pass through
+unmodified. If the specified request header is present, the plugin returns
immediately without
+performing any country, IP, regex, or anonymous evaluation.
+
+``header``
+ Required sub-key. The name of the HTTP request header to look for, e.g.
``@GeoBypass``.
+
+``value``
+ Required sub-key. The header field value must match this string exactly for
the bypass to
+ trigger. Both ``header`` and ``value`` must be present and non-empty;
omitting either
+ disables the bypass entirely and a warning is emitted to the ATS error log.
+
+The comparison uses the complete, raw field value of the first occurrence of
the named header.
+Requests where the header appears multiple times (comma-separated or repeated
lines) will not
+match, because the combined multi-value string will not equal the configured
``value``.
Review Comment:
The note about repeated header lines being combined into a single
multi-value string is inaccurate for ATS MIME headers: repeated header fields
are stored as duplicates and `TSMimeHdrFieldFind` returns only the first
occurrence unless you iterate `TSMimeHdrFieldNextDup`. Since the implementation
only checks the first field, the doc should say that only the first occurrence
is evaluated and later duplicates are ignored (in addition to comma-separated
multi-values in a single field not matching an exact value).
##########
doc/admin-guide/plugins/maxmind_acl.en.rst:
##########
@@ -113,4 +113,47 @@ The plugin also supports optional fields from GeoGuard
databases which includes:
``vpn_datacenter``
``relay_proxy``
``proxy_over_vpn``
-``smart_dns_proxy``
\ No newline at end of file
+``smart_dns_proxy``
+
+Bypass
+======
+
+An optional ``bypass`` field allows a request to skip all geo checks entirely
and pass through
+unmodified. If the specified request header is present, the plugin returns
immediately without
+performing any country, IP, regex, or anonymous evaluation.
+
+``header``
+ Required sub-key. The name of the HTTP request header to look for, e.g.
``@GeoBypass``.
+
+``value``
+ Required sub-key. The header field value must match this string exactly for
the bypass to
+ trigger. Both ``header`` and ``value`` must be present and non-empty;
omitting either
+ disables the bypass entirely and a warning is emitted to the ATS error log.
Review Comment:
This section says bypass triggers if the header is present, but then states
`value` is required and that missing `value` disables bypass. This is
internally inconsistent and also conflicts with the PR description that `value`
is optional. Please align the documentation with the intended behavior (either
describe header-only bypass, or update earlier sentences to make clear that
both header+value are required).
--
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]
