Skip to content

Conversation

Myestery
Copy link
Contributor

@Myestery Myestery commented Jul 17, 2025

Update Content-Disposition header to include 'inline' for image responses.

meant to fix #8914

Old Behaviour

As seen in the linked issue, when Content Disposition is fetched for an image using the view_image function, it is erroneous to parse as it doesn't follow RFC standard

Screenshot 2025-07-18 at 00 41 46

New Behaviour

Standard RFC pattern for Content Disposition is used to render images inline in the browser ( 100% backwards compatible as this is the browser's default)
Screenshot 2025-07-18 at 00 43 37

@Myestery Myestery changed the title WIP Content-Disposition headers Content-Disposition headers Jul 17, 2025
@Myestery Myestery marked this pull request as ready for review July 17, 2025 23:47
@Myestery Myestery requested a review from comfyanonymous as a code owner July 17, 2025 23:47
@jtydhr88
Copy link
Contributor

jtydhr88 commented Jul 20, 2025

hi @OrangeDoro, same to #8716, I suggest do not use working/reviewing PR for your study, it may be confuse for later core team reviewer.
Other than these, I suggest you choose draft PRs which put on hold, for example, you could use #8265 and Comfy-Org/ComfyUI_frontend#3976 which contain BE and FE changes, I am the owner of these two PRs

@kevinpaik1
Copy link

PR Review Summary

Status: ✅ Approved for Core Team Review

For the Contributor

Thank you for this clean fix to the Content-Disposition headers! Your PR properly addresses issue #8914 by making the headers RFC 6266 compliant. The implementation is minimal and correct - exactly what we want to see.

Technical Summary

  • Scope: Updates Content-Disposition headers to include 'inline' directive
  • Files Changed: 1 file (+4/-4 lines)
  • Build Status: ✅ PASS
  • Test Status: ✅ PASS (manual verification)
  • Security: ✅ Clean
  • Architecture: ✅ Aligned

Review Highlights

Strengths:

  • Follows RFC 6266 standard correctly
  • Minimal change that solves the specific issue
  • Maintains full backward compatibility
  • Good documentation with before/after evidence

Key Findings:

  • The 'inline' directive is the browser default, so this change explicitly declares existing behavior
  • No security implications as filename parameter is already validated
  • Performance impact is negligible (7 bytes per response)

Remaining Items: None

Next Steps

This PR is ready for core team review. The core team will conduct their own evaluation and may request additional changes before merge.

For Core Team

Focus Areas: Standard review - RFC compliance verification
Review Complexity: Low - straightforward header format fix

Copy link
Collaborator

@guill guill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good and safe fix.

@guill
Copy link
Collaborator

guill commented Jul 23, 2025

It looks like you may need to merge back from main (or rebase) to get the updates that allow you to pass the new Windows Line Endings CI. Apologies for the inconvenience.

@Myestery
Copy link
Contributor Author

all done @guill

@guill guill added the Good PR This PR looks good to go, it needs comfy's final review. label Jul 23, 2025
@kevinpaik1
Copy link

Thank you for your contribution @Myestery!

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

Labels

Good PR This PR looks good to go, it needs comfy's final review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Content-Disposition" Header set in view_image function not matching RFC2183 rules

4 participants