Skip to content

Reject redirect-following curl probe flags - #5564

Closed
fallintoplace wants to merge 2 commits into
NVIDIA:mainfrom
fallintoplace:fix/curl-probe-redirect-ssrf
Closed

Reject redirect-following curl probe flags#5564
fallintoplace wants to merge 2 commits into
NVIDIA:mainfrom
fallintoplace:fix/curl-probe-redirect-ssrf

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • remove -L, -sfL, and --location from the shared curl probe allowlist
  • stop the Ollama registry size probe from depending on -sfL
  • add regression coverage that redirect-following flags are rejected before curl is spawned

Why

The curl probe validator normalizes the initial URL, then lets curl execute the rebuilt argv. Allowing redirect-following flags means a URL that starts public can still be followed to loopback, link-local, or another private/internal target outside the validator's view. Default-denying redirects keeps the existing SSRF boundary honest until a redirect-aware validator exists.

Validation

  • npm run build:cli
  • ./node_modules/.bin/vitest run src/lib/adapters/http/probe.test.ts src/lib/inference/ollama/model-size.test.ts
  • npm run typecheck:cli

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Updated HTTP probing and Docker manifest fetching to disable redirect-following in curl operations.
  • Tests

    • Added validation tests to ensure redirect-following curl options are properly rejected.
    • Enhanced test coverage for manifest probing operations.

@copy-pr-bot

copy-pr-bot Bot commented Jun 20, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e3dad282-fee7-4677-a8e4-2e1453593c9b

📥 Commits

Reviewing files that changed from the base of the PR and between 5b12568 and d112ebe.

📒 Files selected for processing (4)
  • src/lib/adapters/http/curl-args.ts
  • src/lib/adapters/http/probe.test.ts
  • src/lib/inference/ollama/model-size.test.ts
  • src/lib/inference/ollama/model-size.ts
💤 Files with no reviewable changes (1)
  • src/lib/adapters/http/curl-args.ts

📝 Walkthrough

Walkthrough

Redirect-following curl flags (-L, -sfL, --location) are removed from the CURL_SAFE_FLAG_OPTIONS allowlist in validateCurlProbeArgs. The one call site using -sfL in probeRegistrySize is updated to -sf. Tests are added and adjusted to match.

Changes

Remove redirect-following curl flags

Layer / File(s) Summary
Allowlist policy change and probe rejection test
src/lib/adapters/http/curl-args.ts, src/lib/adapters/http/probe.test.ts
CURL_SAFE_FLAG_OPTIONS no longer includes -L, -sfL, or --location. A new parameterized test asserts runCurlProbe returns ok: false with an "option is not allowed" message and never calls spawnSyncImpl when those flags are passed.
probeRegistrySize call site update
src/lib/inference/ollama/model-size.ts, src/lib/inference/ollama/model-size.test.ts
The curl flags in probeRegistrySize are changed from -sfL to -sf. The test adds an assertion that -sf is present and a negative assertion that -sfL is absent.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~4 minutes

Poem

🐇 Hoppy news from the warren today,
No more -L flags leading astray!
Redirects are banned, the allowlist is tight,
-sf alone keeps the curl probes right.
The bunny says: follow no paths unknown! 🚫↪️

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title clearly and concisely summarizes the main change: rejecting redirect-following curl probe flags.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@wscurran wscurran added area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression security labels Jun 23, 2026
@wscurran
wscurran requested a review from ericksoa June 23, 2026 15:40
@apurvvkumaria apurvvkumaria self-assigned this Jul 8, 2026
@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Maintainer disposition: closing this PR as obsolete, without merge.

Thank you @fallintoplace for identifying redirect-following curl behavior as an SSRF risk and for adding focused before-spawn negative tests for -L, -sfL, and --location. That security insight is valid.

Current main at fed46043459497011c65884720cb24a354cfb2bb already closes the generic gap through f1e7d878f8 / #5975:

  • Redirect-following flags are no longer globally safe; -L, -sfL, and --location are rejected by default.
  • Redirects require an explicit allowRedirects call-site capability. The only production opt-in is the Ollama registry-size probe, whose URL is built from the hardcoded https://registry.ollama.ai origin plus structurally validated model namespace/tag segments.
  • Exact wrapper verification confirmed that -L, -sfL, --location, --location-trusted, -sL, -Ls, -fsL, and -sSL all fail before curl is spawned by default. With the explicit opt-in, only the three named forms are accepted; the stronger alias and alternate combined forms remain rejected.
  • Current-main focused tests pass: 56/56 across curl-args.test.ts and model-size.test.ts. The PR-head tests also pass 67/67, but its proposed matrix covers only the three removed allowlist entries rather than the broader alias/combination set.

The PR no longer closes a live SSRF gap and is stale against current main. A local three-way merge confirms a conflict in src/lib/inference/ollama/model-size.ts, where current main deliberately retains the validated exception while this PR removes redirect following. Its exact-head gates also remain noncompliant: commit-lint fails because the PR title is not Conventional Commits, dco-check fails because the body lacks a Signed-off-by: declaration, and author commit d112ebec95fdea7971e55cb28c9eb2509a60bd11 is unsigned/unverified. The snapshot has 24 passing checks, 2 failures, and 3 skips; review is still required.

No code was copied and no replacement PR was created. The v0.0.79 label and maintainer assignment are being retained.

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

Labels

area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants