Skip to content

Update X-Forwarded-Ssl "on" check#192

Open
amohrland wants to merge 1 commit intottys3:mainfrom
amohrland:x-forwarded-ssl-some
Open

Update X-Forwarded-Ssl "on" check#192
amohrland wants to merge 1 commit intottys3:mainfrom
amohrland:x-forwarded-ssl-some

Conversation

@amohrland
Copy link

Update X-Forwarded-Ssl "on" check

Currently, "on" is getting compared against a to_stringed bool

@qodo-free-for-open-source-projects

Review Summary by Qodo

Fix X-Forwarded-Ssl header value comparison

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Fix incorrect X-Forwarded-Ssl header comparison logic
• Replace boolean string conversion with proper header value extraction
• Use and_then to safely convert header to string and compare against "on"
Diagram
flowchart LR
  A["X-Forwarded-Ssl header"] -->|Old: is_some().to_string()| B["Compares 'true'/'false' string"]
  A -->|New: and_then to_str| C["Compares actual header value"]
  C -->|Match 'on'| D["Returns https scheme"]
Loading

Grey Divider

File Changes

1. src/lib.rs 🐞 Bug fix +6/-1

Fix X-Forwarded-Ssl header value comparison logic

• Changed X-Forwarded-Ssl header check from is_some().to_string() == "on" to `and_then(|v|
 v.to_str().ok()) == Some("on")`
• Properly extracts and compares the actual header value instead of converting a boolean to string
• Improves code clarity with better formatting across multiple lines

src/lib.rs


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link

qodo-free-for-open-source-projects bot commented Feb 24, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Remediation recommended

1. Case-sensitive ssl header check 🐞 Bug ✧ Quality
Description
The new X-Forwarded-Ssl logic only matches the exact value "on"; if an upstream sets a different
casing (e.g., "ON") the scheme will be labeled as "http", skewing required url.scheme metric
attributes.
Code

src/lib.rs[R330-335]

+                if req
+                    .headers()
+                    .get("X-Forwarded-Ssl")
+                    .and_then(|v| v.to_str().ok())
+                    == Some("on")
+                {
Evidence
The scheme is derived from request headers and then used as the required url.scheme attribute for
the active-requests metric. The updated check compares the header value to Some("on") exactly,
with no normalization, so alternate casing will not be treated as HTTPS.

src/lib.rs[320-343]
src/lib.rs[345-352]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`X-Forwarded-Ssl` is currently compared to `"on"` with exact case sensitivity. Some upstreams/proxies may send `"ON"`/`"On"`, which would cause `url.scheme` to be labeled as `http` and skew HTTP metrics.

### Issue Context
This value is used as the required `url.scheme` attribute when recording `http.server.active_requests`.

### Fix Focus Areas
- src/lib.rs[330-337]

### Suggested change
Update the check to be case-insensitive (and optionally whitespace-tolerant), e.g.:
```rust
if req
   .headers()
   .get("X-Forwarded-Ssl")
   .and_then(|v| v.to_str().ok())
   .map(|v| v.trim().eq_ignore_ascii_case("on"))
   .unwrap_or(false)
{
   return "https".to_string();
}
```

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant