fix(security): floor litellm/wandb/lxml past known CVEs; relax stale click cap - #1507
Open
nicgupta-nvidia wants to merge 27 commits into
Open
nicgupta-nvidia wants to merge 27 commits into
nicgupta-nvidia wants to merge 27 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDependency requirements now include security floors and compatibility pins, BFCL uses the updated code generator version, Nemo Skills builds and installs a pinned W&B core binary, and regression plus functional tests validate the updated dependency behavior. ChangesDependency security and compatibility updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
2 similar comments
Contributor
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
|
✅ Unit tests committed locally. Commit: |
Contributor
|
✅ Unit tests committed locally. Commit: |
Contributor
|
✅ Created PR with unit tests: #1508 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_requirements_versions.py (1)
95-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract a shared helper for specifier-floor extraction.
The
{spec.operator: spec.version for spec in req.specifier}pattern is repeated across four test methods. A small helper reduces duplication and keeps the assertions consistent if the extraction logic ever needs to change (e.g., handling multiple specifiers of the same operator).♻️ Proposed helper
+def _get_specifier(req: Requirement, operator: str) -> Version: + """Return the parsed Version for the given specifier operator, asserting it's present.""" + specs = {spec.operator: spec.version for spec in req.specifier} + assert operator in specs, f"expected a '{operator}' specifier for {req.name}, got {req.specifier}" + return Version(specs[operator]) + + class TestCoreRequirements: ... def test_litellm_pin_fixes_ghsa_4xpc_pv4p_pm3w(self): req, comment = _find_requirement(CORE_REQUIREMENTS, "litellm") assert "caching" in req.extras, "litellm[caching] extra must be preserved" - - # Must be pinned to an exact version (== specifier) so the resolver is deterministic. - specs = {spec.operator: spec.version for spec in req.specifier} - assert "==" in specs, f"expected an exact pin for litellm, got specifier {req.specifier}" - - pinned_version = Version(specs["=="]) + pinned_version = _get_specifier(req, "==")Also applies to: 113-114, 156-157, 178-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_requirements_versions.py` around lines 95 - 96, The specifier extraction logic is duplicated across multiple test methods in the requirements version tests, so add a shared helper for turning a requirement’s specifiers into a usable mapping or floor value. Update the affected assertions in the test class that currently build specs from req.specifier so they call this helper instead, keeping the exact-pin checks unchanged while centralizing the extraction behavior in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_requirements_versions.py`:
- Around line 95-96: The specifier extraction logic is duplicated across
multiple test methods in the requirements version tests, so add a shared helper
for turning a requirement’s specifiers into a usable mapping or floor value.
Update the affected assertions in the test class that currently build specs from
req.specifier so they call this helper instead, keeping the exact-pin checks
unchanged while centralizing the extraction behavior in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0ea160ee-449e-4976-b41b-78cdb3607f1a
📒 Files selected for processing (2)
requirements/common-tests.txttests/test_requirements_versions.py
gwarmstrong
added a commit
that referenced
this pull request
Jul 28, 2026
Cherry-picked from #1507 (fix/security-dependency-floors), which is the last configuration in which the full CPU suite passed (725 passed, 0 failed on 2026-07-14). With click pinned below 8.2 the ns CLI stops accepting underscore-style option names, so every test that shells out to `ns eval --output_dir=...` or `ns summarize_results --max_seq_len=...` fails with "Missing option '--output-dir'" / "No such option: --max_seq_len". The typer floor moves to 0.16 alongside it because that is the first release compatible with click 8.2 (fixes the make_metavar break that motivated the original pin). Only the CLI-relevant pair is taken here. #1507's litellm, wandb and stem security floors are left to that PR. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
gwarmstrong
added a commit
that referenced
this pull request
Jul 28, 2026
Dropping the click<8.2 cap alone was not enough: wandb 0.26.1 only requires click>=8.0.1, so uv still settled on click 8.1.8 and the CPU suite failed identically (10 failed, 685 passed). wandb 0.27.1 is the first release requiring click>=8.2.0, which is what actually moves the resolver. Taken from #1507, the last configuration in which the full suite passed. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
gwarmstrong
added a commit
that referenced
this pull request
Jul 28, 2026
This is the actual root cause of the CPU suite failures, and the reason
removing the repo's own `click < 8.2.0` cap changed nothing: litellm
1.83.14 declares an exact `click==8.1.8` dependency, capping the entire
tree below click 8.2 regardless of what this repo asks for.
uv spelled it out when wandb>=0.27.1 was added:
Because wandb>=0.27.1 depends on click>=8.2.0 and litellm==1.83.14
depends on click==8.1.8, we can conclude that litellm==1.83.14 and
wandb>=0.27.1 are incompatible.
litellm 1.84.10 relaxes that to click>=8.0.0,<9.0 (and clears
GHSA-4xpc-pv4p-pm3w). With it, `uv pip install -e .[dev]` resolves to
click 8.4.2 / typer 0.27.0 / wandb 0.28.1.
Taken from #1507 together with the wandb floor and the click cap removal;
the three only work as a set.
Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
nicgupta-nvidia
enabled auto-merge (squash)
July 30, 2026 16:34
Collaborator
Author
|
August 6 High-severity follow-up (commit 75d0c6a):
Focused validation: 35 security tests passed, Ruff lint/format passed (excluding one pre-existing C408 on an untouched line), the Ray private import resolved |
nicgupta-nvidia
force-pushed
the
fix/security-dependency-floors
branch
2 times, most recently
from
September 10, 2026 18:37
a20c5ba to
70256e1
Compare
…click cap - litellm[caching] 1.83.14 -> 1.84.10: GHSA-4xpc-pv4p-pm3w (Critical) — 1.83.x leaks the API key to an arbitrary attacker-controlled Host header; fixed in 1.84.0. Minimal exact-pin jump; resolves with the existing httpx[http2]>=0.28.1 override (litellm 1.84.10 needs httpx>=0.28.0). - wandb -> >=0.27.1: the bundled wandb-core Go binary in older wheels ships golang.org/x/crypto 0.50.0 + Go 1.26.2 stdlib with 7 Critical / 13 High CVEs (incl. GHSA-x527-x647-q7gg et al.); 0.27.1 is the first release embedding patched x/crypto 0.52.0 (verified on both arches). - click < 8.2.0 cap removed + typer >= 0.16: the cap guarded against the typer/click-8.2 make_metavar break (ai-dynamo/dynamo#1039, closed 2025-06-26, fixed in typer >=0.16); wandb>=0.27.1 requires click>=8.2, and requires-python >=3.10 satisfies click 8.2's floor. - lxml -> >=6.1.0 (stem extra): GHSA-vfmq-68hx-4jfw (High). Validation: uv pip compile of core+pipeline (py3.10, with the pyproject overrides) resolves cleanly — litellm 1.84.10 / wandb 0.28.0 / click 8.4.2 / typer 0.26.8 / httpx 0.28.1; stem extra resolves with lxml 6.1.1. Runtime smoke on the resolved set: litellm/wandb import clean; typer --help rendering (the exact make_metavar crash path) passes under click 8.4. Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
CodeRabbit's test generation ran twice and committed two near-duplicate suites (test_dependency_pins.py + test_requirements_versions.py) covering the same pins. Keep the more robust one (operator-keyed specifier parsing instead of next(iter(specifier)), which is order-fragile on multi-spec requirements) and graft the three tests unique to the deleted file: pyproject stale-comment guards, pipeline-lines-parseable, and the wandb/typer click-comment consistency check. Also: - fix the copyright year (2026, not 2025) - add tomli (python_version < 3.11) to common-tests.txt so the pyproject override tests actually RUN on the CI's Python 3.10 instead of silently skipping (tomllib is stdlib only from 3.11) - add the missing trailing newline that failed the pre-commit end-of-file-fixer hook on the generated files 17 tests pass on Python 3.10 with the CI's -m 'not gpu' selection; pre-commit (pinned ruff) clean. Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
…ed deps
test_requirements_versions.py only asserts the pins statically. Add functional
coverage that drives litellm 1.84.10, typer/click, and wandb through NeMo-Skills'
own code paths (CPU-only, hermetic — no sandbox, no live endpoint, no API keys)
so a resolve to a behavior-divergent version fails CI, not a production run:
* litellm: OpenAIModel.litellm_kwargs binds api_key to the configured
base_url/api_base only (GHSA-4xpc-pv4p-pm3w regression), generate_async
calls litellm.acompletion with those credentials and parses the 1.84
response, and the imported litellm exception/type surface still exists.
* typer/click: ns CLI --help + per-command help render Parameter.make_metavar
(the click 8.2 break typer>=0.16 fixes) and unknown commands are usage errors.
* wandb: log_random_samples matches the wandb 0.28 init/save/summary/finish
contract, and a real offline init/finish cycle runs with no account.
* lxml: importorskip-guarded real parse (optional stem extra).
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
nicgupta-nvidia
force-pushed
the
fix/security-dependency-floors
branch
from
September 11, 2026 18:39
70256e1 to
94d105c
Compare
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
nicgupta-nvidia
force-pushed
the
fix/security-dependency-floors
branch
from
September 11, 2026 18:45
94d105c to
da8459a
Compare
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Collaborator
Author
|
Merge-blocker status update (September 14):
@gwarmstrong A qualifying review is the only remaining merge blocker. A review is already requested; could you please review the current head when available? |
Signed-off-by: Nick Gupta <nicgupta@nvidia.com>
Collaborator
Author
|
Added the October 2 Trivy refresh in commit Fresh scan resultsScanned the fully resolved runtime + dev graph for Linux Python 3.10 with Trivy
Remediated:
The remaining High on both architectures is NLTK The AnyIO and instrumentator floors are resolver overrides because LeptonAI Validation
Scan report SHA-256:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Remediates the fixable Critical/High dependency findings observed across the NeMo-Skills repository and container scans, while retaining the earlier LiteLLM, GitPython, datamodel-code-generator, lxml, aiohttp, msgpack, and setuptools fixes already carried by this PR.
The branch head is based on merge base
cb54e911. At the September 8 live-base revalidation,mainwas0ce744ab; this PR was 26 commits ahead and 1 behind, and GitHub generated the conflict-free merge reff098f788from that base and the exact PR head.September 4 security refresh
A fresh scan of
nvcr.io/0953339617667984/nvflow-nemo-skills:v1.1.3-rayfound 2 Critical and 14 High instances on amd64. Fifteen were fixable; the remaining High (CVE-2026-81726in NLTK) has no fixed release.wandb-coreGo stdlibwandb-corex/cryptowandb-corego-gitwandb-corex/imagewandb-coregRPCThe NLTK floor is enforced in
[tool.uv].override-dependencies, thestemrequirements, the container install, and static/functional regression checks.W&B 0.28.1 ships a vulnerable release binary, so the Dockerfile rebuilds only
wandb-corefrom the pinned W&B commit. The build rejects the binary unlessgo version -mreports:go-git5.19.2x/crypto0.55.0x/image0.45.0x/text0.41.0Only the verified executable is copied into the runtime image; the Go toolchain and source are not retained.
The runtime image also replaces
/usr/localwith the fully resolved dependency artifact and removes pip's build-input CycloneDX BOM. This prevents stale pre-resolution metadata and build-only dependencies from being reported as runtime vulnerabilities.Earlier security scope retained
litellm[caching]==1.84.10for GHSA-4xpc-pv4p-pm3wGitPython>=3.1.58datamodel-code-generator>=0.64.0, including the BFCL runtime pinlxml>=6.1.0aiohttp>=3.14.3, including Ray's private vendored copymsgpack>=1.2.1setuptools>=78.1.1wandb==0.28.1, paired with the patched coretyper>=0.16Verified validation
Source:
a20c5bad7266d4947447f3a72507573a2bfecac9mainsnapshot and current merge base:cb54e911ad5b2cee87444fc89656fabca021c8cc(the PR was then 26 ahead, 0 behind)UpdatedAt 2026-09-04T13:08:55.575059601Z)main-snapshot tracked-source scan: 0 vulnerabilities (report SHA-25657aaff384ade15f4921fecc01930279105e085b5cba2da0e28314ddb2af1df36)352340141f93c4bed5be69fd93ee764e38464b27728ac97597dfa9be2d8f31fb)Multi-architecture image:
nvcr.io/0953339617667984/nvflow-nemo-skills:pr1507-a20c5bad-security-20260904sha256:6f98043019f5972daf592533d7324644ded4312283e768f28727d1c5470b2c94sha256:ebca9c4ba99898e20ee21e8cd7662c65d99178a2f0d2ca02d01a8d19d37a6373; Linux/arm64sha256:3c0aabf388e30a7020c7fa376818b0e06f137f8b00a28c3e52fc71af32cb2007a0b1ec69429f3a7a01c1b06d834b51ad5be98ff65e49633fe4cac955be4eb8c7319649fecbed007c94fac98c05a759f78ac77e640728cd43ab4fb750115a7532All final NLTK, W&B module, dependency-metadata, architecture, and cleanup assertions passed. The sole remaining High on each platform is unfixed NLTK
CVE-2026-81726; it is intentionally not hidden or ignored.CI status
DCO, pre-commit, and copyright checks pass. Both manually built image architectures and their security/runtime gates pass. The only failing hosted check is
CPU tests / unit-tests: 754 passed, 1 skipped, 37 deselected, and 7 failed. Its log confirms the external endpoint returns HTTP 410Gonebecausenvidia/nemotron-3-nano-30b-a3breached end of life at2026-09-01T09:00:00Z; the seven failures are downstream missing-output or summarization failures from that response. The affected test files are unchanged by this PR, and an unrelated current PR has the same seven failures. This is not a dependency or container regression from this PR.September 8 fresh-database revalidation
Trivy 0.73.0 was rerun with vulnerability DB
UpdatedAt 2026-09-08T07:08:01.235696926Zagainst the exact source revisions and immutable amd64/arm64 manifests. Severity columns are Critical / High / Medium / Low / Unknown.cb54e911sourcec683a89c8bfc51c682ef15e16953065d41b8378dc766a30d2c4286b959962c48main0ce744absourcea289fb31978dd8c605a3b7c44844e67ff504e4749d01c8db4199967baee3bde4a20c5badsource655ef24082cd43241a13722b5c8f101498ef4b231e0fb22db1004ece6cbdedf2f098f788source2f1ec91eed350824c93c51ff2fc6f0ce346a029618204c6a96b1a19d53e0762475398905f9072cbd2579ccc14f12c41f661b8f5aee0d815b15d46fb245a7464f4225e2829a2acf028ca170e24b5ebb4a64df82037231a0300186fce1007dc9b5Every remaining image High is
CVE-2026-81726in NLTK 3.10.3, for which Trivy reports no fixed version. No additional Critical/High code change was required by the September 8 refresh.