Copilot commented on code in PR #13708:
URL: https://github.com/apache/trafficserver/pull/13708#discussion_r4080344271
##########
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:
Using `std::from_chars` with `unsigned char` is implementation-fragile and
can fail to compile on some standard libraries (overload coverage for `unsigned
char`/`char` varies). Parse into an `unsigned int` (or `uint32_t`) and
range-check/cast to `unsigned char` after successful conversion.
##########
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 {
Review Comment:
For request paths (`isScope=false`), the current normalization resolves `..`
by popping segments instead of rejecting the path. This can create
authorization mismatches if ATS/origins treat traversal segments differently
(e.g., forwarding raw `/a/../b` vs normalizing), and can allow alternate
encodings to map to an allowed normalized path. Safer behavior is to treat
`.`/`..` segments in request paths as invalid (return false), rather than
normalizing them.
##########
plugins/experimental/access_control/plugin.cc:
##########
@@ -529,12 +529,25 @@ 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);
+ if (path == nullptr) {
+ pathLen = 0;
+ }
+ StringView reqPath(path ? path : "", pathLen);
+
+ if (!validateScope(reqPath, token->getScope())) {
+ data->_vaState = OUT_OF_SCOPE;
+ remapStatus =
+ handleInvalidToken(txnp, data, reject,
accessTokenStateToHttpStatus(data->_vaState, config), data->_vaState);
+ } else {
Review Comment:
Scope enforcement currently collapses multiple failure modes into
`OUT_OF_SCOPE`: (1) genuinely out-of-scope requests, (2) invalid/malformed
`scope` claims (e.g., forbidden traversal per `normalizePath(...,
isScope=true)`), and (3) request-path decode/normalization failures. This loses
useful distinction (e.g., `INVALID_SCOPE` vs `OUT_OF_SCOPE`) and forces all
failures down the same HTTP status mapping. Consider changing
`validateScope(...)` to return a richer result (e.g., enum) so the caller can
set `INVALID_SCOPE` for malformed scopes and optionally a different status for
invalid request paths.
##########
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 start of the sentence after the period: change 'an absent' to
'An absent'.
--
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]
