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

change "dnn" to "app" in dnn_IsMobile cookie #4045

Closed
wants to merge 1 commit into from

Conversation

thabaum
Copy link
Contributor

@thabaum thabaum commented Sep 3, 2020

Fixes #4044

Summary

Small fix to help hide the app being used for the site by changing the cookie name used from dnn_IsMobile and dnn_NoMobile to app_IsMobile and app_NoMobile in FriendlyUrlController.cs file.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

I am all for this change, the only thing is in which version?

Although I would never use this cookie in my modules, are there 3rd party modules consuming this?
Do we just get this in 9.7.2 with a note in the release or do we hold it for 9.8.0 or 10 ?

@mitchelsellers
Copy link
Contributor

Paging @bdukes on this one.

Not an issue to me. But could be breaking

@thabaum
Copy link
Contributor Author

thabaum commented Sep 3, 2020

A good point is only if some other module was using this same cookie name.

I am also suggesting changing .DOTNETNUKE in the web.config to something less revealing out of the box as well. I figured that since it is easier to modify that manually in web.config not as big of an issue as a hard coded cookie.

@bdukes is suggesting possibly putting this cookie in the web.config as well to allow editing it.

@thabaum
Copy link
Contributor Author

thabaum commented Sep 3, 2020

Folks do have configurations and documentation making use of that name, so just changing the name would be a breaking change for some people. Making the name configurable would not be a breaking change, and it much more likely to get accepted. Hope it helps!

@mitchelsellers @valadas

I know the PR was accepted for changing the name however @bdukes suggesting that this would break configurations and documentation in the comment from the issue #4043 #4044 .

I suppose the change to making it configurable in the web config file is the solution near term.

I am guessing that if this fix is used as is it would need to be a v10 PR due to some folks having configurations and documentation that rely on the current cookie name?

I will dig into making the web.config change, never putting in a PR that adjusts the web.config in the platform this would be new waters for me to get into.

@thabaum
Copy link
Contributor Author

thabaum commented Sep 3, 2020

If I add a new key to the web.config would I simply add it to the project here and the upgrade script will detect the change and add it to the update package?

I can create a new PR to make this editable and less of a "breaking change" I don't want to break anything if possible, sounds like this would be best solution so I will make a new PR I just need a little help knowing how to make what would work for everyone.

@thabaum
Copy link
Contributor Author

thabaum commented Sep 3, 2020

since this is a PR with a potential to be approved using web config changes I just need a few blanks filled in I have put my concerns in issue #4044 regarding some of the code and where exactly the web.config changes should be added.

I will create a new PR with the other more acceptable solution. I will get my testing environment in shape, this will be a good one for me to do this with...

Thank you!

@SkyeHoefling
Copy link
Contributor

I think adding this to the web.config is a great start. Thanks @thabaum for all the hard work on this.

Is this something that would make more sense as an Admin Module in the Persona Bar? Since the cookie is ultimately controlled server side I would argue it should go in the PortalSettings or the HostSettings in the database and not the web.config. Then we could expose the property on a DI ready interface to easily access the current cookie name.

Questions

  • What is the reason for adding this to the web.config? It is easy for developers but might not be easy for integrators
  • Is there a compelling argument to make this a host setting where it effects the entire website vs a portal setting.

@bdukes
Copy link
Contributor

bdukes commented Sep 4, 2020

@ahoefling the web.config makes sense to me as a place to set it only because the auth cookie, .DOTNETNUKE is also configured in the web.config. If we're going to use settings, portal settings makes more sense to me than host settings.

@thabaum
Copy link
Contributor Author

thabaum commented Sep 4, 2020

I agree with putting this into the database if that sounds like a better solution. I like the idea of Host and Portal similar to SMTP settings, a configuration for each portal that defaults to host until you change it.

I would like to put the auth cookie in the persona bar as well. Does it make sense and is it doable to make the .DOTNETNUKE cookie per portal instead of host and have this in the database as well?

