Cap-go / CLI

CLI to help you to manage your version in capgo cloud
https://capgo.app
GNU Affero General Public License v3.0
18 stars 36 forks source link

Prevent native file update #126

Closed WcaleNieWolny closed 1 year ago

WcaleNieWolny commented 1 year ago

If update is not major (example 1.1.0 -> 2.0.0 is major) prevent changing native code. This is ensured in the following way:

  1. Find all .java, .kt, .scala, .swift files in node modules
  2. create a hash of said files
  3. compare these hash with hash from remote database, if there is any differences then disallow upload

/claim #87

WcaleNieWolny commented 1 year ago

A capgo PR will follow soon so that the new native_files_sha256 column is added with the first SQL migration

WcaleNieWolny commented 1 year ago

image

This should be fine, I tested this regex and the whole read node_modules operation only took 200 ms

WcaleNieWolny commented 1 year ago

If performance ever becomes a problem we can implement workers or some other solution. However, I believe performance is good enough for now

WcaleNieWolny commented 1 year ago

Here is the schema PR: https://github.com/Cap-go/capgo/pull/299

DO NOT MERGE THIS PR WITHOUT MERGING THIS SCHEMA PR FIRST

riderx commented 1 year ago

The point of the proposed solution was to have the pakages list and check version by version to allow display the issue and help users prevent they do something wrong. You solution is very smart, but the sha should be link to each version by the cli After that it's the update endpoint who should decide not the CLI

WcaleNieWolny commented 1 year ago

I believe storing the versions of packages instead of native files sha256 could go very wrong very fast. Let's say that someone uses npm, he will have package.json and some dependency lock file. Now let's say someone else uses yarn. He also will have package.json and some different lock file. If we just store versions of all packages then we have to deal with a lot of edge cases.

WcaleNieWolny commented 1 year ago

About the update url. That I do not understand. This check operation is quite expensive and doing it on the server is a waste of money. We would be paying for cpu usage that is not necessary as this operation can be done in the cli

WcaleNieWolny commented 1 year ago

Now, we also have a privacy issue. Let's say we store something like this: [{"package_name": "@capacitor/android", "version": "^4.4.0"}]

This raises the issue that we are storing information about a project that the client might now want us to have. We probably would have to store hash of both of these fields

riderx commented 1 year ago

My idea was to ignore any lock and just keep the version from package.json The problem we have is when we distribute update, not when upload. Since many users will have different version installed. Blocking the upload don’t help

WcaleNieWolny commented 1 year ago

The problem we have is when we distribute update

Well, perhaps but we have a mechanism to prevent this exact issue

{"major":true,"message":"Cannot upgrade major version","error":"disable_auto_update_to_major","version":"2.0.0","old":"1.3.14"}

This should prevent updating native files as updating native files requires new major release

WcaleNieWolny commented 1 year ago

@riderx what is the status on this? Will you merge this or do you want me to do exactly as you said in the review?

sonarcloud[bot] commented 1 year ago

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

WcaleNieWolny commented 1 year ago

@riderx changed as requested, added an option to skip this native files check

riderx commented 1 year ago

@WcaleNieWolny the problem is semver require the user to do it properly where the feedback i get is it's hard for team to do semver for many reason. So it would be a second check