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.

Sign up for GitHub

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

Jump to bottom

feat: respect x-* headers for redirections #9054

Merged
merged 5 commits into from
Oct 9, 2024
Merged

feat: respect x-* headers for redirections #9054

merged 5 commits into from
Oct 9, 2024

Conversation

kurokobo
Copy link
Contributor

@kurokobo kurokobo commented Oct 7, 2024

Checklist:

Important

Please review the checklist below before submitting your pull request.

  • Please open an issue before creating a PR or link to an existing issue
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I ran dev/reformat(backend) and cd web && npx lint-staged(frontend) to appease the lint gods

Description

To adjusts WSGI to respect X-* headers of redirected HTTP requests, apply Proxy Fix middlewares. Refer to: https://werkzeug.palletsprojects.com/en/3.0.x/middleware/proxy_fix/

Fixes #9052

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update, included: Dify Document
  • Improvement, including but not limited to code refactoring, performance optimization, and UI/UX improvement
  • Dependency upgrade

Testing Instructions

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Deploy API with this PR with HTTPS enabled configuration, then send HTTP request to curl -vL https://<FQCN>/v1 (HTTPS endpoint)

    $ curl -kv https://<FQCN>/v1
...
< HTTP/1.1 308 PERMANENT REDIRECT
...
< Location: https://<FQCN>/v1/   ✅ HTTPS
...
<!doctype html>
<html lang=en>
<title>Redirecting...</title>
<h1>Redirecting...</h1>
<p>You should be redirected automatically to the target URL: <a href="https://<FQCN>/v1/">https://<FQCN>/v1/</a>. If not, click the link.
                                                                      👆✅ HTTPS          👆✅ HTTPS
* Connection #0 to host <FQCN> left intact

  • Deploy API with this PR with HTTPS disabled configuration, then send HTTP request to curl -vL http://<FQCN>/v1 (HTTP endpoint)

    $ curl -kvL http://<FQCN>/v1
...
< HTTP/1.1 308 PERMANENT REDIRECT
...
< Location: http://<FQCN>/v1/   ✅ HTTP
...

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. 🐞 bug Something isn't working labels Oct 7, 2024
bowenliang123
bowenliang123 suggested changes Oct 7, 2024
View reviewed changes
Copy link
Contributor

@bowenliang123 bowenliang123 left a comment

Choose a reason for hiding this comment

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

Interesting to have ProxyFix middleware.
It's suggested to add this feature as an optional one with a switch config, instead of enforcing overwriting by default. It could be natively semantic to distinguish the original headers for most cases, especially for the detailed trouble shooting.

@kurokobo
Copy link
Contributor Author

kurokobo commented Oct 7, 2024

@bowenliang123
Thanks for your comment! I will update this PR to have a flags to enable Proxy Fix 😃

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Oct 7, 2024
@kurokobo
Copy link
Contributor Author

kurokobo commented Oct 7, 2024

@bowenliang123
Updated. Please let me know if there are any issues with where to add the new flag and code or with the explanation, thanks! 😃

@crazywoola crazywoola requested a review from laipz8200 October 8, 2024 00:25
bowenliang123
bowenliang123 approved these changes Oct 8, 2024
View reviewed changes
Copy link
Contributor

@bowenliang123 bowenliang123 left a comment

Choose a reason for hiding this comment

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

LGTM overall. Some comments to follow.
And the prefix of this PR should be feat:.
cc @laipz8200

docker/.env.example Outdated Show resolved Hide resolved
api/configs/feature/__init__.py Outdated Show resolved Hide resolved
api/configs/feature/__init__.py Outdated Show resolved Hide resolved
docker/docker-compose.yaml Outdated Show resolved Hide resolved
laipz8200
laipz8200 requested changes Oct 8, 2024
View reviewed changes
api/app.py Outdated Show resolved Hide resolved
@laipz8200 laipz8200 changed the title fix: respect x-* headers for redirections feat: respect x-* headers for redirections Oct 8, 2024
bowenliang123
bowenliang123 suggested changes Oct 8, 2024
View reviewed changes
api/app.py Outdated Show resolved Hide resolved
@kurokobo
Copy link
Contributor Author

kurokobo commented Oct 8, 2024

@bowenliang123 @laipz8200
Hi, thanks for your rapid and detailed review! I've updated my branch. Could you please check this again? Thanks!

@kurokobo
Copy link
Contributor Author

kurokobo commented Oct 9, 2024

Updated the branch to follow the changes on #9089

bowenliang123
bowenliang123 approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@bowenliang123 bowenliang123 left a comment

Choose a reason for hiding this comment

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

LGTM.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 9, 2024
@laipz8200 laipz8200 merged commit c0b71f8 into langgenius:main Oct 9, 2024
6 checks passed
@kurokobo kurokobo deleted the redirect branch October 9, 2024 10:58
lau-td pushed a commit to heydevs-io/dify that referenced this pull request Oct 23, 2024
@kurokobo @lau-td
feat: respect x-* headers for redirections (langgenius#9054)
546c77b
idonotknow pushed a commit to AceDataCloud/Dify that referenced this pull request Nov 16, 2024
@kurokobo @JunXu01
feat: respect x-* headers for redirections (langgenius#9054)
b457b10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowenliang123 bowenliang123 bowenliang123 approved these changes

@crazywoola crazywoola crazywoola approved these changes

@laipz8200 laipz8200 laipz8200 approved these changes

Assignees
No one assigned
Labels
🐞 bug Something isn't working lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

https://<FQDN>/v1 redirects users to HTTP instead of HTTPS
4 participants
@kurokobo @bowenliang123 @laipz8200 @crazywoola