Copilot commented on code in PR #13708:
URL: https://github.com/apache/trafficserver/pull/13708#discussion_r4068008965
##########
plugins/experimental/access_control/access_control.cc:
##########
@@ -29,6 +29,50 @@
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 String
+normalizePath(StringView path)
+{
+ if (path.empty()) {
+ return "/";
+ }
+
+ String decoded(path.size(), '\0');
+ size_t decodedLen = urlDecode(path.data(), path.size(), decoded.data(),
decoded.size());
Review Comment:
`urlDecode()` applies `application/x-www-form-urlencoded` semantics and
converts `+` to a space (`utils.cc:156-157`), but `+` is a literal character in
an HTTP URL path. As a result, a scope such as `/reports+archive` also
authorizes `/reports%20archive` (and vice versa), even though these are
distinct path segments to the origin. Decode percent escapes for path matching
without translating `+`.
##########
plugins/experimental/access_control/access_control.cc:
##########
@@ -29,6 +29,50 @@
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 String
+normalizePath(StringView path)
+{
+ if (path.empty()) {
+ return "/";
+ }
+
+ String decoded(path.size(), '\0');
+ size_t decodedLen = urlDecode(path.data(), path.size(), decoded.data(),
decoded.size());
Review Comment:
This feeds the counted request path to `urlDecode`, whose `%` branch reads
`src[1]` and `src[2]` without checking that those bytes are within `inLen`
(`utils.cc:149-155`). A request path ending in `%` or `%a` can therefore
trigger an out-of-bounds read while enforcing a token. Make the decoder
length-aware (or reject/handle incomplete escapes) before using it here.
##########
plugins/experimental/access_control/access_control.cc:
##########
@@ -470,6 +513,29 @@ 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 normRequestPath = normalizePath(requestPath);
+ String normScope = normalizePath(scope);
+ if (normScope == "/") {
+ return true;
Review Comment:
Canonicalizing `..` in the scope itself can broaden a token's authorization:
a signed scope such as `/reports/..` normalizes to `/`, and the next branch
treats `/` as unrestricted. The request path should be normalized for
comparison, but scopes containing traversal segments should be rejected as
invalid rather than converted into a broader scope.
--
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]
