snipe / snipe-it

A free open source IT asset/license management system
https://snipeitapp.com
GNU Affero General Public License v3.0
11.11k stars 3.18k forks source link

git pull overwrites your web.config on IIS #9275

Open MrCaspan opened 3 years ago

MrCaspan commented 3 years ago

Describe the bug When running upgrade.php or doing a 'git pull' a copy of web.config from the repo overwrites your production web.config

To Reproduce Modify existing web.config on your machine to add some special beacon you can check for later run php upgrade.php or do a git pull Look at web.config and see your beacon is now gone and file has been overwrite with repo version

Server (please complete the following information):

I removed the rest of the stuff as its not needed for this issue. The issue is the git pull overwrites the web.config and is not connected to and browser or desktop environment in any way.

My recommendation would be:

Rename web.config to web.config.example so that it when doing a 'git pull' it cannot overwrite your production web.config with an example default one during an upgrade.

Documentation should be added for IIS users to rename this file upon first install and on upgrades compare the production one to the example one and make any appropriate changes to your production web.config if needed.

welcome[bot] commented 3 years ago

πŸ‘‹ Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

snipe commented 3 years ago

That's expected behavior though, as it's checked into the repo, and anything checked into the repo is going to overwrite what you have locally, the same way any changes a user makes to their .htaccess on a linux machine would overwrite their local changes. It's not really intended to be changed.

yosiasz commented 3 years ago

may I ask why it is checked into the repo? can it be provided as webconfig.example like .env file?

snipe commented 3 years ago

@yosiasz because most people would forget to create it or don't know how, and it's a pretty bog-standard file that shouldn't really need to be changed often (like htaccess).

MrCaspan commented 3 years ago

In IIS that web.config file is like the apache .conf file for a site and .htaccess all in one. I get that the stuff needs to be in there and its great Snipe-IT provides this. Some machines might have certain setting in there that are specific to your server and you will loose them on a get pull as it will get over written. Imagine the same scenario and the .env file gets overwritten with a default one, same basic reasoning here if you understand.

snipe commented 3 years ago

Honestly, a default apache config would probably be fine on 99% of systems. In 7+ years, this is the first time the web.config issue has come up, and I know we have a ton of IIS users (and customers). IIS users tend to be the ones that require the most support, so even adding this one additional step of having them copy over an example config seems like it's going to cause a lot of problems, but I'll consider it.

yosiasz commented 3 years ago

if not, no worries, I have workaround. copy out the webconfig file before any upgrades

snipe commented 3 years ago

The upgrade also does a git stash before a git pull so your changes are probably in the stash that happened right before the git pull.

MrCaspan commented 3 years ago

NP as long as you understand the issue, it's your software to make the big decisions like this :) We were just chatting on Gitter about the issue thought we would let the powers that be know.

snipe commented 3 years ago

@MrCaspan Yeah, @uberbrady snitched on you ;-)

I don't really have super strong feelings about it tbh - it's just that now that people have been using this software, some for the entire 7 years, we have to be careful that any changes we make don't screw up their installs. Just have to think it through so that everybody gets the best outcome.

MrCaspan commented 3 years ago

completely understand!

snipe commented 3 years ago

I could also potentially add the copying of the web.config.example into the composer process. We can do things like:


"scripts": {
        "post-root-package-install": [
            "@php -r \"file_exists('.env') || copy('.env.example', '.env');\""
        ],
        "post-create-project-cmd": [
            "@php artisan key:generate"
        ],
        "post-autoload-dump": [
            "Illuminate\\Foundation\\ComposerScripts::postAutoloadDump",
            "@php artisan package:discover"
        ],
      "post-update-cmd": [
        "Illuminate\\Foundation\\ComposerScripts::postUpdate",
        "php artisan ide-helper:generate",
        "php artisan ide-helper:meta"
        ]
    },

So potentially, we could ease the friction for new IIS users by automagically copying over that web config when they run the composer install, but only if it doesn't exist.

Something like:

"post-install-cmd": [
            "@php -r \"file_exists('web.config') || copy('web.config.example', 'web.config');\""
],

My only concern there is that if people manually deleted it, it will remake it every time. "Why would someone delete that file? If you're on Linux, it doesn't do anything, and if you're on IIS, you need it!" one might ask. But we've had security compliance teams request we delete it for "information disclosure", even though all of our customers are hosted on our Linux systems, and it's a bog standard file with no actual information disclosure in it. (It's literally the one in our source, which is public, which comes from the Laravel source, which is also public. Nothing customer-specific in there.)

yosiasz commented 3 years ago

can it check operating system and create or not create accordingly?

snipe commented 3 years ago

@yosiasz Kinda....? But it's pretty gross: https://stackoverflow.com/questions/38895739/composer-script-section-os-dependent

(Unless something has changed in Composer 2 - and we don't even require Composer 2.)

Stack Overflow
composer script section os.dependent
Is it possible to check for the current operating system? The following cmd fails on windows, because chmod does not exist { ... "scripts": { "post-update-cmd": "chmod -R 777 ../log" } } Is
yosiasz commented 3 years ago

I think I agree with you, not worth the effort. Maybe just a blurb in the documentation saying "if you have any custom configurations copy it out and restore it" => gross also

snipe commented 3 years ago

Yeah, I hate it (especially since nobody reads the docs), but I can't think of an easier way.

Since we do a git stash, we could potentially grep through the stash(es) and re-copy the modified web.config, but that path seems fraught with peril. What if they didn't actually want those changes? Now they think their install is haunted because the old changes keep coming back.

Ugh.

It's always the small things that are the hardest, especially on "old" projects.

snipe commented 3 years ago

I can't even use in the built-in Laravel console commands to help us here, since the upgrade script is written in vanilla PHP, not Laravel. (We do that on purpose, in case the upgrade script needs to be used as a recovery tool to reset whatever weird stuff somebody did that made Laravel break.) If we could, we could have a nice interactive dialog that could copy out the existing web config and then copy it back.

Or maybe that's the simplest solution. In upgrade.php, we can make a temp version of the web.config, do the upgrade, and then copy it back - weird Windows permissions problems notwithstanding. I'd hope that if you know enough Windows IIS administration to be dicking around with the web.config, you probably know enough to make the permissions work.

Dunno - let me chew on this some more - and thanks for letting me spitball this here. I haven't used windows consistently for literally well over a decade, so while I try hard to accommodate IIS users, I don't always nail it perfectly for every IIS use case.

yosiasz commented 3 years ago

it's nice to try to accommodate IIS users but at end of the day, the onus should be on the IIS server Admin. It's not snipeit's responsibility

snipe commented 3 years ago

@yosiasz tell that to my angry users who don't know how to run a web server lol

yosiasz commented 3 years ago

if someone gets angry using a free tool.....

yosiasz commented 3 years ago

one good test is to see if they use IUSR πŸ˜„

snipe commented 3 years ago

if someone gets angry using a free tool.....

Do NOT even get me started.

one good test is to see if they use IUSR

We have a couple of ways to check for Windows, and we even do that in the upgrade script:

if ((strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') || (!function_exists('posix_getpwuid'))) {
    echo "Skipping user check as it is not supported on Windows or Posix is not installed on this server. \n";
} else {
    $pwu_data = posix_getpwuid(posix_geteuid());
    $username = $pwu_data['name'];

    if (($username=='root') || ($username=='admin')) {
        die("\nERROR: This script should not be run as root/admin. Exiting.\n\n");
    }
}

So that might be helpful in figuring out how to best handle this.

yosiasz commented 3 years ago

It is not uncommon to edit the webconfig. Usually done via IIS. Doable via notepad++