gradle / wrapper-validation-action

Gradle Wrapper Validation Action
https://github.com/marketplace/actions/gradle-wrapper-validation
MIT License
257 stars 58 forks source link

No way to specify running in a subdirectory #59

Closed kevinm416 closed 8 months ago

kevinm416 commented 2 years ago

I would like to use this with similar parameters to gradle-build-action so that it can run in a subdirectory: https://github.com/gradle/gradle-build-action#gradle-build-located-in-a-subdirectory. Right now, from the readme, I don't see a way to do this.

LouisCAD commented 8 months ago

Can we expect this feature to ever be supported?

bigdaz commented 8 months ago

From my understanding, this action will recursively search for gradle-wrapper.jar in your repository. This means that it will work for top-level projects as well as nested Gradle builds.

Is there a reason that you want to avoid wrapper-validation checks in particular directories? Or is this requested purely for performance reasons, to avoid searching for gradle-wrapper.jar in directories where it shouldn't exist?

LouisCAD commented 8 months ago

I didn't know that fact because it's not documented clearly (or at all?) in the README.

That's matching my use case, and I have no performance complaints about that for now, especially since I set paths for the GitHub Action to run, so it's running only on wrapper change.

LouisCAD commented 8 months ago

Thank you for the info!

TWiStErRob commented 8 months ago

I didn't know that fact because it's not documented clearly (or at all?) in the README. -- @LouisCAD

image


I wonder if there's any other use cases where one wouldn't want all jars to be checked. Judging by the 15 upvotes on this, there might be, or everyone missed this 5-year-old callout in the README?

JLLeitschuh commented 8 months ago

I'm similarly confused. The entire use case of this action is to be a guard against supply chain attacks. In order to do that, this action looks for gradle-wrapper.jar files (as well as homoglyph attacks) to attempt to find any and all gradle-wrapper.jar files present in the codebase.

I'm scratching my head trying to understand why you'd not want to verify the entire repository for this attack, only do so for a specific subdirectory. That seems like it's likely to open end-users up to attack if they ever change the directory structure, and forget to update the GH action applying this verification.

MateusRodCosta commented 8 months ago

Hi, so I have tried to use this action before but it turned out it was not fit for my use. Apparently the Gradle wrapper jar is not committed to git in Android projects by default, it seems some script (gradlew?) fetches it on first system run.

But still here's a useful scenario to allow to restrict on subdirectory:

Let's say I have a Flutter project which supports both iOS and Android. Only the Android part of the project will use Gradle, so there's no point in letting it search the iOS dir or the entire project, it would make much more sense to restrict it to the android folder.

JLLeitschuh commented 8 months ago

Apparently the Gradle wrapper jar is not committed to git in Android projects by default, it seems some script (gradlew?) fetches it on first system run.

This seems incorrect. Almost all modern projects that use Gradle have the gradle-wrapper.jar checked into source control.

If you'd like to learn more about it see the docs here: https://docs.gradle.org/current/userguide/gradle_wrapper.html

it seems some script (gradlew?) fetches it on first system run.

It's my understanding that all versions of gradlew that have shipped bundled with gradle don't ever attempt to download the gradle-wrapper.jar file.

so there's no point

The point is to verify, exhaustively, that your repository is free from any malicious gradle-wrapper.jar files. Ultimately, this action exists to provide security guarantees about the entire repository.

But still here's a useful scenario to allow to restrict on subdirectory:

Let's say I have a Flutter project which supports both iOS and Android. Only the Android part of the project will use Gradle, so there's no point in letting it search the iOS dir or the entire project, it would make much more sense to restrict it to the android folder.

I can understand the solution, but could you further elaborate upon the problem you are trying to solve here? Are you experiencing performance issues with the wrapper-validation-action taking significant time walking the directory structure of your repository? Is it throwing exceptions when it scans certain directories? Something else?

Rather than starting with a solution, I'd prefer if we started with a problem and worked to create a solution from there. 🙂

TWiStErRob commented 8 months ago

Tagging other upvoters, just in case you didn't Subscribe as well as when you voted: @cubuspl42 @ReallyLiri @joaobrandt @jamilbk @Kang3498 @mzbyszynski @luzfcb @msmoljan @ghostbuster91 (5 more not visible).

What are your problems and in what context, that you're trying to solve by running in a subdir?

Tip: press the "Unsubscribe" button on the right of this issue, if you don't want to receive further emails/notifications about this issue.

MateusRodCosta commented 8 months ago

This seems incorrect. Almost all modern projects that use Gradle have the gradle-wrapper.jar checked into source control.

I have these lines in my .gitignore inside the android folder of a Flutter project:

gradle-wrapper.jar
/.gradle
/gradlew
/gradlew.bat

From here: https://github.com/MateusRodCosta/vidya_music/blob/2f0b2edfe3e5e25450a7291393ac65a0f1d78f5d/android/.gitignore

Apparently from my Android project the gradle-wrapper.jar is actually committed: https://github.com/MateusRodCosta/Share2Storage/blob/main/gradle/wrapper/gradle-wrapper.jar.

So, most likely it's a problem in the Flutter template from when I created the app. I should think about filling a bug later if it still happens on a freshly created app.

