Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File endpoints: use query parameters for NGINX/Caddy compatibility #6376

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bigcat88
Copy link
Contributor

@bigcat88 bigcat88 commented Jan 7, 2025

PR Description

When deploying ComfyUI behind NGINX, we encountered issues with file-related endpoints due to the use of path parameters for file paths. Proxy servers may modify the encoding of URLs, leading to inconsistent behavior. For example:

  • The server may receive:
    http://127.0.0.1:8188/api/userdata/workflows%2Fphoto_stickers2.json?overwrite=false&full_info=true
  • But proxies like NGINX may decode it to:
    http://127.0.0.1:8188/api/userdata/workflows/photo_stickers2.json?overwrite=false&full_info=true

This inconsistency caused the ComfyUI server to respond with a 405 Method Not Allowed status.

Changes Made:

  • Updated the file-related endpoints (getuserdata, post_userdata, delete_userdata, and move_userdata) to use query parameters instead of path parameters for file paths. This ensures compatibility with proxy servers by avoiding reliance on URL path encoding.
  • For the move_userdata endpoint, both the source and destination paths are now passed as query parameters (source and dest).

Notes:

  • The FrontEnd must be updated to reflect these changes, as the file paths have been moved from path parameters to query parameters. Without these updates, the modified endpoints will not function as expected.

This PR replaces old PR #6363 which was only a workaround which we applied on server to temporary make ComfyUI work.

Resolves: #5629

Partially fixes problem from #4534

@bigcat88 bigcat88 changed the title File endpoints: use query parameters for proxy compatibility File endpoints: use query parameters for NGINX/Caddy compatibility Jan 12, 2025
@bigcat88
Copy link
Contributor Author

If necessary, I can theoretically write a test for this, but I'm not sure that the design of endpoint should be covered by tests.

I can describe in more detail why these changes are needed, but in the linked issues that were not created by me, everything seems clear.

@asagi4
Copy link
Contributor

asagi4 commented Jan 16, 2025

This PR might be easier to merge if you supported both types of endpoint, so that people who have an older or alternative frontend don't just break since the frontend can be installed separately from the backend and isn't guaranteed to be in sync.

For nginx specifically, it seems it urlencodes the path if your proxy_pass directive contains a url path component. So

proxy_pass http://127.0.0.1:8188;

works properly, but

proxy_pass http://127.0.0.1:8188/;

or anything specifying a subpath does not

…parameters for proxy compatibility

Signed-off-by: bigcat88 <[email protected]>
@bigcat88 bigcat88 force-pushed the fix/user_manager/file-endpoints branch from 7e1ae9b to 720b03e Compare January 16, 2025 12:40
@bigcat88
Copy link
Contributor Author

Thank you very much for the correct and accurate remark, - re-pushed the commit.

After your remark:

  1. All old endpoints remain in place and are not touched.
  2. The same endpoints are added but with the /v1 prefix in the path - they use query parameters.
  3. Tests use new endpoints.
  4. Old endpoints can be removed after some time.

Is there anything else that is missing?

@lupsin
Copy link

lupsin commented Jan 22, 2025

May I ask about the approximate date when this or a similar PR that corrects this situation might be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

api/userdata/workflows 405
3 participants