Fix test workflow

[mob-sakai]

Changes

• Fixed: The merged Dockerfile not being tested.
• Checkout the minimum files needed to test from the merge commit.
  • There is a vulnerability in the pull_request_target event.
  • The files that the workflow depends on (e.g. shellscript) MUST NOT be checked out of the merge commit.
  • https://securitylab.github.com/research/github-actions-preventing-pwn-requests
  • A great thanks for @asottile
• Use the pull_request event instead of the pull_request_target event.
• Change the machine-id. (This image will not be pushed.)
• Embed UNITY_LICENSE.

Result: https://github.com/game-ci/docker/actions/runs/545523357

Checklist

• Read the contribution guide and accept the code of conduct
• Readme (updated or not needed)

EDIT: The test workflow should be stopped.

[github-actions]

[mob-sakai]

Oh, it's not enough.
The test workflow should be stopped.

[mob-sakai]

NOTE: I will refrain from disclosing the specific vulnerability (i.e., how to get the secret).

[webbertakken]

Actually there is nothing to gain by getting access to the repo and the threshold to abuse the vulnerability would require some manual work and can not easily be automated.

I think we can make the workflow more self-contained. Will get back to you about it after work.

[mob-sakai]

If I fix the test workflow, a malicious user could get secrets.UNITY_LICENSE.

NOTE: For now, the test workflow is broken and do not test the Dockerfiles for merge commits.
Therefore, the test workflow is safe.

[asottile]

Actually there is nothing to gain by getting access to the repo

A malicious user could compromise the integrity of the code, or outright delete it. A proof of concept is easy and would take at most a few minutes to write for this repository. Note that I detected this repository using some very simple automated tooling so anyone else could do the same.

The current workflow on [the primary branch] is susceptible to leaking both a write token and the UNITY_LICENSE -- I would really start by disabling the workflow entirely to mitigate the vulnerability and then work on a solution.

[mob-sakai]

@webbertakken
What do you think about the idea of setting up another machine_id for testing and embedding UNITY_LICENSE into the test workflow?
The embedded UNITY_LICENSE is worthless to most people who use unity-builder or unity-test-runner.

[webbertakken]

I think it's a nice idea, however we actually need to verify that the machine id is in-tact after any changes.

I'm thinking perhaps lets revert and just hardcode the license afterall.

[mob-sakai]

My suggestion:

• Use the pull_request event instead of the pull_request_target event.
• Change the machine-id. (This image will not be pushed.)
• Embed UNITY_LICENSE.

Result: https://github.com/game-ci/docker/actions/runs/545523357

[mob-sakai]

We can modify the test workflows for unity-builder and unity-test-runner with the same solution. 👍

[webbertakken]

Yea lets do that. but let's just use the regular machine id to not create extra complexity and potential test-misses for when something actually goes wrong that affects the machine id.

I don't feel comfortable messing with machine id just for sake of saving a license that's going to be public anyway. If you remove that part, i'll merge this.

[mob-sakai]

👍