Skip to content

fix(security): stop reporting User-Agent versions as IP addresses - #168

Merged
rng1995 merged 4 commits into
NVIDIA:mainfrom
JayOfTheKeyboard:fix/pii-user-agent-product-versions
Sep 30, 2026
Merged

fix(security): stop reporting User-Agent versions as IP addresses#168
rng1995 merged 4 commits into
NVIDIA:mainfrom
JayOfTheKeyboard:fix/pii-user-agent-product-versions

Conversation

@JayOfTheKeyboard

@JayOfTheKeyboard JayOfTheKeyboard commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Fixes #167.

Summary

The PII scan reports Chromium User-Agent versions as public IP addresses. Since Chrome 101, the reduced User-Agent writes every version as <major>.0.0.0, so Chrome/140.0.0.0 matches the ip_addresses pattern. On main at a2636b2:

Line in SKILL.md Parent This PR
User-Agent: Mozilla/5.0 (X11; Linux x86_64) ... Chrome/140.0.0.0 Safari/537.36 [PII-MEDIUM] Non-RFC1918 IP address: 140.0.0.0 not reported
HeadlessChrome/151.0.0.0 -> Chrome/151.0.0.0 reported not reported
Edge reports `Edg/140.0.0.0` and Opera reports OPR/122.0.0.0. reported not reported
page = browser.new_page(user_agent='Chrome/140.0.0.0', base_url=base) reported not reported
Backups/8.8.8.8, whitelist: Office/52.14.1.9, Proxy/8.8.8.8 reported reported
Chrome/140.0.0.0 through proxy 8.8.8.8 reported reported (for 8.8.8.8)
Allow 52.0.0.0 through the firewall. reported reported

_is_version_literal treats an IPv4-shaped match as a version when both of these hold:

  • the value has the reduced-User-Agent shape, \d{1,3}\.0\.0\.0;
  • a product token ([A-Za-z][A-Za-z0-9]*) sits directly before its /.

A .0.0.0 value is never a host address, so the rule cannot hide a real host after a slash. My first version keyed on the product token alone. An independent review found it hid Backups/8.8.8.8, Office/52.14.1.9 and $HOME/8.8.8.8, and requiring the value's shape closed all three. The check sits next to the package-artifact rule, before the network-word check. That way a User-Agent that shares a line with base_url or proxy is still recognised, while URL authorities (https://52.0.0.0/) are still reported.

Not changed

  • Full, unreduced versions such as Chrome/140.0.7339.80 were never IPv4-shaped. Four-part versions from other browsers with non-zero components, for example YaBrowser/24.12.1.712, are still reported. Covering them would need a list of browser names, and I left that to you.
  • Chrome 140.0.0.0 with a space and no slash is still reported. It is not User-Agent syntax.

Verification

  • I am familiar with the Contributing Guidelines
  • Added or updated focused tests: test_user_agent_product_versions_are_not_pii (4 cases) and test_ip_outside_a_reduced_user_agent_version_remains_pii (7 guard cases) in tests/validators/test_security.py. All 4 positive cases fail on the parent and pass with the fix; the 7 guards pass on both. Mutations, each separately: dropping the .0.0.0 shape check turns 5 guards red, dropping the product-token check turns Allow 52.0.0.0 ... red, and moving the rule after the network-word check turns the base_url case red.
  • Updated documentation for user-visible changes (none needed)
  • Ran make lint: passed
  • Ran make test: with -n 6, 6,324 passed, 49 skipped and 588 failed. All 582 failures seen on pristine a2636b2 in the same run also fail there. They are Harbor Tier 3 staging tests that fail on this host with staging tree has unexpected or missing entries. The other 6 (Bedrock preflight and a DNS-deadline test) pass when rerun serially. The full suite is not claimed green
  • Ran make build: sdist and wheel built
  • Did not add credentials, private datasets, or proprietary benchmark content

Real-tool check: skillevaluator pii-scan on the skill from #167 reports nothing after the fix. So does a browser-automation skill of mine whose docstring was the original trigger.

Release Impact

  • No user-visible release note needed
  • Updated CHANGELOG.md

@JayOfTheKeyboard
fix(security): stop reporting User-Agent versions as IP addresses
7152e05 
Chromium's reduced User-Agent reports every version as <major>.0.0.0,
so Chrome/140.0.0.0 matched the ip_addresses PII pattern in any skill
that documents or sets a browser User-Agent. Treat an IPv4-shaped value
as a version when it has that <major>.0.0.0 shape and a product token
sits directly before its slash. A .0.0.0 value is never a host address,
so real IPs after a slash (paths, labels, URL paths) stay reported.

Signed-off-by: Jeremy Levartovsky <[email protected]>
rng1995
rng1995 reviewed Sep 28, 2026
View reviewed changes

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @JayOfTheKeyboard, thank you for this contribution, and for the careful report in #167! The before/after table, the "Not changed" notes, and the mutation results made this easy to review.

The fix is small and sits in the right place, next to the package-artifact rule in _is_version_literal. The new tests follow the conventions of their neighbors in TestSpdxAndIpFalsePositiveHardening. Here's what I checked locally on 7152e05:

  • All 356 tests in tests/validators/test_security.py pass, and ruff check src tests is clean.
  • I reproduced your three mutations. Dropping the .0.0.0 shape check fails 5 guards, dropping the product-token check fails Allow 52.0.0.0 ..., and moving the rule below the network-word check fails the base_url case.
  • A few more real-world UA forms are now clean: Edge desktop (... Safari/537.36 Edg/140.0.0.0), Android Chrome, CriOS/140.0.0.0, Electron/38.0.0 Chrome/140.0.0.0, a Python headers = {'User-Agent': ...} dict, and curl -A '...'. URL authorities (https://52.0.0.0/, http://140.0.0.0:8080/path), YaBrowser/24.12.1.712, and cidr: 52.0.0.0/8 are still reported, as intended.

I've left three small inline suggestions, none of them blocking:

  1. Pin which IP the mixed-line guard reports. As written, it also passes on main.
  2. Keep CIDR blocks that follow a slash reported, since a UA version is never followed by /.
  3. Optionally, handle JSON-escaped slashes (Chrome\/140.0.0.0). This would also be fine as a follow-up.

Thanks again for tracking this down!

Comment thread src/skillevaluator/validators/security.py Resolved
Comment thread src/skillevaluator/validators/security.py Resolved
Comment thread tests/validators/test_security.py Resolved
@JayOfTheKeyboard
fix(security): keep CIDR blocks reported and accept JSON-escaped UA s…
8c310c0 
…lashes

Review follow-up for NVIDIA#168:
- a reduced UA version is never followed by '/', so allowlist/52.0.0.0/8
  and URL paths ending in a CIDR block are reported again;
- Chrome\/140.0.0.0 (JSON with escaped slashes) is treated like
  Chrome/140.0.0.0;
- the mixed-line guard moves to its own test that pins the reported value
  to 8.8.8.8, since the any() form also passed on main.

Signed-off-by: Jeremy Levartovsky <[email protected]>
@JayOfTheKeyboard

JayOfTheKeyboard commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review. All three suggestions are in 8c310c0:

  • The mixed-line case is now its own test that pins the reported value to 8.8.8.8 (it fails on main with ['140.0.0.0']).
  • A version followed by / is no longer treated as a UA version, so allowlist/52.0.0.0/8 and https://api.example.com/v1/ips/52.0.0.0/8 are reported again. Both are now guard cases.
  • Chrome\/140.0.0.0 (JSON-escaped slashes) is handled, with a positive case.

The three new cases failed on 7152e05 before the change. All 359 tests in tests/validators/test_security.py pass, and ruff check src tests is clean.

rng1995
rng1995 approved these changes Sep 29, 2026
View reviewed changes

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @JayOfTheKeyboard, for the quick and thorough follow-up! All three suggestions are addressed in 8c310c0, and the CHANGELOG.md conflict from the merge with main is resolved cleanly, with both entries kept.

What I checked on d809322:

  • The mixed-line case now has its own test that pins the reported value to 8.8.8.8.
  • allowlist/52.0.0.0/8 and https://api.example.com/v1/ips/52.0.0.0/8 are reported again, and both are guard cases.
  • JSON-escaped Chrome\/140.0.0.0 is suppressed, while Backups\/8.8.8.8 is still reported.
  • All three new cases fail on 7152e05 and pass here.
  • All 414 tests in test_security.py, test_pii_grouping.py and test_security_validators.py pass, and ruff check src tests is clean.

Approving. Thanks again for the fix!

@rng1995
rng1995 enabled auto-merge (squash) September 29, 2026 21:26
@rng1995
rng1995 merged commit d015741 into NVIDIA:main Sep 30, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rng1995 rng1995 rng1995 approved these changes
@chrisknvidia chrisknvidia Awaiting requested review from chrisknvidia chrisknvidia is a code owner
@mosheabr mosheabr Awaiting requested review from mosheabr mosheabr is a code owner
@rmalani-nv rmalani-nv Awaiting requested review from rmalani-nv rmalani-nv is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG]: PII scan reports Chrome User-Agent versions as public IP addresses

2 participants

@JayOfTheKeyboard @rng1995