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

Add option to hide OS information from server ("privacy mode") #3009

Merged
merged 1 commit into from Apr 9, 2017

Conversation

@Piratonym
Copy link
Contributor

Piratonym commented Apr 8, 2017

This patch adds an option to not send information about the user's OS and OS version to the server. It implements the request from #2899.

The Mumble version and release is still sent, since the server seems to use it for certain decisions. This information alone should not reveal much information about the user.

The option is currently placed in the "Misc" section of the "Network" tab.

Copy link
Member

mkrautz left a comment

I have a few minor changes I would like to have made.

Looks very good though.

@@ -680,6 +681,7 @@ void Settings::load(QSettings* settings_ptr) {
SAVELOAD(qsServicePrefix, "net/serviceprefix");
SAVELOAD(iMaxInFlightTCPPings, "net/maxinflighttcppings");
SAVELOAD(bUdpForceTcpAddr, "net/udpforcetcpaddr");
SAVELOAD(bHideOS, "net/hideos");

This comment has been minimized.

@mkrautz

mkrautz Apr 9, 2017 Member

Let's use "privacy/hideos"

This comment has been minimized.

@mkrautz

mkrautz Apr 9, 2017 Member

With the move of the bHideOS variable, can you move this to match that, too?

@@ -1003,6 +1005,7 @@ void Settings::save() {
SAVELOAD(qsServicePrefix, "net/serviceprefix");
SAVELOAD(iMaxInFlightTCPPings, "net/maxinflighttcppings");
SAVELOAD(bUdpForceTcpAddr, "net/udpforcetcpaddr");
SAVELOAD(bHideOS, "net/hideos");

This comment has been minimized.

@mkrautz

mkrautz Apr 9, 2017 Member

Ditto (new name, new position)

@@ -306,6 +306,7 @@ struct Settings {
/// socket to the same address as its TCP
/// connection is using.
bool bUdpForceTcpAddr;
bool bHideOS;

This comment has been minimized.

@mkrautz

mkrautz Apr 9, 2017 Member

Please move this down below QString qsSslCiphers;
Add a comment above it:

// Privacy settings
Prevents the client from sending potentially identifying information about the operating system to the server.</string>
</property>
<property name="text">
<string>Hide OS information</string>

This comment has been minimized.

@mkrautz

mkrautz Apr 9, 2017 Member

This is hard to name precisely and concisely.
How about "Do not send OS information to Mumble servers"?
The name "Hide OS information" seems a little too vague for my tates... It can mean many things.

@@ -292,6 +292,20 @@ Prevents the client from downloading images embedded into chat messages with the
</property>
</widget>
</item>
<item>

This comment has been minimized.

@mkrautz

mkrautz Apr 9, 2017 Member

How about we put this in a "Privacy" QGroupBox?

Prevents the client from sending potentially identifying information about the operating system to the server.</string>
</property>
<property name="text">
<string>Do not send OS information</string>

This comment has been minimized.

@mkrautz

mkrautz Apr 9, 2017 Member

The problem with this wording is that the option only affects Mumble servers.

The client will (probably?) still send its OS information in HTTP requests, and in statistics to the Mumble project.

This comment has been minimized.

@Piratonym

Piratonym Apr 9, 2017 Author Contributor

Changed it to your suggestion.

I could check if the information is sent in other places and try to include those in the privacy option.

This comment has been minimized.

@mkrautz

mkrautz Apr 9, 2017 Member

Let's get this merged first, then we can work on it?

On a related note, I believe the change is starting to look ready... Could you squash the commits into one?

@Piratonym Piratonym force-pushed the Piratonym:feature-hideos branch from d5e653b to e8f42d8 Apr 9, 2017
@Piratonym
Copy link
Contributor Author

Piratonym commented Apr 9, 2017

Squashed the changes.

Concerning other places with OS information: It seems to be sent by the crash reporter (only after the user agrees, CrashReporter.cpp:183) and as part of the User-Agent header in HTTP requests (NetworkConfig.cpp:161). RFC 7231 does not require the OS information (see section 5.5.3), so removing it there should not cause any problems.

Copy link
Member

mkrautz left a comment

LGTM except this small typo.

@@ -684,6 +685,9 @@ void Settings::load(QSettings* settings_ptr) {
// Network settings - SSL
SAVELOAD(qsSslCiphers, "net/sslciphers");

// Privacy setting

This comment has been minimized.

@mkrautz

mkrautz Apr 9, 2017 Member

Privacy settings

@@ -1007,6 +1011,9 @@ void Settings::save() {
// Network settings - SSL
SAVELOAD(qsSslCiphers, "net/sslciphers");

// Privacy setting

This comment has been minimized.

@mkrautz

mkrautz Apr 9, 2017 Member

Privacy settings

This fixes issue #2899.
@Piratonym Piratonym force-pushed the Piratonym:feature-hideos branch from e8f42d8 to d0e2cdc Apr 9, 2017
@Piratonym
Copy link
Contributor Author

Piratonym commented Apr 9, 2017

Thanks, corrected.

@mkrautz
Copy link
Member

mkrautz commented Apr 9, 2017

Concerning other places with OS information: It seems to be sent by the crash reporter (only after the user agrees, CrashReporter.cpp:183) and as part of the User-Agent header in HTTP requests (NetworkConfig.cpp:161). RFC 7231 does not require the OS information (see section 5.5.3), so removing it there should not cause any problems.

OK, feel free to address that in a separate PR. Thanks!

@mkrautz
mkrautz approved these changes Apr 9, 2017
@mkrautz mkrautz merged commit 65909b8 into mumble-voip:master Apr 9, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Piratonym Piratonym deleted the Piratonym:feature-hideos branch Apr 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.