Add IP-based rate limiting for referral invites#2106
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR introduces a new ip_extractor crate with a ClientIp extractor and ClientIpError response mapping, removes model::tracking::IPContext, and re-exports ip_extractor from macro_middleware::tracking. Multiple Axum handlers and middleware were updated to accept ClientIp and removed IP-context-attaching middleware from several routers. The rate_limit domain was refactored to Result-style outcomes (RateLimitOk/RateLimitExceeded), value-based key/config APIs, decrement/rollback semantics, and an axum-gated inbound module with RateLimitExtractor and rate_limit_middleware. ApiContext gained a rate_limit_service field; referral router state and handlers were generalized to accept a rate limiter type. FusionAuth APIs now use std::net::IpAddr. Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/cloud-storage/rate_limit/src/outbound/redis/test.rs (1)
49-63:⚠️ Potential issue | 🔴 CriticalFix pattern matching:
Okis a tuple variant, not a struct.
RateLimitResult::Ok { .. }won't compile. SinceRateLimitResultisResult<RateLimitOk, RateLimitExceeded>,Okis a tuple variant and must be matched asOk(_).Suggested fix
- RateLimitResult::Ok { .. } => { + RateLimitResult::Ok(_) => { panic!("expected rate limit to be exceeded after max_count requests"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/rate_limit/src/outbound/redis/test.rs` around lines 49 - 63, The test's match arm uses `RateLimitResult::Ok { .. }` but `RateLimitResult` is a `Result<RateLimitOk, RateLimitExceeded>` so `Ok` is a tuple variant; change the match arm to use the correct tuple pattern (e.g., `Ok(_)` or `Ok(rate_limit_ok)`) instead of `RateLimitResult::Ok { .. }` so the pattern compiles and properly handles the success case in the match over `ticket`.rust/cloud-storage/referral/src/domain/service.rs (2)
105-109: 🧹 Nitpick | 🔵 TrivialMissing
#[tracing::instrument]attribute.Unlike other methods in this impl block (e.g.,
get_referral_code_for_user,track_referral),send_referral_invitelacks the#[tracing::instrument(skip(self), err)]attribute. This creates inconsistent observability. Based on learnings: Includeerrattribute when addingtracing::instrumentto functions that returnResult.♻️ Proposed fix
+ #[tracing::instrument(skip(self), err)] async fn send_referral_invite( &self, sending_user: MacroUserIdStr<'_>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/referral/src/domain/service.rs` around lines 105 - 109, Add the missing tracing instrumentation to the send_referral_invite method by annotating it with #[tracing::instrument(skip(self), err)] so it matches the observability behavior of get_referral_code_for_user and track_referral; place the attribute immediately above the async fn send_referral_invite(...) declaration to ensure errors and context are recorded.
15-15: 🧹 Nitpick | 🔵 TrivialRemove unused rate limit imports.
The imports
RateLimitConfig,RateLimitKey, andRateLimitResultare not used in this file. KeepRateLimitServiceas it's needed for the trait bound on the generic parameterRl(line 38), but remove the other three unused imports from line 15.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/referral/src/domain/service.rs` at line 15, Remove the unused imports RateLimitConfig, RateLimitKey, and RateLimitResult from the use declaration and keep only RateLimitService (needed for the trait bound on the generic Rl in the ReferralService definition); update the use line to import rate_limit::domain::ports::RateLimitService so the file no longer includes unused symbols while preserving the trait bound that references RateLimitService.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rs`:
- Line 37: The tracing instrumentation on the generate_email_link handler is
missing the "err" flag for a function that returns Result<Response, Response>;
update the attribute on the handler (the #[tracing::instrument(...)] line above
the generate_email_link function) to include ", err" at the end of the macro
arguments so it becomes #[tracing::instrument(skip(ctx, user_context,
ip_context, req), fields(client_ip=?ip_context, email=%req.email,
fusion_user_id=%user_context.fusion_user_id), err)]; also scan other modified
handlers in this PR that return Result to ensure their #[tracing::instrument]
macros include err as well.
In
`@rust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rs`:
- Line 30: The tracing instrumentation on the email handler functions is missing
the err flag; update each handler's #[tracing::instrument(...)] to include err
so returned Result errors are recorded (for example change
#[tracing::instrument(skip(ctx, ip_context, req), fields(client_ip=?ip_context,
email=%req.email))] to include err). Apply this change to the four email
handlers: the function handling resend_fusionauth_verify_user_email,
generate_email_link, verify_email_link, and verify_fusionauth_user_email so each
#[tracing::instrument] attribute contains err along with the existing skip and
fields entries.
In
`@rust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rs`:
- Line 29: Update the tracing instrumentation for the email handlers that return
Result so the span records errors: add the err attribute to the
#[tracing::instrument(...)] on the verify_fusionauth_user_email handler (i.e.,
change #[tracing::instrument(skip(ctx, ip_context),
fields(client_ip=?ip_context))] to include err) and apply the same change to the
other email handlers that return Result (verify_email_link,
resend_fusionauth_verify_user_email, generate_email_link) so each attribute
includes err alongside existing skip/fields entries.
In
`@rust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rs`:
- Line 32: The tracing attribute on the handler is missing the err argument even
though the function returns Result; update the #[tracing::instrument(...)] on
the create_in_progress_link handler to include err (e.g., add err to the
attribute alongside the existing skip(...) and fields(...)) so errors are
recorded when the function returns Err.
In `@rust/cloud-storage/authentication_service/src/api/link/github.rs`:
- Line 70: The tracing span on the GitHub link handler omits the err flag;
update the attribute on the existing #[tracing::instrument(skip(ctx, ip_context,
user_context), fields(client_ip=?ip_context, user_id=%user_context.user_id,
fusion_user_id=%user_context.fusion_user_id))] to include err (i.e. add , err)
so the function's Result error path is attached to the span; modify the
attribute on the handler that uses ctx/ip_context/user_context accordingly.
In `@rust/cloud-storage/authentication_service/src/api/login/password.rs`:
- Line 31: The tracing attribute on the password login handler is missing the
err attribute even though the function returns Result<Response, Response>;
update the #[tracing::instrument(...)] on the function that takes ctx, req,
ip_context (the password login handler in password.rs) to include err so
failures are recorded (i.e. change #[tracing::instrument(skip(ctx, req,
ip_context), fields(email=%req.email, client_ip=?ip_context))] to include err in
the macro invocation).
In `@rust/cloud-storage/authentication_service/src/api/login/passwordless.rs`:
- Around line 33-36: The tracing attribute on the Result-returning function
handler is missing the err flag; update the #[tracing::instrument(...)] on the
handler function to include err (e.g., add err to the attribute list alongside
skip(ctx, req, ip_context) and fields(...)) so that errors from the handler
(which returns Result<Response, Response>) are automatically recorded; locate
the handler function and modify its #[tracing::instrument(...)] annotation to
include err.
In
`@rust/cloud-storage/authentication_service/src/api/merge/create_merge_request.rs`:
- Around line 42-48: The handler function declares ip_context: ClientIp and only
uses it in the tracing::instrument fields — either remove the unused extractor
or actually use it for IP-based checks: update handler (fn handler) to pass
ip_context into your IP rate-limiter/check (e.g., call the same IP check used by
referral invites, such as rate_limiter.check_by_ip(ctx, &ip_context) or
check_rate_limit_by_ip(&ctx, &ip_context)) before processing the request, or
drop the ip_context parameter and remove it from tracing::instrument if you only
need it for tracing.
In
`@rust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rs`:
- Line 38: The handler's tracing span currently omits the error attribute;
update the tracing attribute on the verify_merge_request handler to include err
so errors are attached to the span—for example add err to the existing attribute
(keep skip(ctx, user_context, ip_context) and fields(client_ip=?ip_context,
fusion_user_id=%user_context.fusion_user_id) unchanged) so the attribute becomes
a tracing::instrument with err present.
In
`@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rs`:
- Around line 15-18: The handler function currently takes an unused ClientIp
parameter (ip_context) and its tracing::instrument lacks the err attribute;
either remove the ip_context parameter from the handler signature and the
fields(client_ip=?ip_context) in the tracing::instrument if IP-based rate
limiting is not required, or implement IP-based checks using ip_context
alongside the existing email-based rate-limiting logic (e.g., include ip_context
in rate-limiter keys and logging); also add err to the tracing::instrument macro
(tracing::instrument(skip(...), err, fields(...))) so errors are recorded for
the Result-returning handler named handler.
In
`@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/passwordless.rs`:
- Line 21: The tracing::instrument attribute on the passwordless rate-limit
middleware needs the err flag because the handler returns Result<Response,
Response>; update the attribute from #[tracing::instrument(skip(ctx, req, next,
ip_context), fields(client_ip=?ip_context))] to include err (e.g.
#[tracing::instrument(skip(ctx, req, next, ip_context),
fields(client_ip=?ip_context), err)]) so errors returned by the middleware
(Result<Response, Response>) are recorded; apply this change to the middleware
function that takes ctx, req, next, ip_context and returns Result<Response,
Response>.
In `@rust/cloud-storage/authentication_service/src/api/user/create_user.rs`:
- Line 38: The tracing::instrument attribute on the handler should include the
err attr because the function returns Result<Response, Response>; update the
attribute to include err (e.g. #[tracing::instrument(err, skip(ctx, ip_context,
req), fields(client_ip=?ip_context))]) so errors are recorded; locate the
attribute on the handler in create_user.rs (the line with
#[tracing::instrument(skip(ctx, ip_context, req),
fields(client_ip=?ip_context))]) and add err to the attribute list without
changing the existing skip/fields entries.
In `@rust/cloud-storage/ip_extractor/src/lib.rs`:
- Around line 45-47: Fix the typo in the error message for the InvalidAscii
variant: update the #[error("invalid asci {0:?}")] attribute on the
InvalidAscii(#[from] Utf8Error) enum variant to use "ascii" instead of "asci" so
the message reads "invalid ascii {0:?}" (keeping the existing formatting and
error interpolation).
In `@rust/cloud-storage/ip_extractor/src/test.rs`:
- Around line 204-217: Add a unit test that verifies ClientIpError::ExtensionErr
maps to HTTP 500: create a test function (e.g.,
error_response_extension_err_is_500) that constructs a
axum::extract::rejection::ExtensionRejection (use the MissingExtension variant)
and wraps it in ClientIpError::ExtensionErr, call err.into_response() and assert
the response.status() equals StatusCode::INTERNAL_SERVER_ERROR; reference
ClientIpError::ExtensionErr and the into_response() method to locate where to
add the test.
In `@rust/cloud-storage/rate_limit/src/inbound.rs`:
- Around line 91-93: The inspect_err closure currently injects the error into
the log string; change the tracing call inside the closure used in the
inspect_err on the rate limit increment to use structured logging: call
tracing::error!(error=?e, "Failed to increment rate limit counter in
middleware") instead of formatting {e:?} into the message; locate the
inspect_err(|e| { ... }) invocation around the rate limit increment and replace
the tracing::error invocation accordingly.
- Around line 72-73: The doc comment on the rate-limit middleware is misleading:
it implies the middleware itself "checks and asserts the limit before the inner
handler runs," but the pre-handler check is performed by RateLimitExtractor and
this middleware only increments counters after a successful (2xx) response.
Update the comment in inbound.rs to state that RateLimitExtractor performs the
pre-handler check and that the middleware wraps the handler to increment the
rate-limit counter on 2xx responses (i.e., post-response), mentioning the
middleware symbol (e.g., the RateLimit middleware function/type) and
RateLimitExtractor so future readers can find the relevant logic.
- Around line 32-36: The IntoResponse impl for RateLimitExceeded currently
returns only StatusCode::TOO_MANY_REQUESTS; update fn into_response(&self) ->
axum::response::Response to include a Retry-After header derived from the
RateLimitExceeded.retry_after field (e.g., use seconds via
retry_after.as_secs().to_string() or format as an HTTP-date if the field
represents an Instant), setting the header on the response while keeping the 429
status; locate the impl block for RateLimitExceeded and construct the response
with the header (using axum/http::HeaderName or hyper::header::RETRY_AFTER as
appropriate) before returning.
In `@rust/cloud-storage/rate_limit/src/lib.rs`:
- Around line 13-15: The module re-exports omit the success type RateLimitOk
from domain::models causing inconsistency; update the pub use in lib.rs (the
list exporting RateLimitConfig, RateLimitExceeded, RateLimitKey,
RateLimitKeyBuilder, RateLimitResult) to also include RateLimitOk so consumers
can import rate_limit::RateLimitOk directly and match the pattern used for the
other domain types.
In `@rust/cloud-storage/rate_limit/src/outbound/redis.rs`:
- Around line 81-85: The check() implementation currently performs a stale read
then increments later, creating a TOCTOU race; replace the two-step allow +
increment with a single atomic Redis operation (e.g., EVAL Lua script) that
reads the key, compares against the limit from RateLimitConfig, and either
increments and returns allowed or returns denied in one call; update check() to
call that script (and remove or adapt the separate increment logic it pairs
with), and apply the same atomic change to the other similar branch around the
94-104 region so both per-IP/per-user paths use the atomic GET/compare/INCR (+
set expiry when key absent) instead of separate read and increment.
In `@rust/cloud-storage/referral/src/inbound/axum_router/send_invite.rs`:
- Around line 63-68: Extract the duplicate rate limit values into a shared
constant (e.g., const REFERRAL_RATE_LIMIT: RateLimitConfig or two constants
REFERRAL_MAX_COUNT: u32 = 50 and REFERRAL_WINDOW: Duration =
Duration::from_mins(60)) and use that constant inside both
PerUserReferralRateLimit::config() and PerIpReferralRateLimit::config() instead
of repeating max_count: 50 and window: Duration::from_mins(60); ensure the
constant is declared at module scope and referenced by the existing config()
functions so both types share the same config.
---
Outside diff comments:
In `@rust/cloud-storage/rate_limit/src/outbound/redis/test.rs`:
- Around line 49-63: The test's match arm uses `RateLimitResult::Ok { .. }` but
`RateLimitResult` is a `Result<RateLimitOk, RateLimitExceeded>` so `Ok` is a
tuple variant; change the match arm to use the correct tuple pattern (e.g.,
`Ok(_)` or `Ok(rate_limit_ok)`) instead of `RateLimitResult::Ok { .. }` so the
pattern compiles and properly handles the success case in the match over
`ticket`.
In `@rust/cloud-storage/referral/src/domain/service.rs`:
- Around line 105-109: Add the missing tracing instrumentation to the
send_referral_invite method by annotating it with
#[tracing::instrument(skip(self), err)] so it matches the observability behavior
of get_referral_code_for_user and track_referral; place the attribute
immediately above the async fn send_referral_invite(...) declaration to ensure
errors and context are recorded.
- Line 15: Remove the unused imports RateLimitConfig, RateLimitKey, and
RateLimitResult from the use declaration and keep only RateLimitService (needed
for the trait bound on the generic Rl in the ReferralService definition); update
the use line to import rate_limit::domain::ports::RateLimitService so the file
no longer includes unused symbols while preserving the trait bound that
references RateLimitService.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2121098-7561-420e-ae79-abd1a9c9e49d
⛔ Files ignored due to path filters (17)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lockrust/cloud-storage/target/lambda/dataloss_prevention_handler/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/delete_chat_handler/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/deleted_item_poller/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/document_text_extractor/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/docx_unzip_handler/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/email_refresh_handler/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/email_scheduled_handler/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/email_sfs_delete_handler/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/email_suppression_handler/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/organization_retention_handler/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/organization_retention_trigger/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/search_upload_handler/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/upload_extractor_lambda_handler/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/upload_extractor_lambda_trigger/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/user_link_cleanup_handler/bootstrap.zipis excluded by!**/*.ziprust/cloud-storage/target/lambda/worker_trigger/bootstrap.zipis excluded by!**/*.zip
📒 Files selected for processing (52)
rust/cloud-storage/authentication_service/src/api/context.rsrust/cloud-storage/authentication_service/src/api/email/generate_email_link.rsrust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rsrust/cloud-storage/authentication_service/src/api/email/verify_email_link.rsrust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rsrust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rsrust/cloud-storage/authentication_service/src/api/link/github.rsrust/cloud-storage/authentication_service/src/api/link/mod.rsrust/cloud-storage/authentication_service/src/api/login/mod.rsrust/cloud-storage/authentication_service/src/api/login/password.rsrust/cloud-storage/authentication_service/src/api/login/passwordless.rsrust/cloud-storage/authentication_service/src/api/merge/create_merge_request.rsrust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rsrust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rsrust/cloud-storage/authentication_service/src/api/middleware/rate_limit/passwordless.rsrust/cloud-storage/authentication_service/src/api/mod.rsrust/cloud-storage/authentication_service/src/api/oauth/mod.rsrust/cloud-storage/authentication_service/src/api/user/create_user.rsrust/cloud-storage/authentication_service/src/api/user/mod.rsrust/cloud-storage/authentication_service/src/main.rsrust/cloud-storage/fusionauth/src/user/create.rsrust/cloud-storage/fusionauth/src/user/mod.rsrust/cloud-storage/fusionauth/src/user/verify.rsrust/cloud-storage/image_proxy_service/src/api/mod.rsrust/cloud-storage/ip_extractor/Cargo.tomlrust/cloud-storage/ip_extractor/src/lib.rsrust/cloud-storage/ip_extractor/src/test.rsrust/cloud-storage/macro_middleware/Cargo.tomlrust/cloud-storage/macro_middleware/src/tracking/mod.rsrust/cloud-storage/model/src/lib.rsrust/cloud-storage/model/src/tracking.rsrust/cloud-storage/notification/Cargo.tomlrust/cloud-storage/notification/src/domain/service/egress.rsrust/cloud-storage/notification/src/domain/service/test.rsrust/cloud-storage/notification_sandbox/Cargo.tomlrust/cloud-storage/notification_sandbox/src/adapters/noop_rate_limiter.rsrust/cloud-storage/rate_limit/Cargo.tomlrust/cloud-storage/rate_limit/src/domain/models.rsrust/cloud-storage/rate_limit/src/domain/ports.rsrust/cloud-storage/rate_limit/src/domain/service.rsrust/cloud-storage/rate_limit/src/inbound.rsrust/cloud-storage/rate_limit/src/lib.rsrust/cloud-storage/rate_limit/src/outbound/redis.rsrust/cloud-storage/rate_limit/src/outbound/redis/test.rsrust/cloud-storage/referral/Cargo.tomlrust/cloud-storage/referral/src/domain/service.rsrust/cloud-storage/referral/src/domain/service/test.rsrust/cloud-storage/referral/src/inbound/axum_router.rsrust/cloud-storage/referral/src/inbound/axum_router/get_referral_code.rsrust/cloud-storage/referral/src/inbound/axum_router/send_invite.rsrust/cloud-storage/referral/src/inbound/axum_router/test.rsrust/cloud-storage/seed_cli/src/service/auth/mod.rs
💤 Files with no reviewable changes (4)
- rust/cloud-storage/model/src/lib.rs
- rust/cloud-storage/authentication_service/src/api/oauth/mod.rs
- rust/cloud-storage/authentication_service/src/api/user/mod.rs
- rust/cloud-storage/authentication_service/src/api/link/mod.rs
rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rs
Outdated
Show resolved
Hide resolved
| ), | ||
| )] | ||
| #[tracing::instrument(skip(ctx, ip_context,req), fields(client_ip=%ip_context.client_ip, email=%req.email))] | ||
| #[tracing::instrument(skip(ctx, ip_context,req), fields(client_ip=?ip_context, email=%req.email))] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the target file
find . -type f -name "resend_fusionauth_verify_user_email.rs" | head -20Repository: macro-inc/macro
Length of output: 155
🏁 Script executed:
# Get the file and check around line 30
if [ -f "rust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rs" ]; then
echo "=== File content around line 30 ==="
head -50 "rust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rs" | cat -n
fiRepository: macro-inc/macro
Length of output: 2082
🏁 Script executed:
# Check the full file to see the function signature
wc -l "rust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rs"Repository: macro-inc/macro
Length of output: 157
🏁 Script executed:
# Look for all tracing::instrument attributes in the authentication_service
find rust/cloud-storage/authentication_service -type f -name "*.rs" -exec grep -l "tracing::instrument" {} \; | head -20Repository: macro-inc/macro
Length of output: 3268
🏁 Script executed:
# Check all email handlers for tracing::instrument attributes and their function signatures
for file in rust/cloud-storage/authentication_service/src/api/email/*.rs; do
echo "=== $file ==="
grep -n "tracing::instrument" "$file"
# Get the function signature after the tracing::instrument
grep -A 6 "tracing::instrument" "$file" | grep -E "(->|Result)" | head -2
echo ""
doneRepository: macro-inc/macro
Length of output: 1064
🏁 Script executed:
# More detailed check - get full context around tracing::instrument in email handlers
for file in rust/cloud-storage/authentication_service/src/api/email/*.rs; do
if grep -q "tracing::instrument" "$file"; then
echo "=== $file ==="
grep -B 2 -A 8 "tracing::instrument" "$file"
echo ""
fi
doneRepository: macro-inc/macro
Length of output: 2037
Add err to the #[tracing::instrument] attributes on all email handler functions.
All four email handlers in rust/cloud-storage/authentication_service/src/api/email/ are missing the err attribute despite returning Result:
resend_fusionauth_verify_user_email.rsline 30generate_email_link.rsline 37verify_email_link.rsline 32verify_fusionauth_user_email.rsline 29
Each needs err added to its #[tracing::instrument] attribute per the coding guidelines for functions returning Result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rs`
at line 30, The tracing instrumentation on the email handler functions is
missing the err flag; update each handler's #[tracing::instrument(...)] to
include err so returned Result errors are recorded (for example change
#[tracing::instrument(skip(ctx, ip_context, req), fields(client_ip=?ip_context,
email=%req.email))] to include err). Apply this change to the four email
handlers: the function handling resend_fusionauth_verify_user_email,
generate_email_link, verify_email_link, and verify_fusionauth_user_email so each
#[tracing::instrument] attribute contains err along with the existing skip and
fields entries.
| ), | ||
| )] | ||
| #[tracing::instrument(skip(ctx, ip_context), fields(client_ip=%ip_context.client_ip))] | ||
| #[tracing::instrument(skip(ctx, ip_context), fields(client_ip=?ip_context))] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me check if the file exists and examine the context around line 29
cat -n rust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rs | head -50Repository: macro-inc/macro
Length of output: 2132
🏁 Script executed:
# Search for all tracing::instrument attributes in the authentication_service directory
rg '#\[tracing::instrument' rust/cloud-storage/authentication_service/src/ -A 3Repository: macro-inc/macro
Length of output: 39386
🏁 Script executed:
# Check other handlers in the email API directory for similar issues
cat -n rust/cloud-storage/authentication_service/src/api/email/verify_email_link.rs | head -35Repository: macro-inc/macro
Length of output: 1344
🏁 Script executed:
# Check the return type of the verify_email_link handler
cat -n rust/cloud-storage/authentication_service/src/api/email/verify_email_link.rs | sed -n '32-40p'Repository: macro-inc/macro
Length of output: 110
🏁 Script executed:
# Check the return type of verify_email_link handler
sed -n '32,45p' rust/cloud-storage/authentication_service/src/api/email/verify_email_link.rsRepository: macro-inc/macro
Length of output: 528
🏁 Script executed:
# Check the resend_fusionauth_verify_user_email handler
sed -n '1,45p' rust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rsRepository: macro-inc/macro
Length of output: 1571
🏁 Script executed:
# Check generate_email_link handler
sed -n '1,45p' rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rsRepository: macro-inc/macro
Length of output: 1560
🏁 Script executed:
# Let me check if there are any more handlers in the email directory
fd . rust/cloud-storage/authentication_service/src/api/email/ -type f -name "*.rs" | xargs -I {} basename {}Repository: macro-inc/macro
Length of output: 229
🏁 Script executed:
# List all handlers in the email API directory
ls rust/cloud-storage/authentication_service/src/api/email/Repository: macro-inc/macro
Length of output: 208
🏁 Script executed:
# Verify all email handlers return Result and check their tracing attributes
for file in rust/cloud-storage/authentication_service/src/api/email/*.rs; do
if grep -q "pub async fn handler" "$file"; then
echo "=== $(basename $file) ==="
grep -A 4 "#\[tracing::instrument" "$file" | head -5
echo "---"
grep "fn handler" -A 7 "$file" | grep ")" | tail -1
echo ""
fi
doneRepository: macro-inc/macro
Length of output: 1395
🏁 Script executed:
# Confirm return types for all these handlers
for file in rust/cloud-storage/authentication_service/src/api/email/*.rs; do
if grep -q "pub async fn handler" "$file"; then
echo "=== $(basename $file) ==="
grep -A 8 "pub async fn handler" "$file" | grep ")"
fi
doneRepository: macro-inc/macro
Length of output: 1055
Add err to this handler's instrumentation.
This function returns Result<Response, Response>, so the err attribute must be included on the #[tracing::instrument] attribute to attach error information to the span. Please also verify and apply the same fix to other email handlers in this directory that return Result (e.g., verify_email_link.rs, resend_fusionauth_verify_user_email.rs, generate_email_link.rs).
Per coding guidelines: Include err attribute when adding tracing::instrument to functions that return Result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rs`
at line 29, Update the tracing instrumentation for the email handlers that
return Result so the span records errors: add the err attribute to the
#[tracing::instrument(...)] on the verify_fusionauth_user_email handler (i.e.,
change #[tracing::instrument(skip(ctx, ip_context),
fields(client_ip=?ip_context))] to include err) and apply the same change to the
other email handlers that return Result (verify_email_link,
resend_fusionauth_verify_user_email, generate_email_link) so each attribute
includes err alongside existing skip/fields entries.
rust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rs
Outdated
Show resolved
Hide resolved
rust/cloud-storage/authentication_service/src/api/link/github.rs
Outdated
Show resolved
Hide resolved
Comment on lines
+72
to
+73
| /// Rate limit middleware which leverages [RateLimitExtractor] to check and asser the limit before the inner handler runs. | ||
| /// If the response status code from the handler is 2xx, then the counter will be incremented for the rate limit. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify misleading doc comment.
The doc comment states the middleware "check and assert the limit before the inner handler runs," but the middleware itself only handles post-response incrementing. The actual rate-limit check happens during extraction via RateLimitExtractor, which runs before the handler. Consider clarifying:
📝 Proposed doc fix
-/// Rate limit middleware which leverages [RateLimitExtractor] to check and asser the limit before the inner handler runs.
+/// Rate limit middleware that increments the rate limit counter after successful responses.
+///
+/// The rate limit check happens during extraction via [`RateLimitExtractor`] before the handler runs.
/// If the response status code from the handler is 2xx, then the counter will be incremented for the rate limit.📝 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.
Suggested change
| /// Rate limit middleware which leverages [RateLimitExtractor] to check and asser the limit before the inner handler runs. | |
| /// If the response status code from the handler is 2xx, then the counter will be incremented for the rate limit. | |
| /// Rate limit middleware that increments the rate limit counter after successful responses. | |
| /// | |
| /// The rate limit check happens during extraction via [`RateLimitExtractor`] before the handler runs. | |
| /// If the response status code from the handler is 2xx, then the counter will be incremented for the rate limit. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/rate_limit/src/inbound.rs` around lines 72 - 73, The doc
comment on the rate-limit middleware is misleading: it implies the middleware
itself "checks and asserts the limit before the inner handler runs," but the
pre-handler check is performed by RateLimitExtractor and this middleware only
increments counters after a successful (2xx) response. Update the comment in
inbound.rs to state that RateLimitExtractor performs the pre-handler check and
that the middleware wraps the handler to increment the rate-limit counter on 2xx
responses (i.e., post-response), mentioning the middleware symbol (e.g., the
RateLimit middleware function/type) and RateLimitExtractor so future readers can
find the relevant logic.
Comment on lines
13
to
15
| pub use domain::models::{ | ||
| RateLimitConfig, RateLimitExceeded, RateLimitKey, RateLimitKeyBuilder, RateLimitResult, | ||
| RateLimitTicket, | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if RateLimitOk is used directly outside the rate_limit crate
rg -n "RateLimitOk" --type rust -g '!rate_limit/*'Repository: macro-inc/macro
Length of output: 2489
🏁 Script executed:
# Check the current lib.rs file to see what's being re-exported
cat rust/cloud-storage/rate_limit/src/lib.rsRepository: macro-inc/macro
Length of output: 670
🏁 Script executed:
# Search for actual uses of the re-exported RateLimitResult (rate_limit::RateLimitResult form)
rg -n "rate_limit::(RateLimitResult|RateLimitExceeded|RateLimitKey|RateLimitConfig)" --type rustRepository: macro-inc/macro
Length of output: 486
Re-export RateLimitOk for consistency with other domain types.
The RateLimitResult is re-exported, but RateLimitOk (the success type inside the Result) is not. This is inconsistent with the pattern used for RateLimitConfig, RateLimitExceeded, and RateLimitKey. Consumers using this type directly should be able to import it as rate_limit::RateLimitOk:
Suggested change
pub use domain::models::{
- RateLimitConfig, RateLimitExceeded, RateLimitKey, RateLimitKeyBuilder, RateLimitResult,
+ RateLimitConfig, RateLimitExceeded, RateLimitKey, RateLimitKeyBuilder, RateLimitOk, RateLimitResult,
};📝 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.
Suggested change
| pub use domain::models::{ | |
| RateLimitConfig, RateLimitExceeded, RateLimitKey, RateLimitKeyBuilder, RateLimitResult, | |
| RateLimitTicket, | |
| }; | |
| pub use domain::models::{ | |
| RateLimitConfig, RateLimitExceeded, RateLimitKey, RateLimitKeyBuilder, RateLimitOk, RateLimitResult, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/rate_limit/src/lib.rs` around lines 13 - 15, The module
re-exports omit the success type RateLimitOk from domain::models causing
inconsistency; update the pub use in lib.rs (the list exporting RateLimitConfig,
RateLimitExceeded, RateLimitKey, RateLimitKeyBuilder, RateLimitResult) to also
include RateLimitOk so consumers can import rate_limit::RateLimitOk directly and
match the pattern used for the other domain types.
Comment on lines
+63
to
+68
| fn config() -> rate_limit::RateLimitConfig { | ||
| // The fixed window rate limit config for the number of invites a user can send to others | ||
| RateLimitConfig { | ||
| max_count: 50, | ||
| window: Duration::from_mins(60), | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
What Rust version stabilized Duration::from_mins?
💡 Result:
std::time::Duration::from_mins was stabilized in Rust 1.91.0. [1]
🏁 Script executed:
# Find the Cargo.toml for the referral service
find . -path "*cloud-storage/referral/Cargo.toml" -type fRepository: macro-inc/macro
Length of output: 99
🏁 Script executed:
# Check for rust-toolchain file
find . -name "rust-toolchain*" -o -name ".rust-version" | head -20Repository: macro-inc/macro
Length of output: 85
🏁 Script executed:
# Verify the file and code snippet location
cat -n rust/cloud-storage/referral/src/inbound/axum_router/send_invite.rs | sed -n '60,75p'Repository: macro-inc/macro
Length of output: 640
🏁 Script executed:
# Check the rust-toolchain.toml file
cat rust-toolchain.tomlRepository: macro-inc/macro
Length of output: 110
🏁 Script executed:
# Check the Cargo.toml for MSRV specification
cat rust/cloud-storage/referral/Cargo.tomlRepository: macro-inc/macro
Length of output: 2063
🏁 Script executed:
# Find the workspace root Cargo.toml
find . -maxdepth 2 -name "Cargo.toml" -type f | head -5Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
# Search for rust-toolchain files anywhere
find . -name "rust-toolchain*" -type f 2>/dev/nullRepository: macro-inc/macro
Length of output: 85
🏁 Script executed:
# Check the root directory Cargo.toml to find MSRV
cat Cargo.toml | head -50Repository: macro-inc/macro
Length of output: 101
🏁 Script executed:
# Read the rust-toolchain.toml file
cat rust/rust-toolchain.tomlRepository: macro-inc/macro
Length of output: 141
🏁 Script executed:
# Check for workspace Cargo.toml in rust directory
cat rust/Cargo.toml | head -50Repository: macro-inc/macro
Length of output: 106
🏁 Script executed:
# Search for PerIpReferralRateLimit to verify the duplicate configuration claim
cat -n rust/cloud-storage/referral/src/inbound/axum_router/send_invite.rs | sed -n '95,115p'Repository: macro-inc/macro
Length of output: 703
🏁 Script executed:
# Also check if there's more context around lines 101-105 mentioned in the scratchpad
wc -l rust/cloud-storage/referral/src/inbound/axum_router/send_invite.rsRepository: macro-inc/macro
Length of output: 129
Consider extracting duplicate rate limit configuration.
Both PerUserReferralRateLimit and PerIpReferralRateLimit use identical configuration values (max_count: 50, window: Duration::from_mins(60)). Extracting to a constant would improve maintainability.
♻️ Proposed refactor
+/// Rate limit configuration for referral invites: 50 per hour.
+const REFERRAL_RATE_LIMIT_CONFIG: RateLimitConfig = RateLimitConfig {
+ max_count: 50,
+ window: Duration::from_secs(3600),
+};
+
impl<S> RateLimitExtractable<S> for PerUserReferralRateLimit
where
S: Send + Sync,
{
fn config() -> rate_limit::RateLimitConfig {
- // The fixed window rate limit config for the number of invites a user can send to others
- RateLimitConfig {
- max_count: 50,
- window: Duration::from_mins(60),
- }
+ REFERRAL_RATE_LIMIT_CONFIG
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/referral/src/inbound/axum_router/send_invite.rs` around
lines 63 - 68, Extract the duplicate rate limit values into a shared constant
(e.g., const REFERRAL_RATE_LIMIT: RateLimitConfig or two constants
REFERRAL_MAX_COUNT: u32 = 50 and REFERRAL_WINDOW: Duration =
Duration::from_mins(60)) and use that constant inside both
PerUserReferralRateLimit::config() and PerIpReferralRateLimit::config() instead
of repeating max_count: 50 and window: Duration::from_mins(60); ensure the
constant is declared at module scope and referenced by the existing config()
functions so both types share the same config.
seanaye
force-pushed
the
seanaye/feat/referral-ip-rate-limit
branch
from
b114665 to
b00c3fb
Compare
Contributor
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)
rust/cloud-storage/authentication_service/src/api/mod.rs (1)
82-118:⚠️ Potential issue | 🔴 CriticalWire
ConnectInfointo the server before handlers depend onClientIp.The
/linkand/referralroutes now have handlers that extractClientIp(inlink/github.rs,link/create_in_progress_link.rs, andreferral/send_invite.rs). WhenClientIpfalls back to extractingConnectInfo<SocketAddr>at runtime (when nox-forwarded-forheader is present), the server must be configured withinto_make_service_with_connect_info::<SocketAddr>(). Currently, line 63 usesinto_make_service(), which leavesConnectInfounavailable, causing handlers to fail with a 500 error.Fix
-use std::time::Duration; +use std::{net::SocketAddr, time::Duration}; ... - axum::serve(listener, app.into_make_service()) + axum::serve(listener, app.into_make_service_with_connect_info::<SocketAddr>())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/mod.rs` around lines 82 - 118, Handlers under link::router and referral::inbound::axum_router::referral_router extract ClientIp which falls back to ConnectInfo<SocketAddr>, but the server is currently built with into_make_service() so ConnectInfo is unavailable; change server construction to use into_make_service_with_connect_info::<SocketAddr>() so ConnectInfo is wired into the service, ensuring ClientIp extraction succeeds for handlers like link/github.rs, link/create_in_progress_link.rs and referral/send_invite.rs.rust/cloud-storage/notification/src/domain/service/egress.rs (1)
215-248:⚠️ Potential issue | 🟠 MajorThe new rate-limit reservation is not cancellation-safe.
deliver_emailreserves quota before the send at lines 211-213, butpoll_and_deliverwrapsdeliver_notificationintokio::time::timeoutat lines 284-293. Ifsend_emailhangs and the timeout expires, Tokio cancels the wrapped future, dropping it before the explicit rollback at lines 242-245 executes. The reserved ticket is never rolled back, and the retried message can hit the rate limit even though nothing was delivered.
♻️ Duplicate comments (12)
rust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rs (1)
30-35:⚠️ Potential issue | 🟡 MinorRecord handler errors on this span.
This handler returns
Result<Response, Response>.tracingrecordserrvalues withDisplayby default, butaxum::response::Responseishttp::Response<Body>and the documented impls here areDebug-based, so this span should useerr(Debug). (docs.rs)Based on learnings: "Applies to rust/cloud-storage/**/*.rs : Include
errattribute when addingtracing::instrumentto functions that returnResult; do not includeerron functions returningOption,(), or other non-Resulttypes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rs` around lines 30 - 35, The tracing span on the handler function should record error values using Debug; update the tracing::instrument attribute on the handler (the function named handler in resend_fusionauth_verify_user_email.rs) to include err(Debug) in its attribute list (e.g., tracing::instrument(skip(ctx, ip_context, req), fields(client_ip=?ip_context, email=%req.email), err(Debug)) so that returned Result<Response, Response> errors are recorded with Debug formatting.rust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rs (1)
38-44:⚠️ Potential issue | 🟡 MinorRecord handler errors on this span.
This handler returns
Result<Response, Response>.tracingrecordserrvalues withDisplayby default, butaxum::response::Responseishttp::Response<Body>and the documented impls here areDebug-based, so this span should useerr(Debug). (docs.rs)Based on learnings: "Applies to rust/cloud-storage/**/*.rs : Include
errattribute when addingtracing::instrumentto functions that returnResult; do not includeerron functions returningOption,(), or other non-Resulttypes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rs` around lines 38 - 44, The tracing span on the handler function should record error values using Debug because the function returns Result<Response, Response>; update the tracing attribute on the handler (the function named handler in verify_merge_request.rs) to include err(Debug) in its attribute (e.g. tracing::instrument(..., err(Debug))), keeping the existing skip(ctx, user_context, ip_context) and fields(...) entries unchanged so errors are logged with Debug formatting.rust/cloud-storage/authentication_service/src/api/email/verify_email_link.rs (1)
32-37:⚠️ Potential issue | 🟡 MinorRecord handler errors on this span.
This handler returns
Result<Response, Response>.tracingrecordserrvalues withDisplayby default, butaxum::response::Responseishttp::Response<Body>and the documented impls here areDebug-based, so this span should useerr(Debug). (docs.rs)Based on learnings: "Applies to rust/cloud-storage/**/*.rs : Include
errattribute when addingtracing::instrumentto functions that returnResult; do not includeerron functions returningOption,(), or other non-Resulttypes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/email/verify_email_link.rs` around lines 32 - 37, The tracing span for the handler function should record errors using Debug because the function returns Result<Response, Response> whose error type implements Debug; update the #[tracing::instrument(...)] on handler to include err(Debug) (i.e., add err(Debug) alongside skip(ctx, ip_context) and fields(...)) so that returned Response errors are logged with Debug formatting; locate the handler function named handler (signature: pub async fn handler(State(ctx): State<ApiContext>, ip_context: ClientIp, extract::Path(Params { verification_id }): extract::Path<Params>,) -> Result<Response, Response>) and modify its tracing attribute accordingly.rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rs (1)
37-43:⚠️ Potential issue | 🟡 MinorRecord handler errors on this span.
This handler returns
Result<Response, Response>.tracingrecordserrvalues withDisplayby default, butaxum::response::Responseishttp::Response<Body>and the documented impls here areDebug-based, so this span should useerr(Debug). (docs.rs)Based on learnings: "Applies to rust/cloud-storage/**/*.rs : Include
errattribute when addingtracing::instrumentto functions that returnResult; do not includeerron functions returningOption,(), or other non-Resulttypes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rs` around lines 37 - 43, The tracing span on the handler function should record errors with Debug because handler returns Result<Response, Response>; update the tracing attribute on the handler (the function named handler handling ApiContext, UserContext, ClientIp and GenerateEmailLinkRequest) to include err(Debug) in the attribute (e.g. add err(Debug) alongside skip(...) and fields(...)) so tracing will record the Response error via Debug.rust/cloud-storage/authentication_service/src/api/login/passwordless.rs (1)
33-38:⚠️ Potential issue | 🟡 MinorRecord handler errors on this span.
This handler returns
Result<Response, Response>.tracingrecordserrvalues withDisplayby default, butaxum::response::Responseishttp::Response<Body>and the documented impls here areDebug-based, so this span should useerr(Debug). (docs.rs)Based on learnings: "Applies to rust/cloud-storage/**/*.rs : Include
errattribute when addingtracing::instrumentto functions that returnResult; do not includeerron functions returningOption,(), or other non-Resulttypes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/login/passwordless.rs` around lines 33 - 38, The tracing span on the passwordless login handler should record error values with Debug; update the #[tracing::instrument(...)] on the async function handler (the one taking State(ApiContext), ClientIp, and extract::Json<PasswordlessRequest>) to include err(Debug), e.g. add err(Debug) alongside the existing skip(...) and fields(...), so errors returned as Response are recorded with Debug formatting.rust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rs (1)
32-32:⚠️ Potential issue | 🟡 MinorCapture link-creation failures on this span.
This handler returns
Result, but the span omits the error path. Adderrhere; ifResponsedoes not satisfy tracing's default bound, useerr(Debug)when applying the fix.As per coding guidelines: "Include
errattribute when addingtracing::instrumentto functions that returnResult; do not includeerron functions returningOption,(), or other non-Resulttypes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rs` at line 32, The tracing span on the create_in_progress_link handler omits the error attribute for a function that returns Result; update the #[tracing::instrument(...)] macro to include err (e.g. add err or err(Debug) to the attribute list) while keeping existing skip(ctx, ip_context, user_context) and fields(client_ip=?ip_context, user_id=%user_context.user_id, fusion_user_id=%user_context.fusion_user_id)); if the Response type doesn't implement tracing's Error/Display bounds use err(Debug) to capture the error via Debug.rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rs (1)
15-15:⚠️ Potential issue | 🟡 MinorCapture middleware failures on this span.
This function returns
Result, but the span still drops the error path. Adderrhere; ifResponsedoes not satisfy tracing's default bound, useerr(Debug)when applying the fix.As per coding guidelines: "Include
errattribute when addingtracing::instrumentto functions that returnResult; do not includeerron functions returningOption,(), or other non-Resulttypes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rs` at line 15, The tracing span on the middleware in login_code.rs currently omits error capture; update the #[tracing::instrument(...)] attribute on the middleware function to include the err attribute (e.g., add err or err(Debug)) so errors from the function's Result return path are recorded; modify the existing attribute that references skip(ctx, req, next, ip_context) and fields(client_ip=?ip_context) to also include err (use err(Debug) if Response does not implement tracing::field::Display).rust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rs (1)
29-29:⚠️ Potential issue | 🟡 MinorCapture verification failures on this span.
This handler returns
Result, but the span omits the error path. Adderrhere; ifResponsedoes not satisfy tracing's default bound, useerr(Debug)when applying the fix.As per coding guidelines: "Include
errattribute when addingtracing::instrumentto functions that returnResult; do not includeerron functions returningOption,(), or other non-Resulttypes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rs` at line 29, The tracing span on the email verification handler omits error capture even though the function returns Result; update the attribute on the handler (the #[tracing::instrument(...)] line in verify_fusionauth_user_email.rs that currently has skip(ctx, ip_context) and fields(client_ip=?ip_context)) to include err so failures are recorded (use err if the Response type satisfies tracing's default bound, otherwise use err(Debug)). Ensure you add the err/err(Debug) attribute next to the existing skip(...) and fields(...) entries.rust/cloud-storage/authentication_service/src/api/link/github.rs (1)
70-70:⚠️ Potential issue | 🟡 MinorCapture
InitGithubLinkErroron this span.This handler returns
Result, so the error path should be attached to the span witherr.As per coding guidelines: "Include
errattribute when addingtracing::instrumentto functions that returnResult; do not includeerron functions returningOption,(), or other non-Resulttypes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/link/github.rs` at line 70, The tracing span on the InitGithubLink handler is missing the err attribute; update the #[tracing::instrument(...)] on the init_github_link handler to include err (e.g., err or err(Debug)) alongside the existing skip(...) and fields(...) so that returned InitGithubLinkError values are recorded on the span; keep skip(ctx, ip_context, user_context) and the existing fields(client_ip=?ip_context, user_id=%user_context.user_id, fusion_user_id=%user_context.fusion_user_id) but add err to the attribute list.rust/cloud-storage/authentication_service/src/api/user/create_user.rs (1)
38-38:⚠️ Potential issue | 🟡 MinorCapture create-user failures on this span.
This handler returns
Result, but the span omits the error path. Adderrhere; ifResponsedoes not satisfy tracing's default bound, useerr(Debug)when applying the fix.As per coding guidelines: "Include
errattribute when addingtracing::instrumentto functions that returnResult; do not includeerron functions returningOption,(), or other non-Resulttypes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/user/create_user.rs` at line 38, The tracing span on the create-user handler is missing the error capture; update the tracing::instrument attribute on the handler (the attribute shown with skip(ctx, ip_context, req) and fields(client_ip=?ip_context)) to include err so failures are recorded; if the handler's Result Err type doesn't implement tracing::Value, use err(Debug) instead (e.g., add err or err(Debug) to the attribute) so create_user response errors are captured on the span.rust/cloud-storage/authentication_service/src/api/login/password.rs (1)
31-31:⚠️ Potential issue | 🟡 MinorCapture handler failures on this span.
This handler returns
Result, but the span omits the error path. Adderrhere; ifResponsedoes not satisfy tracing's default bound, useerr(Debug)when applying the fix.As per coding guidelines: "Include
errattribute when addingtracing::instrumentto functions that returnResult; do not includeerron functions returningOption,(), or other non-Resulttypes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/login/password.rs` at line 31, The tracing span on the login handler in password.rs currently instruments fields(email=%req.email, client_ip=?ip_context) but omits the error path; update the #[tracing::instrument(...)] attribute on the function that returns Result to include err (or err(Debug) if the Response type doesn’t implement tracing::field::Value) so failures are recorded on the span—i.e., add err or err(Debug) inside the attribute alongside the existing skip and fields entries.rust/cloud-storage/referral/src/inbound/axum_router/send_invite.rs (1)
63-68: 🧹 Nitpick | 🔵 TrivialConsider extracting duplicate rate limit configuration.
Both
PerUserReferralRateLimit::config()andPerIpReferralRateLimit::config()return identical values (max_count: 50,window: Duration::from_mins(60)). Extracting to a shared constant would improve maintainability and ensure both limits stay in sync.♻️ Proposed refactor
+const REFERRAL_RATE_LIMIT_CONFIG: RateLimitConfig = RateLimitConfig { + max_count: 50, + window: Duration::from_secs(3600), // 60 minutes +}; + impl<S> RateLimitExtractable<S> for PerUserReferralRateLimit where S: Send + Sync, { fn config() -> rate_limit::RateLimitConfig { - RateLimitConfig { - max_count: 50, - window: Duration::from_mins(60), - } + REFERRAL_RATE_LIMIT_CONFIG.clone() }Also applies to: 101-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/referral/src/inbound/axum_router/send_invite.rs` around lines 63 - 68, Extract the duplicated rate limit values into a single shared constant and use it from both PerUserReferralRateLimit::config() and PerIpReferralRateLimit::config(); specifically, define a module-level constant (e.g., REFERRAL_RATE_LIMIT: rate_limit::RateLimitConfig or separate MAX_REFERRAL_COUNT: usize and REFERRAL_WINDOW: Duration) containing max_count = 50 and window = Duration::from_mins(60), then replace the hard-coded values in the two config() functions to return that constant so both stay in sync and maintainability is improved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@rust/cloud-storage/authentication_service/src/api/merge/create_merge_request.rs`:
- Around line 42-48: The tracing::instrument on the handler function lacks the
err attribute even though handler returns Result<Response, Response>; update the
attribute on the handler (the function named handler in create_merge_request.rs)
to include err so errors are recorded (e.g., add err to the tracing::instrument
attribute list alongside skip and fields), ensuring any returned Err(Response)
is captured by tracing.
In `@rust/cloud-storage/ip_extractor/src/lib.rs`:
- Line 39: Fix the typo in the doc comment in lib.rs: change "extracing" to
"extracting" in the documentation line that references [ClientIp] so the comment
reads "The errors that can occur while extracting a [ClientIp]". Ensure the
corrected spelling appears in the doc comment associated with the
ClientIp-related errors.
In `@rust/cloud-storage/model/src/tracking.rs`:
- Line 1: This file (tracking.rs) is now empty after removing IPContext and the
tracking module is no longer exported from lib.rs; delete the file entirely to
remove dead code and avoid carrying an unused empty module (remove tracking.rs
from the repository and any references to it such as module declarations or use
statements if present).
In `@rust/cloud-storage/notification/src/domain/service/test.rs`:
- Around line 1077-1079: The mock RateLimiter's async fn decrement(&self, _key:
&RateLimitKey) currently does nothing so rollback of consumed slots is
unobservable; modify the mock implementation of decrement to record the provided
RateLimitKey (e.g., push a clone into an internal Vec/Mutex or other thread-safe
collection exposed by the mock), expose a getter or accessor on the mock to read
recorded keys, and update at least one failing-delivery test to assert that the
expected key was recorded (verifying the rollback contract). Ensure you
reference the mock type used in tests and the decrement(&self, _key:
&RateLimitKey) method when making these changes.
In `@rust/cloud-storage/rate_limit/src/outbound/redis/test.rs`:
- Line 145: Tests directly construct Redis keys using format!("rtl:{}",
key.to_hex_string()), which couples them to the internal prefix; extract the
prefix and formatting into a single shared symbol (e.g., a constant
RATE_LIMIT_KEY_PREFIX or a helper function like rate_limit_redis_key(key: &Key)
-> String) used by both production code and tests, then update occurrences that
build the key (the key_str assignment and the other places calling
format!("rtl:{}", key.to_hex_string())) to call that shared constant/helper so
tests no longer hardcode the "rtl:" prefix.
In `@rust/cloud-storage/seed_cli/src/service/auth/mod.rs`:
- Around line 34-38: Replace the explicit localhost construction with the
standard constant for clarity: in the call to create_user (the .create_user
invocation), change IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)) to
IpAddr::V4(Ipv4Addr::LOCALHOST) so the code uses the typed Ipv4Addr::LOCALHOST
constant for readability.
---
Outside diff comments:
In `@rust/cloud-storage/authentication_service/src/api/mod.rs`:
- Around line 82-118: Handlers under link::router and
referral::inbound::axum_router::referral_router extract ClientIp which falls
back to ConnectInfo<SocketAddr>, but the server is currently built with
into_make_service() so ConnectInfo is unavailable; change server construction to
use into_make_service_with_connect_info::<SocketAddr>() so ConnectInfo is wired
into the service, ensuring ClientIp extraction succeeds for handlers like
link/github.rs, link/create_in_progress_link.rs and referral/send_invite.rs.
---
Duplicate comments:
In
`@rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rs`:
- Around line 37-43: The tracing span on the handler function should record
errors with Debug because handler returns Result<Response, Response>; update the
tracing attribute on the handler (the function named handler handling
ApiContext, UserContext, ClientIp and GenerateEmailLinkRequest) to include
err(Debug) in the attribute (e.g. add err(Debug) alongside skip(...) and
fields(...)) so tracing will record the Response error via Debug.
In
`@rust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rs`:
- Around line 30-35: The tracing span on the handler function should record
error values using Debug; update the tracing::instrument attribute on the
handler (the function named handler in resend_fusionauth_verify_user_email.rs)
to include err(Debug) in its attribute list (e.g., tracing::instrument(skip(ctx,
ip_context, req), fields(client_ip=?ip_context, email=%req.email), err(Debug))
so that returned Result<Response, Response> errors are recorded with Debug
formatting.
In
`@rust/cloud-storage/authentication_service/src/api/email/verify_email_link.rs`:
- Around line 32-37: The tracing span for the handler function should record
errors using Debug because the function returns Result<Response, Response> whose
error type implements Debug; update the #[tracing::instrument(...)] on handler
to include err(Debug) (i.e., add err(Debug) alongside skip(ctx, ip_context) and
fields(...)) so that returned Response errors are logged with Debug formatting;
locate the handler function named handler (signature: pub async fn
handler(State(ctx): State<ApiContext>, ip_context: ClientIp,
extract::Path(Params { verification_id }): extract::Path<Params>,) ->
Result<Response, Response>) and modify its tracing attribute accordingly.
In
`@rust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rs`:
- Line 29: The tracing span on the email verification handler omits error
capture even though the function returns Result; update the attribute on the
handler (the #[tracing::instrument(...)] line in verify_fusionauth_user_email.rs
that currently has skip(ctx, ip_context) and fields(client_ip=?ip_context)) to
include err so failures are recorded (use err if the Response type satisfies
tracing's default bound, otherwise use err(Debug)). Ensure you add the
err/err(Debug) attribute next to the existing skip(...) and fields(...) entries.
In
`@rust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rs`:
- Line 32: The tracing span on the create_in_progress_link handler omits the
error attribute for a function that returns Result; update the
#[tracing::instrument(...)] macro to include err (e.g. add err or err(Debug) to
the attribute list) while keeping existing skip(ctx, ip_context, user_context)
and fields(client_ip=?ip_context, user_id=%user_context.user_id,
fusion_user_id=%user_context.fusion_user_id)); if the Response type doesn't
implement tracing's Error/Display bounds use err(Debug) to capture the error via
Debug.
In `@rust/cloud-storage/authentication_service/src/api/link/github.rs`:
- Line 70: The tracing span on the InitGithubLink handler is missing the err
attribute; update the #[tracing::instrument(...)] on the init_github_link
handler to include err (e.g., err or err(Debug)) alongside the existing
skip(...) and fields(...) so that returned InitGithubLinkError values are
recorded on the span; keep skip(ctx, ip_context, user_context) and the existing
fields(client_ip=?ip_context, user_id=%user_context.user_id,
fusion_user_id=%user_context.fusion_user_id) but add err to the attribute list.
In `@rust/cloud-storage/authentication_service/src/api/login/password.rs`:
- Line 31: The tracing span on the login handler in password.rs currently
instruments fields(email=%req.email, client_ip=?ip_context) but omits the error
path; update the #[tracing::instrument(...)] attribute on the function that
returns Result to include err (or err(Debug) if the Response type doesn’t
implement tracing::field::Value) so failures are recorded on the span—i.e., add
err or err(Debug) inside the attribute alongside the existing skip and fields
entries.
In `@rust/cloud-storage/authentication_service/src/api/login/passwordless.rs`:
- Around line 33-38: The tracing span on the passwordless login handler should
record error values with Debug; update the #[tracing::instrument(...)] on the
async function handler (the one taking State(ApiContext), ClientIp, and
extract::Json<PasswordlessRequest>) to include err(Debug), e.g. add err(Debug)
alongside the existing skip(...) and fields(...), so errors returned as Response
are recorded with Debug formatting.
In
`@rust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rs`:
- Around line 38-44: The tracing span on the handler function should record
error values using Debug because the function returns Result<Response,
Response>; update the tracing attribute on the handler (the function named
handler in verify_merge_request.rs) to include err(Debug) in its attribute (e.g.
tracing::instrument(..., err(Debug))), keeping the existing skip(ctx,
user_context, ip_context) and fields(...) entries unchanged so errors are logged
with Debug formatting.
In
`@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rs`:
- Line 15: The tracing span on the middleware in login_code.rs currently omits
error capture; update the #[tracing::instrument(...)] attribute on the
middleware function to include the err attribute (e.g., add err or err(Debug))
so errors from the function's Result return path are recorded; modify the
existing attribute that references skip(ctx, req, next, ip_context) and
fields(client_ip=?ip_context) to also include err (use err(Debug) if Response
does not implement tracing::field::Display).
In `@rust/cloud-storage/authentication_service/src/api/user/create_user.rs`:
- Line 38: The tracing span on the create-user handler is missing the error
capture; update the tracing::instrument attribute on the handler (the attribute
shown with skip(ctx, ip_context, req) and fields(client_ip=?ip_context)) to
include err so failures are recorded; if the handler's Result Err type doesn't
implement tracing::Value, use err(Debug) instead (e.g., add err or err(Debug) to
the attribute) so create_user response errors are captured on the span.
In `@rust/cloud-storage/referral/src/inbound/axum_router/send_invite.rs`:
- Around line 63-68: Extract the duplicated rate limit values into a single
shared constant and use it from both PerUserReferralRateLimit::config() and
PerIpReferralRateLimit::config(); specifically, define a module-level constant
(e.g., REFERRAL_RATE_LIMIT: rate_limit::RateLimitConfig or separate
MAX_REFERRAL_COUNT: usize and REFERRAL_WINDOW: Duration) containing max_count =
50 and window = Duration::from_mins(60), then replace the hard-coded values in
the two config() functions to return that constant so both stay in sync and
maintainability is improved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a831133b-0b94-47bf-ae7f-ceb92373fd6c
⛔ Files ignored due to path filters (1)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rsrust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rsrust/cloud-storage/authentication_service/src/api/email/verify_email_link.rsrust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rsrust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rsrust/cloud-storage/authentication_service/src/api/link/github.rsrust/cloud-storage/authentication_service/src/api/link/mod.rsrust/cloud-storage/authentication_service/src/api/login/mod.rsrust/cloud-storage/authentication_service/src/api/login/password.rsrust/cloud-storage/authentication_service/src/api/login/passwordless.rsrust/cloud-storage/authentication_service/src/api/merge/create_merge_request.rsrust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rsrust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rsrust/cloud-storage/authentication_service/src/api/middleware/rate_limit/passwordless.rsrust/cloud-storage/authentication_service/src/api/mod.rsrust/cloud-storage/authentication_service/src/api/oauth/mod.rsrust/cloud-storage/authentication_service/src/api/user/create_user.rsrust/cloud-storage/authentication_service/src/api/user/mod.rsrust/cloud-storage/fusionauth/src/user/create.rsrust/cloud-storage/fusionauth/src/user/mod.rsrust/cloud-storage/fusionauth/src/user/verify.rsrust/cloud-storage/image_proxy_service/src/api/mod.rsrust/cloud-storage/ip_extractor/Cargo.tomlrust/cloud-storage/ip_extractor/src/lib.rsrust/cloud-storage/model/src/lib.rsrust/cloud-storage/model/src/tracking.rsrust/cloud-storage/notification/src/domain/service/egress.rsrust/cloud-storage/notification/src/domain/service/test.rsrust/cloud-storage/notification_sandbox/src/adapters/noop_rate_limiter.rsrust/cloud-storage/rate_limit/src/domain/models.rsrust/cloud-storage/rate_limit/src/domain/ports.rsrust/cloud-storage/rate_limit/src/domain/service.rsrust/cloud-storage/rate_limit/src/inbound.rsrust/cloud-storage/rate_limit/src/outbound/redis.rsrust/cloud-storage/rate_limit/src/outbound/redis/test.rsrust/cloud-storage/referral/Cargo.tomlrust/cloud-storage/referral/src/domain/service/test.rsrust/cloud-storage/referral/src/inbound/axum_router.rsrust/cloud-storage/referral/src/inbound/axum_router/send_invite.rsrust/cloud-storage/referral/src/inbound/axum_router/test.rsrust/cloud-storage/seed_cli/src/service/auth/mod.rs
💤 Files with no reviewable changes (4)
- rust/cloud-storage/authentication_service/src/api/link/mod.rs
- rust/cloud-storage/authentication_service/src/api/oauth/mod.rs
- rust/cloud-storage/authentication_service/src/api/user/mod.rs
- rust/cloud-storage/model/src/lib.rs
rust/cloud-storage/authentication_service/src/api/merge/create_merge_request.rs
Outdated
Show resolved
Hide resolved
Comment on lines
+1077
to
1079
| async fn decrement(&self, _key: &RateLimitKey) -> Result<(), Report> { | ||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make rollback observable in the mock.
This no-op decrement can't tell whether failure paths actually release a consumed slot. Record the keys here and assert them in at least one failing-delivery test so the new rollback contract is covered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/notification/src/domain/service/test.rs` around lines 1077
- 1079, The mock RateLimiter's async fn decrement(&self, _key: &RateLimitKey)
currently does nothing so rollback of consumed slots is unobservable; modify the
mock implementation of decrement to record the provided RateLimitKey (e.g., push
a clone into an internal Vec/Mutex or other thread-safe collection exposed by
the mock), expose a getter or accessor on the mock to read recorded keys, and
update at least one failing-delivery test to assert that the expected key was
recorded (verifying the rollback contract). Ensure you reference the mock type
used in tests and the decrement(&self, _key: &RateLimitKey) method when making
these changes.
6e0056b to
e8f01b8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
rust/cloud-storage/referral/src/domain/service/test.rs (3)
180-189:⚠️ Potential issue | 🟡 MinorTest may pass for wrong reasons.
This test iterates and expects success, which will now always pass since rate limiting was removed from the service. However, the test's purpose was to validate that multiple invites succeed under the rate limit—a distinction that no longer exists at the service layer.
Consider removing this test or relocating it to the router tests where rate limiting is actually enforced.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/referral/src/domain/service/test.rs` around lines 180 - 189, The test multiple_invites_succeed_when_under_rate_limit in test.rs no longer validates meaningful behavior because rate limiting was removed from the service layer; update the test by either deleting it from referral/src/domain/service/test.rs or moving it to the router-level tests where rate limiting is enforced, and remove/replace uses of build_service and allowing_rate_limiter() and calls to service.send_referral_invite/test_user() accordingly so the assertion is only present in tests that exercise the actual rate-limiter (router tests) rather than the service unit tests.
143-156:⚠️ Potential issue | 🔴 CriticalTests expect rate-limit behavior that was removed from the service.
This test expects
send_referral_inviteto returnReferralError::RateLimitExceeded, but the rate-limit logic was removed fromReferralServiceImpl::send_referral_invite(inservice.rs). The service now directly sends notifications without checking rate limits.Since rate limiting was moved to the Axum middleware layer, these domain-level tests are now testing behavior that no longer exists in the service. These tests will fail because
send_referral_invitewill succeed even withexceeding_rate_limiter().Either:
- Remove these tests since rate limiting is now enforced at the router layer (covered by
axum_router/test.rs)- Or, if the service should still perform rate limiting, restore that logic in
service.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/referral/src/domain/service/test.rs` around lines 143 - 156, The test send_referral_invite_fails_when_rate_limit_exceeded is asserting behavior (ReferralError::RateLimitExceeded) that no longer exists because ReferralServiceImpl::send_referral_invite in service.rs no longer enforces rate limits; either delete or update this domain test: remove the test entirely if rate limiting is now covered by the axum middleware tests (axum_router tests), or change it to assert success (expect Ok(())) and stop using exceeding_rate_limiter(); if you want service-level rate limiting instead, reintroduce the rate-check and return ReferralError::RateLimitExceeded from ReferralServiceImpl::send_referral_invite so the test again becomes valid.
158-178:⚠️ Potential issue | 🔴 CriticalSame issue: test expects removed rate-limit behavior.
This test also relies on rate-limit enforcement in
send_referral_invite, which no longer exists. Theunwrap_err()at line 165 will panic because the service will returnOk(()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/referral/src/domain/service/test.rs` around lines 158 - 178, The test rate_limit_error_contains_exceeded_info assumes send_referral_invite will return a RateLimitExceeded error, but rate limiting was removed; update the test to reflect the new behavior by not calling unwrap_err() on service.send_referral_invite; instead call service.send_referral_invite(...).await and assert it returns Ok(()) (or remove the test if rate-limit semantics are no longer relevant). Locate this logic in the test function rate_limit_error_contains_exceeded_info and the helpers build_service and exceeding_rate_limiter to adjust or remove assertions referencing ReferralError::RateLimitExceeded.rust/cloud-storage/rate_limit/src/domain/service.rs (1)
19-26: 🧹 Nitpick | 🔵 TrivialConsider adding
#[tracing::instrument]tocheck_rate_limit.The
rollback_ticketmethod has#[tracing::instrument(err, skip(self))], butcheck_rate_limitdoes not. Since both methods returnResult, adding instrumentation tocheck_rate_limitwould improve observability for debugging rate-limit issues.🔧 Suggested addition
+ #[tracing::instrument(err, skip(self))] async fn check_rate_limit( &self, key: RateLimitKey, config: RateLimitConfig, ) -> Result<RateLimitResult, Report> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/rate_limit/src/domain/service.rs` around lines 19 - 26, Add tracing instrumentation to check_rate_limit to match rollback_ticket: annotate the async fn check_rate_limit(&self, key: RateLimitKey, config: RateLimitConfig) -> Result<RateLimitResult, Report> with #[tracing::instrument(err, skip(self))] so errors are recorded and self is skipped from spans; ensure the tracing macro is available in scope (same usage as rollback_ticket) and keep function body unchanged.rust/cloud-storage/referral/src/domain/service.rs (2)
105-138:⚠️ Potential issue | 🟡 MinorMissing
#[tracing::instrument]attribute onsend_referral_invite.This function returns
Result<(), ReferralError>but lacks the#[tracing::instrument(skip(self), err)]attribute that other methods in this impl block have (lines 44, 55, 67, 78). As per coding guidelines, functions returningResultshould includeerrin the instrument attribute.🔧 Proposed fix
+ #[tracing::instrument(skip(self), err)] async fn send_referral_invite( &self, sending_user: MacroUserIdStr<'_>, recipient: EmailStr<'static>, ) -> Result<(), ReferralError> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/referral/src/domain/service.rs` around lines 105 - 138, The send_referral_invite function is missing the tracing instrument attribute; add #[tracing::instrument(skip(self), err)] above the async fn send_referral_invite(...) declaration (matching the pattern used on the other methods in this impl) so that errors are captured by tracing while skipping the self parameter; keep the rest of the function (uses of MacroUserIdStr, InviteToMacro, notification_ingress, SendNotificationRequestBuilder) unchanged.
15-15:⚠️ Potential issue | 🟡 MinorRemove unused rate-limiting imports and field from ReferralServiceImpl.
The imports
RateLimitConfig,RateLimitKey,RateLimitResult, anddomain::ports::RateLimitServicefromrate_limitare no longer used. Therate_limit: Rlfield and theRl: RateLimitServicegeneric bound are also unused, as rate limiting is now handled at the middleware layer. Remove line 15 and the generic type parameter from the struct definition and impl block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/referral/src/domain/service.rs` at line 15, Remove unused rate-limiting imports and the generic rate-limit parameter: delete the imports RateLimitConfig, RateLimitKey, RateLimitResult and domain::ports::RateLimitService from the top-level use statement, remove the Rl generic parameter and the rate_limit: Rl field from the ReferralServiceImpl struct, and remove the Rl: RateLimitService bound from the impl block for ReferralServiceImpl so the struct and its impl no longer reference the unused rate-limiting types or field.rust/cloud-storage/referral/src/inbound/axum_router/send_invite.rs (1)
31-40:⚠️ Potential issue | 🟡 MinorAdd 429 response to the OpenAPI schema.
The
/referral/sendroute is wrapped by rate-limit middleware that rejects requests before the handler runs. Theutoipa::pathmetadata should document the 429 TOO_MANY_REQUESTS response that clients can receive.Suggested update
responses( (status = StatusCode::NO_CONTENT), (status = 401, body = model_error_response::ErrorResponse), + (status = 429), (status = 500, body = model_error_response::ErrorResponse), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/referral/src/inbound/axum_router/send_invite.rs` around lines 31 - 40, The OpenAPI metadata for the send_referral_code handler is missing the 429 TOO_MANY_REQUESTS response which the rate-limit middleware can return; update the #[utoipa::path(...)] annotation for operation_id "send_referral_code" (path "/referral/send") to add a responses entry for status = 429 with body = model_error_response::ErrorResponse so clients see the rate-limit response in the generated schema.
♻️ Duplicate comments (6)
rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/passwordless.rs (2)
21-21:⚠️ Potential issue | 🟡 MinorAdd
errto this middleware span.Append
, errto the existing#[tracing::instrument(...)]; this function returnsResult<Response, Response>, so the current span drops the error path.Based on learnings: Applies to rust/cloud-storage/**/*.rs : Include
errwhen adding thetracing::instrumentattribute to functions that returnResult.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/passwordless.rs` at line 21, The tracing span on the passwordless middleware currently omits the error field, so update the #[tracing::instrument(...)] attribute on the passwordless middleware function to include err (e.g., add ", err" to the attribute parameters) so the span captures failures for the function that returns Result<Response, Response>; keep the existing skip(ctx, req, next, ip_context) and fields(client_ip=?ip_context) entries and only append err.
21-24:⚠️ Potential issue | 🔴 CriticalFix service wiring for
ClientIpextractors in multiple handlers across authentication_service.The service is currently served with
.into_make_service()atrust/cloud-storage/authentication_service/src/api/mod.rs:63, butClientIpis used as an extractor in at least 15 handlers throughout the service (including this middleware). SinceClientIpfalls back toConnectInfo<SocketAddr>whenX-Forwarded-Foris absent, direct requests will fail at extraction time unless the service is wired withinto_make_service_with_connect_info::<std::net::SocketAddr>(). Update the service entrypoint to use the proper wiring.Additionally, add
errto the#[tracing::instrument]attribute on line 21, as this function returnsResult<Response, Response>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/passwordless.rs` around lines 21 - 24, The handler uses the ClientIp extractor and the tracing attribute is missing error reporting: update the tracing attribute on the handler function (handler) to include err so failures are captured, and fix service wiring so ClientIp can fall back to ConnectInfo by replacing into_make_service() with into_make_service_with_connect_info::<std::net::SocketAddr>() at the service entrypoint (where the service is currently constructed), ensuring all handlers that extract ClientIp succeed on direct connections.rust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rs (1)
32-32:⚠️ Potential issue | 🟡 MinorAdd
errto this handler span.Append
, errto the existing#[tracing::instrument(...)]; this function returnsResult<Response, Response>, so the current span drops the error path.Based on learnings: Applies to rust/cloud-storage/**/*.rs : Include
errwhen adding thetracing::instrumentattribute to functions that returnResult.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rs` at line 32, The tracing span on the handler is missing error tracking; update the #[tracing::instrument(...)] on the function in create_in_progress_link.rs to include err by appending ", err" inside the attribute list (i.e., #[tracing::instrument(skip(ctx, ip_context, user_context), fields(client_ip=?ip_context, user_id=%user_context.user_id, fusion_user_id=%user_context.fusion_user_id), err)]), so the span will capture the error path for the function that returns Result<Response, Response>.rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rs (1)
37-37:⚠️ Potential issue | 🟡 MinorAdd
errto this handler span.Append
, errto the existing#[tracing::instrument(...)]; this function returnsResult<Response, Response>, so the current span drops the error path.Based on learnings: Applies to rust/cloud-storage/**/*.rs : Include
errwhen adding thetracing::instrumentattribute to functions that returnResult.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rs` at line 37, The tracing span on the handler in generate_email_link.rs (the function annotated with #[tracing::instrument(skip(ctx, user_context, ip_context,req), fields(client_ip=?ip_context, email=%req.email, fusion_user_id=%user_context.fusion_user_id))]) omits error capture; because the function returns Result<Response, Response> you should append , err to the instrument attribute (i.e., include err in the list) so errors are recorded in the span for the generate_email_link handler.rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rs (1)
15-15:⚠️ Potential issue | 🟡 MinorAdd
errto this middleware span.Append
, errto the existing#[tracing::instrument(...)]; this function returnsResult<Response, Response>, so the current span drops the error path.Based on learnings: Applies to rust/cloud-storage/**/*.rs : Include
errwhen adding thetracing::instrumentattribute to functions that returnResult.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rs` at line 15, The tracing span on the middleware is missing the error field so error paths are not recorded; update the #[tracing::instrument(...)] on the login_code middleware to include err (e.g., add ", err" alongside the existing skip(ctx, req, next, ip_context) and fields(client_ip=?ip_context)) so the function’s Result<Response, Response> error branch is captured in the span.rust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rs (1)
38-38:⚠️ Potential issue | 🟡 MinorAdd
errto this handler span.Append
, errto the existing#[tracing::instrument(...)]; this function returnsResult<Response, Response>, so the current span drops the error path.Based on learnings: Applies to rust/cloud-storage/**/*.rs : Include
errwhen adding thetracing::instrumentattribute to functions that returnResult.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rs` at line 38, The tracing span on the function with the attribute #[tracing::instrument(skip(ctx, user_context, ip_context), fields(client_ip=?ip_context, fusion_user_id=%user_context.fusion_user_id))] needs to include the error capture so the error path is recorded; modify the attribute on the verify_merge_request handler to add err (e.g., #[tracing::instrument(skip(ctx, user_context, ip_context), fields(...), err)]) so functions that return Result<Response, Response> record the error in the span.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/authentication_service/src/api/mod.rs`:
- Around line 84-89: The routers (e.g., link::router and the /referral router)
now extract ClientIp which falls back to ConnectInfo<SocketAddr>, so before
removing the old IP-context middleware you must ensure the server supplies
ConnectInfo; update the server bootstrap in setup_and_serve to create the
make-service with connect info (use
app.into_make_service_with_connect_info::<SocketAddr>() or the equivalent server
API) instead of app.into_make_service(), so ConnectInfo<SocketAddr> is available
to the decode_jwt middleware and the routers that rely on ClientIp.
In `@rust/cloud-storage/ip_extractor/src/lib.rs`:
- Around line 72-83: The extractor from_request_parts currently trusts
X_FORWARDED_FOR unconditionally (returning ClientIp::ForwardedFor) which lets
callers spoof origin for rate-limiting; update from_request_parts (and
ClientIp::origin_ip usage) to only accept X-Forwarded-For when the request comes
from a trusted proxy (e.g., check the socket address from
parts.extract::<ConnectInfo<SocketAddr>>() against a configured trusted-proxies
list or boolean flag) and otherwise ignore the header and return
ClientIp::DirectIp(conn); ensure the trusted-proxy check happens before
returning ClientIp::ForwardedFor so the rate-limiter in
rust/cloud-storage/referral/src/inbound/axum_router/send_invite.rs that uses
ClientIp::origin_ip() cannot be bypassed by a forged header.
In `@rust/cloud-storage/rate_limit/src/inbound.rs`:
- Around line 57-60: The call to service.check_rate_limit currently maps any
error to Either3::E3(StatusCode::INTERNAL_SERVER_ERROR) without logging; change
this to capture the error, log it with the same logger used elsewhere (matching
the rollback path's pattern), then map to
Either3::E3(StatusCode::INTERNAL_SERVER_ERROR). Concretely, update the
expression around service.check_rate_limit(rate_limit_key,
config).await.map_err(|_| ...) to .await.map_err(|e| { log::error!(target:
"rate_limit", "rate-limiter backend failure: {:?}", e);
Either3::E3(StatusCode::INTERNAL_SERVER_ERROR) })? so the underlying error is
recorded while preserving the same external behavior.
In `@rust/cloud-storage/rate_limit/src/outbound/redis.rs`:
- Around line 68-78: The Lua script in
rust/cloud-storage/rate_limit/src/outbound/redis.rs must restore missing TTLs:
after reading ttl = redis.call('TTL', key) check for ttl < 0 and call
redis.call('EXPIRE', key, window) to reapply the window before returning (both
in the deny branch where current >= max_count and in the allow branch when a key
exists but INCR returns >1 and TTL is missing); ensure you still return the same
tuple structure ({0, current, ttl} or {1, new_count, 0} or updated ttl) but
guarantee EXPIRE(window) is invoked whenever TTL < 0 so counters without expiry
are healed.
In `@rust/cloud-storage/rate_limit/src/outbound/redis/test.rs`:
- Around line 206-226: The test rollback_does_not_go_below_zero currently allows
both None and Some(0) but the decrement Lua script in adapter.decrement never
creates the key when current > 0 is false; update the assertion that reads the
key via super::redis_key(&key) to require count.is_none() only (remove the
allowance for Some(0)) so the test matches the decrement implementation's
behavior.
In `@rust/cloud-storage/referral/Cargo.toml`:
- Around line 8-16: The manifest currently enables rate_limit's axum integration
unconditionally which leaks the HTTP stack; make the rate_limit axum integration
conditional on the crate feature named "axum": add a crate feature "axum" and
wire it to enable the dependency feature on rate_limit (i.e. only include
rate_limit's "axum" feature when the top-level feature "axum" is enabled), mark
the dependency optional if needed and avoid enabling that subfeature by default
so consumers with default-features = false won’t pull in the axum inbound HTTP
stack; update the dependency entry for rate_limit to be gated by the "axum"
feature and adjust the axum = [...] feature list to reflect that gating.
In `@rust/cloud-storage/referral/src/inbound/axum_router/test.rs`:
- Around line 79-87: Replace the nested Ok(Err(...))/Ok(Ok(...)) returns with
the explicit enum constructors to improve readability: when self.should_exceed
is true return Ok(RateLimitResult::Err(RateLimitExceeded { current_count:
config.max_count.saturating_add(1), max_count: config.max_count, retry_after:
config.window })), otherwise return
Ok(RateLimitResult::Ok(RateLimitOk::new_testing_value(0, key, config))); update
the branch that references should_exceed, RateLimitExceeded,
RateLimitOk::new_testing_value and RateLimitResult accordingly.
---
Outside diff comments:
In `@rust/cloud-storage/rate_limit/src/domain/service.rs`:
- Around line 19-26: Add tracing instrumentation to check_rate_limit to match
rollback_ticket: annotate the async fn check_rate_limit(&self, key:
RateLimitKey, config: RateLimitConfig) -> Result<RateLimitResult, Report> with
#[tracing::instrument(err, skip(self))] so errors are recorded and self is
skipped from spans; ensure the tracing macro is available in scope (same usage
as rollback_ticket) and keep function body unchanged.
In `@rust/cloud-storage/referral/src/domain/service.rs`:
- Around line 105-138: The send_referral_invite function is missing the tracing
instrument attribute; add #[tracing::instrument(skip(self), err)] above the
async fn send_referral_invite(...) declaration (matching the pattern used on the
other methods in this impl) so that errors are captured by tracing while
skipping the self parameter; keep the rest of the function (uses of
MacroUserIdStr, InviteToMacro, notification_ingress,
SendNotificationRequestBuilder) unchanged.
- Line 15: Remove unused rate-limiting imports and the generic rate-limit
parameter: delete the imports RateLimitConfig, RateLimitKey, RateLimitResult and
domain::ports::RateLimitService from the top-level use statement, remove the Rl
generic parameter and the rate_limit: Rl field from the ReferralServiceImpl
struct, and remove the Rl: RateLimitService bound from the impl block for
ReferralServiceImpl so the struct and its impl no longer reference the unused
rate-limiting types or field.
In `@rust/cloud-storage/referral/src/domain/service/test.rs`:
- Around line 180-189: The test multiple_invites_succeed_when_under_rate_limit
in test.rs no longer validates meaningful behavior because rate limiting was
removed from the service layer; update the test by either deleting it from
referral/src/domain/service/test.rs or moving it to the router-level tests where
rate limiting is enforced, and remove/replace uses of build_service and
allowing_rate_limiter() and calls to service.send_referral_invite/test_user()
accordingly so the assertion is only present in tests that exercise the actual
rate-limiter (router tests) rather than the service unit tests.
- Around line 143-156: The test
send_referral_invite_fails_when_rate_limit_exceeded is asserting behavior
(ReferralError::RateLimitExceeded) that no longer exists because
ReferralServiceImpl::send_referral_invite in service.rs no longer enforces rate
limits; either delete or update this domain test: remove the test entirely if
rate limiting is now covered by the axum middleware tests (axum_router tests),
or change it to assert success (expect Ok(())) and stop using
exceeding_rate_limiter(); if you want service-level rate limiting instead,
reintroduce the rate-check and return ReferralError::RateLimitExceeded from
ReferralServiceImpl::send_referral_invite so the test again becomes valid.
- Around line 158-178: The test rate_limit_error_contains_exceeded_info assumes
send_referral_invite will return a RateLimitExceeded error, but rate limiting
was removed; update the test to reflect the new behavior by not calling
unwrap_err() on service.send_referral_invite; instead call
service.send_referral_invite(...).await and assert it returns Ok(()) (or remove
the test if rate-limit semantics are no longer relevant). Locate this logic in
the test function rate_limit_error_contains_exceeded_info and the helpers
build_service and exceeding_rate_limiter to adjust or remove assertions
referencing ReferralError::RateLimitExceeded.
In `@rust/cloud-storage/referral/src/inbound/axum_router/send_invite.rs`:
- Around line 31-40: The OpenAPI metadata for the send_referral_code handler is
missing the 429 TOO_MANY_REQUESTS response which the rate-limit middleware can
return; update the #[utoipa::path(...)] annotation for operation_id
"send_referral_code" (path "/referral/send") to add a responses entry for status
= 429 with body = model_error_response::ErrorResponse so clients see the
rate-limit response in the generated schema.
---
Duplicate comments:
In
`@rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rs`:
- Line 37: The tracing span on the handler in generate_email_link.rs (the
function annotated with #[tracing::instrument(skip(ctx, user_context,
ip_context,req), fields(client_ip=?ip_context, email=%req.email,
fusion_user_id=%user_context.fusion_user_id))]) omits error capture; because the
function returns Result<Response, Response> you should append , err to the
instrument attribute (i.e., include err in the list) so errors are recorded in
the span for the generate_email_link handler.
In
`@rust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rs`:
- Line 32: The tracing span on the handler is missing error tracking; update the
#[tracing::instrument(...)] on the function in create_in_progress_link.rs to
include err by appending ", err" inside the attribute list (i.e.,
#[tracing::instrument(skip(ctx, ip_context, user_context),
fields(client_ip=?ip_context, user_id=%user_context.user_id,
fusion_user_id=%user_context.fusion_user_id), err)]), so the span will capture
the error path for the function that returns Result<Response, Response>.
In
`@rust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rs`:
- Line 38: The tracing span on the function with the attribute
#[tracing::instrument(skip(ctx, user_context, ip_context),
fields(client_ip=?ip_context, fusion_user_id=%user_context.fusion_user_id))]
needs to include the error capture so the error path is recorded; modify the
attribute on the verify_merge_request handler to add err (e.g.,
#[tracing::instrument(skip(ctx, user_context, ip_context), fields(...), err)])
so functions that return Result<Response, Response> record the error in the
span.
In
`@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rs`:
- Line 15: The tracing span on the middleware is missing the error field so
error paths are not recorded; update the #[tracing::instrument(...)] on the
login_code middleware to include err (e.g., add ", err" alongside the existing
skip(ctx, req, next, ip_context) and fields(client_ip=?ip_context)) so the
function’s Result<Response, Response> error branch is captured in the span.
In
`@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/passwordless.rs`:
- Line 21: The tracing span on the passwordless middleware currently omits the
error field, so update the #[tracing::instrument(...)] attribute on the
passwordless middleware function to include err (e.g., add ", err" to the
attribute parameters) so the span captures failures for the function that
returns Result<Response, Response>; keep the existing skip(ctx, req, next,
ip_context) and fields(client_ip=?ip_context) entries and only append err.
- Around line 21-24: The handler uses the ClientIp extractor and the tracing
attribute is missing error reporting: update the tracing attribute on the
handler function (handler) to include err so failures are captured, and fix
service wiring so ClientIp can fall back to ConnectInfo by replacing
into_make_service() with
into_make_service_with_connect_info::<std::net::SocketAddr>() at the service
entrypoint (where the service is currently constructed), ensuring all handlers
that extract ClientIp succeed on direct connections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d972ebb-820a-43f3-b7cc-7391ff3d6b4e
⛔ Files ignored due to path filters (1)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
rust/cloud-storage/authentication_service/src/api/context.rsrust/cloud-storage/authentication_service/src/api/email/generate_email_link.rsrust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rsrust/cloud-storage/authentication_service/src/api/email/verify_email_link.rsrust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rsrust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rsrust/cloud-storage/authentication_service/src/api/link/github.rsrust/cloud-storage/authentication_service/src/api/link/mod.rsrust/cloud-storage/authentication_service/src/api/login/mod.rsrust/cloud-storage/authentication_service/src/api/login/password.rsrust/cloud-storage/authentication_service/src/api/login/passwordless.rsrust/cloud-storage/authentication_service/src/api/merge/create_merge_request.rsrust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rsrust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rsrust/cloud-storage/authentication_service/src/api/middleware/rate_limit/passwordless.rsrust/cloud-storage/authentication_service/src/api/mod.rsrust/cloud-storage/authentication_service/src/api/oauth/mod.rsrust/cloud-storage/authentication_service/src/api/user/create_user.rsrust/cloud-storage/authentication_service/src/api/user/mod.rsrust/cloud-storage/authentication_service/src/main.rsrust/cloud-storage/fusionauth/src/user/create.rsrust/cloud-storage/fusionauth/src/user/mod.rsrust/cloud-storage/fusionauth/src/user/verify.rsrust/cloud-storage/image_proxy_service/src/api/mod.rsrust/cloud-storage/ip_extractor/Cargo.tomlrust/cloud-storage/ip_extractor/src/lib.rsrust/cloud-storage/ip_extractor/src/test.rsrust/cloud-storage/macro_middleware/Cargo.tomlrust/cloud-storage/macro_middleware/src/tracking/mod.rsrust/cloud-storage/model/src/lib.rsrust/cloud-storage/model/src/tracking.rsrust/cloud-storage/notification/Cargo.tomlrust/cloud-storage/notification/src/domain/service/egress.rsrust/cloud-storage/notification/src/domain/service/test.rsrust/cloud-storage/notification_sandbox/Cargo.tomlrust/cloud-storage/notification_sandbox/src/adapters/noop_rate_limiter.rsrust/cloud-storage/rate_limit/Cargo.tomlrust/cloud-storage/rate_limit/src/domain/models.rsrust/cloud-storage/rate_limit/src/domain/ports.rsrust/cloud-storage/rate_limit/src/domain/service.rsrust/cloud-storage/rate_limit/src/inbound.rsrust/cloud-storage/rate_limit/src/lib.rsrust/cloud-storage/rate_limit/src/outbound/redis.rsrust/cloud-storage/rate_limit/src/outbound/redis/test.rsrust/cloud-storage/referral/Cargo.tomlrust/cloud-storage/referral/src/domain/service.rsrust/cloud-storage/referral/src/domain/service/test.rsrust/cloud-storage/referral/src/inbound/axum_router.rsrust/cloud-storage/referral/src/inbound/axum_router/get_referral_code.rsrust/cloud-storage/referral/src/inbound/axum_router/send_invite.rsrust/cloud-storage/referral/src/inbound/axum_router/test.rsrust/cloud-storage/seed_cli/src/service/auth/mod.rs
💤 Files with no reviewable changes (5)
- rust/cloud-storage/authentication_service/src/api/oauth/mod.rs
- rust/cloud-storage/model/src/tracking.rs
- rust/cloud-storage/authentication_service/src/api/link/mod.rs
- rust/cloud-storage/authentication_service/src/api/user/mod.rs
- rust/cloud-storage/model/src/lib.rs
Comment on lines
+68
to
+78
| local current = tonumber(redis.call('GET', key) or '0') | ||
| if current >= max_count then | ||
| local ttl = redis.call('TTL', key) | ||
| if ttl < 0 then ttl = 0 end | ||
| return {0, current, ttl} | ||
| end | ||
| local new_count = redis.call('INCR', key) | ||
| if new_count == 1 then | ||
| redis.call('EXPIRE', key, window) | ||
| end | ||
| return {1, new_count, 0} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "redis.rs" | grep -E "rate.*limit|outbound"Repository: macro-inc/macro
Length of output: 175
🏁 Script executed:
cat -n rust/cloud-storage/rate_limit/src/outbound/redis.rs | head -100Repository: macro-inc/macro
Length of output: 4085
Repair keys that have lost their expiry.
This Lua script only sets EXPIRE when INCR returns 1. If a counter already exists without TTL (from data migration, manual repair, or earlier bugs), the deny path returns ttl = 0 without restoring it, and the allow path keeps incrementing without resetting the expiry. That leaves the rate limit permanently stuck for that key instead of healing within the configured window.
Suggested Lua fix
local current = tonumber(redis.call('GET', key) or '0')
if current >= max_count then
local ttl = redis.call('TTL', key)
- if ttl < 0 then ttl = 0 end
+ if ttl < 0 then
+ redis.call('EXPIRE', key, window)
+ ttl = window
+ end
return {0, current, ttl}
end
local new_count = redis.call('INCR', key)
- if new_count == 1 then
+ local ttl = redis.call('TTL', key)
+ if new_count == 1 or ttl < 0 then
redis.call('EXPIRE', key, window)
end
return {1, new_count, 0}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/rate_limit/src/outbound/redis.rs` around lines 68 - 78,
The Lua script in rust/cloud-storage/rate_limit/src/outbound/redis.rs must
restore missing TTLs: after reading ttl = redis.call('TTL', key) check for ttl <
0 and call redis.call('EXPIRE', key, window) to reapply the window before
returning (both in the deny branch where current >= max_count and in the allow
branch when a key exists but INCR returns >1 and TTL is missing); ensure you
still return the same tuple structure ({0, current, ttl} or {1, new_count, 0} or
updated ttl) but guarantee EXPIRE(window) is invoked whenever TTL < 0 so
counters without expiry are healed.
Comment on lines
+206
to
+226
| #[tokio::test] | ||
| async fn rollback_does_not_go_below_zero() { | ||
| let client = redis_client(); | ||
| let adapter = adapter(); | ||
| let key = unique_key("rollback_floor"); | ||
| let key_str = super::redis_key(&key); | ||
|
|
||
| // Decrement on a key that was never incremented. | ||
| adapter.decrement(&key).await.unwrap(); | ||
|
|
||
| let count: Option<u64> = redis::AsyncCommands::get( | ||
| &mut client.get_multiplexed_async_connection().await.unwrap(), | ||
| &key_str, | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
| assert!( | ||
| count.is_none() || count == Some(0), | ||
| "counter should not go negative, got {count:?}" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the decrement implementation to understand expected behavior
rg -n -A 20 'async fn decrement' rust/cloud-storage/rate_limit/src/outbound/redis.rsRepository: macro-inc/macro
Length of output: 893
Strengthen the rollback floor assertion to match the implementation.
The assertion on line 223 accepts both None and Some(0), but the Lua script in the decrement implementation prevents key creation. When decrement is called on a non-existent key, the script evaluates current > 0 as false (since the missing key defaults to 0), so it never invokes DECR, leaving the key non-existent. The test should assert only None:
Suggested change
assert!(
- count.is_none() || count == Some(0),
- "counter should not go negative, got {count:?}"
+ count.is_none(),
+ "key should remain non-existent after decrement on non-existent key, got {count:?}"
);📝 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.
Suggested change
| #[tokio::test] | |
| async fn rollback_does_not_go_below_zero() { | |
| let client = redis_client(); | |
| let adapter = adapter(); | |
| let key = unique_key("rollback_floor"); | |
| let key_str = super::redis_key(&key); | |
| // Decrement on a key that was never incremented. | |
| adapter.decrement(&key).await.unwrap(); | |
| let count: Option<u64> = redis::AsyncCommands::get( | |
| &mut client.get_multiplexed_async_connection().await.unwrap(), | |
| &key_str, | |
| ) | |
| .await | |
| .unwrap(); | |
| assert!( | |
| count.is_none() || count == Some(0), | |
| "counter should not go negative, got {count:?}" | |
| ); | |
| } | |
| #[tokio::test] | |
| async fn rollback_does_not_go_below_zero() { | |
| let client = redis_client(); | |
| let adapter = adapter(); | |
| let key = unique_key("rollback_floor"); | |
| let key_str = super::redis_key(&key); | |
| // Decrement on a key that was never incremented. | |
| adapter.decrement(&key).await.unwrap(); | |
| let count: Option<u64> = redis::AsyncCommands::get( | |
| &mut client.get_multiplexed_async_connection().await.unwrap(), | |
| &key_str, | |
| ) | |
| .await | |
| .unwrap(); | |
| assert!( | |
| count.is_none(), | |
| "key should remain non-existent after decrement on non-existent key, got {count:?}" | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/rate_limit/src/outbound/redis/test.rs` around lines 206 -
226, The test rollback_does_not_go_below_zero currently allows both None and
Some(0) but the decrement Lua script in adapter.decrement never creates the key
when current > 0 is false; update the assertion that reads the key via
super::redis_key(&key) to require count.is_none() only (remove the allowance for
Some(0)) so the test matches the decrement implementation's behavior.
Comment on lines
8
to
16
| axum = [ | ||
| "dep:axum", | ||
| "dep:axum-extra", | ||
| "dep:ip_extractor", | ||
| "dep:model-error-response", | ||
| "dep:model_user", | ||
| "dep:tower", | ||
| "dep:utoipa", | ||
| ] |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Gate rate_limit's Axum integration behind referral's axum feature.
Line 38 enables rate_limit's axum support for every build, so default-features = false consumers still pull the inbound HTTP stack. That makes this manifest's feature split leaky.
♻️ Suggested feature wiring
[features]
axum = [
"dep:axum",
"dep:axum-extra",
"dep:ip_extractor",
"dep:model-error-response",
"dep:model_user",
"dep:tower",
+ "rate_limit/axum",
"dep:utoipa",
]
...
-rate_limit = { path = "../rate_limit", features = ["axum"] }
+rate_limit = { path = "../rate_limit" }Also applies to: 38-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/referral/Cargo.toml` around lines 8 - 16, The manifest
currently enables rate_limit's axum integration unconditionally which leaks the
HTTP stack; make the rate_limit axum integration conditional on the crate
feature named "axum": add a crate feature "axum" and wire it to enable the
dependency feature on rate_limit (i.e. only include rate_limit's "axum" feature
when the top-level feature "axum" is enabled), mark the dependency optional if
needed and avoid enabling that subfeature by default so consumers with
default-features = false won’t pull in the axum inbound HTTP stack; update the
dependency entry for rate_limit to be gated by the "axum" feature and adjust the
axum = [...] feature list to reflect that gating.
Comment on lines
+79
to
+87
| if self.should_exceed { | ||
| Ok(Err(RateLimitExceeded { | ||
| current_count: config.max_count.saturating_add(1), | ||
| max_count: config.max_count, | ||
| retry_after: config.window, | ||
| })) | ||
| } else { | ||
| Ok(Ok(RateLimitOk::new_testing_value(0, key, config))) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using explicit RateLimitResult::Ok/RateLimitResult::Err for clarity.
The current pattern Ok(Err(...)) and Ok(Ok(...)) works because RateLimitResult is a type alias for Result<RateLimitOk, RateLimitExceeded>, but it's less readable than the explicit form used in domain/service/test.rs:
Ok(RateLimitResult::Err(RateLimitExceeded { ... }))
Ok(RateLimitResult::Ok(RateLimitOk::new_testing_value(...)))This makes it clearer that the outer Ok is for the Report error path while the inner is the rate-limit outcome.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/referral/src/inbound/axum_router/test.rs` around lines 79
- 87, Replace the nested Ok(Err(...))/Ok(Ok(...)) returns with the explicit enum
constructors to improve readability: when self.should_exceed is true return
Ok(RateLimitResult::Err(RateLimitExceeded { current_count:
config.max_count.saturating_add(1), max_count: config.max_count, retry_after:
config.window })), otherwise return
Ok(RateLimitResult::Ok(RateLimitOk::new_testing_value(0, key, config))); update
the branch that references should_exceed, RateLimitExceeded,
RateLimitOk::new_testing_value and RateLimitResult accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/cloud-storage/notification/src/domain/service/egress.rs (1)
227-247:⚠️ Potential issue | 🔴 CriticalDon't roll the ticket back on every email error.
send_emailis a non-idempotent external call. AnErrhere only means we didn't observe success; it does not prove the provider rejected the message. Because failed deliveries are retried later (the queue message remains), rolling the ticket back unconditionally can reopen quota after an ambiguous timeout and allow duplicate emails past the configured rate limit.The
EmailSendertrait and SES implementation provide no error classification or idempotency markers. Only rollback on failures that are guaranteed pre-send, or add idempotency tokens (Message-ID deduplication) and error classification to distinguish "definitely not sent" from ambiguous transport failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/notification/src/domain/service/egress.rs` around lines 227 - 247, The code currently calls rate_limiter.rollback_ticket(ticket_ok) whenever send_email(recipient.clone(), &content).await returns Err, which can reopen quota for ambiguous delivery failures; change this so we do NOT unconditionally rollback on every send_email error: update the EmailSender trait (and SES implementation) to return a classified result or error kind (e.g., EmailSendResult { Sent, NotSentPreSend, AmbiguousTransport }) or an ErrorKind/Idempotency marker, then only call rate_limiter.rollback_ticket(...) when the error is a guaranteed pre-send failure (e.g., NotSentPreSend/ErrorKind::PreSend); alternatively implement idempotency (Message-ID deduplication) and use that classification to decide rollback; adjust the egress::send flow to inspect the new classification from send_email instead of rolling back for all Errs and map ambiguous errors to DeliveryFailure::Other without rollback.rust/cloud-storage/referral/src/domain/service.rs (1)
105-138: 🧹 Nitpick | 🔵 TrivialAdd
#[tracing::instrument]attribute for consistency and observability.The
send_referral_invitemethod returnsResult<(), ReferralError>but is missing the#[tracing::instrument(skip(self), err)]attribute that all other methods in this impl have. This inconsistency reduces observability for this endpoint.♻️ Proposed fix
+ #[tracing::instrument(skip(self), err)] async fn send_referral_invite( &self, sending_user: MacroUserIdStr<'_>, recipient: EmailStr<'static>, ) -> Result<(), ReferralError> {As per coding guidelines: "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/referral/src/domain/service.rs` around lines 105 - 138, Add the missing tracing attribute to improve observability: annotate the async method send_referral_invite in the Referral service impl with #[tracing::instrument(skip(self), err)] (matching the pattern used on other methods in this impl) so the function arguments are traced and errors are recorded; ensure the attribute is placed immediately above the async fn send_referral_invite(...) signature and include skip(self) to avoid capturing self.
♻️ Duplicate comments (22)
rust/cloud-storage/authentication_service/src/api/mod.rs (1)
84-89:⚠️ Potential issue | 🟠 MajorWire
ConnectInfobefore removing the old IP middleware.
/linkhandlers and the/referralIP-rate-limit path now depend onClientIp, butsetup_and_servestill usesapp.into_make_service(). Requests withoutX-Forwarded-Forwill fail extraction before the handler unless the server is built withinto_make_service_with_connect_info::<SocketAddr>().🔧 Required bootstrap change
+use std::net::SocketAddr; ... - axum::serve(listener, app.into_make_service()) + axum::serve(listener, app.into_make_service_with_connect_info::<SocketAddr>())Also applies to: 114-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/mod.rs` around lines 84 - 89, The server must be built with ConnectInfo so ClientIp extraction succeeds; update setup_and_serve to use into_make_service_with_connect_info::<SocketAddr>() instead of into_make_service(), and ensure the router(s) that include link::router(...) and the /referral IP-rate-limit path (which rely on ClientIp) remain mounted before removing the old IP middleware; keep the axum::middleware::from_fn_with_state wrapping macro_middleware::auth::decode_jwt::handler as-is but ensure the service is created via into_make_service_with_connect_info::<SocketAddr>() so ClientIp can be extracted at handler time.rust/cloud-storage/authentication_service/src/api/login/passwordless.rs (1)
33-33:⚠️ Potential issue | 🟡 MinorAdd
errto thistracing::instrumentannotation.This handler returns
Result<Response, Response>, so the instrumentation should capture the error path too.♻️ Suggested fix
-#[tracing::instrument(skip(ctx, req, ip_context), fields(email=%req.email, client_ip=?ip_context))] +#[tracing::instrument(skip(ctx, req, ip_context), fields(email=%req.email, client_ip=?ip_context), err)]As per coding guidelines:
Include err when adding the tracing::instrument attribute to functions that return Result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/login/passwordless.rs` at line 33, The tracing instrumentation on the passwordless handler currently uses #[tracing::instrument(skip(ctx, req, ip_context), fields(email=%req.email, client_ip=?ip_context))] but omits error capture; update the attribute to include err (e.g. #[tracing::instrument(skip(ctx, req, ip_context), fields(email=%req.email, client_ip=?ip_context), err)]) so the handler (the function annotated with tracing::instrument that takes ctx, req, ip_context and returns Result<Response, Response>) records error information on failure.rust/cloud-storage/referral/Cargo.toml (1)
8-16: 🛠️ Refactor suggestion | 🟠 MajorGate
rate_limit's Axum integration behindreferral'saxumfeature.Line 38 still enables
rate_limit/axumfor every build, sodefault-features = falseconsumers still pull the inbound HTTP stack. Wire that subfeature through this crate'saxumfeature instead of the dependency declaration.♻️ Suggested feature wiring
[features] axum = [ "dep:axum", "dep:axum-extra", "dep:ip_extractor", "dep:model-error-response", "dep:model_user", + "rate_limit/axum", "dep:tower", "dep:utoipa", ] ... -rate_limit = { path = "../rate_limit", features = ["axum"] } +rate_limit = { path = "../rate_limit" }Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/referral/Cargo.toml` around lines 8 - 16, The Cargo.toml currently enables the rate_limit crate's "axum" subfeature unconditionally; gate that integration behind this crate's "axum" feature by removing the direct "rate_limit/axum" activation from the dependency list and instead add a feature entry mapping your crate's "axum" feature to enable "rate_limit/axum". Concretely, make the rate_limit dependency optional (optional = true or don't enable its axum subfeature inline), add "axum" to your [features] table with "rate_limit/axum" as a member, and ensure consumers can opt in via the crate feature "axum" so default-features = false no longer pulls the Axum inbound stack.rust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rs (1)
32-35:⚠️ Potential issue | 🟡 MinorStill add
errto this tracing span.
handlerreturnsResult<Response, Response>, so the span still won't record the error path.As per coding guidelines: "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rs` around lines 32 - 35, The tracing span on the handler function doesn't include the error field; update the tracing::instrument attribute on the async fn handler (the function declared as pub async fn handler with State(ctx): State<ApiContext, ip_context: ClientIp, user_context: ...) ) to include err so failures are recorded (e.g., add err to the attribute arguments), ensuring fields(...) remains and skip(...) is unchanged; this will capture the Result error path in the span.rust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rs (1)
30-33:⚠️ Potential issue | 🟡 MinorStill add
errto this tracing span.
handlerreturnsResult<Response, Response>, so resend failures still bypass span-level error recording.As per coding guidelines: "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rs` around lines 30 - 33, The tracing span on the handler function must include error capture: update the #[tracing::instrument(...)] on the handler (the async fn handler that returns Result<Response, Response>) to include err in its attributes so failures are recorded at the span level; keep the existing skip(ctx, ip_context, req) and fields(client_ip=?ip_context, email=%req.email) and add err to the attribute list.rust/cloud-storage/authentication_service/src/api/email/verify_email_link.rs (1)
32-35:⚠️ Potential issue | 🟡 MinorStill add
errto this tracing span.
handlerreturnsResult<Response, Response>, so the redirect/error path still won't be attached to the span automatically.As per coding guidelines: "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/email/verify_email_link.rs` around lines 32 - 35, The tracing span on the handler function must capture errors — update the tracing::instrument attribute on fn handler (the function named handler that returns Result<Response, Response>) to include err (e.g., add err to the attribute parameters along with the existing skip and fields entries) so any returned error/redirect path is attached to the span; leave skip(ctx, ip_context) and fields(client_ip=?ip_context) intact while adding err.rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/passwordless.rs (1)
21-24:⚠️ Potential issue | 🟡 MinorStill add
errto this tracing span.This middleware returns
Result<Response, Response>, so the rejection path still won't be recorded by the instrumented span.As per coding guidelines: "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/passwordless.rs` around lines 21 - 24, The tracing span on the handler function doesn't record rejections because the attribute omits the `err` flag; update the #[tracing::instrument(...)] on the handler (the async fn handler in passwordless.rs) to include `err` (e.g. add `, err` in the attribute alongside the existing skip/fields) so that the span captures the error path for the Result<Response, Response> return.rust/cloud-storage/authentication_service/src/api/merge/create_merge_request.rs (1)
42-46:⚠️ Potential issue | 🟡 MinorStill add
errto this tracing span.
handlerreturnsResult<Response, Response>, so merge-request failures still won't be recorded by the instrumented span.As per coding guidelines: "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/merge/create_merge_request.rs` around lines 42 - 46, The tracing span on the handler function doesn't capture returned errors because the #[tracing::instrument(...)] attribute omits err; update the attribute on the handler (the function named handler in create_merge_request.rs) to include err (i.e., add err to the tracing::instrument attribute parameters alongside skip(...) and fields(...)) so returned Result::Err values are recorded by the span.rust/cloud-storage/authentication_service/src/api/user/create_user.rs (1)
38-38:⚠️ Potential issue | 🟡 MinorAdd
errto#[tracing::instrument]oncreate_userhandler.Line 38: this handler returns
Result<Response, Response>and should includeerrin the instrumentation attribute.🔧 Proposed fix
-#[tracing::instrument(skip(ctx, ip_context, req), fields(client_ip=?ip_context))] +#[tracing::instrument(skip(ctx, ip_context, req), fields(client_ip=?ip_context), err)]As per coding guidelines: "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/user/create_user.rs` at line 38, The tracing attribute on the create_user handler must include err because the function returns Result<Response, Response>; update the #[tracing::instrument(...)] on create_user to add err (e.g., #[tracing::instrument(err, skip(ctx, ip_context, req), fields(client_ip=?ip_context))]) so errors are captured in spans for Response-returning functions and keep the existing skip and fields arguments intact.rust/cloud-storage/notification/src/domain/service/test.rs (1)
1077-1079:⚠️ Potential issue | 🟡 MinorMake
decrementobservable in the mock rate limiter.Line 1077: this no-op keeps rollback behavior untestable. Record keys in
decrementand assert at least one rollback path in a failing-delivery test.🔧 Proposed fix
struct MockRateLimitPort { should_exceed: bool, + decremented_keys: Mutex<Vec<RateLimitKey>>, } async fn decrement(&self, _key: &RateLimitKey) -> Result<(), Report> { + self.decremented_keys.lock().unwrap().push(_key.clone()); Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/notification/src/domain/service/test.rs` around lines 1077 - 1079, The mock rate limiter's async method decrement(&self, _key: &RateLimitKey) is currently a no-op, preventing tests from observing rollbacks; modify the mock implementation of decrement to record received RateLimitKey values (e.g., push to an internal Vec/VecDeque or send over an mpsc/watch channel) so tests can assert that a rollback path invoked decrement at least once; update the failing-delivery test to read from that recorded store/receiver and assert that the expected key(s) were observed, referencing the mock rate limiter type and the decrement(&self, _key: &RateLimitKey) method to locate the change.rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rs (1)
37-37:⚠️ Potential issue | 🟡 MinorAdd
errto handler instrumentation.Line 37:
handlerreturnsResult<Response, Response>, but the tracing attribute does not includeerr.🔧 Proposed fix
-#[tracing::instrument(skip(ctx, user_context, ip_context,req), fields(client_ip=?ip_context, email=%req.email, fusion_user_id=%user_context.fusion_user_id))] +#[tracing::instrument(skip(ctx, user_context, ip_context,req), fields(client_ip=?ip_context, email=%req.email, fusion_user_id=%user_context.fusion_user_id), err)]As per coding guidelines: "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rs` at line 37, The tracing attribute on the handler in generate_email_link.rs is missing the err flag for a function returning Result; update the #[tracing::instrument(...)] on the handler function to include err (e.g. #[tracing::instrument(skip(ctx, user_context, ip_context, req), fields(client_ip=?ip_context, email=%req.email, fusion_user_id=%user_context.fusion_user_id), err)]) so errors are captured by tracing for the handler that returns Result<Response, Response>.rust/cloud-storage/authentication_service/src/api/login/password.rs (1)
31-31:⚠️ Potential issue | 🟡 MinorMissing
erron Result-returning password handler span.Line 31: add
errto the tracing attribute so error paths are attached to the span.🔧 Proposed fix
-#[tracing::instrument(skip(ctx, req, ip_context), fields(email=%req.email, client_ip=?ip_context))] +#[tracing::instrument(skip(ctx, req, ip_context), fields(email=%req.email, client_ip=?ip_context), err)]As per coding guidelines: "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/login/password.rs` at line 31, The tracing span on the password handler is missing the err flag so error returns aren't attached to the span; update the #[tracing::instrument(...)] attribute on the function that takes ctx, req, ip_context (the password handler in password.rs) to include err (alongside the existing skip(ctx, req, ip_context) and fields(email=%req.email, client_ip=?ip_context)) so the macro attaches error information for Result-returning paths.rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rs (1)
15-15:⚠️ Potential issue | 🟡 MinorAdd
errto middleware tracing instrumentation.Line 15: this middleware returns
Result<Response, Response>, buterris missing from#[tracing::instrument(...)].🔧 Proposed fix
-#[tracing::instrument(skip(ctx, req, next, ip_context), fields(client_ip=?ip_context))] +#[tracing::instrument(skip(ctx, req, next, ip_context), fields(client_ip=?ip_context), err)]As per coding guidelines: "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rs` at line 15, The tracing attribute on the middleware is missing the err hook even though the function returns Result<Response, Response>; update the attribute on the annotated middleware (the #[tracing::instrument(...)] line in login_code.rs) to include err so errors are recorded, e.g. change #[tracing::instrument(skip(ctx, req, next, ip_context), fields(client_ip=?ip_context))] to include err: #[tracing::instrument(skip(ctx, req, next, ip_context), err, fields(client_ip=?ip_context))].rust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rs (1)
38-38:⚠️ Potential issue | 🟡 MinorInclude
errin tracing span for this Result handler.Line 38: this
#[tracing::instrument(...)]is missingerreven thoughhandlerreturnsResult<Response, Response>.🔧 Proposed fix
-#[tracing::instrument(skip(ctx, user_context, ip_context), fields(client_ip=?ip_context, fusion_user_id=%user_context.fusion_user_id))] +#[tracing::instrument(skip(ctx, user_context, ip_context), fields(client_ip=?ip_context, fusion_user_id=%user_context.fusion_user_id), err)]As per coding guidelines: "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rs` at line 38, The tracing attribute on the handler in verify_merge_request.rs is missing the err option, so errors returned by the handler (which returns Result<Response, Response>) are not recorded; update the #[tracing::instrument(...)] on the handler function to include err (e.g., add err alongside the existing skip(...) and fields(...) arguments) so the span captures the Result error, keeping the same skip(ctx, user_context, ip_context) and fields(client_ip=?ip_context, fusion_user_id=%user_context.fusion_user_id).rust/cloud-storage/authentication_service/src/api/link/github.rs (1)
70-70:⚠️ Potential issue | 🟡 MinorAdd
errto Result-returning instrumentation.Line 70:
init_github_link_handlerreturnsResult, but the#[tracing::instrument(...)]attribute omitserr.🔧 Proposed fix
-#[tracing::instrument(skip(ctx, ip_context, user_context), fields(client_ip=?ip_context, user_id=%user_context.user_id, fusion_user_id=%user_context.fusion_user_id))] +#[tracing::instrument(skip(ctx, ip_context, user_context), fields(client_ip=?ip_context, user_id=%user_context.user_id, fusion_user_id=%user_context.fusion_user_id), err)]As per coding guidelines: "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/link/github.rs` at line 70, The tracing::instrument on init_github_link_handler omits the err flag for a function that returns Result; update the attribute on init_github_link_handler to include err (e.g., add , err to the #[tracing::instrument(...)] attributes) while keeping the existing skip(ctx, ip_context, user_context) and fields(client_ip=?ip_context, user_id=%user_context.user_id, fusion_user_id=%user_context.fusion_user_id) entries so errors are recorded in traces.rust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rs (1)
29-29:⚠️ Potential issue | 🟡 MinorAdd
errto email verification handler instrumentation.Line 29: this function returns
Result<Response, Response>, but#[tracing::instrument(...)]omitserr.🔧 Proposed fix
-#[tracing::instrument(skip(ctx, ip_context), fields(client_ip=?ip_context))] +#[tracing::instrument(skip(ctx, ip_context), fields(client_ip=?ip_context), err)]As per coding guidelines: "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rs` at line 29, The tracing instrumentation on the verify_fusionauth_user_email handler omits error capture; update the attribute on the verify_fusionauth_user_email function to include err (e.g. add , err to the #[tracing::instrument(...)] attribute) so that functions returning Result<Response, Response> record errors; keep skip(ctx, ip_context) and fields(client_ip=?ip_context) as-is while adding err to the attribute.rust/cloud-storage/ip_extractor/src/test.rs (1)
229-243: 🧹 Nitpick | 🔵 TrivialConsider adding test for
ExtensionErr→ 500 status code.The
into_responsetests coverParseErrandInvalidAsciimapping to 400, but theExtensionErr→ 500 mapping is not tested. This would complete the error response coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/ip_extractor/src/test.rs` around lines 229 - 243, Add a test that constructs a ClientIpError::ExtensionErr, calls its into_response() and asserts the response.status() is StatusCode::INTERNAL_SERVER_ERROR; specifically add a new test function (e.g., error_response_extension_err_is_500) that creates ClientIpError::ExtensionErr, invokes ClientIpError::into_response(), and asserts equality with StatusCode::INTERNAL_SERVER_ERROR to complete coverage for the into_response mapping.rust/cloud-storage/referral/src/inbound/axum_router/test.rs (1)
79-87: 🧹 Nitpick | 🔵 TrivialConsider using explicit
RateLimitResult::Ok/RateLimitResult::Errfor consistency.This mock uses
Ok(Err(...))andOk(Ok(...))whiledomain/service/test.rsuses the explicitRateLimitResult::Err(...)andRateLimitResult::Ok(...)form. Using the explicit form improves readability by making it clear that the outerOkis for theReporterror path while the inner is the rate-limit outcome.♻️ Suggested change for consistency
async fn check( &self, key: RateLimitKey, config: RateLimitConfig, ) -> Result<RateLimitResult, Report> { if self.should_exceed { - Ok(Err(RateLimitExceeded { + Ok(RateLimitResult::Err(RateLimitExceeded { current_count: config.max_count.saturating_add(1), max_count: config.max_count, retry_after: config.window, })) } else { - Ok(Ok(RateLimitOk::new_testing_value(0, key, config))) + Ok(RateLimitResult::Ok(RateLimitOk::new_testing_value(0, key, config))) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/referral/src/inbound/axum_router/test.rs` around lines 79 - 87, The test mock currently returns nested Ok(Err(...)) and Ok(Ok(...)); change these to the explicit enum constructors to match domain/service/test.rs by returning Ok(RateLimitResult::Err(RateLimitExceeded { ... })) when self.should_exceed is true and Ok(RateLimitResult::Ok(RateLimitOk::new_testing_value(0, key, config))) otherwise; update the references to RateLimitExceeded, RateLimitOk, RateLimitResult, and should_exceed accordingly to improve readability and consistency.rust/cloud-storage/rate_limit/src/inbound.rs (1)
57-60:⚠️ Potential issue | 🟠 MajorLog backend rate-limit failures before mapping them to 500.
When
check_rate_limiterrors, the root cause is dropped entirely, which makes Redis/rate-limit outages invisible in production. Mirror the rollback path and log before returningINTERNAL_SERVER_ERROR.Suggested fix
let res = service .check_rate_limit(rate_limit_key, config) .await - .map_err(|_| Either3::E3(StatusCode::INTERNAL_SERVER_ERROR))?; + .inspect_err(|e| { + tracing::error!(error = ?e, "Failed to check rate limit"); + }) + .map_err(|_| Either3::E3(StatusCode::INTERNAL_SERVER_ERROR))?;As per coding guidelines, "Prefer using
.inspect_errinstead ofif let Err(e)in order to do logging in Rust" and "When including an error with a log, include it like so:tracing::error!(error=?e, "error msg");instead of injecting it directly into the error message".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/rate_limit/src/inbound.rs` around lines 57 - 60, When calling service.check_rate_limit(rate_limit_key, config).await, preserve and log any underlying error before mapping it to Either3::E3(StatusCode::INTERNAL_SERVER_ERROR): use .inspect_err (or a map_err closure) to call tracing::error!(error=?e, "rate limit backend check failed for {rate_limit_key}") where e is the error, then return Either3::E3(StatusCode::INTERNAL_SERVER_ERROR) as before; update the call site referencing service.check_rate_limit, rate_limit_key, config, and Either3::E3 so the root error is visible in logs rather than dropped.rust/cloud-storage/rate_limit/src/outbound/redis/test.rs (1)
221-223:⚠️ Potential issue | 🟡 MinorAssert
Noneonly after decrementing a missing key.
rust/cloud-storage/rate_limit/src/outbound/redis.rsonly decrements whencurrent > 0, so this path should leave the key absent. AllowingSome(0)weakens the regression check.Suggested fix
assert!( - count.is_none() || count == Some(0), - "counter should not go negative, got {count:?}" + count.is_none(), + "key should remain non-existent after decrement on a missing key, got {count:?}" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/rate_limit/src/outbound/redis/test.rs` around lines 221 - 223, The test's assertion is too permissive—change the assertion in the Redis outbound test (the assert checking `count` after decrement) to require the key be absent (`count.is_none()`) instead of allowing `Some(0)`; update the failure message accordingly to indicate the counter must be absent (e.g., "counter should be absent after decrementing a missing key, got {count:?}"). This aligns the test with the behavior in the `decrement`/outbound Redis logic which only decrements when `current > 0`, leaving the key missing.rust/cloud-storage/rate_limit/src/outbound/redis.rs (1)
66-74:⚠️ Potential issue | 🟠 MajorReapply expiry when a counter already exists without TTL.
If this key ever loses its expiry, the deny path just returns
ttl = 0, and the allow path only callsEXPIREwhennew_count == 1. That leaves the rate-limit permanently sticky for that key instead of healing on the next check.Suggested fix
local current = tonumber(redis.call('GET', key) or '0') if current >= max_count then local ttl = redis.call('TTL', key) - if ttl < 0 then ttl = 0 end + if ttl < 0 then + redis.call('EXPIRE', key, window) + ttl = window + end return {0, current, ttl} end local new_count = redis.call('INCR', key) - if new_count == 1 then + local ttl = redis.call('TTL', key) + if new_count == 1 or ttl < 0 then redis.call('EXPIRE', key, window) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/rate_limit/src/outbound/redis.rs` around lines 66 - 74, The Lua snippet can leave a counter without expiry (ttl == -1) so the key becomes permanently stuck; modify the logic around the TTL/EXPIRE calls for the symbols key, TTL, INCR, EXPIRE, current, max_count, and window so that: before returning the deny path (when current >= max_count) check TTL and if ttl < 0 call redis.call('EXPIRE', key, window) (then fetch TTL again or return a positive ttl); and also after INCR (when new_count > 1) check the key's TTL and call EXPIRE(key, window) if the TTL is missing. Ensure both deny and allow paths reapply expiry when ttl < 0 so the counter can heal.rust/cloud-storage/ip_extractor/src/lib.rs (1)
76-81:⚠️ Potential issue | 🔴 CriticalDon't trust raw
X-Forwarded-Forfor the new IP limiter.This still prefers a caller-controlled header over
ConnectInfo, andPerIpReferralRateLimitkeys offClientIp::origin_ip(). Unless the edge always strips/replacesX-Forwarded-For, clients can rotate that header to bypass the referral invite cap. Accept forwarded IPs only from trusted proxies, or fall back to the socket address here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/ip_extractor/src/lib.rs` around lines 76 - 81, The code currently prefers X_FORWARDED_FOR (via parse_header) over the socket address, allowing caller-controlled headers to bypass PerIpReferralRateLimit which uses ClientIp::origin_ip(); change the logic to always extract the immediate peer address first (use parts.extract() -> ConnectInfo<SocketAddr>) and only accept the parsed X_FORWARDED_FOR value when that peer is in a configured list/set of trusted proxies; otherwise construct ClientIp::DirectIp(conn). Add/configure a trusted proxies check (e.g., TRUSTED_PROXIES or a TrustedProxies::contains(peer)) and use that check before returning ClientIp::ForwardedFor(parsed_ip).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/rate_limit/src/outbound/redis/test.rs`:
- Around line 62-64: The match arm incorrectly uses brace-style pattern for the
tuple-style Result alias; replace the arm matching RateLimitResult::Ok { .. }
with the tuple-variant pattern RateLimitResult::Ok(_) so it matches the Ok
variant used elsewhere (e.g., the other RateLimitResult::Ok(_) matches in this
test file) and keeps the panic branch behavior unchanged.
---
Outside diff comments:
In `@rust/cloud-storage/notification/src/domain/service/egress.rs`:
- Around line 227-247: The code currently calls
rate_limiter.rollback_ticket(ticket_ok) whenever send_email(recipient.clone(),
&content).await returns Err, which can reopen quota for ambiguous delivery
failures; change this so we do NOT unconditionally rollback on every send_email
error: update the EmailSender trait (and SES implementation) to return a
classified result or error kind (e.g., EmailSendResult { Sent, NotSentPreSend,
AmbiguousTransport }) or an ErrorKind/Idempotency marker, then only call
rate_limiter.rollback_ticket(...) when the error is a guaranteed pre-send
failure (e.g., NotSentPreSend/ErrorKind::PreSend); alternatively implement
idempotency (Message-ID deduplication) and use that classification to decide
rollback; adjust the egress::send flow to inspect the new classification from
send_email instead of rolling back for all Errs and map ambiguous errors to
DeliveryFailure::Other without rollback.
In `@rust/cloud-storage/referral/src/domain/service.rs`:
- Around line 105-138: Add the missing tracing attribute to improve
observability: annotate the async method send_referral_invite in the Referral
service impl with #[tracing::instrument(skip(self), err)] (matching the pattern
used on other methods in this impl) so the function arguments are traced and
errors are recorded; ensure the attribute is placed immediately above the async
fn send_referral_invite(...) signature and include skip(self) to avoid capturing
self.
---
Duplicate comments:
In
`@rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rs`:
- Line 37: The tracing attribute on the handler in generate_email_link.rs is
missing the err flag for a function returning Result; update the
#[tracing::instrument(...)] on the handler function to include err (e.g.
#[tracing::instrument(skip(ctx, user_context, ip_context, req),
fields(client_ip=?ip_context, email=%req.email,
fusion_user_id=%user_context.fusion_user_id), err)]) so errors are captured by
tracing for the handler that returns Result<Response, Response>.
In
`@rust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rs`:
- Around line 30-33: The tracing span on the handler function must include error
capture: update the #[tracing::instrument(...)] on the handler (the async fn
handler that returns Result<Response, Response>) to include err in its
attributes so failures are recorded at the span level; keep the existing
skip(ctx, ip_context, req) and fields(client_ip=?ip_context, email=%req.email)
and add err to the attribute list.
In
`@rust/cloud-storage/authentication_service/src/api/email/verify_email_link.rs`:
- Around line 32-35: The tracing span on the handler function must capture
errors — update the tracing::instrument attribute on fn handler (the function
named handler that returns Result<Response, Response>) to include err (e.g., add
err to the attribute parameters along with the existing skip and fields entries)
so any returned error/redirect path is attached to the span; leave skip(ctx,
ip_context) and fields(client_ip=?ip_context) intact while adding err.
In
`@rust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rs`:
- Line 29: The tracing instrumentation on the verify_fusionauth_user_email
handler omits error capture; update the attribute on the
verify_fusionauth_user_email function to include err (e.g. add , err to the
#[tracing::instrument(...)] attribute) so that functions returning
Result<Response, Response> record errors; keep skip(ctx, ip_context) and
fields(client_ip=?ip_context) as-is while adding err to the attribute.
In
`@rust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rs`:
- Around line 32-35: The tracing span on the handler function doesn't include
the error field; update the tracing::instrument attribute on the async fn
handler (the function declared as pub async fn handler with State(ctx):
State<ApiContext, ip_context: ClientIp, user_context: ...) ) to include err so
failures are recorded (e.g., add err to the attribute arguments), ensuring
fields(...) remains and skip(...) is unchanged; this will capture the Result
error path in the span.
In `@rust/cloud-storage/authentication_service/src/api/link/github.rs`:
- Line 70: The tracing::instrument on init_github_link_handler omits the err
flag for a function that returns Result; update the attribute on
init_github_link_handler to include err (e.g., add , err to the
#[tracing::instrument(...)] attributes) while keeping the existing skip(ctx,
ip_context, user_context) and fields(client_ip=?ip_context,
user_id=%user_context.user_id, fusion_user_id=%user_context.fusion_user_id)
entries so errors are recorded in traces.
In `@rust/cloud-storage/authentication_service/src/api/login/password.rs`:
- Line 31: The tracing span on the password handler is missing the err flag so
error returns aren't attached to the span; update the
#[tracing::instrument(...)] attribute on the function that takes ctx, req,
ip_context (the password handler in password.rs) to include err (alongside the
existing skip(ctx, req, ip_context) and fields(email=%req.email,
client_ip=?ip_context)) so the macro attaches error information for
Result-returning paths.
In `@rust/cloud-storage/authentication_service/src/api/login/passwordless.rs`:
- Line 33: The tracing instrumentation on the passwordless handler currently
uses #[tracing::instrument(skip(ctx, req, ip_context), fields(email=%req.email,
client_ip=?ip_context))] but omits error capture; update the attribute to
include err (e.g. #[tracing::instrument(skip(ctx, req, ip_context),
fields(email=%req.email, client_ip=?ip_context), err)]) so the handler (the
function annotated with tracing::instrument that takes ctx, req, ip_context and
returns Result<Response, Response>) records error information on failure.
In
`@rust/cloud-storage/authentication_service/src/api/merge/create_merge_request.rs`:
- Around line 42-46: The tracing span on the handler function doesn't capture
returned errors because the #[tracing::instrument(...)] attribute omits err;
update the attribute on the handler (the function named handler in
create_merge_request.rs) to include err (i.e., add err to the
tracing::instrument attribute parameters alongside skip(...) and fields(...)) so
returned Result::Err values are recorded by the span.
In
`@rust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rs`:
- Line 38: The tracing attribute on the handler in verify_merge_request.rs is
missing the err option, so errors returned by the handler (which returns
Result<Response, Response>) are not recorded; update the
#[tracing::instrument(...)] on the handler function to include err (e.g., add
err alongside the existing skip(...) and fields(...) arguments) so the span
captures the Result error, keeping the same skip(ctx, user_context, ip_context)
and fields(client_ip=?ip_context, fusion_user_id=%user_context.fusion_user_id).
In
`@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rs`:
- Line 15: The tracing attribute on the middleware is missing the err hook even
though the function returns Result<Response, Response>; update the attribute on
the annotated middleware (the #[tracing::instrument(...)] line in login_code.rs)
to include err so errors are recorded, e.g. change
#[tracing::instrument(skip(ctx, req, next, ip_context),
fields(client_ip=?ip_context))] to include err: #[tracing::instrument(skip(ctx,
req, next, ip_context), err, fields(client_ip=?ip_context))].
In
`@rust/cloud-storage/authentication_service/src/api/middleware/rate_limit/passwordless.rs`:
- Around line 21-24: The tracing span on the handler function doesn't record
rejections because the attribute omits the `err` flag; update the
#[tracing::instrument(...)] on the handler (the async fn handler in
passwordless.rs) to include `err` (e.g. add `, err` in the attribute alongside
the existing skip/fields) so that the span captures the error path for the
Result<Response, Response> return.
In `@rust/cloud-storage/authentication_service/src/api/mod.rs`:
- Around line 84-89: The server must be built with ConnectInfo so ClientIp
extraction succeeds; update setup_and_serve to use
into_make_service_with_connect_info::<SocketAddr>() instead of
into_make_service(), and ensure the router(s) that include link::router(...) and
the /referral IP-rate-limit path (which rely on ClientIp) remain mounted before
removing the old IP middleware; keep the axum::middleware::from_fn_with_state
wrapping macro_middleware::auth::decode_jwt::handler as-is but ensure the
service is created via into_make_service_with_connect_info::<SocketAddr>() so
ClientIp can be extracted at handler time.
In `@rust/cloud-storage/authentication_service/src/api/user/create_user.rs`:
- Line 38: The tracing attribute on the create_user handler must include err
because the function returns Result<Response, Response>; update the
#[tracing::instrument(...)] on create_user to add err (e.g.,
#[tracing::instrument(err, skip(ctx, ip_context, req),
fields(client_ip=?ip_context))]) so errors are captured in spans for
Response-returning functions and keep the existing skip and fields arguments
intact.
In `@rust/cloud-storage/ip_extractor/src/lib.rs`:
- Around line 76-81: The code currently prefers X_FORWARDED_FOR (via
parse_header) over the socket address, allowing caller-controlled headers to
bypass PerIpReferralRateLimit which uses ClientIp::origin_ip(); change the logic
to always extract the immediate peer address first (use parts.extract() ->
ConnectInfo<SocketAddr>) and only accept the parsed X_FORWARDED_FOR value when
that peer is in a configured list/set of trusted proxies; otherwise construct
ClientIp::DirectIp(conn). Add/configure a trusted proxies check (e.g.,
TRUSTED_PROXIES or a TrustedProxies::contains(peer)) and use that check before
returning ClientIp::ForwardedFor(parsed_ip).
In `@rust/cloud-storage/ip_extractor/src/test.rs`:
- Around line 229-243: Add a test that constructs a ClientIpError::ExtensionErr,
calls its into_response() and asserts the response.status() is
StatusCode::INTERNAL_SERVER_ERROR; specifically add a new test function (e.g.,
error_response_extension_err_is_500) that creates ClientIpError::ExtensionErr,
invokes ClientIpError::into_response(), and asserts equality with
StatusCode::INTERNAL_SERVER_ERROR to complete coverage for the into_response
mapping.
In `@rust/cloud-storage/notification/src/domain/service/test.rs`:
- Around line 1077-1079: The mock rate limiter's async method decrement(&self,
_key: &RateLimitKey) is currently a no-op, preventing tests from observing
rollbacks; modify the mock implementation of decrement to record received
RateLimitKey values (e.g., push to an internal Vec/VecDeque or send over an
mpsc/watch channel) so tests can assert that a rollback path invoked decrement
at least once; update the failing-delivery test to read from that recorded
store/receiver and assert that the expected key(s) were observed, referencing
the mock rate limiter type and the decrement(&self, _key: &RateLimitKey) method
to locate the change.
In `@rust/cloud-storage/rate_limit/src/inbound.rs`:
- Around line 57-60: When calling service.check_rate_limit(rate_limit_key,
config).await, preserve and log any underlying error before mapping it to
Either3::E3(StatusCode::INTERNAL_SERVER_ERROR): use .inspect_err (or a map_err
closure) to call tracing::error!(error=?e, "rate limit backend check failed for
{rate_limit_key}") where e is the error, then return
Either3::E3(StatusCode::INTERNAL_SERVER_ERROR) as before; update the call site
referencing service.check_rate_limit, rate_limit_key, config, and Either3::E3 so
the root error is visible in logs rather than dropped.
In `@rust/cloud-storage/rate_limit/src/outbound/redis.rs`:
- Around line 66-74: The Lua snippet can leave a counter without expiry (ttl ==
-1) so the key becomes permanently stuck; modify the logic around the TTL/EXPIRE
calls for the symbols key, TTL, INCR, EXPIRE, current, max_count, and window so
that: before returning the deny path (when current >= max_count) check TTL and
if ttl < 0 call redis.call('EXPIRE', key, window) (then fetch TTL again or
return a positive ttl); and also after INCR (when new_count > 1) check the key's
TTL and call EXPIRE(key, window) if the TTL is missing. Ensure both deny and
allow paths reapply expiry when ttl < 0 so the counter can heal.
In `@rust/cloud-storage/rate_limit/src/outbound/redis/test.rs`:
- Around line 221-223: The test's assertion is too permissive—change the
assertion in the Redis outbound test (the assert checking `count` after
decrement) to require the key be absent (`count.is_none()`) instead of allowing
`Some(0)`; update the failure message accordingly to indicate the counter must
be absent (e.g., "counter should be absent after decrementing a missing key, got
{count:?}"). This aligns the test with the behavior in the `decrement`/outbound
Redis logic which only decrements when `current > 0`, leaving the key missing.
In `@rust/cloud-storage/referral/Cargo.toml`:
- Around line 8-16: The Cargo.toml currently enables the rate_limit crate's
"axum" subfeature unconditionally; gate that integration behind this crate's
"axum" feature by removing the direct "rate_limit/axum" activation from the
dependency list and instead add a feature entry mapping your crate's "axum"
feature to enable "rate_limit/axum". Concretely, make the rate_limit dependency
optional (optional = true or don't enable its axum subfeature inline), add
"axum" to your [features] table with "rate_limit/axum" as a member, and ensure
consumers can opt in via the crate feature "axum" so default-features = false no
longer pulls the Axum inbound stack.
In `@rust/cloud-storage/referral/src/inbound/axum_router/test.rs`:
- Around line 79-87: The test mock currently returns nested Ok(Err(...)) and
Ok(Ok(...)); change these to the explicit enum constructors to match
domain/service/test.rs by returning Ok(RateLimitResult::Err(RateLimitExceeded {
... })) when self.should_exceed is true and
Ok(RateLimitResult::Ok(RateLimitOk::new_testing_value(0, key, config)))
otherwise; update the references to RateLimitExceeded, RateLimitOk,
RateLimitResult, and should_exceed accordingly to improve readability and
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87286f5d-a5af-47c1-8c26-8e614a81afd3
⛔ Files ignored due to path filters (1)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rsrust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rsrust/cloud-storage/authentication_service/src/api/email/verify_email_link.rsrust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rsrust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rsrust/cloud-storage/authentication_service/src/api/link/github.rsrust/cloud-storage/authentication_service/src/api/link/mod.rsrust/cloud-storage/authentication_service/src/api/login/mod.rsrust/cloud-storage/authentication_service/src/api/login/password.rsrust/cloud-storage/authentication_service/src/api/login/passwordless.rsrust/cloud-storage/authentication_service/src/api/merge/create_merge_request.rsrust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rsrust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rsrust/cloud-storage/authentication_service/src/api/middleware/rate_limit/passwordless.rsrust/cloud-storage/authentication_service/src/api/mod.rsrust/cloud-storage/authentication_service/src/api/oauth/mod.rsrust/cloud-storage/authentication_service/src/api/user/create_user.rsrust/cloud-storage/authentication_service/src/api/user/mod.rsrust/cloud-storage/fusionauth/src/user/create.rsrust/cloud-storage/fusionauth/src/user/mod.rsrust/cloud-storage/fusionauth/src/user/verify.rsrust/cloud-storage/image_proxy_service/src/api/mod.rsrust/cloud-storage/ip_extractor/Cargo.tomlrust/cloud-storage/ip_extractor/src/lib.rsrust/cloud-storage/ip_extractor/src/test.rsrust/cloud-storage/model/src/lib.rsrust/cloud-storage/model/src/tracking.rsrust/cloud-storage/notification/src/domain/service/egress.rsrust/cloud-storage/notification/src/domain/service/test.rsrust/cloud-storage/notification_sandbox/src/adapters/noop_rate_limiter.rsrust/cloud-storage/rate_limit/src/domain/models.rsrust/cloud-storage/rate_limit/src/domain/ports.rsrust/cloud-storage/rate_limit/src/domain/service.rsrust/cloud-storage/rate_limit/src/inbound.rsrust/cloud-storage/rate_limit/src/outbound/redis.rsrust/cloud-storage/rate_limit/src/outbound/redis/test.rsrust/cloud-storage/referral/Cargo.tomlrust/cloud-storage/referral/src/domain/service.rsrust/cloud-storage/referral/src/domain/service/test.rsrust/cloud-storage/referral/src/inbound/axum_router.rsrust/cloud-storage/referral/src/inbound/axum_router/send_invite.rsrust/cloud-storage/referral/src/inbound/axum_router/test.rsrust/cloud-storage/seed_cli/src/service/auth/mod.rs
💤 Files with no reviewable changes (5)
- rust/cloud-storage/authentication_service/src/api/link/mod.rs
- rust/cloud-storage/model/src/lib.rs
- rust/cloud-storage/model/src/tracking.rs
- rust/cloud-storage/authentication_service/src/api/oauth/mod.rs
- rust/cloud-storage/authentication_service/src/api/user/mod.rs
seanaye
force-pushed
the
seanaye/feat/referral-ip-rate-limit
branch
2 times, most recently
from
9bd83bb to
6f7575d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
rust/cloud-storage/rate_limit/src/outbound/redis/test.rs (1)
221-223:⚠️ Potential issue | 🟡 MinorAssert only
Noneafter decrementing a missing key.
decrementleaves a non-existent key non-existent, so allowingSome(0)here can mask a regression that starts materializing zero-valued counters.Suggested assertion
assert!( - count.is_none() || count == Some(0), - "counter should not go negative, got {count:?}" + count.is_none(), + "key should remain non-existent after decrement on a missing counter, got {count:?}" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/rate_limit/src/outbound/redis/test.rs` around lines 221 - 223, The test currently allows count to be Some(0) after calling decrement on a missing key; change the assertion to require only None so a missing key remains non-existent: update the assert in the test that checks count (the variable named count after calling decrement) to assert!(count.is_none(), "counter should not exist after decrementing missing key, got {count:?}"), referencing the decrement behavior and the count variable to locate the check.rust/cloud-storage/rate_limit/src/outbound/redis.rs (1)
66-75:⚠️ Potential issue | 🟠 MajorReapply the window when a counter has lost its TTL.
If a key already exists without expiry, the deny path returns
ttl = 0and the allow path keeps incrementing without restoring the TTL. Once that key reachesmax_count, it can stay blocked forever instead of healing within the configured window.Suggested Lua fix
if current >= max_count then local ttl = redis.call('TTL', key) - if ttl < 0 then ttl = 0 end + if ttl < 0 then + redis.call('EXPIRE', key, window) + ttl = window + end return {0, current, ttl} end local new_count = redis.call('INCR', key) - if new_count == 1 then + local ttl = redis.call('TTL', key) + if new_count == 1 or ttl < 0 then redis.call('EXPIRE', key, window) end return {1, new_count, 0}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/rate_limit/src/outbound/redis.rs` around lines 66 - 75, The Redis Lua script currently returns ttl=0 when a key has no expiry and increments the counter without restoring the window; update the script so that when ttl <= 0 you call redis.call('EXPIRE', key, window) to reapply the TTL (both in the deny branch where you read TTL and in the allow branch after INCR when the key exists but has no TTL). Use the existing variables (key, window, current, max_count, new_count) to detect ttl <= 0 and invoke EXPIRE so counters heal within the configured window.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/authentication_service/src/api/user/create_user.rs`:
- Line 41: The ClientIp extractor currently returns the leftmost hop from
X-Forwarded-For and so ip_context.origin_ip() passed to
ctx.auth_client.create_user can be spoofed; update the extractor in
rust/cloud-storage/ip_extractor/src/lib.rs (the ClientIp type and its extraction
logic) to prefer the CloudFront-Viewer-Address header if present, otherwise take
the rightmost non-empty hop from the X-Forwarded-For list (trim spaces) instead
of the leftmost, and ensure ClientIp::origin_ip() returns that verified viewer
IP so FusionAuth receives the correct source IP for abuse/audit controls.
In `@rust/cloud-storage/rate_limit/src/domain/ports.rs`:
- Around line 25-28: The decrement API currently takes only &RateLimitKey which
loses the original window/ticket context and can decrement a recreated bucket;
change the port signature (RateLimitPort::decrement) to accept the full
RateLimitOk (or a wrapper containing the original window/ticket identifier and
key) instead of just RateLimitKey so adapters (e.g., the Redis adapter
referenced in redis.rs) can verify the window/ticket identity and no-op if the
original bucket has expired/recreated; update callers such as rollback_ticket to
pass the full RateLimitOk and update implementations to check the window/ticket
id (or encoded key) before DECR.
In `@rust/cloud-storage/referral/src/domain/service.rs`:
- Line 15: Remove the dead RateLimitService import and the unused rate_limit
field and generic parameter from ReferralService: delete the use
rate_limit::domain::ports::RateLimitService; line, remove the rate_limit member
from the ReferralService struct, and drop the unused generic parameter Rl:
RateLimitService from the ReferralService type signature (and any related type
parameter declarations). Ensure constructors/new functions (e.g.,
ReferralService::new) no longer accept or store a rate_limit argument and update
any instantiations accordingly so no references to self.rate_limit or Rl remain.
In `@rust/cloud-storage/referral/src/inbound/axum_router.rs`:
- Around line 79-82: The PerIpReferralRateLimit is vulnerable because
ClientIp.origin_ip() currently uses the leftmost X-Forwarded-For value (which
can be spoofed); update the extractor used by rate_limit_middleware::<R,
PerIpReferralRateLimit, R> so ClientIp.origin_ip() (or the function that builds
ClientIp) selects the rightmost X-Forwarded-For token (the CloudFront-appended
client IP) or, if present, prefers the CloudFront-Viewer-Address header;
implement fallback/validation (trim, parse as IP, ignore non-IP entries) and
ensure the rate limiter uses that value for bucketing to prevent spoofing.
In `@rust/cloud-storage/referral/src/inbound/axum_router/test.rs`:
- Around line 69-88: The mock rate limiter currently uses a boolean
should_exceed that denies every check; change MockRateLimitPort to be key-aware
(for example replace should_exceed: bool with something like denied_key:
Option<RateLimitKey> or denied_keys: HashSet<RateLimitKey>) and update the impl
of rate_limit::RateLimitPort::check to compare the incoming key parameter to
that denied key(s) and only return a RateLimitExceeded for matching keys; for
non-matching keys return RateLimitOk as before. Update any tests that construct
MockRateLimitPort to set the specific key(s) you want denied so the per-user and
per-IP buckets are exercised independently.
---
Duplicate comments:
In `@rust/cloud-storage/rate_limit/src/outbound/redis.rs`:
- Around line 66-75: The Redis Lua script currently returns ttl=0 when a key has
no expiry and increments the counter without restoring the window; update the
script so that when ttl <= 0 you call redis.call('EXPIRE', key, window) to
reapply the TTL (both in the deny branch where you read TTL and in the allow
branch after INCR when the key exists but has no TTL). Use the existing
variables (key, window, current, max_count, new_count) to detect ttl <= 0 and
invoke EXPIRE so counters heal within the configured window.
In `@rust/cloud-storage/rate_limit/src/outbound/redis/test.rs`:
- Around line 221-223: The test currently allows count to be Some(0) after
calling decrement on a missing key; change the assertion to require only None so
a missing key remains non-existent: update the assert in the test that checks
count (the variable named count after calling decrement) to
assert!(count.is_none(), "counter should not exist after decrementing missing
key, got {count:?}"), referencing the decrement behavior and the count variable
to locate the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3055bf4-d4f6-4418-a2b6-e3b1ef6a508b
⛔ Files ignored due to path filters (1)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
rust/cloud-storage/authentication_service/src/api/email/generate_email_link.rsrust/cloud-storage/authentication_service/src/api/email/resend_fusionauth_verify_user_email.rsrust/cloud-storage/authentication_service/src/api/email/verify_email_link.rsrust/cloud-storage/authentication_service/src/api/email/verify_fusionauth_user_email.rsrust/cloud-storage/authentication_service/src/api/link/create_in_progress_link.rsrust/cloud-storage/authentication_service/src/api/link/github.rsrust/cloud-storage/authentication_service/src/api/login/password.rsrust/cloud-storage/authentication_service/src/api/login/passwordless.rsrust/cloud-storage/authentication_service/src/api/merge/create_merge_request.rsrust/cloud-storage/authentication_service/src/api/merge/verify_merge_request.rsrust/cloud-storage/authentication_service/src/api/middleware/rate_limit/login_code.rsrust/cloud-storage/authentication_service/src/api/middleware/rate_limit/passwordless.rsrust/cloud-storage/authentication_service/src/api/mod.rsrust/cloud-storage/authentication_service/src/api/user/create_user.rsrust/cloud-storage/ip_extractor/src/lib.rsrust/cloud-storage/model/src/tracking.rsrust/cloud-storage/rate_limit/src/domain/ports.rsrust/cloud-storage/rate_limit/src/outbound/redis.rsrust/cloud-storage/rate_limit/src/outbound/redis/test.rsrust/cloud-storage/referral/src/domain/service.rsrust/cloud-storage/referral/src/domain/service/test.rsrust/cloud-storage/referral/src/inbound/axum_router.rsrust/cloud-storage/referral/src/inbound/axum_router/test.rsrust/cloud-storage/seed_cli/src/service/auth/mod.rs
💤 Files with no reviewable changes (1)
- rust/cloud-storage/model/src/tracking.rs
Comment on lines
+69
to
+88
| #[derive(Clone)] | ||
| struct MockRateLimitPort { | ||
| should_exceed: bool, | ||
| } | ||
|
|
||
| impl rate_limit::RateLimitPort for MockRateLimitPort { | ||
| async fn check( | ||
| &self, | ||
| key: RateLimitKey, | ||
| config: RateLimitConfig, | ||
| ) -> Result<RateLimitResult, Report> { | ||
| if self.should_exceed { | ||
| Ok(Err(RateLimitExceeded { | ||
| current_count: config.max_count.saturating_add(1), | ||
| max_count: config.max_count, | ||
| retry_after: config.window, | ||
| })) | ||
| } else { | ||
| Ok(Ok(RateLimitOk::new_testing_value(0, key, config))) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make the mock rate limiter key-aware.
should_exceed denies every check, so the new /send tests still pass if either the per-user or per-IP layer is accidentally removed. Have the mock deny one expected RateLimitKey at a time so the suite actually proves both buckets are wired.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/referral/src/inbound/axum_router/test.rs` around lines 69
- 88, The mock rate limiter currently uses a boolean should_exceed that denies
every check; change MockRateLimitPort to be key-aware (for example replace
should_exceed: bool with something like denied_key: Option<RateLimitKey> or
denied_keys: HashSet<RateLimitKey>) and update the impl of
rate_limit::RateLimitPort::check to compare the incoming key parameter to that
denied key(s) and only return a RateLimitExceeded for matching keys; for
non-matching keys return RateLimitOk as before. Update any tests that construct
MockRateLimitPort to set the specific key(s) you want denied so the per-user and
per-IP buckets are exercised independently.
6f7575d to
bcc1fdf
Compare
seanaye
force-pushed
the
seanaye/feat/referral-ip-rate-limit
branch
from
bcc1fdf to
862bec9
Compare
whutchinson98
approved these changes
Mar 24, 2026
This was referenced Mar 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ip_extractorcrate: A standalone axum extractor (ClientIp) that parses the client IP fromX-Forwarded-Forheader, falling back toConnectInfo<SocketAddr>. Returns a typedIpAddrinstead of a raw string. Replaces the oldattach_ip_context_handlermiddleware andIPContextmodel.rate_limit::inbound): Adds a genericRateLimitExtractor<K, Svc>andrate_limit_middlewarethat can be composed with anyRateLimitExtractablekey type. This moves rate limiting from service-layer logic into axum middleware/extractors./sendendpoint now has both a per-user and per-IP rate limit applied via middleware, preventing abuse from a single IP address.RateLimitResultfrom an enum toResult<RateLimitOk, RateLimitExceeded>, removing theRateLimitTicketwrapper.RateLimitOkis now an opaque type that carries the key and config needed forincrement_ticket, with atestingfeature flag for downstream test construction.attach_ip_context_handlermiddleware: All handlers now use theClientIpextractor directly instead of relying on middleware to injectIPContextinto extensions. Themodel::tracking::IPContextstruct is removed.fusionauthclient IP types:create_userandverify_emailnow acceptIpAddrinstead of&str.Test plan
ip_extractor(header parsing, fallback to ConnectInfo, error cases)rate_limit::outbound::redisupdated for newRateLimitResulttypeRateLimitOkconstruction🤖 Generated with Claude Code