Replace fake user agent with something reasonable #453
Conversation
There was a problem hiding this comment.
The reason for the fake UA was because it was more convenient than following the correct syntax with somewhat reasonable values and it made a bunch more websites load, since some just block browsers with no UA header in their requests.
We can switch to a more Hastur-ish UA if you follow the correct syntax. :P
|
@robinlinden I don't see a version for Hastur defined anywhere. Where should we put that? I imagine we'll want to define it once and then include it wherever we need it (e.g, the user-agent string). @GrayHatter What do you think about determining the platform using the provided "platform" macros ( |
|
@Zer0-One I'm unconvinced that the UA should report the OS to untrusted servers, which is part of the reason I went for the short UA. I think having a single source of truth is the way to go about it, but also didn't know where it should belong. Hoping someone would tell me where to put it. |
The javascript runtime is already going to give all that info away. You don't need to replicate Firefox/Chrome and have 50 different fields, but at the very least, you need to provide some way to differentiate between a mobile and a desktop device. And of course, now or in the future, you should include a way to override the UA string so that the user can spoof another user agent, or remove data that they perceive to be "sensitive". |
|
On second thought, I guess Hastur doesn't run on any mobile devices, does it... |
Why would it?
Why? |
https://developer.mozilla.org/en-US/docs/Web/API/Navigator
Like I said, I guess it doesn't make sense in this particular case unless we want to develop for a mobile platform, but there are plenty of sites that serve different documents depending on whether or not the user-agent in the request indicates a mobile platform. I'm sure that there are also sites that use OS version information in the UA string to, for example, serve you a platform-specific downloads page, or things like that, but I can't come up with any examples off the top of my head. |
I'm not sure that narrow use case is worth doxing user information to servers. However we decide to expose that information via JS isn't behavior that I think we'd want to depend on here; especially given I'm hopeful the JS engine will have better privacy isolation as well. Either way, I think there's likely to be more work around UA, so I added what can be expanded into something more reasonable down the line. It could all be insane, so I leave it to the group for comments :) |
browser/BUILD
Outdated
| @@ -6,6 +6,22 @@ alias( | |||
| actual = "gui", | |||
| ) | |||
|
|
|||
| dependencies = { | |||
There was a problem hiding this comment.
actually no, I omitted it not realizing it's a name used on 19 deps = dependencies.get(
There was a problem hiding this comment.
But there aren't actually any dependencies there, so you should just be able to remove the "deps" argument entirely.
There was a problem hiding this comment.
TBH, if you want to make this a library, i would keep it outside of //browser entirely. I'd put it in //util. If it's just for the browser frontend, then i would just add it to the sources for the browser, not make a separate library out of it.
There was a problem hiding this comment.
But there aren't actually any dependencies there, so you should just be able to remove the "deps" argument entirely.
TIL, thanks @Zer0-One
TBH, if you want to make this a library,
I don't, but it wouldn't build at all without it.
I'd put it in //util. If it's just for the browser frontend
Util shouldn't exist... it only does when people are bad at naming things.
There was a problem hiding this comment.
If you don't want a library, then just change the sources to look like this: srcs = ["tui.cpp", "branding.h"],
There was a problem hiding this comment.
https://bazel.build/reference/be/c-cpp#cc_binary
Bazel is a motherfucker, but it starts to make sense after a while.
| // | ||
| // SPDX-License-Identifier: BSD-2-Clause | ||
|
|
||
| #ifndef BROWSER_BRANDING_H |
There was a problem hiding this comment.
Nit-pick, but I would have called the module "version" or "constants" or something like that. Ultimately doesn't really matter.
Codecov ReportBase: 86.84% // Head: 86.84% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #453 +/- ##
=======================================
Coverage 86.84% 86.84%
=======================================
Files 84 84
Lines 3785 3785
=======================================
Hits 3287 3287
Misses 498 498 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There's no reason for this, and anyone who might care will use browser based finger printing, and will identify this as deceit.
Unconvinced that browser/branding.h is the best place for this to live, it's at least enough for PR and comments. I also have qualms about browser/BUILD, but I'm not sure how pedantic is reasonable about build files, given they're mostly undoc'd.
There's no reason for this, and anyone who might care will use browser
based finger printing, and will identify this as deceit.