Copilot commented on code in PR #13708:
URL: https://github.com/apache/trafficserver/pull/13708#discussion_r4110700368
##########
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;
+ }
Review Comment:
`from_chars` may successfully parse only the first hex digit and still
return `std::errc{}` when the second character is non-hex (for example, `%2g`).
Because the returned pointer is ignored, malformed escapes are silently
converted and the request can be authorized using a path that was never validly
decoded. Require the parse result to consume both hex characters; otherwise
reject the path.
##########
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;
Review Comment:
This percent-decodes reserved path delimiters before matching. For example,
scope `/reports` accepts the raw request `/reports%2Fprivate` because `%2F`
becomes a segment separator, while ATS deliberately preserves encoded path
bytes for origin processing/cache identity (`src/proxy/hdrs/URL.cc:1822-1826`).
That makes authorization operate on a different resource identity than
downstream routing; avoid decoding reserved delimiters here or apply the same
canonicalization to the URL used by remapping and forwarding.
--
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]
