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

Issue 097 - Fix https download link showing as http text behind nginx reverse proxy #100

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

andymarden
Copy link
Contributor

Fixed the display text of the links created - see #97

I think I have created the branch andy (which I should have given a better name) off develop, made changes just for #97. Let me know how I did!

I have followed the same syntax conventions as the rest of the code, tested it and it all seems good.

Not sure what else is outstanding on develop but this should be easy enough to pull into the master and release, if you are happy with it.

@epoupon
Copy link
Owner

epoupon commented Feb 1, 2024

Hello!
Thanks for this PR. I am afraid this breaks the case where no reverse proxy is used?

@andymarden
Copy link
Contributor Author

andymarden commented Feb 1, 2024

No - it sets http if the X-Forwarded-Proto scheme is not set.

Do I need to cover the case where https is used but no reverse proxy?

That will be easy enough to do- I will just slot back in the previous logic if this is not set.

I'll do that but have already started working on #98 in the same branch so will do PR for both (yes - I should have done separately!)

@epoupon
Copy link
Owner

epoupon commented Feb 1, 2024

You really should fire another branch for the other problem.
Smaller PR: smaller reviews, shorter times to merge, etc.

@andymarden
Copy link
Contributor Author

andymarden commented Feb 2, 2024

Yes - bear with me!

I have managed to untangle, the branch andy is for this issue only and I have updated it to put the original scheme function as the fallback if X-Forwarded-Proto is not set. Tested with and without nginx (but not https direct) - but it should be fine.

The next issue will come in its own branch (branched from develop again since there is no dependency - so nice and independent)

@epoupon
Copy link
Owner

epoupon commented Feb 3, 2024

Just to be sure, can you show your nginx config? I want to understand why it works on my setup and not on yours (and possibly report the problem to Wt)

@andymarden
Copy link
Contributor Author

Ok - my config is below:

# Fileshelter - dev
server {
    listen 443 ssl http2;

    server_name fileshare-dev.example.org;

    proxy_request_buffering off;
    proxy_buffering off;
    proxy_buffer_size 4k;
    client_max_body_size 10G;

    location / {
      #proxy_ssl_server_name on;
      proxy_set_header        Host $host;
      proxy_set_header        X-Real-IP $remote_addr;
      proxy_set_header        X-Forwarded-For $proxy_add_x_forwarded_for;
      proxy_set_header        X-Forwarded-Proto $scheme;

      proxy_pass http://dev.home:5091;

      proxy_read_timeout  120;
    }
}

Chagpt4 has this to say on the subject:

Default Behavior: By default, Wt might not consider the X-Forwarded-Proto header directly. It usually bases environment().urlScheme() on the protocol used between the client and Wt, which could be misleading if there’s a reverse proxy in between.

Configuration: You might need to configure both your reverse proxy and Wt to ensure that the original protocol is correctly passed and interpreted. This typically involves:

Setting the X-Forwarded-Proto header in your reverse proxy configuration. For example, in Nginx, you might add something like this to your configuration:
nginx

proxy_set_header X-Forwarded-Proto $scheme;

Ensuring Wt is configured to use this header to determine the URL scheme. This might involve additional settings or custom handling in your application code.
Custom Handling: If Wt doesn’t natively support X-Forwarded-Proto, you might need to implement custom logic to check this header. You can access HTTP headers through the environment object in Wt and then decide the scheme based on the value of X-Forwarded-Proto.
cpp

std::string scheme = environment().getHeader("X-Forwarded-Proto");
if (scheme.empty()) {
    scheme = environment().urlScheme(); // Fallback
}

@andymarden
Copy link
Contributor Author

I can see in the Wt4 releases stuff in the last couple of releases which mentions X-Forwarded-Proto. I wonder if you have an older version built at the time and the behaviour is different.

@andymarden andymarden changed the title Issue 97 Issue 97 - Fix https download link showing as http text behind nginx reverse proxy Feb 4, 2024
@andymarden andymarden changed the title Issue 97 - Fix https download link showing as http text behind nginx reverse proxy Issue 097 - Fix https download link showing as http text behind nginx reverse proxy Feb 4, 2024
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.

None yet

2 participants