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.

Sign up for GitHub

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

Jump to bottom

Improvement on firewall batch scripts #1030

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Improvement on firewall batch scripts #1030

wants to merge 13 commits into from
+79 −18

Conversation

VocalFan
Copy link
Contributor

@VocalFan VocalFan commented May 12, 2024 •
edited
Loading

Let's go in a list here...

  1. Fixes SlimeVR installer firewall rules #1026
  2. Makes it where the scripts request admin privileges as they can't modify the firewall without them.
  3. Makes sure all the firewall rules are properly enabled with enable=yes
  4. Fixed the goofball typo of Discovery defauly port to Discovery default port
  5. Prevents duplicate rules by checking if the rule already exists
  6. Echoes/Tells the user what the name of the rule being added is.

Before:

image

After:

image

(UAC prompt quickly appears. It's so fast I had to use timeout 5 to have a delay so I could screenshot)

(User presses No, the script closes.)

(User presses Yes, script continues as below)

image
@VocalFan
Copy link
Contributor Author

@electromuis

For you, my king-
@ImUrX ImUrX requested a review from unlogisch04 May 13, 2024 20:09
@ImUrX ImUrX added Area: Continuous Integration Automated testing and deployment Type: Enhancement Adds or improves a feature OS: Windows Operating system: Windows labels May 13, 2024
@electromuis
Copy link

Just did a quick test and indeed seems to fix the issue. Although the %CD% value does have to be correct (Right click as admin will run it from System32 and thus not specify the correct java binary). But this would have to be setup correctly in the installer I'd assume.
@VocalFan
Copy link
Contributor Author

Just did a quick test and indeed seems to fix the issue. Although the %CD% value does have to be correct (Right click as admin will run it from System32 and thus not specify the correct java binary). But this would have to be setup correctly in the installer I'd assume.

Sadly just a skill issue on Window's part.
@VocalFan VocalFan requested a review from Erimelowo as a code owner May 14, 2024 23:54
@VocalFan
Copy link
Contributor Author

@electromuis Try again! I think I fixed the issue with right clicking and using Run As Administrator... Also first, use uninstall_firewall a few times as I also added duplicate detection to the install script... So you might currently have duplicate rules.
@unlogisch04
Copy link
Contributor

Just short, I think you need to allow javaw.exe and not java.exe do you have a way to confirm?
@VocalFan
Copy link
Contributor Author

Just short, I think you need to allow javaw.exe and not java.exe do you have a way to confirm?

...Look above, @electromuis had his issue fixed with java.exe being allowed (also some internet searches agree with this)
@ButterscotchV
Copy link
Member

Just short, I think you need to allow javaw.exe and not java.exe do you have a way to confirm?

You could do both, I believe we're only using java.exe though
@ImUrX
Copy link
Member

ImUrX commented May 15, 2024

do we need to have the UAC prompt script?
@VocalFan
Copy link
Contributor Author

do we need to have the UAC prompt script?

As discussed on the top post, firewall modification fails without admin perms. And besides, if the script is already given admin, the prompt is skipped
@ButterscotchV
Copy link
Member

do we need to have the UAC prompt script?

iirc it's the best way to do it, batch scripts stink. I have worries about it having different behaviour in different versions of Windows but it's probably alright?
@VocalFan
Copy link
Contributor Author

It's been basically a tried and true method for most versions of windows... Atleast any that support UAC :P
@ButterscotchV
Copy link
Member

It's been basically a tried and true method for most versions of windows... Atleast any that support UAC :P

Well if it works then sweet at least Windows can do something good lmao, we already had a blunder with the installer crashing because we had the audacity to format JSON on Windows 7.

This PR looks good to be, but not reviewing because I can't really thoroughly check it rn. May do so later.
unlogisch04
unlogisch04 reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@unlogisch04 unlogisch04 left a comment

Choose a reason for hiding this comment

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

Well i tested some Stuff on Windows 11 Version 22631.3593
I did not test the script itself. To this date.

For me Windows 11 only adds rules on Incomming to the firewall when i deny or allow the Program via popup. So not sure if this is different in the Version @electromuis uses.
server/core/resources/firewall.bat Outdated Show resolved Hide resolved
server/core/resources/firewall.bat Show resolved Hide resolved
server/core/resources/firewall.bat Outdated Show resolved Hide resolved
@VocalFan
Copy link
Contributor Author

VocalFan commented Jun 5, 2024

So uh, idk if I should do those changes or keep the rules for 'better safe than sorry'
VocalFan and others added 2 commits June 27, 2024 15:22
@VocalFan @unlogisch04
Begone goobers
a1b3ad5 
Co-authored-by: unlogisch04 <98281608+unlogisch04@users.noreply.github.com>
@VocalFan
Merge branch 'main' into firewall-go-brr
cb8b6c4
ButterscotchV
ButterscotchV reviewed Jun 29, 2024
View reviewed changes
server/core/resources/firewall.bat
"%temp%\getadmin.vbs"
exit /B
)

echo Installing firewall rules...

rem Discovery default port
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rem Discovery default port
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Continuous Integration Automated testing and deployment OS: Windows Operating system: Windows Type: Enhancement Adds or improves a feature
6 participants
@VocalFan @electromuis @unlogisch04 @ButterscotchV @ImUrX @loucass003