[rust] Restrict Selenium Manager stats to vetted values before sending to Plausible

[titusfortner]

🔗 Related Issues

I noticed some stray strings in some of the Plausible fields. Nothing that gets sent should be something the user can pass directly.

💥 What does this PR do?

• Selenium Manager now only sends a fixed, vetted vocabulary to Plausible; unrecognized os and language_binding values report as other instead of being forwarded verbatim.
• browser_version is reported as the major version only (e.g. 120), or a known channel (beta, dev, etc.), no longer the full 120.0.6099.109 string.
• Closes the path where a caller-supplied --os/--browser-version/--n value (from CLI or env) could push arbitrary strings, including markup, into the analytics dashboard as a custom property.

🔧 Implementation Notes

• Only os, browser_version, and language_binding are sanitized here; browser (already errors on unknown names), arch (already bucketed by get_normalized_arch), and selenium_version (the crate version) are bounded upstream.

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s): Claude (Anthropic)
  • What was generated: identified the unvalidated telemetry fields, the sanitization boundary, tests, and this description
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

• Root cause is analytics-side: the dashboard would render a custom property unescaped. This change removes Selenium Manager as the delivery vehicle; a separate note on the rendering assumption may be worth filing.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Summary by Qodo

Vetted Plausible telemetry: sanitize OS/lang and bucket browser version

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Sanitize telemetry properties before sending Selenium Manager usage stats to Plausible.
• Bucket unknown OS/language values to "other" and reduce browser_version to major/channel.
• Add unit tests preventing caller-controlled strings from reaching analytics payloads.

Diagram

graph TD
  A["SeleniumManager::stats"] --> B["Props::sanitized"] --> C["send_stats_to_plausible"] --> D{{"Plausible API"}}
  E["str_to_os"] --> B

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Strongly-typed telemetry fields (enums/newtypes)

• ➕ Compile-time enforcement prevents reintroducing raw strings in future call sites
• ➕ Centralizes allowed vocabularies and conversions
• ➕ Makes telemetry schema explicit and self-documenting
• ➖ Larger refactor touching config/CLI parsing and any downstream formatting
• ➖ May require broader public API changes depending on crate boundaries

2. Drop/omit untrusted properties instead of bucketing to "other"

• ➕ Even less information leakage; avoids ambiguous "other" aggregates
• ➕ Simplifies logic (reject rather than normalize)
• ➖ Reduces analytics usefulness (loses signal for uncommon but valid values)
• ➖ Harder to distinguish "missing" vs "invalid" vs "unknown"

3. Rely on analytics-side escaping/validation only

• ➕ Fixes root cause globally for all event sources
• ➕ No client-side vocabulary maintenance
• ➖ Does not prevent Selenium Manager from being a delivery vehicle today
• ➖ Requires coordination/deployment outside this repo; slower mitigation

Recommendation: The PR’s approach (client-side allowlisting/bucketing at a single Props::sanitized boundary) is the best immediate mitigation: it’s localized, low-risk, and blocks caller-controlled strings from reaching Plausible. Consider a follow-up to model os/language_binding/browser_version as enums/newtypes to make the bounded vocabulary compile-time enforced, and separately track the analytics-side escaping issue.

Files changed (2) +127 / -11

Bug fix (2) +127 / -11

lib.rsRoute stats payload through Props::sanitized() +8/-11

Route stats payload through Props::sanitized()

• Replaces direct lowercasing of telemetry fields with a single Props::sanitized(...) constructor. This ensures CLI/env-provided values are vetted before any analytics send is triggered.

rust/src/lib.rs

stats.rsAdd allowlisted/bucketed telemetry sanitizers with unit tests +119/-0

Add allowlisted/bucketed telemetry sanitizers with unit tests

• Introduces sanitization helpers for os, language binding, and browser version (major-only or known channel labels), defaulting unknowns to "other". Adds unit tests including an XSS-like payload to verify untrusted strings cannot flow into serialized props.

rust/src/stats.rs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. DotNet lang misclassified ✗ Dismissed 🐞 Bug ◔ Observability

Description

sanitize_language_binding() only accepts VALID_LANGUAGE_BINDINGS, which does not include the
CLI-documented "DotNet" value, so "DotNet" is lowercased to "dotnet" and reported as "other". This
breaks Plausible language-binding attribution for DotNet callers.

Code

rust/src/stats.rs[R37-38]

+const VALID_LANGUAGE_BINDINGS: &[&str] =
+    &["java", "javascript", "python", "csharp", "ruby", "rust"];

Evidence

The CLI documents DotNet as a valid example input for --language-binding, but the new whitelist
does not include dotnet, and the sanitizer maps any unknown value to other.

rust/src/main.rs[147-150]
rust/src/stats.rs[37-38]
rust/src/stats.rs[91-97]

Agent prompt

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

### Issue description
`sanitize_language_binding()` only allows values in `VALID_LANGUAGE_BINDINGS`. The CLI help text suggests callers may pass `DotNet`, but `dotnet` is not in the allowed list, so telemetry will be reported as `other`.

