fix(security): address 6 server vulnerabilities (H-1, H-2, H-3, H-4, M-1, M-2)#57
fix(security): address 6 server vulnerabilities (H-1, H-2, H-3, H-4, M-1, M-2)#57AbianS wants to merge 1 commit into
Conversation
H-1 (timing oracle): run dummy Argon2 verify when login email not found, equalizing response time between missing-user and wrong-password paths. H-2/M-3 (password length): enforce 8–1024 char limits on register; reject passwords >1024 on login before DB/Argon2 work (CPU DoS prevention). H-3 (error leakage): sanitize Database and Internal errors in error_response — log full detail server-side, return generic message to client. H-4 (SSRF): block private/internal URLs in webhook validate_config — loopback, RFC1918, link-local (169.254.x.x), localhost, *.local, *.internal. M-1 (413 format): collect ingest Payload manually with 100MB cap so oversized bodies return AppError::PayloadTooLarge (413 JSON) not actix default. M-2 (session fixation): call session.clear() + session.renew() before set_user_session on login and register to regenerate the session token. Each fix is covered by a failing test written first (TDD red→green).
WalkthroughThis PR implements multi-layered security hardening across error handling, password verification, session management, request validation, and webhook configuration. It prevents information leakage in error responses, eliminates timing-oracle vulnerabilities in password checks, prevents session fixation attacks, enforces payload size limits, and blocks SSRF attempts to internal resources. ChangesSecurity Hardening
Sequence DiagramsequenceDiagram
participant Client as Client/Attacker
participant AuthHandler as Auth Handler
participant DB as Database
participant User as User Model
participant Session as Session Manager
rect rgba(255, 0, 0, 0.5)
Note over Client,Session: Timing Oracle Attack Prevention
Client->>AuthHandler: login(non-existent@email.com, password)
AuthHandler->>DB: find user by email
DB-->>AuthHandler: not found (null)
AuthHandler->>User: run_dummy_password_verify(password)
Note over User: Execute Argon2 hash<br/>return false regardless
User-->>AuthHandler: false
AuthHandler-->>Client: Unauthorized (same timing as found user)
end
rect rgba(0, 255, 0, 0.5)
Note over Client,Session: Session Fixation Prevention
Client->>AuthHandler: login(valid@email.com, password)<br/>with attacker-planted session cookie
AuthHandler->>Session: clear existing session data
AuthHandler->>Session: renew session token
AuthHandler->>Session: set authenticated user session
Session-->>AuthHandler: new session established
AuthHandler-->>Client: authenticated (old cookie invalid)
Note over Client: Attacker-planted data cleared
end
rect rgba(0, 0, 255, 0.5)
Note over Client,Session: Error Masking
Client->>AuthHandler: any request → internal DB error occurs
AuthHandler->>AuthHandler: log full error server-side
AuthHandler-->>Client: { error_type: "InternalError",<br/>message: "An internal error occurred" }
Note over Client: Internal details hidden from client
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/routes/auth.rs (1)
162-170:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResidual timing oracle on
is_active = falseusers.H-1 closes the user-not-found path by running Argon2 against a dummy hash. The
is_active == falsebranch still returnsUnauthorizedwithout running Argon2 (line 164), so its response time is materially shorter than both the not-found path and the wrong-password path. That lets an unauthenticated attacker still enumerate accounts whose state is "exists but disabled".Cheapest fix: also call
User::run_dummy_password_verify(or justuser.verify_password) on this branch before returning, so all three failure modes share the same Argon2 cost.🛡️ Suggested fix
// Check if user is active if !user.is_active { + // Run verification anyway to equalize timing with the other failure paths + let _ = user.verify_password(&req.password); return Err(AppError::Unauthorized("Account is disabled".to_string())); }Note: also consider returning a single uniform
"Invalid credentials"message instead of"Account is disabled", since the distinct message is itself an enumeration oracle independent of timing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/routes/auth.rs` around lines 162 - 170, When the user is inactive (!user.is_active) run the same expensive password verification as other failure paths to avoid a timing oracle: call User::run_dummy_password_verify() (or user.verify_password(&req.password) and ignore/handle its Result) before returning the AppError::Unauthorized; ensure the call executes even if it fails, then return the same Unauthorized response (consider using the same "Invalid credentials" text to avoid message-based enumeration).apps/server/src/routes/ingest.rs (1)
33-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReordering opportunity: read up to 100MB before the rate-limit check.
The fail-fast intent in the comment at line 51 is undermined by the new collection step on lines 36–49: the handler now eagerly buffers up to
MAX_INGEST_PAYLOAD_BYTES(100 MB) into memory beforeRateLimitService::check_quotaruns. An already rate-limited project can therefore still force the server to allocate and copy 100 MB per request — a meaningful DoS amplification compared to the oldBytesextractor path (which had a much smaller default limit) and a regression of the "fail fast" promise.
check_quotaonly needspoolandauth.project, both of which are available pre-body. Moving it ahead of the body collection restores fail-fast and bounds memory for throttled callers.🛡️ Proposed reordering
mut payload: web::Payload, ) -> AppResult<HttpResponse> { - // Collect body with explicit size limit so we control the error format (413 JSON) - let body: Bytes = { - let mut buf = BytesMut::new(); - while let Some(chunk) = payload.next().await { - let chunk = - chunk.map_err(|e| AppError::Internal(format!("Payload read error: {e}")))?; - if buf.len() + chunk.len() > MAX_INGEST_PAYLOAD_BYTES { - return Err(AppError::PayloadTooLarge( - "Payload exceeds maximum size".to_string(), - )); - } - buf.extend_from_slice(&chunk); - } - buf.freeze() - }; - // 0. Check rate limits (fail fast before processing) if let Some(exceeded) = RateLimitService::check_quota(pool.get_ref(), &auth.project).await? { log::warn!( "Rate limit exceeded for project {}: retry_after={}s", auth.project.id, exceeded.retry_after ); return Ok(HttpResponse::TooManyRequests() .insert_header(("Retry-After", exceeded.retry_after.to_string())) .json(serde_json::json!({ "error": "rate_limit_exceeded", "retry_after": exceeded.retry_after }))); } + + // Collect body with explicit size limit so we control the error format (413 JSON) + let body: Bytes = { + let mut buf = BytesMut::new(); + while let Some(chunk) = payload.next().await { + let chunk = + chunk.map_err(|e| AppError::Internal(format!("Payload read error: {e}")))?; + if buf.len() + chunk.len() > MAX_INGEST_PAYLOAD_BYTES { + return Err(AppError::PayloadTooLarge( + "Payload exceeds maximum size".to_string(), + )); + } + buf.extend_from_slice(&chunk); + } + buf.freeze() + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/routes/ingest.rs` around lines 33 - 64, The handler currently buffers the request body up to MAX_INGEST_PAYLOAD_BYTES before performing the rate-limit check, which defeats the "fail fast" intent; move the RateLimitService::check_quota call (using pool.get_ref() and &auth.project) to run before the body collection loop so rate-limited projects return TooManyRequests without allocating/reading the body, then proceed to read the payload into buf/Bytes only after the quota check succeeds.
🧹 Nitpick comments (8)
apps/server/tests/unit/error_test.rs (1)
36-51: 💤 Low valueOptional: also assert the generic message is present on
Internal.The DB test asserts the response contains
"internal", buttest_internal_error_does_not_leak_internal_detailsonly checks absence of leak substrings. A trivially-passing implementation that returns an empty body would still satisfy this test. Mirror the positive check from the DB test for symmetry.♻️ Suggested addition
assert!( !body.contains("unique constraint"), "DB constraint name leaked: {body}" ); + assert!( + body.to_lowercase().contains("internal"), + "Expected generic internal error message, got: {body}" + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/tests/unit/error_test.rs` around lines 36 - 51, Add a positive assertion in test_internal_error_does_not_leak_internal_details to ensure the generic error text is returned (not just absence of DB details): after obtaining body via body_string(app_err.error_response()), assert that body contains the same generic/internal message used elsewhere (e.g., the string checked in the DB test, such as "internal" or the standardized generic phrase) so the test verifies presence of the public message when AppError::Internal is used.apps/server/src/routes/ingest.rs (3)
18-49: ⚡ Quick winBoundary check is correct; consider a tiny capacity hint and a head-of-stream
Content-Lengthreject.The
buf.len() + chunk.len() > MAX_INGEST_PAYLOAD_BYTESguard correctly rejects before extending, so the buffer never grows past the cap — good. Two cheap optional improvements:
- Read
Content-Lengthfromreq.headers()first and short-circuit withPayloadTooLargebefore consuming any bytes. This avoids streaming and discarding ~100 MB from a misbehaving/malicious client just to reach the same verdict.- Pre-size
BytesMut::with_capacity(...)from a clampedContent-Lengthwhen present, to reduce reallocations on the hot path.♻️ Sketch
if let Some(len) = req .headers() .get(actix_web::http::header::CONTENT_LENGTH) .and_then(|v| v.to_str().ok()) .and_then(|s| s.parse::<usize>().ok()) { if len > MAX_INGEST_PAYLOAD_BYTES { return Err(AppError::PayloadTooLarge( "Payload exceeds maximum size".to_string(), )); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/routes/ingest.rs` around lines 18 - 49, The ingest_envelope handler currently streams the payload and checks buf.len() + chunk.len() against MAX_INGEST_PAYLOAD_BYTES, but you should early-reject by checking the request Content-Length and pre-size the buffer to avoid streaming large unwanted bodies and repeated reallocations: read req.headers() for actix_web::http::header::CONTENT_LENGTH, parse it to usize, return AppError::PayloadTooLarge immediately if it exceeds MAX_INGEST_PAYLOAD_BYTES, and when Content-Length is present and <= MAX_INGEST_PAYLOAD_BYTES initialize the accumulator with BytesMut::with_capacity(clamped_len) (falling back to a small default otherwise) before the payload loop in ingest_envelope.
17-18: Operational note: 100 MB × concurrency is the real memory ceiling.
MAX_INGEST_PAYLOAD_BYTES = 100 MBbounds a single request, but the ingest hot path is fully in-memory (Bytes→decompress_body→EnvelopeParser), so peak server memory scales with concurrent envelope uploads. Worth pairing this constant with one (or several) of:
- An app-level concurrency limiter / semaphore around
ingest_envelope(e.g.,tokio::sync::Semaphore) to cap simultaneous large bodies.- Documenting the expected
worker_threads × max body × decompress amplificationenvelope and surfacing it via metrics (in-flight ingest bytes, rejection counts forPayloadTooLarge).- Considering a lower per-request cap unless 100 MB envelopes are a real customer need — Sentry itself caps envelopes well below that.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/routes/ingest.rs` around lines 17 - 18, MAX_INGEST_PAYLOAD_BYTES only limits a single request but peak memory scales with concurrent uploads; wrap the ingest hot path (ingest_envelope → decompress_body → EnvelopeParser) with an application-level concurrency limiter (e.g., a tokio::sync::Semaphore or similar) to cap simultaneous large-body processing, acquire a permit at the start of ingest_envelope and release on completion/failure, and add basic metrics (in_flight_ingest_bytes, ingest_rejections_payload_too_large) so we can observe and reject when capacity is exceeded; alternatively reduce MAX_INGEST_PAYLOAD_BYTES if 100 MB per-request is not required.
146-156: 💤 Low valueLegacy
/store/still uses default-limitBytesextractor — inconsistent but probably fine.The deprecated
/store/handler still takes_body: Bytes, which uses Actix's default extractor size limit (different from the new 100 MB cap on/envelope/). Since this endpoint immediately returns aValidationerror and never inspects the body, the practical impact is just that a large/store/body may now produce a 400-ish "payload too large" coming from the extractor itself, rather than the friendly deprecation message. Worth either dropping theBytesextractor entirely (cheaper) or aligning with the new manual reader.♻️ Suggested simplification
pub async fn ingest_store( _pool: web::Data<DbPool>, _config: web::Data<Config>, _req: HttpRequest, _auth: SentryAuth, - _body: Bytes, ) -> AppResult<HttpResponse> { Err(AppError::Validation( "The /store/ endpoint is deprecated. Please use /envelope/ instead.".to_string(), )) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/routes/ingest.rs` around lines 146 - 156, The ingest_store handler currently accepts a _body: Bytes extractor which invokes Actix's default payload size limit and can yield a "payload too large" before returning the deprecation Validation error; remove the _body: Bytes parameter from the ingest_store function signature (keeping the function name ingest_store and the AppError::Validation return) so the handler no longer triggers the extractor, and update any route registration that references this handler signature if necessary; alternatively, if you prefer to preserve a body argument, replace Bytes with a non-extracting type (e.g., remove or accept HttpRequest only) or read the payload manually with the same 100MB limit used by the /envelope/ path to keep behavior consistent.apps/server/tests/integration/auth_test.rs (1)
1150-1245: ⚡ Quick winSession fixation test verifies
clear()but notrenew()cookie rotation.The test confirms attacker-planted session entries are not readable after login — that exercises
session.clear(). However, it does not assert that the session cookie value itself changed, which is the propertysession.renew()enforces. The login endpoint calls bothsession.clear()andsession.renew()(lines 176–177 inapps/server/src/routes/auth.rs), and withCookieSessionStorethe cookie value rotation is the actual identifier change that prevents session fixation.Adding
assert_ne!(new_session_cookie, session_cookie)provides complete M-2 coverage for session fixation defense (both data clearing and identifier rotation).♻️ Proposed addition
let new_session_cookie = login_resp .headers() .get("set-cookie") .unwrap() .to_str() .unwrap() .split(';') .next() .unwrap() .to_string(); + + // session.renew() must change the cookie value so the pre-login identifier + // cannot be reused post-auth (true session-fixation defense). + assert_ne!( + new_session_cookie, session_cookie, + "Session fixation: session identifier must change across login" + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/tests/integration/auth_test.rs` around lines 1150 - 1245, The test test_login_clears_pre_existing_session_data currently verifies session.clear() but misses checking cookie rotation from session.renew(); after you extract new_session_cookie (the response from the login call) add an assertion asserting the cookie changed (assert_ne!(new_session_cookie, session_cookie)) to ensure the session identifier was rotated when using CookieSessionStore; place this assertion immediately after obtaining new_session_cookie and before the subsequent request that reads the marker.apps/server/src/services/notification/webhook.rs (2)
305-324: ⚡ Quick winAdd a test for IPv4-mapped IPv6 (
::ffff:127.0.0.1).The
to_ipv4_mapped()branch inis_private_ipv6(line 229) is currently uncovered. A single assertion would lock in that behavior and guard against regressions, since this is exactly the kind of edge case attackers probe for.🧪 Suggested addition
#[test] fn test_rejects_ipv6_loopback() { let notifier = WebhookNotifier::new(); assert!(notifier .validate_config(&make_webhook_config("http://[::1]/hook")) .is_err()); + // IPv4-mapped IPv6 pointing at loopback must also be rejected. + assert!(notifier + .validate_config(&make_webhook_config("http://[::ffff:127.0.0.1]/hook")) + .is_err()); + // Unique-local and link-local IPv6 should be rejected. + assert!(notifier + .validate_config(&make_webhook_config("http://[fc00::1]/hook")) + .is_err()); + assert!(notifier + .validate_config(&make_webhook_config("http://[fe80::1]/hook")) + .is_err()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/services/notification/webhook.rs` around lines 305 - 324, Add a unit test that covers the IPv4-mapped IPv6 case so the to_ipv4_mapped() branch in is_private_ipv6 is exercised: create a test function (similar to test_rejects_ipv6_loopback) that constructs a WebhookNotifier and calls validate_config with a webhook URL using the IPv4-mapped IPv6 loopback (e.g. "http://[::ffff:127.0.0.1]/hook") and assert it returns Err; reference WebhookNotifier, validate_config, and is_private_ipv6/to_ipv4_mapped in the test name or comment to make the intent clear.
185-233: ⚡ Quick winConsider tightening the SSRF block list with a few more well-known ranges.
For defense in depth the IPv4/IPv6 classifiers miss a handful of ranges that are commonly abused or non-routable:
- IPv4:
100.64.0.0/10(CGNAT / shared address space),255.255.255.255(limited broadcast),224.0.0.0/4(multicast).- IPv6:
::(unspecified — can resolve to local services on some stacks),ff00::/8(multicast),64:ff9b::/96(well-known NAT64), and the deprecated IPv4-compatible form::a.b.c.d(noteto_ipv4_mappedonly catches::ffff:a.b.c.d, not the IPv4-compatible variant which would needto_ipv4()instead).None of these are exploitable by themselves in most deployments, but they're cheap to add and round out the policy. Alternatively, you can leverage
Ipv4Addr::is_loopback/is_private/is_link_local/is_broadcast/is_multicast/is_unspecifiedandIpv6Addr::is_loopback/is_unspecified/is_multicastfrom std to reduce hand-rolled bitmasks.♻️ Sketch of an std-based simplification
fn is_private_ipv4(ip: std::net::Ipv4Addr) -> bool { - let octets = ip.octets(); - if octets[0] == 127 { return true; } - if octets[0] == 0 { return true; } - if octets[0] == 10 { return true; } - if octets[0] == 172 && (16..=31).contains(&octets[1]) { return true; } - if octets[0] == 192 && octets[1] == 168 { return true; } - if octets[0] == 169 && octets[1] == 254 { return true; } - false + let o = ip.octets(); + ip.is_loopback() + || ip.is_private() + || ip.is_link_local() + || ip.is_broadcast() + || ip.is_multicast() + || ip.is_unspecified() + // CGNAT / shared address space (RFC 6598) + || (o[0] == 100 && (64..=127).contains(&o[1])) }
Ipv6Addrsimilarly exposesis_loopback,is_unspecified, andis_multicaston stable. The unique-local / link-local checks remain manual on stable Rust.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/services/notification/webhook.rs` around lines 185 - 233, The SSRF IP classifiers in is_private_ipv4 and is_private_ipv6 miss several non-routable/misused ranges; update is_private_ipv4 to also treat 100.64.0.0/10, 255.255.255.255 (limited broadcast), and 224.0.0.0/4 (multicast) as private/non-routable, and update is_private_ipv6 to treat :: (unspecified), ff00::/8 (multicast), and 64:ff9b::/96 (NAT64) as private; additionally detect IPv4-compatible IPv6 (::a.b.c.d) by using ip.to_ipv4() (in addition to to_ipv4_mapped()) and pass that IPv4 into is_private_ipv4; alternatively consider replacing some hand-rolled checks with std methods like Ipv4Addr::is_loopback/is_private/is_link_local/is_broadcast/is_multicast/is_unspecified and Ipv6Addr::is_loopback/is_unspecified/is_multicast where appropriate to simplify is_private_ipv4 and is_private_ipv6.apps/server/tests/integration/ingest_test.rs (1)
602-603: 💤 Low valueAvoid
expect(&format!(...)); preferunwrap_or_elsewith a panic.
.expect(&format!(...))allocates the message unconditionally on every call and is flagged by clippy'sexpect_fmt_panic_stringlint. In test code this is harmless but trivially fixable.♻️ Suggested change
- let json: Value = serde_json::from_str(body_str) - .expect(&format!("413 response must be JSON, got: {body_str}")); + let json: Value = serde_json::from_str(body_str) + .unwrap_or_else(|_| panic!("413 response must be JSON, got: {body_str}"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/tests/integration/ingest_test.rs` around lines 602 - 603, Replace the expect(&format!(...)) call on serde_json::from_str in apps/server/tests/integration/ingest_test.rs to avoid unconditional allocation: call serde_json::from_str(body_str).unwrap_or_else(|_| panic!("413 response must be JSON, got: {}", body_str)); this keeps the same panic message but builds it only on error—locate the usage that assigns to json (let json: Value = ...) and replace the expect with unwrap_or_else as described.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/error.rs`:
- Around line 64-66: The mapping for AppError::Database was changed to return
"InternalError" (see AppError::Database and the match arm returning
"InternalError"), which is intentional for H-3 but you must update documentation
and any mock test data that expects the old public contract; update the mock
event payload in apps/server/tests/integration/digest_test.rs (the test input
that contains "DatabaseError") to use "InternalError" or adjust the test comment
to clarify it's a mock, and add a note in the CHANGELOG describing this
deliberate breaking change to the JSON error_type contract for audit/security
purposes.
In `@apps/server/src/models/user.rs`:
- Around line 65-70: The run_dummy_password_verify function silently skips
Argon2 if PasswordHash::new(DUMMY_PASSWORD_HASH) fails; replace the `if let
Ok(parsed)` pattern with a forced parse (e.g.,
PasswordHash::new(DUMMY_PASSWORD_HASH).expect(...)) so malformed constant panics
immediately, and additionally parse-and-cache the resulting PasswordHash in a
OnceLock (or similar static cache) so you avoid re-parsing on every call but
still ensure the parser runs at startup; keep calling
Argon2::default().verify_password(&parsed) inside the function to preserve
timing-equalization semantics.
In `@apps/server/src/routes/ingest.rs`:
- Around line 38-40: The payload read error from payload.next() should not be
mapped to AppError::Internal; change the mapping to a client error (e.g.,
AppError::BadRequest or a new AppError::PayloadRead/BadRequest variant) and
return a 4xx response instead of a 500; update the code that currently does
chunk.map_err(|e| AppError::Internal(format!("Payload read error: {e}"))) to map
to the chosen 4xx variant (include the original error text in the message) and
avoid calling log::error! for this case (use debug or warn if you want to record
the event). Ensure you update any match/handler logic that converts AppError to
HTTP responses so this new/changed variant yields a 400-class status.
In `@apps/server/src/services/notification/webhook.rs`:
- Around line 178-183: Remove the dead wrapper function is_private_ip: it is
never called and duplicates logic already performed by is_ssrf_blocked_url_host
which calls is_private_ipv4 and is_private_ipv6 directly; delete the
is_private_ip function declaration to satisfy Clippy's dead_code lint and keep
only the direct uses of is_private_ipv4/is_private_ipv6.
- Around line 169-176: The hostname filter in is_blocked_hostname currently
misses hostnames with trailing dots (e.g., "localhost.") and so should normalize
the input by stripping any trailing dots and lowercasing before checks (e.g.,
use name.trim_end_matches('.').to_lowercase() or equivalent) and then apply the
existing blocked checks (localhost, ip6-localhost, ip6-loopback, .local,
.internal); additionally, update the webhook send() flow to mitigate DNS
rebinding by resolving the hostname at request time and re-running the
IP/address checks (or routing requests through a configured egress/proxy) so the
final destination IP cannot be an RFC1918/loopback address even if DNS changes
after config time.
In `@apps/server/tests/integration/ingest_test.rs`:
- Around line 580-598: The test currently posts 300KB but asserts a 413 even
though the handler uses streaming web::Payload and the actual enforced limits
are MAX_INGEST_PAYLOAD_BYTES (100MB), MAX_EVENT_SIZE (1MB in parser.rs), and
MAX_COMPRESSED_SIZE/MAX_DECOMPRESSED_SIZE (100MB); update the test to actually
exceed the intended limit or clarify which limit is being tested: either
increase the payload to >1MB (e.g., 1.1MB) to trigger MAX_EVENT_SIZE and keep
expecting 413, or change the test comment and assertion to reflect that 300KB
does not trigger any limit; locate the test block that builds the request and
adjust the large_body size or the expected status to match the chosen limit
(referencing MAX_EVENT_SIZE in parser.rs or
MAX_INGEST_PAYLOAD_BYTES/MAX_COMPRESSED_SIZE/MAX_DECOMPRESSED_SIZE as
appropriate).
---
Outside diff comments:
In `@apps/server/src/routes/auth.rs`:
- Around line 162-170: When the user is inactive (!user.is_active) run the same
expensive password verification as other failure paths to avoid a timing oracle:
call User::run_dummy_password_verify() (or user.verify_password(&req.password)
and ignore/handle its Result) before returning the AppError::Unauthorized;
ensure the call executes even if it fails, then return the same Unauthorized
response (consider using the same "Invalid credentials" text to avoid
message-based enumeration).
In `@apps/server/src/routes/ingest.rs`:
- Around line 33-64: The handler currently buffers the request body up to
MAX_INGEST_PAYLOAD_BYTES before performing the rate-limit check, which defeats
the "fail fast" intent; move the RateLimitService::check_quota call (using
pool.get_ref() and &auth.project) to run before the body collection loop so
rate-limited projects return TooManyRequests without allocating/reading the
body, then proceed to read the payload into buf/Bytes only after the quota check
succeeds.
---
Nitpick comments:
In `@apps/server/src/routes/ingest.rs`:
- Around line 18-49: The ingest_envelope handler currently streams the payload
and checks buf.len() + chunk.len() against MAX_INGEST_PAYLOAD_BYTES, but you
should early-reject by checking the request Content-Length and pre-size the
buffer to avoid streaming large unwanted bodies and repeated reallocations: read
req.headers() for actix_web::http::header::CONTENT_LENGTH, parse it to usize,
return AppError::PayloadTooLarge immediately if it exceeds
MAX_INGEST_PAYLOAD_BYTES, and when Content-Length is present and <=
MAX_INGEST_PAYLOAD_BYTES initialize the accumulator with
BytesMut::with_capacity(clamped_len) (falling back to a small default otherwise)
before the payload loop in ingest_envelope.
- Around line 17-18: MAX_INGEST_PAYLOAD_BYTES only limits a single request but
peak memory scales with concurrent uploads; wrap the ingest hot path
(ingest_envelope → decompress_body → EnvelopeParser) with an application-level
concurrency limiter (e.g., a tokio::sync::Semaphore or similar) to cap
simultaneous large-body processing, acquire a permit at the start of
ingest_envelope and release on completion/failure, and add basic metrics
(in_flight_ingest_bytes, ingest_rejections_payload_too_large) so we can observe
and reject when capacity is exceeded; alternatively reduce
MAX_INGEST_PAYLOAD_BYTES if 100 MB per-request is not required.
- Around line 146-156: The ingest_store handler currently accepts a _body: Bytes
extractor which invokes Actix's default payload size limit and can yield a
"payload too large" before returning the deprecation Validation error; remove
the _body: Bytes parameter from the ingest_store function signature (keeping the
function name ingest_store and the AppError::Validation return) so the handler
no longer triggers the extractor, and update any route registration that
references this handler signature if necessary; alternatively, if you prefer to
preserve a body argument, replace Bytes with a non-extracting type (e.g., remove
or accept HttpRequest only) or read the payload manually with the same 100MB
limit used by the /envelope/ path to keep behavior consistent.
In `@apps/server/src/services/notification/webhook.rs`:
- Around line 305-324: Add a unit test that covers the IPv4-mapped IPv6 case so
the to_ipv4_mapped() branch in is_private_ipv6 is exercised: create a test
function (similar to test_rejects_ipv6_loopback) that constructs a
WebhookNotifier and calls validate_config with a webhook URL using the
IPv4-mapped IPv6 loopback (e.g. "http://[::ffff:127.0.0.1]/hook") and assert it
returns Err; reference WebhookNotifier, validate_config, and
is_private_ipv6/to_ipv4_mapped in the test name or comment to make the intent
clear.
- Around line 185-233: The SSRF IP classifiers in is_private_ipv4 and
is_private_ipv6 miss several non-routable/misused ranges; update is_private_ipv4
to also treat 100.64.0.0/10, 255.255.255.255 (limited broadcast), and
224.0.0.0/4 (multicast) as private/non-routable, and update is_private_ipv6 to
treat :: (unspecified), ff00::/8 (multicast), and 64:ff9b::/96 (NAT64) as
private; additionally detect IPv4-compatible IPv6 (::a.b.c.d) by using
ip.to_ipv4() (in addition to to_ipv4_mapped()) and pass that IPv4 into
is_private_ipv4; alternatively consider replacing some hand-rolled checks with
std methods like
Ipv4Addr::is_loopback/is_private/is_link_local/is_broadcast/is_multicast/is_unspecified
and Ipv6Addr::is_loopback/is_unspecified/is_multicast where appropriate to
simplify is_private_ipv4 and is_private_ipv6.
In `@apps/server/tests/integration/auth_test.rs`:
- Around line 1150-1245: The test test_login_clears_pre_existing_session_data
currently verifies session.clear() but misses checking cookie rotation from
session.renew(); after you extract new_session_cookie (the response from the
login call) add an assertion asserting the cookie changed
(assert_ne!(new_session_cookie, session_cookie)) to ensure the session
identifier was rotated when using CookieSessionStore; place this assertion
immediately after obtaining new_session_cookie and before the subsequent request
that reads the marker.
In `@apps/server/tests/integration/ingest_test.rs`:
- Around line 602-603: Replace the expect(&format!(...)) call on
serde_json::from_str in apps/server/tests/integration/ingest_test.rs to avoid
unconditional allocation: call serde_json::from_str(body_str).unwrap_or_else(|_|
panic!("413 response must be JSON, got: {}", body_str)); this keeps the same
panic message but builds it only on error—locate the usage that assigns to json
(let json: Value = ...) and replace the expect with unwrap_or_else as described.
In `@apps/server/tests/unit/error_test.rs`:
- Around line 36-51: Add a positive assertion in
test_internal_error_does_not_leak_internal_details to ensure the generic error
text is returned (not just absence of DB details): after obtaining body via
body_string(app_err.error_response()), assert that body contains the same
generic/internal message used elsewhere (e.g., the string checked in the DB
test, such as "internal" or the standardized generic phrase) so the test
verifies presence of the public message when AppError::Internal is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1a07e36-e4f0-45e1-af7f-49ec40c33727
📒 Files selected for processing (10)
apps/server/src/error.rsapps/server/src/models/user.rsapps/server/src/routes/auth.rsapps/server/src/routes/ingest.rsapps/server/src/services/notification/webhook.rsapps/server/tests/integration/auth_test.rsapps/server/tests/integration/ingest_test.rsapps/server/tests/unit/error_test.rsapps/server/tests/unit/mod.rsapps/server/tests/unit/user_test.rs
| AppError::Database(_) => "InternalError", | ||
| AppError::Internal(_) => "InternalError", | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any remaining references to "DatabaseError" as an error_type value across the repo.
rg -nP '"DatabaseError"' -C2
# Also look for client-side handling keyed off the previous type string.
rg -nP "DatabaseError" --type=ts --type=tsx --type=js --type=rs -C2Repository: AbianS/rustrak
Length of output: 519
🏁 Script executed:
# First, verify the exact code in error.rs around lines 64-66
sed -n '55,80p' apps/server/src/error.rsRepository: AbianS/rustrak
Length of output: 1130
🏁 Script executed:
# Search more broadly for all "DatabaseError" references without type restrictions
rg -n "DatabaseError" --no-headingRepository: AbianS/rustrak
Length of output: 152
🏁 Script executed:
# Check the test file that was flagged to understand its context
sed -n '130,150p' apps/server/tests/integration/digest_test.rsRepository: AbianS/rustrak
Length of output: 780
🏁 Script executed:
# Look at the full test function to understand what it's testing
rg -B20 "DatabaseError" apps/server/tests/integration/digest_test.rs | head -50Repository: AbianS/rustrak
Length of output: 884
🏁 Script executed:
# Check if there are any assertions or expectations about the "DatabaseError" type value
rg -A10 "DatabaseError" apps/server/tests/integration/digest_test.rsRepository: AbianS/rustrak
Length of output: 390
🏁 Script executed:
# Check if there's an API endpoint that returns these error responses
rg -n "error_response" apps/server/src/ -A5 -B2Repository: AbianS/rustrak
Length of output: 587
🏁 Script executed:
# Look at what calls error_response() - does it go to the HTTP response?
rg -n "error_response\|ErrorResponse" apps/server/src/ --type=rs | head -20Repository: AbianS/rustrak
Length of output: 85
🏁 Script executed:
# Check the full context of what this test is actually validating
sed -n '100,200p' apps/server/tests/integration/digest_test.rsRepository: AbianS/rustrak
Length of output: 3392
🏁 Script executed:
# Check if error_response() is actually used to return errors to clients over HTTP
rg -n "error_response\|impl.*ResponseError" apps/server/src/ --type=rust -B2 -A5Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Search for where Database errors would be returned in API responses
rg -n "AppError::Database" apps/server/src/ -B3 -A3 | head -40Repository: AbianS/rustrak
Length of output: 2222
🏁 Script executed:
# Check if there are any integration tests that call the actual API and check error responses
rg -n "DatabaseError" apps/server/tests/ --type=rust -B5 -A5Repository: AbianS/rustrak
Length of output: 964
🏁 Script executed:
# Check if error_response() is actually used to construct HTTP responses
rg -n "error_response()" apps/server/src/ -B3 -A3Repository: AbianS/rustrak
Length of output: 463
🏁 Script executed:
# Check the ResponseError implementation to understand how errors are serialized
rg -n "impl.*ResponseError\|fn error_response" apps/server/src/error.rs -B2 -A10Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Search for any tests that actually call the API and verify error response content
rg -n "assert.*error_type\|assert.*DatabaseError\|assert.*InternalError" apps/server/tests/ -B2 -A2Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Check if error_response is actually called anywhere
rg -n "error_response" apps/server/src/ --type=rustRepository: AbianS/rustrak
Length of output: 133
🏁 Script executed:
# Check how AppError is used in handlers - look for ResponseError trait implementation
rg -n "impl.*ResponseError.*for.*AppError" apps/server/src/ -A30Repository: AbianS/rustrak
Length of output: 2355
🏁 Script executed:
# Look for where http errors are actually returned in handlers
rg -n "return Err\|Err(AppError" apps/server/src/handlers/ --type=rust -B2 -A2 | head -60Repository: AbianS/rustrak
Length of output: 149
🏁 Script executed:
# Look for where AppError is returned in handlers
rg -n "Err(AppError" apps/server/src/handlers/ -B2 -A2 | head -80Repository: AbianS/rustrak
Length of output: 125
🏁 Script executed:
# Check if there are any API integration tests that verify the actual HTTP response
find apps/server/tests -name "*.rs" -type f | xargs grep -l "assert\|expect" | head -5Repository: AbianS/rustrak
Length of output: 270
🏁 Script executed:
# Look for tests that verify error responses contain error_type field
rg -n "error_type\|error\.type" apps/server/tests/ -B3 -A3Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Look for actual HTTP response testing - check if any tests call endpoints and check response status/body
rg -n "status\|response\|assert_eq" apps/server/tests/integration/auth_test.rs -B2 -A2 | head -80Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# See if there are any tests verifying HTTP responses contain error_type
rg -n "json\|body\|error" apps/server/tests/integration/alerts_api_test.rs -B1 -A1 | head -60Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Check if error_response is actually being tested anywhere
find apps/server/tests -name "*.rs" -exec grep -l "error_response\|InternalError" {} \;Repository: AbianS/rustrak
Length of output: 95
🏁 Script executed:
# Check the error unit tests
cat apps/server/tests/unit/error_test.rsRepository: AbianS/rustrak
Length of output: 2066
🏁 Script executed:
# Get the full error_response implementation to see what JSON structure is returned
sed -n '57,90p' apps/server/src/error.rsRepository: AbianS/rustrak
Length of output: 1369
🏁 Script executed:
# Check if there's any JSON serialization in the response
rg -n "json\|serde_json\|error_type" apps/server/src/error.rs -B2 -A2Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Look at the response structure - what does the response body contain?
rg -n "HttpResponse\|Json\|json!" apps/server/src/error.rs -B2 -A2Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Get the ErrorResponse and ErrorDetail struct definitions
rg -n "struct ErrorResponse\|struct ErrorDetail" apps/server/src/ -A5Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Check if any tests actually verify the error_type value in HTTP responses
rg -n "error_type" apps/server/tests/unit/error_test.rs -B3 -A3Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Look for e2e tests that might call the API and verify error responses
rg -n "assert.*error" apps/server/tests/e2e/sentry_sdk_test.rs -B2 -A2 | head -60Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Search for ErrorResponse struct definition
rg -n "ErrorResponse\|ErrorDetail" apps/server/src/error.rs -B2 -A3Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Check if the unit tests verify error_type field value
cat apps/server/tests/unit/error_test.rs | grep -n "error_type\|InternalError\|DatabaseError"Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# See all tests in the unit error test file to check if error_type is verified anywhere
cat apps/server/tests/unit/error_test.rsRepository: AbianS/rustrak
Length of output: 2066
🏁 Script executed:
# Check if the error_type value is actually verified in unit tests by searching for assertions on it
grep -n "error_type\|DatabaseError\|InternalError" apps/server/tests/unit/error_test.rsRepository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Check the complete error.rs file to find ErrorResponse/ErrorDetail struct definitions
cat apps/server/src/error.rs | head -100Repository: AbianS/rustrak
Length of output: 3012
🏁 Script executed:
# Check if there are any tests that actually call the API and parse error responses
find apps/server/tests -name "*.rs" -type f -exec grep -l "error_response\|error.*type\|DatabaseError.*api" {} \;Repository: AbianS/rustrak
Length of output: 359
🏁 Script executed:
# Search for API tests that might verify error response structure
rg -n "assert.*error\|response.*error" apps/server/tests/integration/ --type=rust -B2 -A2 | head -40Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Check if there are any documented breaking changes or migration notes
find . -name "CHANGELOG*" -o -name "BREAKING*" -o -name "MIGRATION*" 2>/dev/nullRepository: AbianS/rustrak
Length of output: 207
🏁 Script executed:
# Check the server CHANGELOG for any mentions of this error type change
cat apps/server/CHANGELOG.md | head -100Repository: AbianS/rustrak
Length of output: 2785
🏁 Script executed:
# Search for any API integration tests that verify error response structure/content
cat apps/server/tests/integration/auth_test.rs | grep -n "assert\|error\|type" | head -40Repository: AbianS/rustrak
Length of output: 1852
🏁 Script executed:
# Check what happens with the event after it's stored in digest_test - does it verify the error type?
sed -n '120,180p' apps/server/tests/integration/digest_test.rsRepository: AbianS/rustrak
Length of output: 2022
🏁 Script executed:
# Check if there are any other references to "DatabaseError" as a literal in the codebase
rg -rn "DatabaseError" . --type=rust --type=json --type=yamlRepository: AbianS/rustrak
Length of output: 138
🏁 Script executed:
# Verify if any actual API handlers return Database errors to clients
rg -n "AppError::Database" apps/server/src/handlers/ -B3 -A3Repository: AbianS/rustrak
Length of output: 124
Clarify: the "DatabaseError" reference found is in test input data, not API response validation.
The change from "DatabaseError" to "InternalError" for AppError::Database errors is intentional for H-3 (preventing database-specific error leakage). A search found one reference to "DatabaseError" in apps/server/tests/integration/digest_test.rs:139, but this is a mock event payload used as input to test event grouping logic—not a validation of actual API error responses. No client-side code (frontend, SDKs, tests) in the repository that depends on parsing error_type == "DatabaseError" was found. This is a breaking change to the public JSON contract, but no actual clients are affected within this codebase. Consider documenting this security-focused change in the CHANGELOG.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/error.rs` around lines 64 - 66, The mapping for
AppError::Database was changed to return "InternalError" (see AppError::Database
and the match arm returning "InternalError"), which is intentional for H-3 but
you must update documentation and any mock test data that expects the old public
contract; update the mock event payload in
apps/server/tests/integration/digest_test.rs (the test input that contains
"DatabaseError") to use "InternalError" or adjust the test comment to clarify
it's a mock, and add a note in the CHANGELOG describing this deliberate breaking
change to the JSON error_type contract for audit/security purposes.
| pub fn run_dummy_password_verify(password: &str) -> bool { | ||
| if let Ok(parsed) = PasswordHash::new(DUMMY_PASSWORD_HASH) { | ||
| let _ = Argon2::default().verify_password(password.as_bytes(), &parsed); | ||
| } | ||
| false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the Argon2::default() params in the argon2 crate v0.5.3 (m_cost, t_cost, p_cost)?
💡 Result:
Argon2::default (from argon2 crate v0.5.3) uses these Params defaults: - m_cost = 19_456 (KiB) - t_cost = 2 - p_cost = 1 These values come from the crate’s default Params constants (DEFAULT_M_COST, DEFAULT_T_COST, DEFAULT_P_COST) used as the “default set of Params” configuration for Argon2/Argon2::default. [1][2]
Citations:
- 1: https://docs.rs/argon2/latest/argon2/struct.Params.html
- 2: https://docs.rs/argon2/0.5.3/argon2/struct.Argon2.html
🏁 Script executed:
# Locate and read the file
fd -t f "user.rs" apps/server/src/models/ 2>/dev/null | head -5Repository: AbianS/rustrak
Length of output: 89
🏁 Script executed:
# Read the specific file to verify the code and context
cat -n apps/server/src/models/user.rs | sed -n '1,100p'Repository: AbianS/rustrak
Length of output: 3035
🏁 Script executed:
# Search for DUMMY_PASSWORD_HASH constant and any tests
rg -n "DUMMY_PASSWORD_HASH|run_dummy_password_verify" apps/server/src/models/Repository: AbianS/rustrak
Length of output: 326
🏁 Script executed:
# Search for tests of run_dummy_password_verify
rg -n "run_dummy_password_verify" apps/server/src/ -A 5 -B 2Repository: AbianS/rustrak
Length of output: 1256
🏁 Script executed:
# Check if there's a test module in user.rs or separate test file
fd -t f "*.rs" apps/server/src/ | xargs rg -l "run_dummy_password_verify" 2>/dev/nullRepository: AbianS/rustrak
Length of output: 458
🏁 Script executed:
# Look for AppError definition and error handling patterns
rg -n "enum AppError|impl.*AppError" apps/server/src/error.rs | head -20Repository: AbianS/rustrak
Length of output: 118
🏁 Script executed:
# Read the test file to check existing tests
cat -n apps/server/tests/unit/user_test.rsRepository: AbianS/rustrak
Length of output: 1022
🏁 Script executed:
# Read the full AppError enum definition
cat -n apps/server/src/error.rsRepository: AbianS/rustrak
Length of output: 3670
🏁 Script executed:
# Check the Argon2 crate version used
rg "argon2" apps/server/Cargo.tomlRepository: AbianS/rustrak
Length of output: 75
🏁 Script executed:
# Check if there are any hardening measures in place (e.g., OnceLock, LazyLock)
rg -n "OnceLock|LazyLock|once_cell" apps/server/src/Repository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
# Verify the constant format is valid by checking if any linting or build time checks exist
cat -n apps/server/Cargo.toml | head -30Repository: AbianS/rustrak
Length of output: 918
Silent failure path defeats the timing-equalization goal.
If PasswordHash::new(DUMMY_PASSWORD_HASH) ever fails (typo, format change, Argon2 crate version bump altering parser strictness), the function silently returns false without running Argon2 — at which point H-1 regresses without any test failing. The current unit tests only assert the return value is false; they cannot distinguish "ran Argon2 then returned false" from "skipped Argon2 then returned false".
Two cheap hardenings worth considering:
- Replace
if let Ok(parsed)with.expect()— the constant is compile-time validated, so panicking on malformed input catches regressions immediately. - Cache the parsed
PasswordHashin aOnceLockto avoid re-parsing on every login miss.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/models/user.rs` around lines 65 - 70, The
run_dummy_password_verify function silently skips Argon2 if
PasswordHash::new(DUMMY_PASSWORD_HASH) fails; replace the `if let Ok(parsed)`
pattern with a forced parse (e.g.,
PasswordHash::new(DUMMY_PASSWORD_HASH).expect(...)) so malformed constant panics
immediately, and additionally parse-and-cache the resulting PasswordHash in a
OnceLock (or similar static cache) so you avoid re-parsing on every call but
still ensure the parser runs at startup; keep calling
Argon2::default().verify_password(&parsed) inside the function to preserve
timing-equalization semantics.
| while let Some(chunk) = payload.next().await { | ||
| let chunk = | ||
| chunk.map_err(|e| AppError::Internal(format!("Payload read error: {e}")))?; |
There was a problem hiding this comment.
Avoid wrapping stream read failures as AppError::Internal.
A payload.next() error is typically a client-side transport problem (aborted connection, malformed chunked encoding, TLS reset) rather than a server fault. Mapping it to AppError::Internal produces a 500 + a server-side log::error! entry for what is in most cases not an internal bug, which will pollute alerting and metrics. A 400-class error (or a dedicated BadRequest) better reflects the cause.
♻️ Suggested fix
- let chunk =
- chunk.map_err(|e| AppError::Internal(format!("Payload read error: {e}")))?;
+ let chunk = chunk.map_err(|e| {
+ log::warn!("Payload read error from client: {e}");
+ AppError::Validation("Failed to read request body".to_string())
+ })?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while let Some(chunk) = payload.next().await { | |
| let chunk = | |
| chunk.map_err(|e| AppError::Internal(format!("Payload read error: {e}")))?; | |
| while let Some(chunk) = payload.next().await { | |
| let chunk = chunk.map_err(|e| { | |
| log::warn!("Payload read error from client: {e}"); | |
| AppError::Validation("Failed to read request body".to_string()) | |
| })?; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/routes/ingest.rs` around lines 38 - 40, The payload read
error from payload.next() should not be mapped to AppError::Internal; change the
mapping to a client error (e.g., AppError::BadRequest or a new
AppError::PayloadRead/BadRequest variant) and return a 4xx response instead of a
500; update the code that currently does chunk.map_err(|e|
AppError::Internal(format!("Payload read error: {e}"))) to map to the chosen 4xx
variant (include the original error text in the message) and avoid calling
log::error! for this case (use debug or warn if you want to record the event).
Ensure you update any match/handler logic that converts AppError to HTTP
responses so this new/changed variant yields a 400-class status.
| fn is_blocked_hostname(name: &str) -> bool { | ||
| let lower = name.to_lowercase(); | ||
| lower == "localhost" | ||
| || lower == "ip6-localhost" | ||
| || lower == "ip6-loopback" | ||
| || lower.ends_with(".local") | ||
| || lower.ends_with(".internal") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Quickly confirm url crate normalization behavior for trailing dot / uppercase / numeric IPv4 forms.
# (No repo lookup needed; this is a sanity check on the url crate version in use.)
rg -nP '^\s*url\s*=' apps/server/Cargo.tomlRepository: AbianS/rustrak
Length of output: 75
🏁 Script executed:
cat -n apps/server/src/services/notification/webhook.rs | head -200 | tail -50Repository: AbianS/rustrak
Length of output: 1808
🏁 Script executed:
rg -n "is_blocked_hostname|is_private_ip" apps/server/src/services/notification/webhook.rs -B 3 -A 3Repository: AbianS/rustrak
Length of output: 1426
🏁 Script executed:
rg -n "is_private_ip" apps/server/src/ -t rustRepository: AbianS/rustrak
Length of output: 893
🏁 Script executed:
rg -n "is_ssrf_blocked_url_host|validate_webhook_url" apps/server/src/services/notification/webhook.rs -B 5 -A 5Repository: AbianS/rustrak
Length of output: 885
🏁 Script executed:
rg -n "validate_webhook_url" apps/server/src/ -t rustRepository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
cat -n apps/server/src/services/notification/webhook.rs | head -160 | tail -50Repository: AbianS/rustrak
Length of output: 2233
🏁 Script executed:
rg -n "fn validate|struct.*Webhook|impl.*Webhook" apps/server/src/services/notification/webhook.rs -B 2 -A 5 | head -100Repository: AbianS/rustrak
Length of output: 1171
🏁 Script executed:
cat -n apps/server/src/services/notification/webhook.rs | head -90 | tail -30Repository: AbianS/rustrak
Length of output: 1347
🏁 Script executed:
cd apps/server && python3 << 'EOF'
import subprocess
import sys
# Check url crate documentation and behavior for trailing dots
# We'll create a quick test to see how url::Url parses domains with trailing dots
test_code = '''
use url::Url;
fn main() {
let test_urls = vec![
"http://localhost./webhook",
"http://localhost/webhook",
"http://example.com./webhook",
"http://LOCALHOST/webhook",
"http://LOcalhost/webhook",
];
for url_str in test_urls {
if let Ok(url) = Url::parse(url_str) {
if let Some(host) = url.host() {
println!("URL: {} => Host: {:?}", url_str, host);
}
}
}
}
'''
print("Note: url crate v2.5.8 behavior with trailing dots:")
print("url::Url::parse will include trailing dots in domain names.")
print("url::Host<&str>::Domain preserves the exact string including dots.")
print()
print("The is_blocked_hostname function receives the domain string as-is,")
print("so 'localhost.' would NOT match the exact string comparison with 'localhost'")
EOFRepository: AbianS/rustrak
Length of output: 382
🏁 Script executed:
rg -n "validate_config|WebhookNotifier" apps/server/src/ -t rust | head -30Repository: AbianS/rustrak
Length of output: 3450
🏁 Script executed:
rg -n "localhost\." apps/server/src/services/notification/webhook.rsRepository: AbianS/rustrak
Length of output: 40
🏁 Script executed:
cat -n apps/server/src/services/notification/webhook.rs | sed -n '249,315p'Repository: AbianS/rustrak
Length of output: 2790
🏁 Script executed:
cat -n apps/server/src/services/notification/webhook.rs | sed -n '313,330p'Repository: AbianS/rustrak
Length of output: 762
🏁 Script executed:
rg -n "send_alert|send\(" apps/server/src/services/notification/webhook.rs -A 3 | head -30Repository: AbianS/rustrak
Length of output: 381
🏁 Script executed:
rg -n "resolve|dns|ip" apps/server/src/services/notification/webhook.rs -iRepository: AbianS/rustrak
Length of output: 966
Hostname filter can be bypassed with trailing dots; also note DNS rebinding risk.
Two valid concerns:
-
lower == "localhost"won't match the FQDN form"localhost."(trailing dot). Theurlcrate v2.5.8 preserves trailing dots in domain strings, andis_blocked_hostnamereceives the string as-is without normalization. Worth fixing withlower.trim_end_matches('.')or similar. -
SSRF validation runs only at webhook config time. An attacker-controlled public domain configured at setup can still resolve to RFC1918 addresses at request time (DNS rebinding). The
send()method uses the stored URL without re-validating the resolved IP. If this server has access to internal networks, consider resolving the hostname at send time and re-applying IP checks, or routing through an egress proxy.
The first is a low-effort fix; the second is an operational consideration.
♻️ Suggested normalization for point 1
fn is_blocked_hostname(name: &str) -> bool {
- let lower = name.to_lowercase();
- lower == "localhost"
- || lower == "ip6-localhost"
- || lower == "ip6-loopback"
- || lower.ends_with(".local")
- || lower.ends_with(".internal")
+ let lower = name.to_lowercase();
+ let host = lower.trim_end_matches('.');
+ host == "localhost"
+ || host == "ip6-localhost"
+ || host == "ip6-loopback"
+ || host.ends_with(".local")
+ || host.ends_with(".internal")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn is_blocked_hostname(name: &str) -> bool { | |
| let lower = name.to_lowercase(); | |
| lower == "localhost" | |
| || lower == "ip6-localhost" | |
| || lower == "ip6-loopback" | |
| || lower.ends_with(".local") | |
| || lower.ends_with(".internal") | |
| } | |
| fn is_blocked_hostname(name: &str) -> bool { | |
| let lower = name.to_lowercase(); | |
| let host = lower.trim_end_matches('.'); | |
| host == "localhost" | |
| || host == "ip6-localhost" | |
| || host == "ip6-loopback" | |
| || host.ends_with(".local") | |
| || host.ends_with(".internal") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/services/notification/webhook.rs` around lines 169 - 176, The
hostname filter in is_blocked_hostname currently misses hostnames with trailing
dots (e.g., "localhost.") and so should normalize the input by stripping any
trailing dots and lowercasing before checks (e.g., use
name.trim_end_matches('.').to_lowercase() or equivalent) and then apply the
existing blocked checks (localhost, ip6-localhost, ip6-loopback, .local,
.internal); additionally, update the webhook send() flow to mitigate DNS
rebinding by resolving the hostname at request time and re-running the
IP/address checks (or routing requests through a configured egress/proxy) so the
final destination IP cannot be an RFC1918/loopback address even if DNS changes
after config time.
| fn is_private_ip(ip: std::net::IpAddr) -> bool { | ||
| match ip { | ||
| std::net::IpAddr::V4(v4) => is_private_ipv4(v4), | ||
| std::net::IpAddr::V6(v6) => is_private_ipv6(v6), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm there are no other callers of `is_private_ip` in the crate.
rg -nP '\bis_private_ip\s*\(' --type=rustRepository: AbianS/rustrak
Length of output: 160
🏁 Script executed:
# Read the webhook.rs file to see the full context of is_private_ip and is_ssrf_blocked_url_host
cat apps/server/src/services/notification/webhook.rs | head -220 | tail -100Repository: AbianS/rustrak
Length of output: 3174
is_private_ip is unused — remove this dead code.
The function has no callers in the codebase. is_ssrf_blocked_url_host handles IP validation by dispatching directly to is_private_ipv4 and is_private_ipv6, bypassing this wrapper entirely. Clippy's dead_code lint will flag this during CI.
♻️ Suggested removal
-fn is_private_ip(ip: std::net::IpAddr) -> bool {
- match ip {
- std::net::IpAddr::V4(v4) => is_private_ipv4(v4),
- std::net::IpAddr::V6(v6) => is_private_ipv6(v6),
- }
-}
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn is_private_ip(ip: std::net::IpAddr) -> bool { | |
| match ip { | |
| std::net::IpAddr::V4(v4) => is_private_ipv4(v4), | |
| std::net::IpAddr::V6(v6) => is_private_ipv6(v6), | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/services/notification/webhook.rs` around lines 178 - 183,
Remove the dead wrapper function is_private_ip: it is never called and
duplicates logic already performed by is_ssrf_blocked_url_host which calls
is_private_ipv4 and is_private_ipv6 directly; delete the is_private_ip function
declaration to satisfy Clippy's dead_code lint and keep only the direct uses of
is_private_ipv4/is_private_ipv6.
| // 300KB plain body — exceeds actix-web default 256KB Bytes extractor limit | ||
| let large_body = vec![b'x'; 300 * 1024]; | ||
|
|
||
| let req = test::TestRequest::post() | ||
| .uri(&format!("/api/{}/envelope/", project_id)) | ||
| .insert_header(( | ||
| "X-Sentry-Auth", | ||
| format!("Sentry sentry_key={}, sentry_version=7", sentry_key), | ||
| )) | ||
| .set_payload(large_body) | ||
| .to_request(); | ||
|
|
||
| let resp = test::call_service(&app, req).await; | ||
| assert_eq!( | ||
| resp.status(), | ||
| 413, | ||
| "Oversized body must return 413, not {}", | ||
| resp.status() | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the ingest handler and identify the actual payload cap constant/literal it enforces.
fd -t f 'ingest.rs' apps/server/src
rg -nP -C5 '(MAX_INGEST_PAYLOAD_BYTES|PayloadTooLarge|to_bytes|Payload)' apps/server/src/routes/ingest.rs
rg -nP -C2 'MAX_INGEST_PAYLOAD_BYTES\s*[:=]'Repository: AbianS/rustrak
Length of output: 1700
🏁 Script executed:
# View the test to understand what it's testing
head -n 610 apps/server/tests/integration/ingest_test.rs | tail -n 50Repository: AbianS/rustrak
Length of output: 1720
🏁 Script executed:
# Check if there are other payload size tests
rg -n 'large_body|oversized|payload.*size|413' apps/server/tests/integration/ingest_test.rsRepository: AbianS/rustrak
Length of output: 353
🏁 Script executed:
# Search for any configuration of actix-web limits in the test setup
rg -n 'HttpServer|actix|JsonConfig|PayloadConfig|256|limit' apps/server/tests/integration/ingest_test.rs | head -30Repository: AbianS/rustrak
Length of output: 1291
🏁 Script executed:
# Search for any actix payload configuration or default limits
rg -n 'payload_limit|max_body_size|PayloadSize|256|DEFAULT' apps/server/src --type rsRepository: AbianS/rustrak
Length of output: 85
🏁 Script executed:
# Check actix-web version and see if there's implicit payload limiting
rg -n 'actix-web' apps/server/Cargo.tomlRepository: AbianS/rustrak
Length of output: 328
🏁 Script executed:
# Look at the error response handling to see how PayloadTooLarge becomes 413
rg -n -A5 'impl.*Error|PayloadTooLarge|to_response' apps/server/src/errors.rs | head -50Repository: AbianS/rustrak
Length of output: 124
🏁 Script executed:
# Find error-related files
fd -t f 'error' apps/server/srcRepository: AbianS/rustrak
Length of output: 83
🏁 Script executed:
# Search for PayloadTooLarge error definition and handling
rg -n 'PayloadTooLarge' apps/server/srcRepository: AbianS/rustrak
Length of output: 789
🏁 Script executed:
# Check main.rs or lib.rs to understand error handling
rg -n 'AppError|to_response|impl.*Error' apps/server/src --max-count=50Repository: AbianS/rustrak
Length of output: 19266
🏁 Script executed:
# Read the error.rs file to see PayloadTooLarge handling
cat -n apps/server/src/error.rsRepository: AbianS/rustrak
Length of output: 3670
🏁 Script executed:
# Check decompression.rs and parser.rs to see if they also check payload limits
cat -n apps/server/src/ingest/decompression.rs | head -50Repository: AbianS/rustrak
Length of output: 2053
🏁 Script executed:
# Check what triggers PayloadTooLarge in the decompression and parser
rg -n -B2 -A2 'PayloadTooLarge' apps/server/src/ingest/Repository: AbianS/rustrak
Length of output: 1711
🏁 Script executed:
# Check MAX_EVENT_SIZE constant in parser.rs
rg -n 'MAX_EVENT_SIZE' apps/server/src/ingest/parser.rsRepository: AbianS/rustrak
Length of output: 232
🏁 Script executed:
# Look at the full parser.rs to see all size limits
head -n 100 apps/server/src/ingest/parser.rsRepository: AbianS/rustrak
Length of output: 3075
🏁 Script executed:
# Check the test to see if it verifies the error message content
sed -n '590,610p' apps/server/tests/integration/ingest_test.rsRepository: AbianS/rustrak
Length of output: 624
Fix the test to accurately reflect payload size limits.
The test sends 300KB and expects PayloadTooLarge (413), but 300KB does not exceed any of the actual limits in the code:
- Raw payload collection checks against
MAX_INGEST_PAYLOAD_BYTES = 100MB(line 41–44 of ingest.rs) - Individual envelope items are checked against
MAX_EVENT_SIZE = 1MB(line 62–65 of parser.rs) - Decompression checks against
MAX_COMPRESSED_SIZEandMAX_DECOMPRESSED_SIZE(both 100MB in decompression.rs)
Since the handler uses web::Payload (streaming) rather than the web::Bytes extractor, the test comment about "actix-web default 256KB limit" is incorrect. Either send a body large enough to actually exceed one of the limits (e.g., >1MB to trigger MAX_EVENT_SIZE), or clarify which limit is being tested. The test assertion should match the limit it intends to verify.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/tests/integration/ingest_test.rs` around lines 580 - 598, The
test currently posts 300KB but asserts a 413 even though the handler uses
streaming web::Payload and the actual enforced limits are
MAX_INGEST_PAYLOAD_BYTES (100MB), MAX_EVENT_SIZE (1MB in parser.rs), and
MAX_COMPRESSED_SIZE/MAX_DECOMPRESSED_SIZE (100MB); update the test to actually
exceed the intended limit or clarify which limit is being tested: either
increase the payload to >1MB (e.g., 1.1MB) to trigger MAX_EVENT_SIZE and keep
expecting 413, or change the test comment and assertion to reflect that 300KB
does not trigger any limit; locate the test block that builds the request and
adjust the large_body size or the expected status to match the chosen limit
(referencing MAX_EVENT_SIZE in parser.rs or
MAX_INGEST_PAYLOAD_BYTES/MAX_COMPRESSED_SIZE/MAX_DECOMPRESSED_SIZE as
appropriate).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| #[actix_web::test] | ||
| async fn test_register_rejects_password_shorter_than_8_chars() { | ||
| let db = TestDb::new().await; | ||
| let session_key = Key::from(&[0u8; 64]); |
| #[actix_web::test] | ||
| async fn test_register_rejects_password_longer_than_1024_chars() { | ||
| let db = TestDb::new().await; | ||
| let session_key = Key::from(&[0u8; 64]); |
| #[actix_web::test] | ||
| async fn test_register_accepts_password_at_exact_boundaries() { | ||
| let db = TestDb::new().await; | ||
| let session_key = Key::from(&[0u8; 64]); |
| #[actix_web::test] | ||
| async fn test_login_rejects_password_longer_than_1024_chars() { | ||
| let db = TestDb::new().await; | ||
| let session_key = Key::from(&[0u8; 64]); |
|
|
||
| let db = TestDb::new().await; | ||
| let config = create_test_config(); | ||
| let session_key = Key::from(&[0u8; 64]); |
| let config = create_test_config(); | ||
| let session_key = Key::from(&[0u8; 64]); | ||
|
|
||
| create_test_user(&db.pool, "sessfix@example.com", "password123", false).await; |
| #[test] | ||
| fn test_dummy_password_verify_always_returns_false() { | ||
| // Must never grant access — runs Argon2 to equalize timing with real verify | ||
| assert!(!User::run_dummy_password_verify("anypassword")); |
|
|
||
| #[test] | ||
| fn test_dummy_password_verify_with_empty_password() { | ||
| assert!(!User::run_dummy_password_verify("")); |
2 participants
Summary
Security audit of
apps/server/src/found 4 HIGH and 4 MEDIUM issues. This PR fixes 6 of them using TDD (failing test written before each fix).User::run_dummy_password_verify()runs Argon2 against a dummy hash when login email doesn't exist, equalizing response time with the wrong-password pathAppError::DatabaseandAppError::Internalnow log full detail server-side and return a generic"An internal error occurred"to clientsvalidate_configblocks loopback, RFC1918, link-local (169.254.x.x), and dangerous hostnames (localhost,*.local,*.internal)Payloadmanually with a 100MB cap, returningAppError::PayloadTooLarge(413 JSON) instead of actix-web's default plain-text responsesession.clear() + session.renew()called beforeset_user_sessionon both login and register to regenerate the session tokenTest plan
tests/unit/error_test.rs,tests/unit/user_test.rstests/integration/auth_test.rs,tests/integration/ingest_test.rs,src/services/notification/webhook.rs(inline)cargo test --no-default-features --features postgres— 135 unit + 97 integration, 0 failuresSummary by CodeRabbit
Bug Fixes
New Features