[PATCH] D117745: issue-subscriber: Fix handling of labels with spaces

[Tom Stellard via Phabricator via llvm-commits]

tstellar marked an inline comment as done.
tstellar added inline comments.


================
Comment at: .github/workflows/issue-subscriber.yml:21-25
         ./github-automation.py \
           --token ${{ secrets.ISSUE_SUBSCRIBER_TOKEN }} \
           issue-subscriber \
           --issue-number ${{ github.event.issue.number }} \
+          --label-name '${{ github.event.label.name }}'
----------------
Quuxplusone wrote:
> tstellar wrote:
> > Quuxplusone wrote:
> > > Quuxplusone wrote:
> > > > 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.)
> > > 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. :)
> > 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.
> 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](https://docs.github.com/en/actions/learn-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.)
The GitHub security hardening guide for actions recommends using environment variables, so I've made this change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117745/new/

https://reviews.llvm.org/D117745