fix: RFC 2183 compliant Content-Disposition header in /view endpoint#13080
Open
TJUEZ wants to merge 1 commit intoComfy-Org:masterfrom
Open
fix: RFC 2183 compliant Content-Disposition header in /view endpoint#13080TJUEZ wants to merge 1 commit intoComfy-Org:masterfrom
TJUEZ wants to merge 1 commit intoComfy-Org:masterfrom
Conversation
Fixes the Content-Disposition header format in the view_image function to comply with RFC 2183. Previously used 'filename="name.ext"' which is not RFC 2183 compliant. Now uses 'attachment; filename="name.ext"'. Fixes Comfy-Org#8914
TJUEZ
requested review from
Kosinkadink,
comfyanonymous and
guill
as code owners
📝 WalkthroughWalkthroughThis pull request modifies the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_view_content_disposition.py (1)
91-151: Test coverage misses 3 changed/viewbranches (preview,channel=rgb,channel=a).Since this PR updates
Content-Dispositionin four branches, consider extending this suite to assert the same header format for each variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_view_content_disposition.py` around lines 91 - 151, Add assertions covering the other /view code paths that were changed: call the /view endpoint with params that exercise preview mode (params include "preview": "true") and with channel set to "rgb" and "a" (params include "channel": "rgb" and "channel": "a"), and for each response assert status_code == 200 and that the Content-Disposition header equals attachment; filename="<filename>" (same expected_prefix logic used in test_view_content_disposition_is_attachment_format and test_view_with_subfolder_content_disposition). Place these new assertions as additional test cases (or extend the existing tests) so the branches exercised by preview, channel=rgb, and channel=a verify RFC 2183 compliant Content-Disposition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_view_content_disposition.py`:
- Around line 63-77: The client fixture retries GET /system_stats but currently
returns the session even if no attempt returned HTTP 200 (only ConnectionError
triggers raise); update the logic in client to fail fast when readiness never
reaches 200 by keeping track of the last response (resp) or a success flag
inside the retry loop and after the loop raise an exception (or call
pytest.fail) if no resp.status_code == 200 was observed; reference the client
function, n_tries, resp, session and the requests.exceptions.ConnectionError
handling so the test stops early with a clear error instead of returning a
session.
---
Nitpick comments:
In `@tests/test_view_content_disposition.py`:
- Around line 91-151: Add assertions covering the other /view code paths that
were changed: call the /view endpoint with params that exercise preview mode
(params include "preview": "true") and with channel set to "rgb" and "a" (params
include "channel": "rgb" and "channel": "a"), and for each response assert
status_code == 200 and that the Content-Disposition header equals attachment;
filename="<filename>" (same expected_prefix logic used in
test_view_content_disposition_is_attachment_format and
test_view_with_subfolder_content_disposition). Place these new assertions as
additional test cases (or extend the existing tests) so the branches exercised
by preview, channel=rgb, and channel=a verify RFC 2183 compliant
Content-Disposition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c60a67f4-e99e-4b2a-bdc9-a68ffaded75d
📒 Files selected for processing (2)
server.pytests/test_view_content_disposition.py
Comment on lines
+63
to
+77
| def client(self, server_url, _server): | ||
| """Create HTTP client with retry.""" | ||
| import requests | ||
| session = requests.Session() | ||
| n_tries = 10 | ||
| for i in range(n_tries): | ||
| time.sleep(2) | ||
| try: | ||
| resp = session.get(f"{server_url}/system_stats", timeout=5) | ||
| if resp.status_code == 200: | ||
| break | ||
| except requests.exceptions.ConnectionError: | ||
| if i == n_tries - 1: | ||
| raise | ||
| return session |
There was a problem hiding this comment.
Fail fast when server readiness never reaches 200.
If no attempt gets HTTP 200 (but no ConnectionError is thrown), the fixture still returns a session and tests fail later with less-clear errors.
Proposed fix
`@pytest.fixture`(scope="class")
def client(self, server_url, _server):
"""Create HTTP client with retry."""
import requests
session = requests.Session()
n_tries = 10
for i in range(n_tries):
time.sleep(2)
try:
resp = session.get(f"{server_url}/system_stats", timeout=5)
if resp.status_code == 200:
break
except requests.exceptions.ConnectionError:
if i == n_tries - 1:
+ session.close()
raise
- return session
+ else:
+ session.close()
+ pytest.fail(f"Server at {server_url} did not become ready after {n_tries} attempts")
+
+ try:
+ yield session
+ finally:
+ session.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_view_content_disposition.py` around lines 63 - 77, The client
fixture retries GET /system_stats but currently returns the session even if no
attempt returned HTTP 200 (only ConnectionError triggers raise); update the
logic in client to fail fast when readiness never reaches 200 by keeping track
of the last response (resp) or a success flag inside the retry loop and after
the loop raise an exception (or call pytest.fail) if no resp.status_code == 200
was observed; reference the client function, n_tries, resp, session and the
requests.exceptions.ConnectionError handling so the test stops early with a
clear error instead of returning a session.
|
This is a duplicate of about 9 other PRs: https://github.com/Comfy-Org/ComfyUI/pulls?q=is%3Apr+is%3Aopen+rfc+2183 |
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
2 participants
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.
Fix: RFC 2183 compliant Content-Disposition header
Issue
Fixes #8914
Problem
The
Content-Dispositionheader set by the/viewendpoint was not RFC 2183 compliant:Content-Disposition: filename="name.ext"Content-Disposition: attachment; filename="name.ext"This caused issues with third-party libraries (e.g., Go's
mime.ParseMediaType) that properly validate RFC 2183 headers.Changes
Modified
server.pyto use RFC 2183 compliant format in 4 places within theview_imagefunction:Testing
Added
tests/test_view_content_disposition.pywith tests that verify:/viewendpoint returns RFC 2183 compliantContent-DispositionheadersNote: Tests require the full ComfyUI environment (torch, etc.) to run. They validate that the header format is exactly
attachment; filename="<filename>".Checklist