Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

RequireHttpsAttribute needs some devmode love #4093

Closed
thewyzard44 opened this issue Feb 14, 2016 · 4 comments
Closed

RequireHttpsAttribute needs some devmode love #4093

thewyzard44 opened this issue Feb 14, 2016 · 4 comments
Assignees
Milestone

Comments

@thewyzard44
Copy link

If you could amend the Constructor

public RequireHttpsAttribute(int devPort = 443, int stagingPort = 443, int productionPort = 443)

and the main body to something more like:

#if DEBUG
            var thisPort = DevPort;
#else
#if STAGING
            var thisPort = StagingPort;
#else
            var thisPort = ProductionPort;
#endif
#endif
            var oldport = ":";
            var newport = "";
            var request = filterContext.HttpContext.Request;
            if (request.Host.Value.Contains(':'))
                oldport = ":" + request.Host.Value.Split(':')[1];
            if (thisPort != 443)
                newport = ":" + thisPort.ToString();
            var newUrl = string.Concat(
                "https://",
                request.Host.ToUriComponent().Replace(oldport, "") + newport,
                request.PathBase.ToUriComponent(),
                request.Path.ToUriComponent(),
                request.QueryString.ToUriComponent());
            // redirect to HTTPS version of page
            filterContext.Result = new RedirectResult(newUrl, permanent: true);

or something Similar, then we could:
[RequireHttps(devPort: 44391)]
public class SomeController {...}

And that would make our dev lives a little easier! ;) I'm not crazy about this in regards to separation of concerns... but the alternative of putting that #if block on every single controller isn't much prettier.

Thank you for your consideration

EDIT: For now I've implemented this as a CustomRequireHttpsAttribute, and perhaps that's the best answer for those of us who woudn't mind mixing in environmental details with our code. Maintainability is about the same with both approaches but this would lead to easier reading. Or perhaps there's a better idea out there? This scenario is not for redirecting an entire site to https (where we could set this up in the middleware), but instead for a large selection of controllers that actually deal in secure data, but also where the site does generate enough public traffic that saving some encryption overhead is beneficial when there's no sensitive data being passed.

At minimum the current implementation does not address custom ports. If the request is http://something:8080 then it will redirect to https://something:8080 which won't work (2 protocols on the same port).

Staging is superfluous, in this example, but having the ability to address custom ports in some way, without the environmental details may be another solution to the dilemma. 99% of the time, I'm sure production will be 443, and staging could be manipulated. And that leaves dev, which doesn't always have the option of binding 443, let alone a simple port 80, so local testing won't be possible otherwise.

@Eilon
Copy link
Member

Eilon commented Feb 18, 2016

@sebastienros and I discussed a rough outline of a design for this:

  1. Have an int? MvcOptions.SslPort { get; set; } option
  2. Default value is null
  3. A user could use configuration/options to create a per-environment value (e.g. Development uses SslPort=12345, Production = null (i.e. default)), etc.

@thewyzard44
Copy link
Author

Home Run!! Thank you very much ;) In my rush to slap together a solution, that evening, I didn't even notice that context parameter coming in @Eilon's past fix. Much more elegant, and even better with taking it off of the constructor and putting it into the config.

@Eilon
Copy link
Member

Eilon commented Feb 18, 2016

@thewyzard44 good teamwork 😄

sebastienros added a commit that referenced this issue Feb 19, 2016
New optional MvcOptions.SslPort. If not defined the redirection uses an empty port (default),
otherwise the custom port is used.
sebastienros added a commit that referenced this issue Feb 20, 2016
New optional MvcOptions.SslPort. If not defined the redirection uses an empty port (default),
otherwise the custom port is used.
sebastienros added a commit that referenced this issue Feb 22, 2016
New optional MvcOptions.SslPort. If not defined the redirection uses an empty port (default),
otherwise the custom port is used.
sebastienros added a commit that referenced this issue Feb 23, 2016
New optional MvcOptions.SslPort. If not defined the redirection uses an empty port (default),
otherwise the custom port is used.
sebastienros added a commit that referenced this issue Feb 23, 2016
New optional MvcOptions.SslPort. If not defined the redirection uses an empty port (default),
otherwise the custom port is used.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants