Github Actions fails on edit to non-CSL files

[rmzelle]

@retorquere, Github Actions seems to always fail when we make edits to non-CSL files. See e.g. the edit to file "spec/spec_helper.rb" in #5256 which causes a failure in https://github.com/citation-style-language/styles/runs/1884910562.

[retorquere]

The error could be more descriptive, but what's happening here is that Sheldon won't test a PR that changes both assets (.csl/.tab/.json files) and non-assets (.rb). The reason for this is that with github actions, on PRs you can do one of two things:

• test the PR on the submitting repo; the PR is already laid out for you on disk, but you have no access to repo secrets
• test the PR on the target repo. You have access to the repo secrets, but the PR is not applied for you

the reason for this is that if laid-out PRs could trigger tests that had access to the repo secrets, anyone can exfiltrate your repo secrets by submitting a targeted PR that replaces part of the test process itself, which then gets run by github actions, so now you have outsiders running arbitrary code against your repo with full access to tokens that have broad permissions on your repo.

We need the secrets though because we want to have sheldon post back to the PR-issue to show the test results, and we do of course need the PR, because that's what we wanted to test. We were shielded from this in the Travis setup because Sheldon ran on Heroku where it is entirely shielded from such shenanigans; the tests themselves did not need secrets, because the secrets lived with Sheldon. In theory anyone could nudge Sheldon into posting messages on arbitrary PRs, but that was the extent of the damage it could do. With Sheldon inlined into the test process, there's less moving parts, but the parts have more power to do damage.

The current setup runs in the context of the styles repo, with access to the repo secrets, and so the PR not laid out on disk. The workflow setup is such that Sheldon is only kicked into action if an asset file changes. Sheldon wakes up, downloads the patch that constitutes the PR (a link to the patch is available in the test environment), but before applying the patch, Sheldon inspects the files it wants to change. Given that Sheldon is started at all, we know that there is at least one asset file in the PR, and we know that Sheldon will try to render it and post back the results.

In order to prevent a malicious actor sneaking in an exfiltration attempt under the cover of Sheldon announcing render results, Sheldon will refuse to act on such mixed PRs (will not apply the patch and will error out) in order to draw attention to the fact that both asset and non-asset files are in the same PR. In this case, the changes were to:

• spec/spec_helper.rb
• the-journal-of-the-acoustical-society-of-america-numeric.csl
• the-journal-of-the-acoustical-society-of-america.csl

It's the first file that Sheldon is complaining about:

Unexpected styles file spec/spec_helper.rb in pull request

The action will not fail when only assets or only non-assets are changed in the PR.

For this PR, some numbers were added to spec/spec_helper.rb, perhaps to help with testing the styles in the PR. Is that a common thing to happen? Can those numbers be split off to an inert file like a json or yaml file rather than code, to prevent exfiltration paths? The list of repo collaborators is also available in the test environment, so I could also make an exemption to the mixed-PR test for people who have push access to the repo.

[rmzelle]

Thanks for the explanation!

For this PR, some numbers were added to spec/spec_helper.rb, perhaps to help with testing the styles in the PR. Is that a common thing to happen? Can those numbers be split off to an inert file like a json or yaml file rather than code, to prevent exfiltration paths?

Our tests include a test to make sure there are no duplicate ISSNs (which is useful to detect bad metadata, and also can help identify redundant journal styles). spec/spec_helper.rb has a list of ISSNs that are exempted. The file has a few other lists of exemptions as well (TITLES_FILTER, CITATION_FORMAT_FILTER, UNUSED_MACROS_FILTER, EXTRA_FILES_FILTER, and EXTRA_FILES_DIRECTORY_FILTER). These could all be moved to inert files.

The list of repo collaborators is also available in the test environment, so I could also make an exemption to the mixed-PR test for people who have push access to the repo.

It's more common for us to make commits in existing pull requests from outside contributors (which is what I did in #5256), so not sure if that would work well.

[retorquere]

PR in #5268

[rmzelle]

@retorquere, could you take a look at https://github.com/citation-style-language/styles/runs/1985017303 in #5297? I thought this should work as renamed-styles.json is an asset?

[retorquere]

Sheldon is currently very picky about what it considers an asset, and for styles it only deems .csl files and a few exceptions as assets. I can make it so Sheldon always just allows .csl, .json etc.

[retorquere]

A re-run should succeed.

[rmzelle]

Thanks! (I forgot to look at the source code Sheldon, and couldn't find any in the GitHub Actions code in the styles repo; I see you changed this in citation-style-language/Sheldon@643bc55)

[rmzelle]

(and a rerun did indeed work correctly)

[retorquere]

Yeah the workflows install the latest git version of Sheldon to that any fixes applied there are immediately available to the Sheldonized repos without fiddling with Gemfiles.

[retorquere]

Sheldon is still fairly picky about what it allows. a .csl change will not be allowed in the locales repo. That can be loosened further by just bringing all options from https://github.com/citation-style-language/Sheldon/blob/master/bin/sheldon#L83 to https://github.com/citation-style-language/Sheldon/blob/master/bin/sheldon#L118.