Skip to content

Commit 03fefce

Browse files
committed
fix(auth): security and performance improvements
- Prevent token enumeration in /auth/revoke (RFC 7009 compliance) - Cache compiled regex patterns in matches_path for performance - Improve PRNG seeding with seed_seq for better thread-local entropy - Return refresh_token in token refresh response for client continuity - Update README docs to clarify refresh token is not rotated
1 parent 14ac6da commit 03fefce

File tree

3 files changed

+84
-47
lines changed

3 files changed

+84
-47
lines changed

src/ros2_medkit_gateway/README.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -484,11 +484,13 @@ curl -X POST http://localhost:8080/api/v1/auth/token \
484484
"access_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...",
485485
"token_type": "Bearer",
486486
"expires_in": 3600,
487-
"refresh_token": "bmV3IHJlZnJlc2ggdG9rZW4...",
487+
"refresh_token": "dGhpcyBpcyBhIHJlZnJlc2ggdG9rZW4...",
488488
"scope": "admin"
489489
}
490490
```
491491

492+
**Note:** The `refresh_token` in the response is the same token that was sent in the request (not a new token). Refresh token rotation is not implemented.
493+
492494
#### POST /api/v1/auth/revoke
493495

494496
Revoke a refresh token to prevent further use.

src/ros2_medkit_gateway/src/auth/auth_manager.cpp

Lines changed: 77 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,8 @@ tl::expected<TokenResponse, AuthErrorResponse> AuthManager::refresh_access_token
201201
response.token_type = "Bearer";
202202
response.expires_in = config_.token_expiry_seconds;
203203
response.scope = role_to_string(record->role);
204+
// Return the existing refresh token so clients can continue to use it
205+
response.refresh_token = refresh_token;
204206

205207
return response;
206208
}
@@ -511,9 +513,14 @@ tl::expected<JwtClaims, std::string> AuthManager::decode_jwt(const std::string &
511513
}
512514

513515
std::string AuthManager::generate_token_id() {
514-
// Generate UUID-like string using thread-local RNG for thread safety
515-
thread_local std::random_device rd;
516-
thread_local std::mt19937_64 gen(rd());
516+
// Generate UUID-like string using thread-local RNG for thread safety.
517+
// Seed the PRNG once per thread using a local random_device and time-based seed_seq
518+
thread_local std::mt19937_64 gen = []() {
519+
std::random_device rd;
520+
auto now = static_cast<uint64_t>(std::chrono::high_resolution_clock::now().time_since_epoch().count());
521+
std::seed_seq seq{rd(), static_cast<uint32_t>(now & 0xFFFFFFFFu), static_cast<uint32_t>((now >> 32) & 0xFFFFFFFFu)};
522+
return std::mt19937_64(seq);
523+
}();
517524
thread_local std::uniform_int_distribution<uint64_t> dis;
518525

519526
std::stringstream ss;
@@ -544,53 +551,81 @@ bool AuthManager::matches_path(const std::string & pattern, const std::string &
544551
return true;
545552
}
546553

547-
// Convert pattern to regex
548-
std::string regex_pattern;
549-
regex_pattern.reserve(pattern.size() * 2);
554+
// Cache compiled regex patterns for performance (authorization is called on every request)
555+
static std::mutex cache_mutex;
556+
static std::unordered_map<std::string, std::optional<std::regex>> regex_cache;
550557

551-
for (size_t i = 0; i < pattern.size(); ++i) {
552-
char c = pattern[i];
553-
if (c == '*') {
554-
// Check for ** (multi-segment wildcard)
555-
if (i + 1 < pattern.size() && pattern[i + 1] == '*') {
556-
regex_pattern += ".*"; // Match anything including slashes
557-
++i; // Skip the second *
558+
// Check cache first
559+
std::optional<std::regex> cached_regex;
560+
{
561+
std::lock_guard<std::mutex> lock(cache_mutex);
562+
auto it = regex_cache.find(pattern);
563+
if (it != regex_cache.end()) {
564+
cached_regex = it->second;
565+
}
566+
}
567+
568+
// If not in cache, compile and store
569+
if (!cached_regex.has_value()) {
570+
// Convert pattern to regex
571+
std::string regex_pattern;
572+
regex_pattern.reserve(pattern.size() * 2);
573+
574+
for (size_t i = 0; i < pattern.size(); ++i) {
575+
char c = pattern[i];
576+
if (c == '*') {
577+
// Check for ** (multi-segment wildcard)
578+
if (i + 1 < pattern.size() && pattern[i + 1] == '*') {
579+
regex_pattern += ".*"; // Match anything including slashes
580+
++i; // Skip the second *
581+
} else {
582+
regex_pattern += "[^/]+"; // Match any non-slash characters (single segment)
583+
}
558584
} else {
559-
regex_pattern += "[^/]+"; // Match any non-slash characters (single segment)
560-
}
561-
} else {
562-
switch (c) {
563-
case '.':
564-
case '[':
565-
case ']':
566-
case '(':
567-
case ')':
568-
case '{':
569-
case '}':
570-
case '\\':
571-
case '^':
572-
case '$':
573-
case '|':
574-
case '?':
575-
case '+':
576-
regex_pattern += '\\';
577-
regex_pattern += c;
578-
break;
579-
default:
580-
regex_pattern += c;
585+
switch (c) {
586+
case '.':
587+
case '[':
588+
case ']':
589+
case '(':
590+
case ')':
591+
case '{':
592+
case '}':
593+
case '\\':
594+
case '^':
595+
case '$':
596+
case '|':
597+
case '?':
598+
case '+':
599+
regex_pattern += '\\';
600+
regex_pattern += c;
601+
break;
602+
default:
603+
regex_pattern += c;
604+
}
581605
}
582606
}
583-
}
584607

585-
// Anchor the pattern
586-
regex_pattern = "^" + regex_pattern + "$";
608+
// Anchor the pattern
609+
regex_pattern = "^" + regex_pattern + "$";
587610

588-
try {
589-
std::regex re(regex_pattern);
590-
return std::regex_match(path, re);
591-
} catch (const std::regex_error &) {
611+
try {
612+
std::regex compiled(regex_pattern);
613+
std::lock_guard<std::mutex> lock(cache_mutex);
614+
regex_cache[pattern] = compiled;
615+
cached_regex = compiled;
616+
} catch (const std::regex_error &) {
617+
std::lock_guard<std::mutex> lock(cache_mutex);
618+
regex_cache[pattern] = std::nullopt; // Cache the failure too
619+
return false;
620+
}
621+
}
622+
623+
// If regex compilation failed previously, return false
624+
if (!cached_regex.has_value()) {
592625
return false;
593626
}
627+
628+
return std::regex_match(path, cached_regex.value());
594629
}
595630

596631
void AuthManager::store_refresh_token(const RefreshTokenRecord & record) {

src/ros2_medkit_gateway/src/rest_server.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2219,11 +2219,11 @@ void RESTServer::handle_auth_revoke(const httplib::Request & req, httplib::Respo
22192219

22202220
std::string token = body["token"].get<std::string>();
22212221

2222-
// Revoke token
2223-
bool revoked = auth_manager_->revoke_refresh_token(token);
2222+
// Revoke token (do not reveal whether it existed to avoid token enumeration)
2223+
auth_manager_->revoke_refresh_token(token);
22242224

2225-
// Per OAuth2 spec, always return 200 even if token wasn't found
2226-
json response = {{"status", revoked ? "revoked" : "not_found"}};
2225+
// Per OAuth2 RFC 7009, always return 200 and do not indicate token validity
2226+
json response = {{"status", "revoked"}};
22272227
res.set_content(response.dump(2), "application/json");
22282228
} catch (const std::exception & e) {
22292229
res.status = StatusCode::InternalServerError_500;

0 commit comments

Comments
 (0)