If we can put both in the database and makes sense I think there could be a "Default" Host setting and a way to override it in Portal Settings for both cookies. I just dont know if it has to be in web.config or if it is ok to put those inside the database with host/portal settings.

@mitchelsellers
Copy link
Contributor

Changing the value should really be exceptions not standards. Using the web config to control the cookies, and cookie domains and similar things is the standard that applies to everything else.

I don’t think we want to make it very easy to change the cookies per portal. As doing so could really break multi test site functionality and other things.

Potentially allowing or documenting a process to change away from disclosing the platform would be nice. But from a security perspective it doesn’t really matter as we already have JavaScript files that tell you

@bdukes
Copy link
Contributor

bdukes commented Sep 4, 2020

There doesn't appear to be a way to change the name of the forms auth cookie outside of the web.config

@thabaum
Copy link
Contributor Author

thabaum commented Sep 4, 2020

Potentially allowing or documenting a process to change away from disclosing the platform would be nice. But from a security perspective it doesn’t really matter as we already have JavaScript files that tell you

@mitchelsellers it is about "Branding" as well as being able to help keep the platform from disclosing what it is. It would be nice to be able to have documentation on how to do this. As you mentioned the Javascript files, I think these should be changed as well to more generic terms if possible in future releases. Anything to help keep a brand, trade secrets so to speak and system protected and secured.

I just wanted to be able to control the security messages in the browser so that dnn or dotnetnuke is not exposed anywhere. I know there are more places that show this and I would love to explore options to resolve those as well.

So for this issue with the mobile cookie only, what is the verdict?

  1. Should we create a persona bar DI feature to edit as @ahoefling is potentially suggesting
    or
  2. should we have it in the web.config as an option you can add to old configs as @bdukes suggests would work. I believe we already have a solution for the web.config I can put into a PR that should work for not creating any type of "Breaking Changes".

I would need to start a new thought process for the personabar but I am willing to do what makes sense long term so it is not something that needs to be revisited again at a later time.

I am willing to go either way just point the direction :)

@bdukes
Copy link
Contributor

bdukes commented Sep 4, 2020

My vote is to configure via web.config, I don't see the need to a bigger feature.

@bdukes bdukes added this to the 9.7.2 milestone Sep 4, 2020
@mitchelsellers
Copy link
Contributor

I would second web.config.

If there is a greater desire to renminbi Dnn or DotNetNuke from rendering I’m not sure I’m fully on board with that due to the massive breaking change nature Of it and the limited value, of any, that it provides from a disclosure perspective

@thabaum
Copy link
Contributor Author

thabaum commented Sep 4, 2020

Sounds good looks like enough community support for this being added as a web.config configuration rather than hard coded so I will get a PR together tonight. I am all for whatever is "long term" solution.

I understand the breaking changes issues as I don't want to ruffle any feathers in this arena, however, if there is ever a chance to change the files that expose dnn or dotnetnuke in any way such as those using JavaScript to replace an older version of a file that can remove Dnn or DotNetNuke I 100% support these changes and finding solutions that would make it happen now or down the road. This will just help eliminate one of those issues along with adding a way to customize the platform further without changing the code for those using the platform and desire more custom branded or generic cookie names to be exposed.

Thank you all for the feedback and support helping me with getting an acceptable solution.

@david-poindexter
Copy link
Contributor

@thabaum thanks so much for this PR - great change! We are putting this PR on hold until the other PR comes in relating to the web.config change.

@thabaum
Copy link
Contributor Author

thabaum commented Sep 8, 2020

here is a link to the other PR #4064

please test it was some new waters for me to explore here in the platform. I am sure the comments need set as desired so please maintainers can make changes as needed.

Thank you!

@thabaum
Copy link
Contributor Author

thabaum commented Sep 8, 2020

I am going to close this now in favor for PR #4064

@thabaum thabaum closed this Sep 8, 2020
@thabaum thabaum deleted the patch-26 branch December 7, 2021 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cookie uses dnn_IsMobile with term "dnn"
6 participants