On Windows, the vimfiles folder is owned by administrator upon install

[xsrvmy]

Steps to reproduce

1. Install VIM
2. Go to the vimfiles folder. Right click -> Properties -> Security -> Advanced. Note that the owner is Administrator

Note that this creates an error if one attempts to initialize a git repo inside vimfiles.

Expected behaviour

The vimfiles folder should be owned by the user. It is in the user folder after all.

Version of Vim

9.0

Environment

Windows 11

Logs and stack traces

No response

[brammool]

What method did you use to install Vim? If you downloaded an installer: from where?

[xsrvmy]

I've used three installers so far actually (to get python to work): the gvim90.exe download (https://ftp.nluug.nl/pub/vim/pc/gvim90.exe) and the 9.0.0000 32 bit and 64 bit installers from vim-win32-installer. Not sure if the last installer or the first installer caused the problem. I'm guessing something is going wrong with the "create plugin directory" option in the installer since it runs as admin.

[zewpo]

I just tried a fresh install, using an installer from vim-win32-installer, and can see that it created the vimfiles directory in my $home, owned by the "Administrators" group.

But, my user account still has full control.

And, so I'm able to do a git init in the new directory.

For you, is the owner a user account object named "Administrator", or is it the "Administrators" group object? I noticed that you wrote Administrator (singular), not Administrators (plural).
Is your user account a member of the Administrators group?

I noticed that my installation of the $home/vimfiles, simply inherited permissions from my $home directory. I wonder if you've maybe customised your $home folder security settings somehow to make subfolders inherit different permissions?

But, regardless, I think there is probably a way to change the installer to make it change the owner to the actual users account, I will investigate if that is possible.

[zewpo]

Update, after investigating what the installer code is doing, (at least what is in the vim-win32-installer project) I can't see anything obvious where its setting any security details.

It looks to me like the new directories are simply inheriting the security settings from their respective parent directory. And, I think this is the correct behavior - unless I'm missing something different with your setup!? I was unable to replicate the problem you described. I could do a git init on a fresh install of Windows 11 - even though the owner is the Administrators group. My user account still has Full Control permissions. Could you check if your user account doesn't have Full Control, and if the security settings aren't just being inherited from the user's home directory?

[xsrvmy]

It was the admins group for me as well.
Actually, IIRC git init was fine, but subsequently git status failed. There was a file in autoload at this point (from vim plug).

I'm not sure if inheriting home is the reason here. I would more suspect it has to do with the folder being created by the installer while elevated.

[zewpo]

ahh, yes, I see it now;

> cd vimfiles
> git status
fatal: detected dubious ownership in repository at 'C:/Users/chris/vimfiles'
'C:/Users/chris/vimfiles' is owned by:
        'S-1-5-32-544'
but the current user is:
        'S-1-5-21-XXXX-YYYYY-ZZZZ-9999'
To add an exception for this directory, call:

        git config --global --add safe.directory C:/Users/chris/vimfiles

It's an interesting issue. And, sure, it's easy enough to change security ownership back to your user account. Also, it's easy enough to add the git config as it suggests. But, I think this seems unnecessary to have to do this, and I think it would be more convenient if the $home/vimfiles directory was created with the users own account ownership.

When I was looking at the source code, for example see src/dosinst.c I got the impression it was intended to work with old dos and/or windows - meaning it's compatible with the FAT file systems, as well as NTFS. A solution might involve either going down the path of including adding some headers like this; <accctrl.h> <aclapi.h> to the c source code. Or, it might involve adding some features to the NSIS script file.

Your bio says you are a cs major, so perhaps this is this something you'd like to have a go at doing a PR for?

[k-takata]

When a user selects to create $HOME/vimfiles, dosinst is called with the -create-directories home option from nsis/gvim.nsi.

A possible solution is to use ShellExecAsUser::ShellExecAsUser in gvim.nsi to create it.

[xsrvmy]

Actually is this an installer bug that should go in that repo instead?

[chrisbra]

we can test it in the vim/vim-win32-installer repo first but if this is a change in gvim.nsi it should eventually end up here as well.

[zewpo]

@k-takata that sounds better :) nice, simple, elegant - and now I see that the ShellExecAsUser::ShellExecAsUser plugin is already used with the nsi scripts.

@xsrvmy maybe :) @k-takata and @chrisbra are across both repos.

@k-takata and @chrisbra How might we do a PR to the vim-win32-installer git repo, for code that is in vim git submodue?

[chrisbra]

I would create a patch (which can reference the vim directory) and place it into the patch directory. that should then be applied during building of the installer.

[zewpo]

i've never done a patch file before, I don't mind having a go and trying to do it.

[chrisbra]

If I recall correctly, you need to go with it like this:
You basically check out the vim-win32-installer repository + the vim submodule. You make your changes in the vim submodule for gvim.nsi. Then you do not commit this change, but rather do: git diff > /path/to/patch/directory and add this to the win-32-installer project.

You can test it, by removing your changes in the vim worktree, going into the vim directory and running git apply /path//to/patch/directory/file.patch. That should apply cleanly, and redo your changes.

[k-takata]

Something like this?

--- a/nsis/gvim.nsi
+++ b/nsis/gvim.nsi
@@ -228,6 +228,28 @@ FunctionEnd
 !insertmacro GetParent ""
 !insertmacro GetParent "un."
 
