evant / kotlin-inject

Dependency injection lib for kotlin
Apache License 2.0
1.21k stars 53 forks source link

Critical obfuscation issue. #417

Open Monabr opened 1 month ago

Monabr commented 1 month ago

Hello. You have a great library, but there is a problem that prevents me from using it.

The generated code uses the full class name including the package as a key. This should not happen after obfuscation! Please change this behavior.

As a solution, I am ready to see a functionality where I would manually assign a key to each class. This could be made by an optional parameter in the annotation or by some additional annotation.

I believe this is a critical issue and should be fixed as soon as possible!

evant commented 1 month ago

They really just need to be unique per type access, so some type of hashing my work here. Alternatively it looks like hilt solved a similar issue https://github.com/google/dagger/issues/3197 though will have to dig to figure out what code changes they actually did.

Monabr commented 1 month ago

@evant Could you tell me how fixable this problem is, should I wait for the nearest time? I am pressed for time and at the moment I am choosing between your library and service locators, which I would really like to avoid using.

Monabr commented 1 month ago

@evant Here is change commit they made https://github.com/google/dagger/compare/1ece31e5ce8a3de25930cc10a8465af183ff276c..2638852a4b499eb7858fd8227595d6e306c0934b

asapha commented 1 month ago

For Proguard/R8, there's the -adaptclassstrings option. The strings containing class names are modified to their obfuscated version. (source)

For example, in a generated mapping file, I have:

[...]
me.tatarka.inject.internal.LazyMap -> dR0:
# {"id":"sourceFile","fileName":"LazyMap.kt"}
[...]

And in an obfuscated component class, I see:

public final class bt0 implements c22, ut1 {
   // --> LazyMap
   public final dR0 b;

   public final lAi C() {
      // --> LazyMap usage
      return (lAi)this.b.a("lAi", bt0::a);
   }
[...]

It doesn't work if the key has backticks or if it points to a generic type, though.

evant commented 1 month ago

It doesn't work if the key has backticks or if it points to a generic type, though.

Hm, I wonder if doing something like

"com.example.MyClass" + "<" + "com.example.MyArg" + ">"

would help it pick up generic types properly

evant commented 1 month ago

@Monabr

Could you tell me how fixable this problem is, should I wait for the nearest time?

I can make no promises on timelines, but this does seem fixable, pr's welcome.

Monabr commented 1 month ago

For Proguard/R8, there's the -adaptclassstrings option.

Are you talking about this as a temporary solution until the library is changed or do you mean that the dagger library team used this? If you mean the second option - it seems like they used something else.

Monabr commented 2 days ago

@evant Want to highlight that Dagger uses Map<Class<?>, Boolean> instead of strings keys. So I believe this could be implemented in this library.

evant commented 1 day ago

You'd need to make sure generics are handled correctly, but yes that's a possible solution