PranshulGG / WeatherMaster

A Weather app for android 🌦🌞☔
GNU General Public License v3.0
505 stars 11 forks source link

please remember to increase versionCode #5

Closed IzzySoft closed 2 months ago

IzzySoft commented 2 months ago

First thanks for making your app available under a FOSS license! I see there are 38 releases currently, but your build.gradle still states versionCode 2. versionCode is what Android uses internally to tell versions apart. So if I had installed one of the previous APKs, I'd never receive an update notification – and Android would think "this is already installed".

So versionCode should be increased with each new release.

Thanks in advance!

Btw: I just wonder that all previous releases are now marked as pre-release. Can't remember the "current" one was marked thus when I looked last. Do I miss something there? "pre-release" just means "not yet ready for release", not "previous release" :wink:

IzzySoft commented 2 months ago

PS, while I'm here, this is the full log from the updater here at the IzzyOnDroid repo:

Binary files repo/com.example.weathermaster_2.apk and repo/com.example.weathermaster_1.4.8.apk differ
! repo/com.example.weathermaster_2.apk declares flag(s): usesCleartextTraffic
! repo/com.example.weathermaster_2.apk contains signature block blobs: 0x504b4453 (DEPENDENCY_INFO_BLOCK; GOOGLE)

The first was what I addressed with my comment above. May I ask why you declare usesCleartextTraffic? Any source used that doesn't support https? As for DEPENDENCY_INFO_BLOCK, that can easily be avoided with a minor addition to your build.gradle:

android {
    dependenciesInfo {
        // Disables dependency metadata when building APKs.
        includeInApk = false
        // Disables dependency metadata when building Android App Bundles.
        includeInBundle = false
    }
}

For some background: that BLOB is supposed to be just a binary representation of your app's dependency tree. But as it's encrypted with a public key belonging to Google, only Google can read it – and nobody else can even verify what it really contains. More details can be found e.g. here: Ramping up security: additional APK checks are in place with the IzzyOnDroid repo.

Thanks in advance!

PranshulGG commented 2 months ago

First thanks for making your app available under a FOSS license! I see there are 38 releases currently, but your build.gradle still states versionCode 2. versionCode is what Android uses internally to tell versions apart. So if I had installed one of the previous APKs, I'd never receive an update notification – and Android would think "this is already installed".

So versionCode should be increased with each new release.

Thanks in advance!

Btw: I just wonder that all previous releases are now marked as pre-release. Can't remember the "current" one was marked thus when I looked last. Do I miss something there? "pre-release" just means "not yet ready for release", not "previous release" :wink:

Thanks for your feedback! And yes, I forgot to change the version code—my bad. I marked it as 'pre-release' cuz it just looks good

PranshulGG commented 2 months ago

PS, while I'm here, this is the full log from the updater here at the IzzyOnDroid repo:

Binary files repo/com.example.weathermaster_2.apk and repo/com.example.weathermaster_1.4.8.apk differ
! repo/com.example.weathermaster_2.apk declares flag(s): usesCleartextTraffic
! repo/com.example.weathermaster_2.apk contains signature block blobs: 0x504b4453 (DEPENDENCY_INFO_BLOCK; GOOGLE)

The first was what I addressed with my comment above. May I ask why you declare usesCleartextTraffic? Any source used that doesn't support https? As for DEPENDENCY_INFO_BLOCK, that can easily be avoided with a minor addition to your build.gradle:

android {
    dependenciesInfo {
        // Disables dependency metadata when building APKs.
        includeInApk = false
        // Disables dependency metadata when building Android App Bundles.
        includeInBundle = false
    }
}

For some background: that BLOB is supposed to be just a binary representation of your app's dependency tree. But as it's encrypted with a public key belonging to Google, only Google can read it – and nobody else can even verify what it really contains. More details can be found e.g. here: Ramping up security: additional APK checks are in place with the IzzyOnDroid repo.

