This is an archive of the discontinued LLVM Phabricator instance.

issue-subscriber: Fix handling of labels with spaces
ClosedPublic

Authored by tstellar on Jan 19 2022, 9:10 PM.

Diff Detail

Event Timeline

tstellar requested review of this revision.Jan 19 2022, 9:10 PM
tstellar created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2022, 9:10 PM
mehdi_amini accepted this revision.Jan 19 2022, 9:16 PM
This revision is now accepted and ready to land.Jan 19 2022, 9:16 PM
asl accepted this revision.Jan 19 2022, 11:27 PM
Quuxplusone added inline comments.
.github/workflows/issue-subscriber.yml
25–29

Consider single-quoting lines 22 and 24 as well, while you're at it. (Economist's $100 bill corollary: If this technique actually works, you'd expect everyone to be doing it.)

.github/workflows/issue-subscriber.yml
25–29

The economist might be right in this case. Did you know it's legal to create a GitHub label containing ', e.g. arthur's test? Single-quotes-before-and-after is not the same thing as shell escaping. (Unless GitHub treats it the same because magic??)

Perhaps all these command-line options should be env vars instead? I don't know what the right answer is (but I bet StackOverflow does).

I'm pleased to report that https://github.com/llvm/llvm-project/runs/4876930402?check_suite_focus=true does helpfully blank out the secret API token with three asterisks --token *** \ instead of showing it in plain text. That's nice. :)

tstellar updated this revision to Diff 401635.Jan 20 2022, 7:10 AM

Correctly handle apostrophes.

tstellar marked an inline comment as done.Jan 20 2022, 7:12 AM
tstellar added inline comments.
.github/workflows/issue-subscriber.yml
25–29

I've fixed this by ignoring labels with apostrophes. I think it's easier to have a policy to not have apostrophes in label names than to add a lot of complex code just to handle them.

Quuxplusone accepted this revision.Jan 20 2022, 7:34 AM
Quuxplusone added inline comments.
.github/workflows/issue-subscriber.yml
25–29

As long as only non-malicious people can create new labels and/or attach labels to issues, sure. I had a vague impression that any GitHub user could create or attach labels, but looking at some repos where I'm not a committer, I see that even attaching labels seems to be restricted. So that seems fine.

It'd be nice to use a safe-list like re.match(r'[a-zA-Z0-9_ :+-]+', ...) instead of contains(github.event.label.name, '''')... but it looks like GitHub Actions Expressions doesn't support general expression syntax here, only contains, startsWith, and endsWith. Bah. (And maybe why pasting user input straight into the shell isn't a best practice and we ought to be looking at passing user input via env vars or files instead. GitHub Actions ought to have some safe way of getting user input into a program! (But I'm too cynical to say it "must" have a way.)

tstellar updated this revision to Diff 401757.Jan 20 2022, 1:09 PM

Use environment variable for label name instead of disallowing apostrophes.

tstellar marked an inline comment as done.Jan 20 2022, 1:10 PM
tstellar marked an inline comment as done.
tstellar added inline comments.
.github/workflows/issue-subscriber.yml
25–29

The GitHub security hardening guide for actions recommends using environment variables, so I've made this change.

This revision was automatically updated to reflect the committed changes.
tstellar marked an inline comment as done.