Copilot commented on code in PR #13708:
URL: https://github.com/apache/trafficserver/pull/13708#discussion_r4234860526
##########
plugins/experimental/access_control/access_control.cc:
##########
@@ -470,6 +574,36 @@ accessTokenStatusToString(const AccessTokenStatus &state)
return s;
}
+/**
+ * Validates the request path against the token scope using normalized segment
boundaries.
+ */
+bool
+validateScope(StringView requestPath, StringView scope)
Review Comment:
This is a second definition of `validateScope(StringView, StringView)`; the
same function is already defined at lines 131-135. Because both definitions are
in this translation unit (and `access_control.cc` is compiled by both the
plugin and unit-test targets), this redefinition prevents the targets from
compiling. Remove this stale duplicate and keep the implementation that
delegates to `validateScopeDetailed`.
##########
plugins/experimental/access_control/access_control.cc:
##########
@@ -23,12 +23,117 @@
#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
+normalizePath(StringView path, String &normalized, bool isScope = false)
+{
+ normalized.clear();
+ if (path.empty()) {
+ normalized = "/";
+ return true;
+ }
+
+ normalized.reserve(path.size() + 1);
+ std::vector<size_t> segment_starts;
+ size_t i = 0;
+
+ while (i < path.size()) {
+ while (i < path.size() && (path[i] == '/' || path[i] == '\\')) {
+ ++i;
+ }
+ if (i >= path.size()) {
+ break;
+ }
+
+ size_t seg_start = normalized.size();
+ normalized.push_back('/');
+
+ while (i < path.size() && path[i] != '/' && path[i] != '\\') {
+ if (path[i] == '%') {
+ unsigned int val = 0;
+ if (i + 2 < path.size() && std::from_chars(path.data() + i + 1,
path.data() + i + 3, val, 16).ec == std::errc{} &&
+ val <= 0xFF) {
Review Comment:
The `from_chars` call only checks `ec`, not that both hex digits were
consumed. A malformed escape such as `%2g` therefore parses as `0x2` and is
silently normalized instead of being rejected, allowing invalid scope/request
paths to participate in authorization matching. Require the returned pointer to
equal the two-digit end pointer.
##########
plugins/experimental/access_control/plugin.cc:
##########
@@ -529,12 +529,28 @@ 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);
+ StringView reqPath(path ? path : "", pathLen);
+ ScopeValidationResult scopeResult = validateScopeDetailed(reqPath,
token->getScope());
+ if (scopeResult != ScopeValidationResult::IN_SCOPE) {
+ if (scopeResult == ScopeValidationResult::INVALID_SCOPE) {
+ data->_vaState = INVALID_SCOPE;
+ } else if (scopeResult ==
ScopeValidationResult::INVALID_REQUEST_PATH) {
Review Comment:
The new enforcement behavior is only covered by direct `validateScope` unit
tests; no test drives `enforceAccessControl()` through `TSUrlPathGet()`. Add a
plugin-level test with a signed scoped token that verifies an out-of-scope
request gets the configured status and does not receive the subject header,
while an in-scope request does. Without this, regressions in the request-path
plumbing, status mapping, or header suppression can pass the current suite.
##########
plugins/experimental/access_control/access_control.h:
##########
@@ -105,8 +105,27 @@ enum AccessTokenStatus {
MAX,
};
+enum class ScopeValidationResult {
+ IN_SCOPE,
+ OUT_OF_SCOPE,
+ INVALID_SCOPE,
+ INVALID_REQUEST_PATH,
+};
+
const char *accessTokenStatusToString(const AccessTokenStatus &state);
+/**
+ * Validates whether a request path falls 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.
+ */
+ScopeValidationResult validateScopeDetailed(StringView requestPath, StringView
Scope);
Review Comment:
The declaration returns `ScopeValidationResult`, but this documentation says
the function returns a boolean. That is especially misleading for callers
because the implementation distinguishes invalid scopes and invalid request
paths from an ordinary out-of-scope result; document the enum result here (and
reserve the boolean wording for `validateScope`).
--
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]
