Improve SSRF rejection logging with request context

[reyhankoyun]

Description

Why is this change being made?

1. SSRF rejection logs currently contain only a generic message with no request context, making it difficult to investigate security incidents or correlate rejected requests.
2. The two distinct failure modes (wrong token vs. missing token header) produce the same log message, preventing operators from distinguishing between misconfigured clients and actual attack attempts.

What is changing?

1. Thread peer_addr from TCP accept() through the request handling chain to validate_token().
2. Append request context (peer address, HTTP method, request path) to each rejection log message after the original message text.
3. Differentiate between three rejection reasons (X-Forwarded-For present, token value mismatch, no recognized token header) via a reason= field in the appended context.
4. Original error message prefixes are preserved verbatim for customer log filter compatibility.

Related Links

• Issue #, if available: N/A

---

Testing

How was this tested?

1. All 93 existing unit tests pass (cargo test --package aws_secretsmanager_provider)
2. Full workspace compiles cleanly (cargo check --workspace)
3. Verified log output does not contain token values, query parameters, or header values

When testing locally, provide testing artifact(s):

1. cargo test --package aws_secretsmanager_provider — 93 passed, 0 failed
2. cargo check --workspace --exclude integration-tests — success, no errors

---

Reviewee Checklist

Update the checklist after submitting the PR

• I have reviewed, tested and understand all changes
  If not, why:
• I have filled out the Description and Testing sections above
  If not, why:
• Build and Unit tests are passing
  If not, why:
• Unit test coverage check is passing
  If not, why:
• Integration tests pass locally
  If not, why: Change is to log message formatting only; no new integration test scenarios needed
• I have updated integration tests (if needed)
  If not, why: No behavioral change to HTTP responses or caching logic
• I have ensured no sensitive information is leaking (i.e., no logging of sensitive fields, or otherwise)
  If not, why:
• I have added explanatory comments for complex logic, new classes/methods and new tests
  If not, why:
• I have updated README/documentation (if needed)
  If not, why: N/A — internal log format change
• I have clearly called out breaking changes (if any)
  If not, why: N/A — no breaking changes

---

Reviewer Checklist

All reviewers please ensure the following are true before reviewing:

• Reviewee checklist has been accurately filled out
• Code changes align with stated purpose in description
• Test coverage adequately validates the changes

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[codecov]

Codecov Report

❌ Patch coverage is 97.87234% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.32%. Comparing base (fdfc8af) to head (a02f397).

Files with missing lines	Patch %	Lines
aws_secretsmanager_provider/src/server.rs	95.65%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #232      +/-   ##
==========================================
+ Coverage   86.27%   86.32%   +0.05%     
==========================================
  Files          34       34              
  Lines        9331     9369      +38     
  Branches     9331     9369      +38     
==========================================
+ Hits         8050     8088      +38     
  Misses       1148     1148              
  Partials      133      133              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Secrets Manager Caching Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite	Current: 012e36d	Previous: d47a49d	Ratio
CacheEviction	78732 ns/iter (± 11867)	49133 ns/iter (± 12612)	1.60

This comment was automatically generated by workflow using github-action-benchmark.

[secrets-manager-ai-code-review]

Claude finished @reyhankoyun's task in 3m 15s —— View job

---

Code Review — PR #232: Improve SSRF rejection logging

• Read repo guidelines (no root CLAUDE.md present)
• Reviewed the diff (server.rs, lib.rs)
• Analyzed code quality, bugs, security, performance
• Posted review feedback

---

Overall this is a clean, well-scoped change. Threading peer_addr from accept() down to validate_token() is the right approach, the log context is genuinely useful for incident correlation, and you correctly addressed both prior review comments (appending to existing messages rather than rewriting them, and adding tests for the two distinct rejection paths). Nice attention to not leaking secret material.

✅ Strengths

• No sensitive data leaked. You log req.uri().path() rather than the full URI, so query params (secretId, versionId, role_arn) and header values stay out of the logs. Verified against the code.
• Original message prefixes preserved verbatim (Rejecting request with incorrect SSRF token, Rejecting request with X-Forwarded-For header) — honors @simonmarty's point about customer log filters.
• Three distinct reasons correctly differentiated: XFF present (400), reason=token_mismatch and reason=missing_header (both 403).
• Doc comment cleaned up — the stale/duplicated # Returns block on validate_token was fixed and the HttpError type reference corrected.

🔎 Observations & suggestions (all non-blocking)

1. Path-style requests embed the secret name in path. For /v1/MyTest-style requests, the rejection log will now contain the secret name (e.g. path=/v1/MyTest). Secret names are usually treated as identifiers, not secret material, so this is likely acceptable — but worth an explicit ack against your data-classification policy. See inline comment on server.rs:243.
2. Tests don't assert the log reason. The new tests verify status/body only, which is identical (403 / Bad Token) for both branches — the actual new behavior (the differentiated reason= field) isn't asserted. A log-capture assertion would lock in the real contract. See inline comment on lib.rs:718.
3. validate_token control flow is correct. token_header_present is only set when a configured header is present, and the loop still returns Ok immediately on a match, so a valid token in any configured header short-circuits before the reason branching. No behavior regression for the success path.

⚡ Performance

The benchmark alert on CacheEviction (1.60× ratio) is almost certainly unrelated to this PR — this change only affects the SSRF rejection path (an early return with a couple of error! logs), which is not exercised by the caching benchmark. This reads as benchmark noise (note the wide ± 11867/± 12612 variance bands and the recent "Increase benchmark performance diff tolerance" commit on main). Re-running the benchmark should confirm.

🔒 Security

No concerns. The change is defensive (better observability for SSRF rejections), doesn't alter the auth decision logic, and .path() returns the percent-encoded path so crafted CRLF/log-injection via the request path is not a new risk here.

No blocking issues. LGTM pending the team's call on point (1).