Copilot commented on code in PR #13160:
URL: https://github.com/apache/trafficserver/pull/13160#discussion_r3262577277
##########
plugins/experimental/maxmind_acl/mmdb.cc:
##########
@@ -503,6 +540,48 @@ 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;
+ if (_bypass_header_value.empty()) {
+ // presence-only check
+ Dbg(dbg_ctl, "check_bypass: bypass header '%s' present",
_bypass_header.c_str());
+ bypassed = true;
+ } else {
+ int val_len = 0;
+ const char *val = TSMimeHdrFieldValueStringGet(mbuf, hdr_loc,
field_loc, 0, &val_len);
+ if (val != nullptr && static_cast<int>(_bypass_header_value.size()) ==
val_len &&
+ _bypass_header_value.compare(0, std::string::npos, val, val_len) == 0)
{
+ Dbg(dbg_ctl, "check_bypass: bypass header '%s' matched value '%s'",
_bypass_header.c_str(), _bypass_header_value.c_str());
+ bypassed = true;
+ } else {
+ Dbg(dbg_ctl, "check_bypass: bypass header present but value did not
match");
Review Comment:
`TSMimeHdrFieldValueStringGet` with index `0` retrieves only the first
comma-separated value, not the full header value. This conflicts with the
documented exact-value match and can allow `Header: configured-value,anything`
to bypass; use the full field value when enforcing an exact match.
##########
plugins/experimental/maxmind_acl/mmdb.cc:
##########
@@ -503,6 +540,48 @@ 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()));
Review Comment:
This only evaluates the first header field returned by `TSMimeHdrFieldFind`.
ATS does not coalesce duplicate MIME fields and its docs say plugins should
iterate duplicates with `TSMimeHdrFieldNextDup`
(doc/developer-guide/plugins/http-headers/mime-headers.en.rst:83-88), so a
matching bypass value in a later duplicate header will be ignored.
##########
plugins/experimental/maxmind_acl/mmdb.cc:
##########
@@ -503,6 +540,48 @@ 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;
+ if (_bypass_header_value.empty()) {
+ // presence-only check
+ Dbg(dbg_ctl, "check_bypass: bypass header '%s' present",
_bypass_header.c_str());
+ bypassed = true;
Review Comment:
Using an empty string as the sentinel for “value omitted” makes an
explicitly configured empty value (for example `value: ""`) behave as
presence-only. Since this path bypasses all ACL checks, the code should either
track whether the `value` key was present separately or reject empty bypass
values instead of broadening the bypass condition.
##########
doc/admin-guide/plugins/maxmind_acl.en.rst:
##########
@@ -113,4 +113,33 @@ 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.
``@GcdTaBypassGeo``.
Review Comment:
Because this bypass skips all ACL checks and the implementation accepts any
configured request header, the documentation should explicitly warn that the
header must be unforgeable by clients (for example an internal `@` header set
by ATS, or a header stripped/overwritten at the edge). Without that guidance,
configuring a normal client-supplied header lets users opt out of geo
restrictions.
--
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]