I can understand the solution, but could you further elaborate upon the problem you are trying to solve here? Are you experiencing performance issues with the wrapper-validation-action taking significant time walking the directory structure of your repository? Is it throwing exceptions when it scans certain directories? Something else?

Actually the problem really was basically a misunderstanding on my part, I tried implementing the action on my Flutter project but I didn't know that the gradle-wrapper.jar wasn't committed due to the .gitignore. So I thought "maybe it's looking in a place that would make sense for Android projects? I should just tell it to start at the android/ folder"

I haven't yet tried on my actual Android project which actually had the gradle-wrapper.jar committed because I thought the issue was in the action and not in the repo 🫠.

Still, you do have a point about the performance stuff as I don't know it will take so long to scan the the entire project enough that it would make sense to restrict to a specific dir. But, I don't think there is much benefit in letting the action scan inside the dirs for iOS, Windows, Mac and Linux (or even the lib/ folder which has basically only dart files) when only Android will be using Gradle as its build system.

JLLeitschuh commented 8 months ago

Actually the problem really was basically a misunderstanding on my part, I tried implementing the action on my Flutter project but I didn't know that the gradle-wrapper.jar wasn't committed due to the .gitignore. So I thought "maybe it's looking in a place that would make sense for Android projects? I should just tell it to start at the android/ folder"

This is by design. We fail when we fail to find more than one gradle-wrapper.jar by default. The concern/rationale being that a malicious attacker has managed to sneak one by the maintainer by renaming the gradle-wrapper.jar using a homoglyph attack that the wrapper-validation-action failed to detect. We made the intentional decision that, when no gradle-wrapper.jar files are detected, to fail closed to protect end-users. (Example of a homoglyph attack: https://github.com/JLLeitschuh/playframework/pull/1/files)

This is also why have the min-wrapper-count. In repositories that have multiple gradle-wrapper.jar we want to ensure that, if a project has more than one, an attacker can't manipulate one, with a homoglyph attack, and this wrapper-validation-action will miss it.

https://github.com/gradle/wrapper-validation-action/blob/83cf5fdbbf627c8da6fec32f28db58967a3136ac/action.yml#L6-L9

But, I don't think there is much benefit in letting the action scan inside the dirs for iOS, Windows, Mac and Linux (or even the lib/ folder which has basically only dart files) when only Android will be using Gradle as its build system.

The benefit is the exhaustive guarantee that this action provides around guaranteeing the most important thing ALL gradle-wrapper.jar files within a given repository are the ones officially published by Gradle:

All of them will have been validated. This is the guarantee this action aims to provide our end users.

ReallyLiri commented 8 months ago

In my case its simply a monorepo made of several modules in different frameworks and languages. Gradle microservice is only one of the bunch, no good reason to put gradlew in the root dir (besides not being able to specify a subdir)

bigdaz commented 8 months ago

In my case its simply a monorepo made of several modules in different frameworks and languages. Gradle microservice is only one of the bunch, no good reason to put gradlew in the root dir (besides not being able to specify a subdir)

You most definitely do not have to put gradlew in the root dir. This action will recursively search the entire repository for gradle-wrapper.jar. Nothing to configure.

The only reasons to restrict to a subdirectory would be:

  1. To improve performance. But we have no evidence that searching a large repo for gradle-wrapper.jar is slow.
  2. If you have gradle-wrapper.jar files that you want to specifically exclude from the search.

Otherwise, just use the action as-is, and it will check if gradle-wrapper.jar files are legit, no matter where they are found in the repo.

cubuspl42 commented 8 months ago

As a person who originally upvoted this issue, I must admit that I just misunderstood the idea of this action. I believe I was biased (possibly like the others) by the similar gradle-build-action action, which (when not provided with a specific subdirectory path) executes in the top-level directory.

Maybe we could consider emphasizing the true behavior in the description?

Currently:

This action validates the checksums of Gradle Wrapper JAR files present in the source tree and fails if unknown Gradle Wrapper JAR files are found.

Suggested:

This action validates the checksums of all Gradle Wrapper JAR files present in the repository and fails if unknown Gradle Wrapper JAR files are found.

Emphasis on changed/added words. We could consider actually emphasising "all", but it's optional.

I know that the current description is already correct. Unfortunately, people are lazy and often in a hurry, which results in misunderstandings like the one we discuss in this issue.

cubuspl42 commented 8 months ago

This is just guessing, but I could argue that the current description could be understood like this...

This action validates the checksums

Checksums, meaning MD5 and SHA1 for example

of Gradle Wrapper JAR files

Files, plural. The single letter "s" could either be missed, or assumed to mean files in general, in all repositories the action is installed to.

present in the source tree

The term "source tree" could be thought to be a specific well-known place in the source tree, especially a root if not overridden.

and fails if unknown Gradle Wrapper JAR files are found.


I know that I'm playing devil's advocate right now, but there must be some reason why I didn't immediately get that this action performs a recursive search 😄

bigdaz commented 8 months ago

@cubuspl42 Thanks for the suggestion.

I've incorporated this change into https://github.com/gradle/wrapper-validation-action/commit/85cde3f5a1033b2adc2442631c24b530f1183a1a. I now consider this issue resolved, since it seems to be primary due a misunderstanding of how the action operates.

LouisCAD commented 8 months ago

Thank you! 🙏🏼