Fix security audit findings: IDOR, rate limiting, action audit trail
All checks were successful
build-and-release / build (push) Successful in 4m14s
All checks were successful
build-and-release / build (push) Successful in 4m14s
- Chat and ticket-creation endpoints now scope lookups/attribution to the authenticated JWT identity instead of trusting a client-supplied user_id in the request body (IDOR) - Add per-user in-memory rate limiter on AI-generation endpoints to guard against unbounded LLM-cost abuse - Bind confirm_action to the authenticated user for audit logging - Bump vulnerable transitive dependencies via cargo update Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
6f67864390
commit
23a707e175
10 changed files with 482 additions and 584 deletions
823
Cargo.lock
generated
823
Cargo.lock
generated
File diff suppressed because it is too large
Load diff
|
|
@ -51,6 +51,7 @@ impl ChatOrchestrator {
|
|||
pub async fn handle_chat(
|
||||
&self,
|
||||
request: ChatMessageRequest,
|
||||
authenticated_user_id: &str,
|
||||
) -> Result<ChatMessageResponse, AppError> {
|
||||
let (intent, confidence) = classify_intent(&request.message);
|
||||
let conversation_id = request.conversation_id.clone();
|
||||
|
|
@ -196,10 +197,13 @@ impl ChatOrchestrator {
|
|||
"reference_lookup" => {
|
||||
let reference_number = extract_reference_number(&request.message);
|
||||
|
||||
let looked_up = match (&reference_number, request.user_id.as_deref()) {
|
||||
(Some(reference_number), Some(user_id)) if !user_id.is_empty() => {
|
||||
// Always scope the lookup to the authenticated caller (from the
|
||||
// verified JWT), never to a client-supplied `user_id` in the
|
||||
// request body - that field is not trustworthy for authorization.
|
||||
let looked_up = match &reference_number {
|
||||
Some(reference_number) if !authenticated_user_id.is_empty() => {
|
||||
self.reference_lookup_provider
|
||||
.lookup(reference_number, user_id)
|
||||
.lookup(reference_number, authenticated_user_id)
|
||||
.await?
|
||||
}
|
||||
_ => None,
|
||||
|
|
|
|||
|
|
@ -17,6 +17,7 @@ pub struct AppConfig {
|
|||
pub nxtgauge_users_url: String,
|
||||
pub jwt_secret: String,
|
||||
pub ai_service_key: String,
|
||||
pub rate_limit_per_minute: u32,
|
||||
}
|
||||
|
||||
impl AppConfig {
|
||||
|
|
@ -63,6 +64,9 @@ impl AppConfig {
|
|||
}
|
||||
v
|
||||
},
|
||||
rate_limit_per_minute: env_or_default("RATE_LIMIT_PER_MINUTE", "20")
|
||||
.parse()
|
||||
.unwrap_or(20),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,10 @@
|
|||
use axum::{extract::State, Json};
|
||||
use axum::{
|
||||
extract::{Extension, State},
|
||||
Json,
|
||||
};
|
||||
|
||||
use crate::{
|
||||
auth::AuthUser,
|
||||
chat::models::{ChatMessageRequest, ChatMessageResponse},
|
||||
error::AppError,
|
||||
state::AppState,
|
||||
|
|
@ -8,6 +12,7 @@ use crate::{
|
|||
|
||||
pub async fn message(
|
||||
State(state): State<AppState>,
|
||||
Extension(auth_user): Extension<AuthUser>,
|
||||
Json(request): Json<ChatMessageRequest>,
|
||||
) -> Result<Json<ChatMessageResponse>, AppError> {
|
||||
if request.message.trim().is_empty() {
|
||||
|
|
@ -16,6 +21,9 @@ pub async fn message(
|
|||
));
|
||||
}
|
||||
|
||||
let response = state.chat_orchestrator.handle_chat(request).await?;
|
||||
let response = state
|
||||
.chat_orchestrator
|
||||
.handle_chat(request, &auth_user.user_id)
|
||||
.await?;
|
||||
Ok(Json(response))
|
||||
}
|
||||
|
|
|
|||
|
|
@ -25,7 +25,7 @@ pub async fn confirm_action(
|
|||
|
||||
let response = state
|
||||
.action_confirmation_service
|
||||
.confirm_action(request)
|
||||
.confirm_action(request, &auth_user.user_id)
|
||||
.await?;
|
||||
|
||||
Ok(Json(response))
|
||||
|
|
|
|||
|
|
@ -1,6 +1,10 @@
|
|||
use axum::{extract::State, Json};
|
||||
use axum::{
|
||||
extract::{Extension, State},
|
||||
Json,
|
||||
};
|
||||
|
||||
use crate::{
|
||||
auth::AuthUser,
|
||||
error::AppError,
|
||||
state::AppState,
|
||||
tickets::models::{CreateTicketRequest, CreateTicketResponse},
|
||||
|
|
@ -8,8 +12,13 @@ use crate::{
|
|||
|
||||
pub async fn create(
|
||||
State(state): State<AppState>,
|
||||
Json(request): Json<CreateTicketRequest>,
|
||||
Extension(auth_user): Extension<AuthUser>,
|
||||
Json(mut request): Json<CreateTicketRequest>,
|
||||
) -> Result<Json<CreateTicketResponse>, AppError> {
|
||||
// Never trust a client-supplied `user_id` for attribution/authorization -
|
||||
// always bind the ticket to the authenticated caller from the verified JWT.
|
||||
request.user_id = auth_user.user_id.clone();
|
||||
|
||||
let created = state.ticket_service.create(request).await?;
|
||||
Ok(Json(created))
|
||||
}
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ mod jobs;
|
|||
mod permissions;
|
||||
mod providers;
|
||||
mod prompts;
|
||||
mod rate_limit;
|
||||
mod retrieval;
|
||||
mod routes;
|
||||
mod services;
|
||||
|
|
|
|||
88
src/rate_limit.rs
Normal file
88
src/rate_limit.rs
Normal file
|
|
@ -0,0 +1,88 @@
|
|||
use std::{
|
||||
collections::HashMap,
|
||||
sync::{Arc, Mutex},
|
||||
time::{Duration, Instant},
|
||||
};
|
||||
|
||||
use axum::{
|
||||
extract::{Extension, State},
|
||||
http::StatusCode,
|
||||
middleware::Next,
|
||||
response::{IntoResponse, Response},
|
||||
Json,
|
||||
};
|
||||
|
||||
use crate::auth::AuthUser;
|
||||
|
||||
/// Simple fixed-window, in-memory rate limiter keyed by authenticated user id.
|
||||
///
|
||||
/// Not distributed - each process instance tracks its own counters. That's
|
||||
/// acceptable for this service's current single-replica deployment; if this
|
||||
/// service is ever scaled horizontally, this should move to a shared store
|
||||
/// (e.g. Redis) instead.
|
||||
#[derive(Clone)]
|
||||
pub struct RateLimiter {
|
||||
inner: Arc<Mutex<HashMap<String, (u32, Instant)>>>,
|
||||
max_requests: u32,
|
||||
window: Duration,
|
||||
}
|
||||
|
||||
impl RateLimiter {
|
||||
pub fn new(max_requests: u32, window: Duration) -> Self {
|
||||
Self {
|
||||
inner: Arc::new(Mutex::new(HashMap::new())),
|
||||
max_requests,
|
||||
window,
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns `true` if the request for `key` is allowed under the current
|
||||
/// window, incrementing its counter as a side effect. Returns `false`
|
||||
/// (and leaves the counter unchanged) if the caller is over the limit.
|
||||
fn check(&self, key: &str) -> bool {
|
||||
let mut map = self.inner.lock().unwrap_or_else(|e| e.into_inner());
|
||||
let now = Instant::now();
|
||||
|
||||
match map.get_mut(key) {
|
||||
Some((count, window_start)) => {
|
||||
if now.duration_since(*window_start) >= self.window {
|
||||
*count = 1;
|
||||
*window_start = now;
|
||||
true
|
||||
} else if *count < self.max_requests {
|
||||
*count += 1;
|
||||
true
|
||||
} else {
|
||||
false
|
||||
}
|
||||
}
|
||||
None => {
|
||||
map.insert(key.to_string(), (1, now));
|
||||
true
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Axum middleware that enforces `RateLimiter` per authenticated user.
|
||||
///
|
||||
/// Must be layered *inside* (i.e. run after) `require_auth`, since it relies
|
||||
/// on the `AuthUser` extension that middleware inserts.
|
||||
pub async fn rate_limit_middleware(
|
||||
State(limiter): State<RateLimiter>,
|
||||
Extension(auth_user): Extension<AuthUser>,
|
||||
request: axum::extract::Request,
|
||||
next: Next,
|
||||
) -> Response {
|
||||
if limiter.check(&auth_user.user_id) {
|
||||
next.run(request).await
|
||||
} else {
|
||||
(
|
||||
StatusCode::TOO_MANY_REQUESTS,
|
||||
Json(serde_json::json!({
|
||||
"error": "Rate limit exceeded, please try again later"
|
||||
})),
|
||||
)
|
||||
.into_response()
|
||||
}
|
||||
}
|
||||
|
|
@ -1,3 +1,5 @@
|
|||
use std::time::Duration;
|
||||
|
||||
use axum::{
|
||||
http::HeaderValue,
|
||||
middleware,
|
||||
|
|
@ -9,7 +11,12 @@ use tower_http::{
|
|||
trace::TraceLayer,
|
||||
};
|
||||
|
||||
use crate::{auth::require_auth, handlers, state::AppState};
|
||||
use crate::{
|
||||
auth::require_auth,
|
||||
handlers,
|
||||
rate_limit::{rate_limit_middleware, RateLimiter},
|
||||
state::AppState,
|
||||
};
|
||||
|
||||
pub fn build_router(state: AppState) -> Router {
|
||||
let frontend_url: HeaderValue = std::env::var("FRONTEND_URL")
|
||||
|
|
@ -26,47 +33,62 @@ pub fn build_router(state: AppState) -> Router {
|
|||
.allow_methods(Any)
|
||||
.allow_headers(Any);
|
||||
|
||||
let rate_limiter = RateLimiter::new(state.config.rate_limit_per_minute, Duration::from_secs(60));
|
||||
|
||||
// AI-generation endpoints: expensive to run and abuse-prone, so they get a
|
||||
// per-user rate limit on top of auth. Keyed off the authenticated user id
|
||||
// from the JWT, so this layer must sit inside (run after) `require_auth`.
|
||||
let ai_generation_routes = Router::new()
|
||||
.route("/chat/message", post(handlers::chat::message))
|
||||
.route(
|
||||
"/jobs/generate-description",
|
||||
post(handlers::jobs::generate_description),
|
||||
)
|
||||
.route(
|
||||
"/cover-letter/generate",
|
||||
post(handlers::cover_letter::generate_cover_letter),
|
||||
)
|
||||
.route("/forms/extract", post(handlers::forms::extract))
|
||||
.route("/resume/improve", post(handlers::content_tools::improve_resume))
|
||||
.route(
|
||||
"/job-seekers/improve-profile",
|
||||
post(handlers::content_tools::improve_jobseeker_profile),
|
||||
)
|
||||
.route(
|
||||
"/companies/improve-profile",
|
||||
post(handlers::content_tools::improve_company_profile),
|
||||
)
|
||||
.route(
|
||||
"/professionals/improve-profile",
|
||||
post(handlers::content_tools::improve_professional_profile),
|
||||
)
|
||||
.route(
|
||||
"/professionals/generate-service-description",
|
||||
post(handlers::content_tools::generate_service_description),
|
||||
)
|
||||
.route("/jobs/improve-post", post(handlers::content_tools::improve_job_post))
|
||||
.route("/kb/generate", post(handlers::content_tools::generate_kb_content))
|
||||
.route(
|
||||
"/admin/support-summary",
|
||||
post(handlers::content_tools::support_summary),
|
||||
)
|
||||
.layer(middleware::from_fn_with_state(
|
||||
rate_limiter,
|
||||
rate_limit_middleware,
|
||||
));
|
||||
|
||||
// Other authenticated (but not AI-generation) endpoints - no rate limit.
|
||||
let other_routes = Router::new()
|
||||
.route("/tickets/create", post(handlers::tickets::create))
|
||||
.route("/help/search", post(handlers::help::search))
|
||||
.route("/actions/confirm", post(handlers::confirm_action::confirm_action));
|
||||
|
||||
Router::new()
|
||||
.route("/health", get(handlers::health::health))
|
||||
.nest(
|
||||
"/api/v1",
|
||||
Router::new()
|
||||
.route("/chat/message", post(handlers::chat::message))
|
||||
.route(
|
||||
"/jobs/generate-description",
|
||||
post(handlers::jobs::generate_description),
|
||||
)
|
||||
.route(
|
||||
"/cover-letter/generate",
|
||||
post(handlers::cover_letter::generate_cover_letter),
|
||||
)
|
||||
.route("/forms/extract", post(handlers::forms::extract))
|
||||
.route("/tickets/create", post(handlers::tickets::create))
|
||||
.route("/help/search", post(handlers::help::search))
|
||||
.route("/actions/confirm", post(handlers::confirm_action::confirm_action))
|
||||
.route("/resume/improve", post(handlers::content_tools::improve_resume))
|
||||
.route(
|
||||
"/job-seekers/improve-profile",
|
||||
post(handlers::content_tools::improve_jobseeker_profile),
|
||||
)
|
||||
.route(
|
||||
"/companies/improve-profile",
|
||||
post(handlers::content_tools::improve_company_profile),
|
||||
)
|
||||
.route(
|
||||
"/professionals/improve-profile",
|
||||
post(handlers::content_tools::improve_professional_profile),
|
||||
)
|
||||
.route(
|
||||
"/professionals/generate-service-description",
|
||||
post(handlers::content_tools::generate_service_description),
|
||||
)
|
||||
.route("/jobs/improve-post", post(handlers::content_tools::improve_job_post))
|
||||
.route("/kb/generate", post(handlers::content_tools::generate_kb_content))
|
||||
.route(
|
||||
"/admin/support-summary",
|
||||
post(handlers::content_tools::support_summary),
|
||||
)
|
||||
ai_generation_routes
|
||||
.merge(other_routes)
|
||||
.layer(middleware::from_fn_with_state(state.clone(), require_auth)),
|
||||
)
|
||||
.layer(cors)
|
||||
|
|
|
|||
|
|
@ -15,8 +15,15 @@ impl ActionConfirmationService {
|
|||
pub async fn confirm_action(
|
||||
&self,
|
||||
request: ConfirmActionRequest,
|
||||
confirmed_by_user_id: &str,
|
||||
) -> Result<ConfirmActionResponse, AppError> {
|
||||
if !request.confirmed {
|
||||
tracing::info!(
|
||||
user_id = %confirmed_by_user_id,
|
||||
action = %request.action,
|
||||
conversation_id = %request.conversation_id,
|
||||
"action confirmation cancelled by user"
|
||||
);
|
||||
return Ok(ConfirmActionResponse {
|
||||
success: false,
|
||||
delegated: false,
|
||||
|
|
@ -35,6 +42,14 @@ impl ActionConfirmationService {
|
|||
.map(|item| item.backend_handler.clone())
|
||||
.unwrap_or_else(|| request.action.clone());
|
||||
|
||||
tracing::info!(
|
||||
user_id = %confirmed_by_user_id,
|
||||
action = %request.action,
|
||||
backend_handler = %backend_handler,
|
||||
conversation_id = %request.conversation_id,
|
||||
"action confirmed by user"
|
||||
);
|
||||
|
||||
Ok(ConfirmActionResponse {
|
||||
success: true,
|
||||
delegated: true,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue