Copilot commented on code in PR #13708:
URL: https://github.com/apache/trafficserver/pull/13708#discussion_r4075600885
##########
plugins/experimental/access_control/plugin.cc:
##########
@@ -529,12 +529,22 @@ enforceAccessControl(TSHttpTxn txnp, TSRemapRequestInfo
*rri, AccessControlConfi
remapStatus =
handleInvalidToken(txnp, data, reject,
accessTokenStateToHttpStatus(data->_vaState, config), data->_vaState);
} else {
- /* Valid token, if configured extract the token subject to a header,
- * only if we can trust it - token is valid to prevent using it by
mistake */
- if (!config->_extrSubHdrName.empty()) {
- String sub(token->getSubject());
- setHeader(rri->requestBufp, rri->requestHdrp,
config->_extrSubHdrName.c_str(), config->_extrSubHdrName.size(),
- sub.c_str(), sub.size());
+ int pathLen = 0;
+ const char *path = TSUrlPathGet(rri->requestBufp,
rri->requestUrl, &pathLen);
+ StringView reqPath(path ? path : "", pathLen);
Review Comment:
If `TSUrlPathGet()` returns `nullptr`, `pathLen` may not be reliable;
constructing `StringView(\"\", pathLen)` can read past the empty string and is
undefined behavior when `pathLen > 0`. Set `pathLen = 0` when `path == nullptr`
before constructing `reqPath`.
##########
plugins/experimental/access_control/access_control.cc:
##########
@@ -23,12 +23,84 @@
#include <iostream>
#include <string>
+#include <charconv>
#include "access_control.h"
size_t calcMessageDigest(const StringView hf, const char *secret, const char
*message, size_t messageLen, char *buffer, size_t len);
const char *getSecretMap(const StringMap &map, const StringView &key, size_t
&secretSize);
+static bool
+percentDecodePath(StringView in, String &out)
+{
+ out.clear();
+ out.reserve(in.size());
+ for (size_t i = 0; i < in.size();) {
+ if (in[i] == '%') {
+ unsigned char val = 0;
+ if (i + 2 < in.size() && std::from_chars(in.data() + i + 1, in.data() +
i + 3, val, 16).ec == std::errc{}) {
+ out.push_back(static_cast<char>(val));
+ i += 3;
+ } else {
+ return false;
+ }
+ } else {
+ out.push_back(in[i]);
+ ++i;
+ }
+ }
+ return true;
+}
+
+static bool
+normalizePath(StringView path, String &normalized, bool isScope = false)
+{
+ normalized.clear();
+ if (path.empty()) {
+ normalized = "/";
+ return true;
+ }
+
+ String decoded;
+ if (!percentDecodePath(path, decoded)) {
+ return false;
+ }
+ StringVector segments;
+ size_t start = 0;
Review Comment:
`normalizePath()` allocates a `StringVector` and builds intermediate strings
on every request. Since this runs in the remap path, consider a
lower-allocation approach (e.g., single-pass normalization directly into
`normalized`, and/or pre-normalizing/storing the token scope once at
parse/validate time) to reduce per-request CPU and heap churn.
##########
doc/admin-guide/plugins/access_control.en.rst:
##########
@@ -237,7 +237,7 @@ Query-Param-Style Named Claim format
* ``iat`` for `issued at time`_, `optional`
* ``tid`` for `token id`_, `optional`
* ``ver`` for `version`_, `optional`, defaults to ``ver=1`` if not
specified.
- * ``scope`` for `scope`_, `optional`, ignored by the current version of the
plugin, still not finalized (more applications and their use cases need to be
studied to finalize the format)
+ * ``scope`` for `scope`_, `optional`. an absent or empty scope is
unrestricted. Otherwise, it is a path-prefix restriction matched against
normalized path segments, including the segment boundary (for example,
``/reports`` matches ``/reports/2026`` but not ``/reports2``); out-of-scope
requests use the configured ``--invalid-scope-status-code``.
Review Comment:
Capitalize the sentence start: change 'an absent or empty scope' to 'An
absent or empty scope'.
##########
plugins/experimental/access_control/access_control.cc:
##########
@@ -470,6 +541,36 @@ accessTokenStatusToString(const AccessTokenStatus &state)
return s;
}
+/**
+ * Validates the request path against the token scope using normalized segment
boundaries.
+ */
+bool
+validateScope(StringView requestPath, StringView scope)
+{
+ if (scope.empty()) {
+ return true;
+ }
+ String normScope;
+ if (!normalizePath(scope, normScope, /* isScope = */ true)) {
+ return false;
+ }
+ String normRequestPath;
+ if (!normalizePath(requestPath, normRequestPath, /* isScope = */ false)) {
+ return false;
+ }
+
+ if (normScope == "/") {
+ return true;
+ }
+ if (normRequestPath == normScope) {
+ return true;
+ }
+ if (normRequestPath.compare(0, normScope.size(), normScope) == 0 &&
normRequestPath[normScope.length()] == '/') {
+ return true;
+ }
+ return false;
+}
+
Review Comment:
`validateScope()` collapses multiple failure modes into a single `false`
result (e.g., invalid scope claim like `..`, invalid percent-encoding in the
request path, and genuine out-of-scope requests). In `plugin.cc`, all `false`
outcomes become `OUT_OF_SCOPE`, which prevents using `INVALID_SCOPE` (and
potentially a different status/config path) for malformed scope claims or
malformed request paths. Consider returning a small enum (e.g., `IN_SCOPE`,
`OUT_OF_SCOPE`, `INVALID_SCOPE`, `INVALID_REQUEST_PATH`) or adding an
out-parameter for the failure reason so callers can set `data->_vaState`
accurately.
##########
plugins/experimental/access_control/access_control.cc:
##########
@@ -23,12 +23,84 @@
#include <iostream>
#include <string>
+#include <charconv>
#include "access_control.h"
size_t calcMessageDigest(const StringView hf, const char *secret, const char
*message, size_t messageLen, char *buffer, size_t len);
const char *getSecretMap(const StringMap &map, const StringView &key, size_t
&secretSize);
+static bool
+percentDecodePath(StringView in, String &out)
+{
+ out.clear();
+ out.reserve(in.size());
+ for (size_t i = 0; i < in.size();) {
+ if (in[i] == '%') {
+ unsigned char val = 0;
+ if (i + 2 < in.size() && std::from_chars(in.data() + i + 1, in.data() +
i + 3, val, 16).ec == std::errc{}) {
Review Comment:
`std::from_chars` support for `unsigned char` is not consistently available
across standard library implementations; this can cause build failures on some
toolchains. Parse into an `unsigned int` (or `uint32_t`) and range-check (<=
0xFF) before casting to `char`.
--
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]