+# Get home directory
+!macro GetHomeDir un
+Function ${un}GetHomeDir
+  Push $0
+  Push $1
+  ReadEnvStr $0 "HOME"
+  ${If} $0 == ""
+    ReadEnvStr $0 "HOMEDRIVE"
+    ReadEnvStr $1 "HOMEPATH"
+    StrCpy $0 "$0$1"
+    ${If} $0 == ""
+      ReadEnvStr $0 "USERPROFILE"
+    ${EndIf}
+  ${EndIf}
+  Pop $1
+  Exch $0  # put $0 on top of stack, restore $0 to original value
+FunctionEnd
+!macroend
+
+!insertmacro GetHomeDir ""
+!insertmacro GetHomeDir "un."
+
 # Check if Vim is already installed.
 # return: Installed directory. If not found, it will be empty.
 Function CheckOldVim
@@ -520,7 +542,7 @@ SectionGroup $(str_group_plugin) id_grou
 	Section "$(str_section_plugin_home)" id_section_pluginhome
 		SectionIn 1 3
 
-		StrCpy $1 "$1 -create-directories home"
+		#StrCpy $1 "$1 -create-directories home"
 	SectionEnd
 
 	Section "$(str_section_plugin_vim)" id_section_pluginvim
@@ -594,6 +616,13 @@ Section -call_install_exe
 	DetailPrint "$(str_msg_registering)"
 	nsExec::Exec "$0\install.exe $1"
 	Pop $3
+
+	${If} ${SectionIsSelected} ${id_section_pluginhome}
+	  ReadEnvStr $3 "COMSPEC"
+	  Call GetHomeDir
+	  Pop $4
+	  ShellExecAsUser::ShellExecAsUser "" "$3" '/c mkdir "$4\vimfiles"' SW_HIDE
+	${EndIf}
 SectionEnd
 
 ##########################################################
@@ -1042,15 +1071,8 @@ SectionEnd
 SectionGroup "un.$(str_ungroup_plugin)" id_ungroup_plugin
 	Section /o "un.$(str_unsection_plugin_home)" id_unsection_plugin_home
 		# get the home dir
-		ReadEnvStr $0 "HOME"
-		${If} $0 == ""
-		  ReadEnvStr $0 "HOMEDRIVE"
-		  ReadEnvStr $1 "HOMEPATH"
-		  StrCpy $0 "$0$1"
-		  ${If} $0 == ""
-		    ReadEnvStr $0 "USERPROFILE"
-		  ${EndIf}
-		${EndIf}
+		Call un.GetHomeDir
+		Pop $0
 
 		${If} $0 != ""
 		  !insertmacro RemoveVimfiles $0

Untested. Even not compiled yet.

[zewpo]

that looks really cool @k-takata

looks like you guys have this pretty much sorted out :)

I know I asked to have a go at doing it, but I've ran out of time today - and because I gotta bail now for the weekend, so I don't mind if you guys want to wrap it up. I will have a look on sunday and see if anything remaining I can do to help.

[k-takata]

I don't have enough time either. If you (or someone else) want to take it over, it's fine for me.

[chrisbra]

@xsrvmy please check the artifact installer from vim/vim-win32-installer#299
https://ci.appveyor.com/project/chrisbra/vim-win32-installer/builds/46068517/job/gbff4bh6qim4yatx/artifacts

[xsrvmy]

No permission issue this time.
Are the subfolders (autoload, etc.) supposed to be created by the installer?
The 9.0.0000 installer does that, but not this one.

[k-takata]

Ah, that should be updated.

[k-takata]

Perhaps, the ShellExecAsUser line should be like this:

	  ShellExecAsUser::ShellExecAsUser "" "$3" '/c "cd /d "$4" & mkdir vimfiles & cd vimfiles & mkdir colors compiler doc ftdetect ftplugin indent keymap plugin syntax"' SW_HIDE

[zewpo]

Anyone got any ideas why MSVC 2022 takes 2x longer than MSVC 2015 would take? Might be some compiler settings we could use to get it faster again?

@k-takata patch for the sub-directories is now in the PR.

Anyway, my earlier test results for this issue - with first patch from @k-takata ; %HOME%\vimfiles directory was correctly owned by the user account on 8.1, 10 and 11 after fresh install. However, on my Win 7.1 test, the owner still remains as Admin group. Not sure if this is just because I might have reduced all the acl settings in my Win 7.1 VM earlier when I set it up to test something else!?. I guess Win7.1 is getting old now, so this is probably a rare edge case - and as this has simple work-arounds anyway - so I don't see it as a problem that needs the installer to deal with right away.

[zewpo]

umm - this is new...

Trojan:Script/Wacatac.B!ml

Alert level: Severe
Status: Active
Category: Trojan
Details: This program is dangerous and executes commands from an attacker.

Affected items:
  file: C:\Program Files\Vim\vim90\install.exe

[zewpo]

That was a Windows Security alert - quarantined the vim install.exe heper file.!? I'll report a separate issue.

Apart from that, testing of @k-takata new patch works fine - creates the set of vimfiles\sub-dirs with correct ownership.

[chrisbra]

okay, if this works, can someone please create a PR against this repo to change it and then to vim-win32-installer to remove the patch again?

[zewpo]

if no-one else does, I can do a pr when i get back home in a few days.