-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 user agent to headers for all RetroAchievements server calls #12580
Add user agent to headers for all RetroAchievements server calls #12580
Conversation
Also, if the point of this is easy detection of the Dolphin build used, you should probably also include the branch name and possibly commit hash. |
Having access to this string would also be useful for other parts of Dolphin that speak HTTP. For instance, I made our code that contacts redump.org set a user agent, but I didn't know about the conventions for user agents and therefore skipped the slash. Exposing your newly added user agent string as a getter in Version.h would let me update that code to use the proper version with a slash. Also, I would very much prefer the hardcoded "Dolphin" to be centralized in Version.cpp, so that people forking Dolphin only have to change it in one place. We wouldn't want forks to confusingly show up as if they are Dolphin with version numbers that don't make sense in the context of upstream Dolphin. |
ee140c3
to
0c3a531
Compare
Not entirely certain I have a complete grasp on what the desired changes are but I made an attempt, anything else specifically you'd like me to modify? |
Right now, "Dolphin" occurs twice in Version.cpp. Could you make it so it's defined only once, and then used in both GetScmRevStr and GetUserAgentStr? |
I mean something like this: 56ad44b The |
Also I have to point out again -- this is technically your problem and you seem to have ignored my last comment about it -- |
Sorry Curtiss, your concern there has been relayed, I just don't have an answer from RC admin on it yet so I haven't changed it. |
I have mixed feelings about this feature. IANAL, but it might raise privacy concerns and issues regarding GDPR. While an exact version in a User-Agent string on modern web browsers most likely won't allow user de-anonymisation, an exact Dolphin build (especially custom ones) and an IP address likely will. Some questions regarding the implementation:
This stackoverflow post summarises the User-Agent format nicely: https://stackoverflow.com/a/2601492 For example,
Additionally, allowing this level of granularity might be used to identify Dolphin and dissociate |
I'm reaching out to RetroAchievements administration to get some solid answers but for now the only question here I feel reasonably confident answering is this:
RetroAchievements will not function without a user account and will not make API requests unless the user is currently logged in, actively logging in, or actively logging out, and it is in logging in that an API key is created. In order to get an API key, the user must have created a RetroAchievements account with all agreements that that entails. I do not recall what messaging is given to the user before logging in so I don't know if there is still a concern about a user clicking the login button and failing to log in without having a RetroAchievements account. |
Hello, Dolphin team! I'm an administrator at RetroAchievements, and I contribute heavily to the webdev team, including helping maintain our 3rd party API. While I'm not privy to the implementation details here, I asked @LillyJadeKatrin to implement this change. Our Connect API (the endpoints which emulators integrate with) is frequently the target of pentesting and abuse. Relatively soon as a sane security measure we will block all Connect API invocations which do not have a user agent header attached to the HTTP call. We only managed to catch this because of a user poking around with Gamecube achievements currently in development, which triggered some notifications on an internal alerting tool. We're currently asking that clients identify themselves by attaching a user agent header on each API call. At a bare minimum, we'll require:
Any additional metadata beyond that is an added bonus and purely up to the integration team's discretion. The version number is specifically there so if something goes wrong with an integration's hardcore mode implementation, we can block unlocks from that version of the emulator. @sepalani raises interesting points, and using an internal version number decoupled from Dolphin's actual release would also be fine. |
I don't think providing an exact version number would make it possible for RetroAchievements to do any tracking that they can't already do using the mandatory login system. With that said, @sepalani, do you feel there is a privacy problem with including the exact version number in the user agent for other integrations like the redump.org one I mentioned? |
I doubt that enforcing a User-Agent will prevent malicious use of the API. A User-Agent can be easily spoofed automatically or programmatically. An attacker can use a proxy such as Burp, ZAP or many well-known tools to change the User-Agent. Many emulators are open source so an attacker can also weaponise the emulator itself. You might want to have a look at security guides (e.g. OWASP API Security) to improve the API security and adapting it according to the project's constraints. I don't have the full picture of the project but you should rather enforce security around the API token such as a user is responsible for its API token. Such token, then might be used to authenticate the user or ban it if it abuses the service you're providing (i.e. like many SaaS companies providing free or paid API keys to avoid bot/attackers abusing their services).
Fair point, I do but to a less extent since that only happens when verifying a disc and checking the redump checkbox. However, I do believe (though I am not a lawyer) that it might impact privacy depending on how the third-party (redump, retroachievements, wiilink, etc.) processes these pieces of information. For instance, sending requests to a third-party with Dolphin major version (5.0) won't deanonymise the user. However, sending the full version and the current branch might deanonymise most Dolphin GitHub contributors when compared to their IP address and GitHub activity. While I think that it might not bother most users, it might bother some of them if they weren't aware nonetheless. |
Considering that the services are optional to use, I think this is an acceptable risk to take given that the services publish a privacy policy describing how they use personal data and they stick to that privacy policy. (Redump.org in particular does not have a privacy policy, and the administrator of the site is nearly impossible to reach. We may want to limit the amount of data sent to Redump.org because of this.) |
I appreciate your insights on the limitations of relying solely on a user agent header for security. It seems there's a slight misunderstanding regarding our security strategy. Our decision to require a user agent header for Connect API calls is indeed part of a broader approach to enhance the platform's security posture. At no point did I suggest it was the sole solution. We're actively exploring enhancing the security of the Connect API with additional measures. Asking integration partners to include a user agent header shouldn't be a tall order and is quite reasonable IMHO. |
e7c187c
to
3c5f0a5
Compare
9321dbb
to
82d7546
Compare
I think something went wrong with the latest push here. |
I don't understand what's going on here, I've had to fix the same conflicts on this like four times and it's still bugging out, sorry about that. I'll look tonight. |
Try an interactive rebase instead (fetch first!) where you use the origin state, and remove any commits that you don't recognize. It looked a bit like you went forward (picking up the new changes,) then back again (copying the new changes onto an old state, but since it was a rebase it re-created all commits one-by-one.) Thats also why it now has conflicts, because it goes backwards. |
82d7546
to
2a0fbdc
Compare
77d7336
to
14954ee
Compare
14954ee
to
44fd0c0
Compare
Should this be considered good to go? Or are there outstanding concerns on it? |
This is holding up the Retroachievements team, so if we're not going to review it, I could just merge it. Someone just let me know what our plan is with this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise this has been fine for a while (although the string initialization could be simplified done), the question has been whether we're okay with sending the exact version number to a server outside our control that may or may not have a privacy policy that explains if they store this information and so on. Whether that has been resolved or not I cannot say.
Requested agent syntax is "Dolphin/5.0-23456"
44fd0c0
to
9499e14
Compare
My opinion was it's fine as long as the service isn't enabled by default and as long as they have a privacy policy, and both of those criteria are met as far as I can tell. |
I think the same. The code LGTM as well. |
Requested agent syntax is "Dolphin/5.0-23456"