Skip to content

test(frontend): align SSRF e2e with sanitized errors - #15540

Open
ericcurtin wants to merge 2 commits into
ai-dynamo:mainfrom
ericcurtin:ericcurtin/ssrf-e2e-sanitized-error
Open

ericcurtin wants to merge 2 commits into
ai-dynamo:mainfrom
ericcurtin:ericcurtin/ssrf-e2e-sanitized-error

Conversation

@ericcurtin

@ericcurtin ericcurtin commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Expect the sanitized 400 from #14396 in the SSRF e2e test. Both SSRF tests now share one assertion helper.

Validation

pre-commit passes. Helper checked with stub responses. The e2e needs a GPU, so not run locally.

Related Issues

Summary by CodeRabbit

  • Tests
    • Expanded coverage for requests to blocked private-network URLs, verifying they receive a client error with a generic message and do not establish a connection.
    • Applied the same checks across HTTP status propagation and media URL scenarios.

Signed-off-by: Eric Curtin <eric.curtin@docker.com>
@ericcurtin
ericcurtin requested review from a team as code owners October 2, 2026 00:13
@copy-pr-bot

copy-pr-bot Bot commented Oct 2, 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.

@ericcurtin
ericcurtin deployed to external_collaborator October 2, 2026 00:13 — with GitHub Actions Active
@ericcurtin
ericcurtin deployed to external_collaborator October 2, 2026 00:13 — with GitHub Actions Active
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

👋 Hi ericcurtin! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions Bot added external-contribution Pull request is from an external contributor test labels Oct 2, 2026
@coderabbitai

coderabbitai Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
.ai/python-guidelines.md — configured

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5413cf5b-df36-42a0-9fd9-55f326d5ccda

📥 Commits

Reviewing files that changed from the base of the PR and between 0a6f58b and 3f15b52.

📒 Files selected for processing (3)
  • tests/frontend/test_http_status_propagation.py
  • tests/mm_router/test_vllm_frontend_ssrf_e2e.py
  • tests/utils/network_canary.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

The change adds a shared helper for checking sanitized SSRF rejection responses and updates two tests to use it. The tests continue to verify that the private policy detail is not exposed and that the connection canary receives no connection.

Changes

SSRF rejection assertions

Layer / File(s) Summary
Define shared rejection assertions
tests/utils/network_canary.py
Adds a helper that checks for a 4xx response, the exact "Invalid request" message, absence of the supplied private detail, and no connection to the canary.
Use shared assertions
tests/frontend/test_http_status_propagation.py, tests/mm_router/test_vllm_frontend_ssrf_e2e.py
Both tests call the shared helper. The multimodal-router test retains its topology-specific private detail.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 3f15b

The changes centralize the sanitized 4xx and zero-egress assertions without an identified merge blocker. The GPU-dependent end-to-end test was not run locally.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: aligning SSRF end-to-end tests with sanitized errors.
Description check ✅ Passed The description explains the test changes, validation status, GPU test limitation, and linked issue. It does not include a separate reviewer-start section, but the required information is mostly compl…
Linked Issues check ✅ Passed The PR satisfies #15491. assert_blocked_url_refused checks a 4xx status, exact JSON message "Invalid request", absence of the topology-specific private detail, and zero canary connections. Both `v…
Out of Scope Changes check ✅ Passed The changes stay within #15491. The shared helper centralizes the required SSRF assertions, and the two affected tests use it. The frontend test update removes duplicated assertions without changing t…
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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

@ericcurtin

Copy link
Copy Markdown
Contributor Author

@pskiran1 @PeaBrane PTAL when you get a chance. Thank you!

@ericcurtin

Copy link
Copy Markdown
Contributor Author

@krishung5 @biswapanda PTAL when you get a chance. Thank you!

@krishung5

Copy link
Copy Markdown
Contributor

/ok to test 3f15b52

@devin-ai-integration

devin-ai-integration Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

✅ Dynamo PR CI passed — run 38321614258 (attempt 1) on c53f93415d

Gate checks: ✅ backend-status-check · ✅ deploy-status-check · ✅ dynamo-status-check

Posted automatically by Devin for run 38321614258. Updated on every full-CI run of this PR.

@krishung5
krishung5 deployed to external_collaborator October 12, 2026 23:20 — with GitHub Actions Active
@dynamo-ops

Copy link
Copy Markdown
Contributor

/ok to test c53f934

This branch was successfully deployed

1 active deployment
external_collaborator — c53f9341 Deployed Oct 12, 2026 by krishung5 via ok-to-test #30968
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contribution Pull request is from an external contributor size/M test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] vllm-runtime / Test cuda13.0, amd64: SSRF e2e test expects policy detail that #14396 now sanitizes

3 participants