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

Always run poetry install #1

Merged
merged 1 commit into from Jul 6, 2021
Merged

Always run poetry install #1

merged 1 commit into from Jul 6, 2021

Conversation

epicfaace
Copy link
Contributor

Because pull requests from forks can access the cache (https://docs.github.com/en/actions/guides/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache), just using the contents of the cache could potentially be a security issue if someone submits a malicious PR.

Because pull requests from forks can access the cache (https://docs.github.com/en/actions/guides/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache), just using the contents of the cache could potentially be a security issue if someone submits a malicious PR.
@epicfaace
Copy link
Contributor Author

Actually, maybe this isn't a security issue. The worst someone could do is to change what dependencies we're using, but this wouldn't affect the package published to pypi at all. What do you think?

@epicfaace epicfaace requested a review from milesmcc July 5, 2021 21:36
@milesmcc
Copy link
Collaborator

milesmcc commented Jul 5, 2021

Can't hurt. We're not running short on GH action worker time, so we might as well install fresh. Plus, perhaps they'd be able to access the PyPI secret if they were able to taint the cache in the release workflow?

@milesmcc
Copy link
Collaborator

milesmcc commented Jul 5, 2021

Is there any reason to leave the cache step at all if we don't use it during poetry install?

@epicfaace
Copy link
Contributor Author

epicfaace commented Jul 6, 2021

Plus, perhaps they'd be able to access the PyPI secret if they were able to taint the cache in the release workflow?

Yeah, that's true.

Is there any reason to leave the cache step at all if we don't use it during poetry install?

I was guessing that poetry install will automatically check for installed dependencies and not re-download them (like what npm does), but maybe that's incorrect. Now that I think of it, though, if this is true and poetry checks for dependencies in a very simplistic way that isn't tamper-proof (such as checking package versions), then that's another potential vulnerability.

@milesmcc milesmcc merged commit 0c729a8 into main Jul 6, 2021
@milesmcc milesmcc deleted the epicfaace-patch-1 branch July 8, 2021 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants