Copilot commented on code in PR #13708:
URL: https://github.com/apache/trafficserver/pull/13708#discussion_r4074869862
##########
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;
+
+ while (start < decoded.size()) {
+ size_t end = decoded.find_first_of("/\\", start);
+ if (end == String::npos) {
+ end = decoded.size();
+ }
+ if (end > start) {
+ StringView seg(decoded.data() + start, end - start);
+ if (seg == ".") {
+ } else if (seg == "..") {
+ if (isScope) {
+ return false;
+ }
+ if (!segments.empty()) {
+ segments.pop_back();
+ }
+ } else {
+ segments.emplace_back(seg);
+ }
+ }
+ start = end + 1;
+ }
Review Comment:
`normalizePath()` allocates/constructs multiple strings per request
(`decoded` plus `StringVector segments` with per-segment copies). Since this
runs on the transaction hot path, this can add noticeable overhead at scale. A
lower-allocation approach would be to build `normalized` directly while
tracking segment boundaries (e.g., a stack of indices into `normalized`) so
`..` can be handled via `resize()` without allocating per-segment strings.
##########
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;
+}
Review Comment:
`percentDecodePath()` decodes all percent-encoded bytes, including reserved
path delimiters like `%2F` (`/`) and `%5C` (`\`). This can make the
authorization check operate on a different path structure than ATS/origin
interpretation, creating an authZ mismatch risk. Consider rejecting
percent-encoded path separators (and other dangerous bytes like NUL/control
chars) during decoding so ambiguous encodings fail scope validation (return
false), rather than being normalized into new segments.
##########
plugins/experimental/access_control/access_control.h:
##########
@@ -107,6 +107,17 @@ enum AccessTokenStatus {
const char *accessTokenStatusToString(const AccessTokenStatus &state);
+/**
+ * Validates whether a request path fails within the scope claim of an access
token.
+ * Matching is performed on normalized path segments. An empty or absent scope
is
+ * treated as unrestricted (returns true).
+ *
+ * @param[in] requestPath The incoming HTTP requestPath.
+ * @param[in] scope The scope string extracted from the token.
+ * @return True if the path is permitted by the scope, false otherwise.
+ */
Review Comment:
The comment reads `fails within` but should be `falls within`.
##########
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:
Fix sentence punctuation/capitalization: after `optional`, this should start
a new sentence (period) with lowercase continuation.
--
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]
