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

Forced dependency versions and excluded dependencies #165

Merged
merged 8 commits into from
Aug 17, 2024

Conversation

ArnyminerZ
Copy link
Member

Got rid of unnecessary transitive dependencies and forced the required ones to their respective latest versions.

Both lang3 and commons-codec are required, but I've forced them to use the latest version. Everything seems to work fine.

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ added the refactoring Quality improvement of existing functions label Aug 7, 2024
@ArnyminerZ ArnyminerZ self-assigned this Aug 7, 2024
@ArnyminerZ ArnyminerZ linked an issue Aug 7, 2024 that may be closed by this pull request
@ArnyminerZ
Copy link
Member Author

I don't know if this will solve any of the security issues reported since most of them are related to bouncycastle, which I don't even know if we are using, since it doesn't show up in the dependency inspector in Android Studio.

@ArnyminerZ ArnyminerZ requested a review from sunkup August 7, 2024 17:51
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@sunkup sunkup force-pushed the 162-take-care-of-ical4j-dependencies-security branch from 76cf1d0 to 25be147 Compare August 8, 2024 10:57
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

Looks okay to me. At least I don't see the security warnings right now :)

lib/build.gradle.kts Outdated Show resolved Hide resolved
lib/build.gradle.kts Outdated Show resolved Hide resolved
…cies-security' into 162-take-care-of-ical4j-dependencies-security
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ requested a review from rfc2822 August 9, 2024 15:01
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

When running the instrumented tests on Android 7 (we set the minimum SDK to Android 6; I run the tests manually to see whether there are errors on Java<8 Android versions), I get 3 errors:

java.lang.AbstractMethodError: 'public abstract int java.io.Reader.read(char[],int,int) throws java.io.IOException' cannot be called

No virtual method getLongVersionCode()J in class Landroid/content/pm/PackageInfo; or its super classes (declaration of 'android.content.pm.PackageInfo' appears in /system/framework/framework.jar)
at at.bitfire.ical4android.JtxICalObjectTest.check_GEOFENCE_RADIUS

No virtual method getLongVersionCode()J in class Landroid/content/pm/PackageInfo; or its super classes (declaration of 'android.content.pm.PackageInfo' appears in /system/framework/framework.jar)
at at.bitfire.ical4android.JtxICalObjectTest.check_XSTATUS(JtxICalObjectTest.kt:142)

We should find out where these errors come from. I think the jtx board errors use a too new API that is only required for the tests, we could adapt that.

The ICalPreprocessorTest error is an "AbstractMethodError", no idea where it comes from. I think it's because mockk requires Android P for mocking objects. Please have a look at the mockk documentation and then annotate the test so that it isn't run on Android P.

@ArnyminerZ
Copy link
Member Author

The issue in IcalPreprocessorTest I think comes from mockk, since mockmObject is not supported on Android < P.

I think we can either ignore this test in Android < P with something like

assumeTrue(
    "Can only run on API Level 24 or newer because mockkObject doesn't support Android < P (SDK 28)",
    Build.VERSION.SDK_INT >= 24
)

which shouldn't be a problem I think, or modify somehow the code to use the verify function without the need of mockkObject.

The first thing that comes to my mind to do this is to make all preprocessors open classes, and reference them through something like

@set:VisibleForTesting
var FixInvalidDayOffsetPreprocessor = FixInvalidDayOffsetPreprocessorImpl()

open class FixInvalidDayOffsetPreprocessorImpl : StreamPreprocessor() {
    // ...
}

And then override fixString with something like:

object FixInvalidDayOffsetPreprocessorTestImpl : StreamPreprocessor() {
    var called: Boolean = false

    fun reset() {
        called = false
    }

    override fun fixString(original: String): String {
        val result = super.fixString(original)
        called = true
        return result
    }
}

And use it somehow in the test with something like

@Test
fun testPreprocessStream_appliesStreamProcessors() {
    try {
        FixInvalidDayOffsetPreprocessor = FixInvalidDayOffsetPreprocessorTestImpl.also { it.reset() }
        FixInvalidUtcOffsetPreprocessor = FixInvalidUtcOffsetPreprocessorTestImpl.also { it.reset() }

        ICalPreprocessor.preprocessStream(StringReader(""))

        assertTrue(FixInvalidDayOffsetPreprocessorTestImpl.called)
        assertTrue(FixInvalidUtcOffsetPreprocessorTestImpl.called)
    } finally {
        FixInvalidDayOffsetPreprocessor = FixInvalidDayOffsetPreprocessorImpl()
        FixInvalidUtcOffsetPreprocessor = FixInvalidUtcOffsetPreprocessorImpl()
    }
}

It should work (not sure about the syntax, I've written it manually). But it's really dirty, and I think not really worth it.

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Yes, looks good :)

@rfc2822 rfc2822 merged commit 6a5df96 into main Aug 17, 2024
8 checks passed
@rfc2822 rfc2822 deleted the 162-take-care-of-ical4j-dependencies-security branch August 17, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Quality improvement of existing functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Take care of ical4j dependencies (Security)
3 participants