harden: request body cap, promo-preview validation, health rate-limit exempt, dev-log scrub#52
Conversation
… exempt, dev-log PII scrub Residual items from the full per-endpoint security scan (the scan confirmed the public reads + auth flows leak no PII and no tokens are logged): - Cap request bodies at 1 MB via Kestrel (no file-upload endpoints exist) to block oversized-payload DoS; the 30 MB default was unnecessarily large. - Validate the promo-preview query (code <= MaxCodeLength, seats 1..10) so a malformed/over-long code is a clean 400, not unbounded work. - Exempt /health + /health/ready from the rate limiter so orchestrator probes are never throttled or starved by other traffic. - Strict dev-log scrub: the dev (no-SMTP) email sink no longer logs the body / verify-reset link/token — only the subject + a masked recipient (use Mailpit to view the link locally). DevDataSeeder masks the email too. New EmailMask helper. 1 new integration test (over-long promo code -> 400). Full suite green (65 unit + 62 integration), build 0 warnings.
|
Warning Review limit reached
More reviews will be available in 50 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR hardens the API server through four coordinated changes: masking email addresses in logs to prevent sensitive data leakage, validating promo code input to reject oversized payloads, enforcing a 1 MB request body size limit at the Kestrel level, and disabling rate limiting on health check endpoints to ensure orchestrator liveness probes are not throttled. ChangesServer Hardening Audit Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/TransportPlatform.IntegrationTests/PromoCodeTests.cs (1)
77-77: ⚡ Quick winMake the overlong input derive from the domain max-length constant.
Using a fixed
100makes this test brittle ifPromoCode.MaxCodeLengthchanges. Deriving from the constant keeps the test aligned with the contract it verifies.Suggested fix
+using TransportPlatform.Domain.Promotions; ... - var longCode = new string('A', 100); + var longCode = new string('A', PromoCode.MaxCodeLength + 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/TransportPlatform.IntegrationTests/PromoCodeTests.cs` at line 77, Replace the hardcoded 100 with a value derived from the domain constant so the test stays correct if the limit changes: construct longCode using PromoCode.MaxCodeLength (e.g., new string('A', PromoCode.MaxCodeLength + 1)) instead of 100 in the test where longCode is defined to ensure the input is one character longer than the allowed maximum.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/TransportPlatform.Api/Program.cs`:
- Around line 236-241: The /health/ready endpoint currently maps via
app.MapHealthChecks("/health/ready", new HealthCheckOptions { Predicate = ...
}).DisableRateLimiting() and runs DatabaseHealthCheck (which queries the DB);
remove .DisableRateLimiting() and apply the same protections used for /metrics
(e.g., require authentication or an internal network/proxy allowlist middleware)
so only trusted callers can reach MapHealthChecks("/health/ready");
alternatively, if internal-only access is desired, wrap the MapHealthChecks call
with an authorization/allowlist policy or middleware that matches the /metrics
protection and ensure DatabaseHealthCheck remains unchanged.
In `@src/TransportPlatform.Application/Promotions/PreviewPromo.cs`:
- Line 17: The current validator RuleFor(x =>
x.Code).NotEmpty().MaximumLength(PromoCode.MaxCodeLength) checks raw length
instead of trimmed length; change the validation on RuleFor(x => x.Code) to
validate against code.Trim().Length (e.g., use a Transform or a custom Must
predicate) so the max-length check uses the trimmed value and treat emptiness on
the trimmed value as well; reference the existing RuleFor(x => x.Code) and
PromoCode.MaxCodeLength when implementing this change.
---
Nitpick comments:
In `@tests/TransportPlatform.IntegrationTests/PromoCodeTests.cs`:
- Line 77: Replace the hardcoded 100 with a value derived from the domain
constant so the test stays correct if the limit changes: construct longCode
using PromoCode.MaxCodeLength (e.g., new string('A', PromoCode.MaxCodeLength +
1)) instead of 100 in the test where longCode is defined to ensure the input is
one character longer than the allowed maximum.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33fcf191-96f8-44ab-b06a-5c6a4aa04ae3
📒 Files selected for processing (8)
PROJECT_STATUS.mdsrc/TransportPlatform.Api/Endpoints/BookingEndpoints.cssrc/TransportPlatform.Api/Program.cssrc/TransportPlatform.Application/Promotions/PreviewPromo.cssrc/TransportPlatform.Infrastructure/Email/EmailMask.cssrc/TransportPlatform.Infrastructure/Email/LoggingEmailSender.cssrc/TransportPlatform.Infrastructure/Persistence/DevDataSeeder.cstests/TransportPlatform.IntegrationTests/PromoCodeTests.cs
| // Exempt probes from the limiter so an orchestrator's frequent liveness/readiness polls are never | ||
| // throttled (and can't be starved by other traffic sharing the IP bucket). | ||
| app.MapHealthChecks("/health", new HealthCheckOptions { Predicate = _ => false }).DisableRateLimiting(); | ||
| // Readiness: runs the "ready"-tagged checks (DB connectivity). Used by orchestrator readiness | ||
| // probes / compose healthchecks to gate traffic until dependencies are actually reachable. | ||
| app.MapHealthChecks("/health/ready", new HealthCheckOptions { Predicate = check => check.Tags.Contains("ready") }); | ||
| app.MapHealthChecks("/health/ready", new HealthCheckOptions { Predicate = check => check.Tags.Contains("ready") }).DisableRateLimiting(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify DatabaseHealthCheck is lightweight and doesn't create new connections on every call
# Search for the DatabaseHealthCheck class implementation
ast-grep --pattern $'class DatabaseHealthCheck {
$$$
}'
# Also check for any IHealthCheck implementations to see the pattern
rg -n -A 10 'class.*HealthCheck.*:.*IHealthCheck'Repository: kashkoool/Transport
Length of output: 1114
Protect /health/ready from untrusted access and reconsider disabling rate limiting
/health is effectively safe because its predicate selects no checks, but /health/ready executes DatabaseHealthCheck on every probe (await db.Companies.AnyAsync(cancellationToken)) while .DisableRateLimiting() is enabled—so if it’s reachable by non-internal clients it can directly hammer the database. Restrict /health/ready (auth and/or internal network/proxy allowlist) to the same level as /metrics rather than relying on “internal” assumptions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TransportPlatform.Api/Program.cs` around lines 236 - 241, The
/health/ready endpoint currently maps via app.MapHealthChecks("/health/ready",
new HealthCheckOptions { Predicate = ... }).DisableRateLimiting() and runs
DatabaseHealthCheck (which queries the DB); remove .DisableRateLimiting() and
apply the same protections used for /metrics (e.g., require authentication or an
internal network/proxy allowlist middleware) so only trusted callers can reach
MapHealthChecks("/health/ready"); alternatively, if internal-only access is
desired, wrap the MapHealthChecks call with an authorization/allowlist policy or
middleware that matches the /metrics protection and ensure DatabaseHealthCheck
remains unchanged.
…e length CodeRabbit follow-ups: - /health/ready touches the DB, so it stays under the global limiter (only the no-I/O /health liveness probe is exempted) — prevents hammering the DB check. - Promo-preview code validation now checks the TRIMMED length, matching the domain's PromoCode contract (no false 400 for surrounding whitespace).
1 participant
Residual hardening from the full per-endpoint security scan. The scan confirmed the public reads and auth flows leak no PII (companies = id+name, reviews = display-name only, seat-map = seat numbers only) and no tokens are logged; these are the remaining polish items.
Changes
MaxRequestBodySizeset to 1 MB (no file-upload endpoints exist; the 30 MB default was excessive). Ignored under the test server.PreviewPromoValidator(code <= MaxCodeLength, seats 1..10) wired intoGET /api/bookings/promo-preview— a malformed/over-long code is now a clean 400, not unbounded processing..DisableRateLimiting()on/healthand/health/readyso orchestrator polls are never throttled/starved.LoggingEmailSenderno longer logs the email body / verify-reset link/token — only the subject + a masked recipient (use Mailpit to view the link locally).DevDataSeedermasks the email too. NewEmailMaskhelper. (Both are dev-only; production is forced to real SMTP by StartupGuards.)Explicitly left as-is (confirmed safe / by design)
*), security headers/HSTS, webhook signature verification, prod exception handler hiding internals — all verified.Tests
1 new integration test (over-long promo code -> 400). Full suite green: 65 unit + 62 integration, build 0 warnings.
Summary by CodeRabbit
Bug Fixes
Security
Tests