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

Move clientRelease url generator #460

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

fcaps
Copy link
Collaborator

@fcaps fcaps commented Nov 12, 2023

closes #418

@@ -0,0 +1,54 @@
import {Octokit} from "https://esm.sh/@octokit/core";
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this was not in the package, but directly downloaded. Why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe you missed that this file is executed in a browser. Just used the recommended way for browsers.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the only potential discomfort I have here is about the presence of the script at the url and potential for injection.

Although to get around this we would have to use some bundler like vite or webpack correct

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

even if we would have a builder, it will not change the potential harm.
if someone gets access to the repository (to any package we use), we can't do anything about it (forking and self-hosting would be an option), but since it's an official sdk from github, i don't have any discomfort.
there are other "security" issues looking into the code.

i would say, if we don't trust github or esm, we should also not trust cloudflare or google-tagmanager/analytics^^

The github client used in node was even more critical, since it could execute code in the container, this combined with node running as root and no checksum checks (already fixed with --frozen-lockfile option and the new Dockerfile) could have been more dramatic than just me ranting and creating a PR to fix that.

We also have to balance security, if the site would get compromised, they could only get a potential login-password, or impersonate FAF to trick the user to download/install/accept something that is not from us, if the browser-security is set to be wide open.

i would "trust" esm and github (maintainers of the repo), but maybe i am missing something here.

Copy link
Member

Choose a reason for hiding this comment

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

I did not realize it was a github official sdk

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was also surprised by the name

Copy link
Member

@Sheikah45 Sheikah45 left a comment

Choose a reason for hiding this comment

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

My main concern would just be that if someone exceeds the GitHub API rate limit then this would fail when querying for the latest but with the fallback that should be handled I believe and it is run client side so won't affect the server.

I would also add /latest to the fallback url so that the user is taken directly to the latest release.

@fcaps
Copy link
Collaborator Author

fcaps commented Nov 12, 2023

good point with the latest, i'll change this.
Yeah, server is not affected.

@fcaps fcaps requested a review from Sheikah45 November 12, 2023 11:58
@fcaps
Copy link
Collaborator Author

fcaps commented Nov 12, 2023

since we are only hitting the github-api on click, we should not be the origin of a rate-limit (if not a developer and clicking the button multiple times^^).
there is a potential issue if the github-api is slow (timeout reached), the user will get the fallback delayed, but at this point it's better than having a container not starting.

@Sheikah45 Sheikah45 merged commit 06a76f8 into FAForever:develop Nov 13, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove usage of github-api
3 participants