statgen / pheweb

A tool to build a website to browse hundreds or thousands of GWAS.
MIT License
154 stars 65 forks source link

url_for in random does not allow https #199

Closed CaliWee closed 1 year ago

CaliWee commented 1 year ago

Running PheWeb 1.1.14

I am running service a web server with the following command:pheweb conf cache=cache urlprefix='https://' serve --port 8080 My server currently does not allow http access. When I direct/click to /random the url returned does not specify an https protocol preventing any data from being returned.

Other than the parameters in the command above I am using the default configuration after installing from pip aside from the changes.

I think a fix could be to have a flag in conf.py like: requirehttps when set to true, args are provided to url_for from this issue

pjvandehaar commented 1 year ago

Does urlprefix work like that? I built it for urlprefix=“/ukb/“.

That’s a great idea, and I’m happy to merge a pull request.

On Wed, Aug 24, 2022 at 2:18 PM CaliWee @.***> wrote:

Running PheWeb 1.1.14

I am running service a web server with the following command:pheweb conf cache=cache urlprefix='https://' serve --port 8080 My server currently does not allow http access. When I direct/click to /random the url returned does not specify an https protocol preventing any data from being returned.

Other than the parameters in the command above I am using the default configuration after installing from pip aside from the changes.

I think a fix could be to have a flag in conf.py like: requirehttps when set to true, args are provided to url_for https://github.com/statgen/pheweb/blob/ac95b154b9b756b3dcf89fa65825102465257cfb/pheweb/serve/server_utils.py#L95-L100 from this issue https://stackoverflow.com/questions/14810795/flask-url-for-generating-http-url-instead-of-https

— Reply to this email directly, view it on GitHub https://github.com/statgen/pheweb/issues/199, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGSPCPA4PAWZ243453CMPTV2ZRQDANCNFSM57QJS7OQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

CaliWee commented 1 year ago

You're right. urlprefix does not work like that. But I would be happy to make a PR for that new flag. I should get time in the next few weeks. I'll make a ticket for myself

pjvandehaar commented 1 year ago

Okay, I'm surprised by the behavior you're reporting. Can you walk me through what's happening? Here's what I assume will happen:

  1. You run pheweb serve --port=8080.
  2. You run Nginx/Apache to serve "https://pheweb.example.com/" (port 443) by proxying to localhost:8080.
  3. You go to "https://pheweb.example.com/random", and it redirects you to "/pheno/728". That is, it returns "301 REDIRECT" with the header "Location: /pheno/738". That happens because get_random_page() returns "/pheno/738", which is a non-external URL because this function is being called inside of an "active request context".
  4. Your browser sends you to "https://pheweb.example.com/pheno/738" because it's a relative redirect.

Are you claiming that in this setup, you're redirected away from https to http? Could you share the console output of pheweb serve as this happens, and maybe also your apache/nginx logs?

Or are you using a setup that's different from what I described?

What version of pheweb and flask are you using?

CaliWee commented 1 year ago

This is the setup that I am working with. It was surprising for me too as this is the only link that seems to redirect away from https to http.

Keep in mind, I am running a version of PheWeb that is 3 years old. Due to the requirements of the project / the data format I am unable to update at this moment.

pheweb==1.1.14 flask==1.0.3 # via flask-compress, flask-login, pheweb

pjvandehaar commented 1 year ago

Oh. What happens if you add this line to server.py:

app.config['PREFERRED_URL_SCHEME'] = 'https'
pjvandehaar commented 1 year ago

If that doesn't work, it sounds like you should just edit the source code to use url_for(..., _external=True, _scheme="https") or whatever. I think a pull request wouldn't make sense because it's unlikely that the current pheweb has the same error.

CaliWee commented 1 year ago

Makes sense. I'll give those a try. Thanks!

CaliWee commented 1 year ago

After adding url_for(..., _external=True, _scheme="https") to all of the url_for calls in get_random_page and specifying proxy_set_header Host $http_host; before proxy_pass in my nginx.conf this issue was solved for me. Like you said, it's probably not an issue in the more recent versions and hopefully I can convince stakeholders to upgrade soon. Thanks!

pjvandehaar commented 1 year ago

I’m glad it worked for you.