Thanks in advance!

Thank you for the explanation! So, if I understand correctly, while this BLOB improves security by ensuring that only Google can verify it, it also means that developers and users can't independently check what’s inside, right? Does this affect app transparency, or is it primarily a security benefit?

And

usesCleartextTraffic is just to allow the live-server to run during debugging."

IzzySoft commented 2 months ago

I marked it as 'pre-release' cuz it just looks good

hehe – but nah, that's not for the looks. There's reasoning behind it. It means e.g. "do not use/pick-up this one, it's just a test build not-yet-ready for production".

I forgot to change the version code—my bad

Happens more often than you imagine… But hopefully you will remember it now? :wink:

Thank you for the explanation! So, if I understand correctly, while this BLOB improves security by ensuring that only Google can verify it

Nope. The reason it would improve security is that at build time, when all those details are known, they are collected and embedded. But as only Google can read it, it at best can increase security at PlayStore. Would it have been meant to increase security in general, it instead would have been signed (to make it tamper-proof). As it's encrypted, nobody else can read it – so for everybody except Google it's rather a security risk as nobody can tell what's really inside.

Does this affect app transparency

As it is opaque (you cannot see it), it's not transparent. And as outlined, not a security benefit except for Playstore. Because if they cannot read it, it means the file has been tampered with. We cannot tell that as we cannot decrypt. So we better make sure it's not there at all – as it's of no use for us, but poses a risk. And is a "proprietary blob". So preferably you make sure it's not in the APK. And as you're unlikely to have the app at PlayStore (they wouldn't accept an app with the com.example.* namespace), it doesn't even matter what would be in the AAB :wink:

usesCleartextTraffic is just to allow the live-server to run during debugging.

Then better have it only in the debug build, not in the one intended for distribution. Insecure traffic is… well, insecure :wink:

PranshulGG commented 2 months ago

I marked it as 'pre-release' cuz it just looks good

hehe – but nah, that's not for the looks. There's reasoning behind it. It means e.g. "do not use/pick-up this one, it's just a test build not-yet-ready for production".

I forgot to change the version code—my bad

Happens more often than you imagine… But hopefully you will remember it now? :wink:

Thank you for the explanation! So, if I understand correctly, while this BLOB improves security by ensuring that only Google can verify it

Nope. The reason it would improve security is that at build time, when all those details are known, they are collected and embedded. But as only Google can read it, it at best can increase security at PlayStore. Would it have been meant to increase security in general, it instead would have been signed (to make it tamper-proof). As it's encrypted, nobody else can read it – so for everybody except Google it's rather a security risk as nobody can tell what's really inside.

Does this affect app transparency

As it is opaque (you cannot see it), it's not transparent. And as outlined, not a security benefit except for Playstore. Because if they cannot read it, it means the file has been tampered with. We cannot tell that as we cannot decrypt. So we better make sure it's not there at all – as it's of no use for us, but poses a risk. And is a "proprietary blob". So preferably you make sure it's not in the APK. And as you're unlikely to have the app at PlayStore (they wouldn't accept an app with the com.example.* namespace), it doesn't even matter what would be in the AAB :wink:

usesCleartextTraffic is just to allow the live-server to run during debugging.

Then better have it only in the debug build, not in the one intended for distribution. Insecure traffic is… well, insecure :wink:

Thank you for the detailed explanation! I’ll make sure to handle these issues in the next build. I'll also be more mindful about version code! Appreciate your insights. 😃

IzzySoft commented 2 months ago

Please handle it for all subsequent builds, yes? 1.5.1 and 1.5.2 both have 42. A nice number, admitted, but… :wink:

PranshulGG commented 2 months ago

Please handle it for all subsequent builds, yes? 1.5.1 and 1.5.2 both have 42. A nice number, admitted, but… :wink:

Yea it slipped my mind. I'll make sure to handle it for all subsequent builds 😖