### Issue Context
This PR’s goal is to restrict telemetry to a vetted vocabulary without losing legitimate categories. "DotNet" is a legitimate (documented) input that should be canonicalized (e.g., to `csharp` or `dotnet`) rather than dropped.

### Fix Focus Areas
- rust/src/stats.rs[35-98]
- rust/src/main.rs[147-150]

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

---

2. OS/lang ignore whitespace ✓ Resolved 🐞 Bug ◔ Observability

Description

sanitize_os() and sanitize_language_binding() validate without trimming, so otherwise-valid values
like "WIN " or "Java " will be bucketed as "other". This is inconsistent with
sanitize_browser_version(), which trims before validation, and can reduce telemetry quality for
whitespace-padded inputs.

Code

rust/src/stats.rs[R84-87]

+fn sanitize_os(os: &str) -> String {
+    match str_to_os(os) {
+        Ok(parsed_os) => parsed_os.to_str_vector()[0].to_string(),
+        Err(_) => STATS_OTHER.to_string(),

Evidence

The new sanitizer passes os directly into str_to_os(os) (no trim) and lowercases language
binding without trimming; str_to_os() also performs matching without trimming, so trailing/leading
spaces will cause a parse error and force the other bucket.

rust/src/stats.rs[84-97]
rust/src/stats.rs[100-107]
rust/src/config.rs[167-176]

Agent prompt

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

### Issue description
`sanitize_browser_version()` trims input before validation, but `sanitize_os()` and `sanitize_language_binding()` do not. This causes whitespace-padded but otherwise valid values (e.g., `"linux "`, `"Java "`) to be classified as `other`.

### Issue Context
This PR intentionally tightens telemetry. Trimming leading/trailing whitespace still keeps the value within a vetted vocabulary while avoiding accidental misclassification.

### Fix Focus Areas
- rust/src/stats.rs[84-97]
- rust/src/config.rs[167-176]

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

Previous review results

Review updated until commit 2a6acdb ⚖️ Balanced

Results up to commit 84a0e51 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. DotNet lang misclassified ✗ Dismissed 🐞 Bug ◔ Observability

Description

sanitize_language_binding() only accepts VALID_LANGUAGE_BINDINGS, which does not include the
CLI-documented "DotNet" value, so "DotNet" is lowercased to "dotnet" and reported as "other". This
breaks Plausible language-binding attribution for DotNet callers.

Code

rust/src/stats.rs[R37-38]

+const VALID_LANGUAGE_BINDINGS: &[&str] =
+    &["java", "javascript", "python", "csharp", "ruby", "rust"];

Evidence

The CLI documents DotNet as a valid example input for --language-binding, but the new whitelist
does not include dotnet, and the sanitizer maps any unknown value to other.

rust/src/main.rs[147-150]
rust/src/stats.rs[37-38]
rust/src/stats.rs[91-97]

Agent prompt

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

### Issue description
`sanitize_language_binding()` only allows values in `VALID_LANGUAGE_BINDINGS`. The CLI help text suggests callers may pass `DotNet`, but `dotnet` is not in the allowed list, so telemetry will be reported as `other`.

### Issue Context
This PR’s goal is to restrict telemetry to a vetted vocabulary without losing legitimate categories. "DotNet" is a legitimate (documented) input that should be canonicalized (e.g., to `csharp` or `dotnet`) rather than dropped.

### Fix Focus Areas
- rust/src/stats.rs[35-98]
- rust/src/main.rs[147-150]

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

---

2. OS/lang ignore whitespace ✓ Resolved 🐞 Bug ◔ Observability

Description

sanitize_os() and sanitize_language_binding() validate without trimming, so otherwise-valid values
like "WIN " or "Java " will be bucketed as "other". This is inconsistent with
sanitize_browser_version(), which trims before validation, and can reduce telemetry quality for
whitespace-padded inputs.

Code

rust/src/stats.rs[R84-87]

+fn sanitize_os(os: &str) -> String {
+    match str_to_os(os) {
+        Ok(parsed_os) => parsed_os.to_str_vector()[0].to_string(),
+        Err(_) => STATS_OTHER.to_string(),

Evidence

The new sanitizer passes os directly into str_to_os(os) (no trim) and lowercases language
binding without trimming; str_to_os() also performs matching without trimming, so trailing/leading
spaces will cause a parse error and force the other bucket.

rust/src/stats.rs[84-97]
rust/src/stats.rs[100-107]
rust/src/config.rs[167-176]

Agent prompt

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

### Issue description
`sanitize_browser_version()` trims input before validation, but `sanitize_os()` and `sanitize_language_binding()` do not. This causes whitespace-padded but otherwise valid values (e.g., `"linux "`, `"Java "`) to be classified as `other`.

### Issue Context
This PR intentionally tightens telemetry. Trimming leading/trailing whitespace still keeps the value within a vetted vocabulary while avoiding accidental misclassification.

### Fix Focus Areas
- rust/src/stats.rs[84-97]
- rust/src/config.rs[167-176]

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

---

Results up to commit 595ee44 ⚖️ Balanced

No changes from previous review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 595ee44

[qodo-code-review]

Code review by qodo was updated up to the latest commit 2a6acdb