Copilot commented on code in PR #13708:
URL: https://github.com/apache/trafficserver/pull/13708#discussion_r4075280201
##########
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;
+ }
+ }
Review Comment:
The percent-decoder turns all `%xx` sequences into bytes, and normalization
treats both `/` and `\\` as path separators. This means encoded separators
(e.g., `%2F`, `%5C`) can alter the segment structure during authorization,
which can lead to inconsistent authorization vs. routing behavior if upstream
components treat encoded separators differently. Consider explicitly rejecting
percent-encoded path separators (and possibly other problematic bytes like NUL)
during decoding/normalization, or clearly documenting that encoded separators
are treated as separators for scope enforcement.
##########
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);
Review Comment:
The percent-decoder turns all `%xx` sequences into bytes, and normalization
treats both `/` and `\\` as path separators. This means encoded separators
(e.g., `%2F`, `%5C`) can alter the segment structure during authorization,
which can lead to inconsistent authorization vs. routing behavior if upstream
components treat encoded separators differently. Consider explicitly rejecting
percent-encoded path separators (and possibly other problematic bytes like NUL)
during decoding/normalization, or clearly documenting that encoded separators
are treated as separators for scope enforcement.
##########
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;
+ }
+ if (segments.empty()) {
+ normalized = "/";
+ return true;
+ }
+
+ for (const auto &seg : segments) {
+ normalized.push_back('/');
+ normalized.append(seg);
+ }
Review Comment:
This normalization allocates and stores all segments in a vector before
rebuilding the normalized path, which adds per-request allocations/copies in
the hot path (`enforceAccessControl`). Consider a single-pass approach that
builds `normalized` incrementally (or tracks segment boundaries in-place) to
avoid the `StringVector` allocation and repeated string materialization, while
preserving the existing `.`/`..` semantics.
##########
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.
Review Comment:
Typo in the public header comment: `fails within` 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:
This doc line has a grammatical/capitalization issue (`. an absent...`) and
is overly dense for admin docs. Consider capitalizing the sentence start and
breaking it into multiple wrapped lines/bullets to improve readability (e.g.,
separate backward-compat behavior, matching rule, and status-code behavior).
--
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]
