Got the idea from here: https://serverfault.com/a/1056120

Google Drive uses a non-standard 308, "Resume Incomplete"...
Yeah, maybe the solution is: "don't use http to POST stuff"
(Edit: ... maybe the internet was a big mistake... existential crisis)

what do you think we should do or workarounds?

301 for GET and 308 for everything else is at least better than current behavior...
Just hope that no one ever sets up a nginx proxy in front of the Google Drive API for some reason.

Currently:
non-GET = broken in all cases

With 308 for non-GET:
non-GET = broken in rare cases

I'm not yet 100% comfortable redirecting everything (GET included) with 308 instead of 301.

Nevertheless, SEO wise 308 should be treated exactly like 301 by Google.

@junderw could you rework this PR to target / be based on main instead ?

Maybe the 301 / 308 redirect behavior should be documented ?

• Base on main
• Added documentation
• Changed PR target to main

Added tests for all available methods on the nginxproxy object.

@junderw can you link issue #1738? So that ill close automatically

Ok, done.

This feature should not be enabled by default. This just hides the underlying problem that the request uri is plain http. The website should generate proper https links instead.
The intial request still contains the (private) payload as well. This creates a false impression of security.

For example the user opens the login page which was loaded with https and thus secured. After entering the credentials the login button create a post request with the http request uri. The current behavior would create errors and thus alerts the admin of this problem. This PR will just redirect it properly and thus hide the problem. Any further request still can be unencrypted and thus leak any private data. Any malicious attacker can intercept the traffic without the user noticing at all.

TLDR: the http to https redirect should only be intended to redirect the initial unencrypted request as it currently is.

This feature should not be enabled by default. This just hides the underlying problem that the request uri is plain http. The website should generate proper https links instead.
The intial request still contains the (private) payload as well. This creates a false impression of security.

For example the user opens the login page which was loaded with https and thus secured. After entering the credentials the login button create a post request with the http request uri. The current behavior would create errors and thus alerts the admin of this problem. This PR will just redirect it properly and thus hide the problem. Any further request still can be unencrypted and thus leak any private data. Any malicious attacker can intercept the traffic without the user noticing at all.

TLDR: the http to https redirect should only be intended to redirect the initial unencrypted request as it currently is.

I don't think that's our problem but rather the problem of whoever consumes the api to always use https. Nevertheless the current behavior is the same where the initial request will use http, so I think this concern has nothing to do with this pr as this does not change the current default behavior.

I don't think that's our problem but rather the problem of whoever consumes the api to always use https. Nevertheless the current behavior is the same where the initial request will use http, so I think this concern has nothing to do with this pr as this does not change the current default behavior.

@OliverRhyme this changes the current default behavior such that the admin will most likely never notice the problem.

The user making the intial request to the website is another problem. But this PR hides the fact that for example the login button makes a plain http request.

If the admin is comfortable to enable this redirect feature, it should be able to be activated, like the no-redirect behavior of http requests.

I don't think that's our problem but rather the problem of whoever consumes the api to always use https. Nevertheless the current behavior is the same where the initial request will use http, so I think this concern has nothing to do with this pr as this does not change the current default behavior.

@OliverRhyme this changes the current default behavior such that the admin will most likely never notice the problem.

The user making the intial request to the website is another problem. But this PR hides the fact that for example the login button makes a plain http request.

If the admin is comfortable to enable this redirect feature, it should be able to be activated, like the no-redirect behavior of http requests.

Correct me if I'm wrong but is the default behavior of nginx-proxy when getting http request is to redirect it to the https? So currently it still does hide the fact that for example the login button will still send plain http request? 301 is basically the same with 308 with the caveat that it preserves the http method used

such that the admin will most likely never notice the problem.

This assumes that private data is not sent as query parameters as a GET request.
Any form that sends GET requests with private data will have the same problem.

Your argument is actually supporting for nohttp as default and only supporting https.

Relying on a quirk of client implementations (301 response being redirected with GET always is a client implementation detail) for security is never a good idea.

If we want to prevent insecure things from happening, we should disable them by default on the server side.

If I explicitly set "redirect http to https" I am not saying "only redirect GET requests please."

So I agree that current behavior can save some people in some rare cases, we are picking and choosing who is saved and who is not, and relying on clients to interpret these errors and pick up on why they're bad. (ie. an admin might pick up on the POST failure and solve it by noticing that GET goes through fine, and changing the login form into a GET request.)

The expected behavior is "redirect all http to https" and this PR brings the feature in line with that expectation.

I think a different discussion could be made to reject all http by default. (besides the ACME wellknown path)

I think maybe another solution could be to reject all methods besides GET for http.

This way at least the admin will be more likely to arrive to a conclusion of "oh, maybe I'm not supposed to use POST with http"

Though they might still stumble upon GET still working, and adding method="get" to their login form to get around it...

I agree with creating another issue to enable disabling redirect http to https all together. I also believe that we can at least alleviate "such that the admin will most likely never notice the problem." by having a strongly worded documentation on what will happen and how to fix or workaround it.

tbh though... even if we reject all http requests. They'll still be sending the packets over the clear to the server anyways... so really, I don't think there's much we can do to help people here...

I'd be open to explicitly rejecting non-GET as well rather than sending 301 and having a hard-to-understand error. A rejection is easier to understand... why isn't it there? *looks at URL* OH I forgot https!

