Vulnerability warning logged when placing proto files in default Android source set

[stkent]

In a minimal Android project with configuration like

apply plugin: 'com.android.application'
apply plugin: 'com.google.protobuf'

//...

dependencies {
    //...
    implementation "com.google.protobuf:protobuf-javalite:3.10.0"
}

protobuf {
    protoc {
        artifact = "com.google.protobuf:protoc:3.10.0"
    }

    generateProtoTasks {
        all().each { task ->
            task.builtins {
                java {
                    option "lite"
                }
            }
        }
    }
}

when I run the testDebug Gradle task, I see the following warning:

proto file '<redacted>/app/src/main/proto/source.proto' directly specified in configuration. It's likely you specified files('path/to/foo.proto') or fileTree('path/to/directory') in protobuf or compile configuration. This makes you vulnerable to https://github.com/google/protobuf-gradle-plugin/issues/248. Please use files('path/to/directory') instead.

As you can see from the build.gradle file, I did not explicitly reference source files using files or fileTree, instead relying on the default plugin behavior to locate source.proto. It seems likely that the default implementation is triggering the warning somehow; perhaps it needs to be updated?

[stkent]

Maybe

protobuf-gradle-plugin/src/main/groovy/com/google/protobuf/gradle/ProtobufPlugin.groovy

Line 346 in 8f78c0f

generateProtoTask.addSourceFiles(project.fileTree(task.destDir) { include "**/*.proto" })

?

[stkent]

Bump! Any thoughts on how to address this? Happy to have a go if someone can validate the suggestion above.

[voidzcy]

Sorry for missing track of this issue.

It mainly indicates that your

dependencies {
    //...
    implementation "com.google.protobuf:protobuf-javalite:3.10.0"
}

for the part you omitted probably has something that effectively doing files('path/to/foo.proto') or fileTree('path/to/directory') for configurations.

The source code line you pointed to is to add the output of proto extraction tasks to proto generation tasks with the file hierarchy preserved, while the warning is logged inside the execution of proto extraction tasks. So it should not be the cause.

If you are able to turn on the debug log, you should be able to see the log message for

protobuf-gradle-plugin/src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy

Line 97 in 15607b4

logger.debug "Extracting protos from ${file} to ${destDir}"

which may give you more insights about what is happening.

[voidzcy]

Now I understand what's happening for this issue.

The warning does (and only) happen in Android projects for extractIncludeProto tasks of source sets belonging to test variants, such as extractIncludeTestProto, extractIncludeDebugUnitTestProto and extractIncludeReleaseUnitTestProto. This is caused by this piece of code:

protobuf-gradle-plugin/src/main/groovy/com/google/protobuf/gradle/ProtobufPlugin.groovy

Lines 408 to 423 in 7f96483

	if (Utils.isAndroidProject(project)) {
	// TODO(zhangkun83): Android sourceSet doesn't have compileClasspath. If it did, we
	// haven't figured out a way to put source protos in 'resources'. For now we use an
	// ad-hoc solution that manually includes the source protos of 'main' and its
	// dependencies.
	if (Utils.isTest(sourceSetOrVariantName)) {
	inputFiles.from getSourceSets()['main'].proto
	inputFiles.from testedCompileClasspathConfiguration ?: project.configurations['compile']
	}
	} else {
	// In Java projects, the compileClasspath of the 'test' sourceSet includes all the
	// 'resources' of the output of 'main', in which the source protos are placed. This is
	// nicer than the ad-hoc solution that Android has, because it works for any extended
	// configuration, not just 'testCompile'.
	inputFiles.from getSourceSets()[sourceSetOrVariantName].compileClasspath
	}

I am highly suspecting this is redundant since the change of using the intermediate compileProtoPath configuration as the bucket for dependencies to extract protos from (added in #366). I would confirm my thoughts further and see if that can indeed be eliminated. Anyway, we are clear about the logged warning now.

Update: those hacks for including main's/variant-under-test's proto in the test variant (unitTest or androidTest) is indeed necessary. Otherwise, the test variant is not able to import protos defined in the variant under test.

One approach I can think of to avoid warning for these internal things is to use a separate inputFiles field (e.g., extraInputFiles), and we only warn for cases when real user input uses .proto.

[gavra0]

@voidzcy @ejona86 I've uploaded #542 to fix this, can one you please take a look?