Comments (6)
If your really concerned with malicious PRs being triggered in this way ASDF should be doing three things:
- Setting
autofix_prs = False
in the.pre-commit-config.yaml
so that autofixes are not automatically triggered. - Using
pull_request_target
instead ofpull_request
for triggers, as according to the GitHub docs changes actions so that they only run the action relative to what is currently committed in the repository rather than any new changes to the workflow. This prevents malicious things from being added to a CI job. - Adding an
if: (github.actor != pre-commit-ci[bot])
to each of the workflows. This prevents the pre-commit.ci commits from being run. These commits should be rebased out in any case, meaning the user or maintainer will be force pushing a new commit in any case.
from asdf.
Thanks for bringing this to my attention. For now I've disabled pre-commit-ci integration and opened an issue: pre-commit-ci/issues#195
from asdf.
This is not a big issue in my opinion, ASDF uses the free github runners provided for all open source software. Many packages with user bases orders of magnitude larger than ASDF use the pre-commit.ci bot without issue, e.g. matplotlib
, xarray
, scikit-image
, and pandas
.
I for one do not think this is worth disabling the pre-commit bot. I do suggest disabling the autofix_prs
setting for this and other reasons.
from asdf.
Using pull_request_target
to checkout code from a PR and run tests is specifically warned against in the docs linked as it opens up more possibilities for malicious PRs.
from asdf.
Using
pull_request_target
to checkout code from a PR and run tests is specifically warned against in the docs linked as it opens up more possibilities for malicious PRs.
True enough, though most of those dangers can be mitigated by configuring the permissions granted to the workflow to be more strict than the defaults. That is set the permissions to read only.
The larger issue in this case is that this would cause the workflow to be essentially useless for CI as it checks out code relative to the base of the branch rather than the head of the PR.
All this is a distraction from my original point, which is that I do not think any of this is an actual issue.
As was pointed out in the issue you raised with pre-commit.ci, disabling pre-commit.ci does not increase the "security" in any meaningful way. All it does is barely limit the pool of attackers as it basically relies on the assumption that ASDF can "trust" its contributors.
It requires nearly the same amount of effort for an attacker to create a meaningless but useful PR, like correcting minor issues in the docs somewhere, in order to gain the same ability to run workflows or not. Similarly, even "trusted" contributors are susceptible to being unwillingly compromised, which again effectively gains an attacker the same ability to run actions. My point here is that there is little relative "security" benefit to being concerned about this particular case.
Since to my knowledge ASDF does not have any self-hosted runners or any paid runners, ASDF does not have any direct liability for actions being run in this way. So things like "crypto" attacks are basically a problem for the GitHub platform itself, not ASDF aside from possibly reporting such behaviors. The only realistic concern is exposure of secrets, which should be extremely limited as we have the permissions for our workflows configured according to current best practices. Any risk in this vein is the risk ASDF or any other public GitHub organization accepts in order to have any level of automation provided by the platform.
from asdf.
I agree. Opening an issue with pre-commit and GH about this gain-of-permission hack is good. But everyone uses the pre-commit bot and I haven't yet heard of this being a problem (hence you opening a new issue). I think turning off the bot is an overcorrection.
Pre-commit has become a standard and staple of quality code development. I use asdf on the regular and would be disheartened to see the org take active steps to decrease its code quality, readability, and maintainability.
Furthermore, many of my friends in sec-ops use pre-commit. This gain-of-permission hack doesn't worry them. It might worry GitHub, but then they should fix it.
from asdf.
Related Issues (20)
- Combine package and build workflows
- masked arrays do not roundtrip with all false masks
- `AsdfSpec` misses expected match
- deprecate `AsdfSpec` and `format_tag`
- Tracking `sunpy` 6.0 and ASDF 1.6.0 HOT 2
- Old (<2.14) versions of asdf do not fully support ASDF standard 1.6.0
- `AsdfFile` instances are not pickleable HOT 1
- Chunking support HOT 2
- Investigate enabling `validate_checksum` as default `True`
- Investigate returning `ndarray` when `lazy_load=False` HOT 8
- Change scope of ndarray custom validators HOT 1
- Add to docs comparison of `tag` vs `$ref` usage in schema.
- Consider a new design for the info and search methods that avoids conversion of nodes when the lazy_tree option is used
- Fix stable docs version in RTD
- FAILED asdf/_tests/test_yaml.py::test_implicit_conversion_warning HOT 3
- `assert_tree_match` and `np.testing` ignores array masks
- Schema_info Returning Non Schema Keyword HOT 2
- ignore_version_mismatch doesn't appear to be used? HOT 10
- Deprecate and remove the now unused `ignore_version_mismatch`
- `asdf.util.load_yaml` fails on recursive object
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from asdf.