Fix npm-http-dependency false positive on metadata URL fields#778
Conversation
The npm-http-dependency rule treats any plain http:// value that is not a
known metadata field as a dependency URL (#http_plain > #http_meta). Nested
metadata objects (author, repository, bugs, funding) carry their URL under a
"url" key, which was missing from the allowlist, so packages with an
http author/repo URL (e.g. "author": { "url": "http://..." }) were
falsely flagged.
Add "url" to the metadata-field allowlist and extend the benign fixture to
cover author/repository/bugs URLs over http.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 611b193b88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Plain-http URLs in package metadata fields (author/repo/homepage). | ||
| // `url` covers the nested URL key used by author/repository/bugs/funding | ||
| // objects (e.g. "author": { "url": "http://..." }), a common false positive. | ||
| $http_meta = /"(url|web|website|homepage|funding|bugs|email|wiki|blog|docs|documentation|repository|author|maintainers|contributors|logo|image)"\s*:\s*"http:\/\// nocase |
There was a problem hiding this comment.
If a package depends on the npm package named url with a plain-HTTP specifier such as "dependencies": {"url": "http://evil.example/pkg"}, this new allowlist entry makes the same string count as both $http_plain and $http_meta. Because the condition only fires on #http_plain > #http_meta, the single suspicious dependency is no longer reported unless it also matches the IP/archive-specific patterns; before this change it was caught. The url allowance needs to be constrained to metadata object contexts rather than any key named url.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed in 0e975e1: the bare url allowlist entry is replaced with a dedicated $http_meta_url pattern that matches the nested url key only inside a metadata object (author, repository, bugs, funding, contributors, maintainers). A dependency literally named url with a plain-http specifier ("dependencies": {"url": "http://..."}) is reported again. Added a positive fixture (threat-npm-http-dependency.url-dep.json) to lock this in.
Addresses review feedback: allowlisting any key named "url" let a dependency
literally named "url" with a plain-http specifier ("dependencies": {"url":
"http://..."}) count as both $http_plain and $http_meta, so it was no longer
reported. Replace the bare "url" allowlist entry with $http_meta_url, which
matches the nested "url" key only inside a metadata object (author, repository,
bugs, funding, contributors, maintainers). A url dependency over http is caught
again, while benign metadata URLs stay suppressed.
Add a positive fixture covering the url-as-dependency case.
2 participants
Problem
The
npm-http-dependencyrule flags packages whosepackage.jsoncontains a plainhttp://URL that isn't a recognized metadata field, on the assumption it's a dependency specifier (#http_plain > #http_meta), but some legitimate package.json files matched, especially some with nestedauthor.urlkeys (see regression test)