as compared to "why is it sending GET when I sent POST? hmmm... *googles "login form get request"* *gets results of "how to set form to get method"* *tries it* *works*

tbh though... even if we reject all http requests. They'll still be sending the packets over the clear to the server anyways... so really, I don't think there's much we can do to help people here...

I'd be open to explicitly rejecting non-GET as well rather than sending 301 and having a hard-to-understand error. A rejection is easier to understand... why isn't it there? looks at URL OH I forgot https!

as compared to "why is it sending GET when I sent POST? hmmm... googles "login form get request" gets results of "how to set form to get method" tries it works

But then, this becomes opinionated and maybe we should instead provide them an option.

Yes, entirely disabling the http part will add more security, like closing port 80. But depending on the browser a single 301 redirect is necessary to have all further requests made using https. Some just try https is http is not working.

For example just entering github.com will first create a plain http get request which is answered with 301 https://github.com/. As long as all (internal) links are https, no http request will be made.

I agree with creating another issue to enable disabling redirect http to https all together.

@OliverRhyme this feature is already in nginx-proxy

To serve traffic in both SSL and non-SSL modes without redirecting to SSL, you can include the environment variable HTTPS_METHOD=noredirect (the default is HTTPS_METHOD=redirect). You can also disable the non-SSL site entirely with HTTPS_METHOD=nohttp, or disable the HTTPS site with HTTPS_METHOD=nohttps. HTTPS_METHOD can be specified on each container for which you want to override the default behavior or on the proxy container to set it globally. If HTTPS_METHOD=noredirect is used, Strict Transport Security (HSTS) is disabled to prevent HTTPS users from being redirected by the client. If you cannot get to the HTTP site after changing this setting, your browser has probably cached the HSTS policy and is automatically redirecting you back to HTTPS. You will need to clear your browser's HSTS cache or use an incognito window / different browser.

By the way maybe this PR should be a new HTTPS_METHOD option, e.g redirect308

By the way maybe this PR should be a new HTTPS_METHOD option, e.g redirect308

and make it default i guess as 308 and 307 should replace 301 and 302 as per RFC docs? maybe also redirect307 as to avoid conflict with different services that dosent use the 308 as intended

The paths I'm seeing are:

1. Add 308 for non-GET. no options added.
2. Add 308 for non-GET. option to disable (making non-GET reject completely (404? 400?)).
3. Make non-GET reject completely. add option to enable with 308.

Maybe 3 is best.

1. redirect default will be 301 for GET and 400 for non-GET.
2. redirectallmethods will be 308 for all methods (since it won't be default, might as well use 308 for all)

One major PITA is the fact that nginx doesn't support variables for response codes, so the more branches we add, the more if {} statements we need, since we can't use map or anything. Otherwise it would be easier to just have a var for REDIRECT_CODE etc.

At the moment I'm not sold on rejecting non GET and not 100% sure about changing the default behavior with ifs in location blocks (something notoriously discouraged by nginx, even if we're in one of the two supposedly 100% safe cases) for an issue that I don't recall seeing here before. Adding the 308 for non GET as a new non default option would be fine.

I'm not saying this is final, I'll have to think a bit more about this one. Any example of if ($request_method = WHATEVER) { in location blocks being used in production without issue would be welcome.

It seems there's a limitation on the return code in nginx, so you can't use a variable as the return code.

Therefore, the only way to change the return code based on some dynamic data is to split the logic using some other way.

The nginx documentation does state that if statements are safe if they return or rewrite because the only way if statements can get you is if you have a bunch of if statements and expect them all to run.

ie.

if ($request_method = GET) {
    set_header X Y
}
if ($scheme = http) {
  set_header A B
}

In this case, if you are expecting "when it is GET and http, header X and A should both be set." this is wrong. Because if {} is wonky.

However, if every if {} statement conditions can not possibly overlap, and each if {} ends in return/rewrite, it is safe.

If we could use variables on the return code it would be easy to use map {} to just map the request method to a code.

for an issue that I don't recall seeing here before

I'm totally fine with a "Won't fix" here. This really feels like "I shot myself in the foot, and it made a weird sound" and we're arguing whether or not to fix the weird sound because the weird sound might alert them to shooting themselves in the foot.

Maybe the answer is: "Don't try to shoot yourself in the foot." if this was a wide spread problem, you're right, someone would have mentioned the issue by now, since this is an issue from the beginning of the project.

Back to this PR, has anyone used it in production so far ?

@junderw sorry for letting this sit in the dust for so long, are you still interested in getting it merged ?

Sure!

I actually stopped using the thing that was using nginx-proxy last year when I had to take down my home lab for various reasons, but I'm still alive and well and willing to push this through. I think it will be helpful.

But at the same time, my motivation behind creating this is gone.

If you don't feel like merging it, I'm fine with a "Won't fix" or "Not a bug" decision here.

If you don't feel like merging it, I'm fine with a "Won't fix" or "Not a bug" decision here.

No, it actually did make sense to me and a few people manifested interest for it, I was rather deterred by the very opinionated discussion about what should or should not be done because of security implications.

I think I got the feature where I wanted it ~3 years ago :

• optional and disabled by default (as suggested by @AlexanderLieret)
• uncoupled from HTTPS_METHOD
• with choice between 307 and 308 redirect (AWS use a 307 for the same purpose on Cloudfront)
• with the HEAD method using the same redirect as GET (again, what AWS does on Cloudfront in this case)

LGTM so far