build(fpb-lint): linting errors as PR comments, resolved #4416

[LuigiImVector]

What does this PR do?

Improve repo

For resources

Description

I improved the fpb-lint.yml workflow so that linter errors were shown via comment, as proposed by @SethFalco on #4416.

In this PR LuigiImVector#10 I conducted some tests, all successful.

Checklist:

• Read our contributing guidelines

Follow-up

• Check the status of GitHub Actions and resolve any reported warnings!

[SethFalco]

The problem from my PR is that if you look at your tests, you'll notice you executed all of those comments from a workflow in the PR.

If given write access to the repo, the PR could easily be modified by a malicious actor to do whatever they want, including spam or execute other actions.

You can see this happening already if you read the error.

failed to create review: Message: Resource not accessible by integration, Locations: [{Line:1 Column:66}]

The solution to this is to use pull_request_target if I understand correctly, but that will open up a whole can of worms.

I recommend you read this:
Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests

In the docs, they create an unprivileged workflow to process the files and create the message as a file and upload it as an artifact. Then in a separate workflow that is privileged, they fetch that artifact and post the review.

This is where I got too busy and put this off, though. Sorry about that! ^-^'

[LuigiImVector]

Before we merge this, would you mind merging the one in your fork, and perhaps I can try to do a malicious PR to it, and we'll see what happens? 🤔

Now I'm going to read the article you linked, in the meantime is it enough for me to write?
on: [push, pull_request_target]

I'm not sure if I understand correctly

[SethFalco]

Now I'm going to read the article you linked, in the meantime is it enough for me to write?

That'd work, I think.

But that's exactly what you shouldn't do! Because then others can trigger modify the GitHub Action with write-access to the repo.

In other words, it'd be a vulnerability.

TL;DR: Combining pull_request_target workflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise.

— Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests

[SethFalco]

I recommend you scroll down to where it says:

Below is an example of the intended usage in which the results of an unprivileged pull_request workflow are combined with a privileged workflow to leave a comment in response to a received PR:

— Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests

You'll see an example of 2 workflows used to leave comments or reviews without opening the vulnerability. The thing you want to avoid is having write permissions in a workflow that checks out the repo.

Workflow 1 (Normal, no write access to repo.)

1. Lint the source
2. Create the message
3. Save message to a file
4. Upload file as artifact

Workflow 2 (Privileged, with write access to repo and no checkout action.)

1. Download artifact
2. Extract zip
3. Pipe contents of file to review

---

It's possible I may have misunderstood something, so if anyone else can chime in, feel free.

[LuigiImVector]

Over here you'll see an example of it using 2 workflows to be able to leave comments/reviews without creating the security issue.

"Incoming data from artifacts is potentially untrusted. When used in a safe manner, like reading PR numbers or reading a code coverage text to comment on the PR, it is safe to use such untrusted data in the privileged workflow context. However if the artifacts were, for example, binaries built from an untrusted PR, it would be a security vulnerability to run them in the privileged workflow_run workflow context. Artifacts resulting from untrusted PR data are themselves untrusted and should be treated as such when handled in privileged contexts."

Even this could not be a totally safe solution

---

Maybe it's enougth to disable the auto-run of the workflow, giving us the ability to approve only non-suspicious PR

[SethFalco]

When used in a safe manner, like reading PR numbers or reading a code coverage text to comment on the PR, it is safe to use such untrusted data in the privileged workflow context

However if the artifacts were, for example, binaries built from an untrusted PR, it would be a security vulnerability to run them in the privileged workflow_run workflow context

You're right, but in this case, it's fine because we're just reading a text file (1st quote), not executing a binary (2nd quote).

We could opt to disable auto-run as well, but I'm thinking we should be fine with the 2 workflow solution. GitHub themselves state it's fine so long as we don't execute untrusted binaries, which we're not doing.

In our case, we're just reading a text file that we ourselves created. The worst thing that could happen is someone intentionally writes something unfavorable to the file, which wouldn't be much different from them just leaving that comment themselves.

[LuigiImVector]

So I'll try to implement the 2 workflows here

[LuigiImVector]

I divided the workflow and now the free-programming-books-lint doesn't any return error; workflow_run is only triggered in the default branch

Here is a test: LuigiImVector#8 (review)

[SethFalco]

I've tested all the suggestions in this repo:
SethFalco#6

---

There is another issue. Since we have 2 workflows named free-programming-books-lint, one for push and one for pull-request, the comment action gets triggered twice, with one always failing since the artifacts don't exist.

I don't think we really need this on push anyway, so perhaps we can drop that one and make this run on pull-request only?

---

Giving it more thought, I originally proposed that we should have the action approve or request. However, I think that might be a bad idea, and we should instead only post a comment.

Since we have many other checks in this repository in other actions, it'd look odd if this action approved the pull request, while other linter actions failed. Contributors will receive contradicting information and won't be sure whether they need to fix something or not. We should probably just post a comment, and later we can try to hide it once it's outdated.

This can be done with the following change:

-             gh pr review $(<PRurl) -r -b "Linter failed, fix the error(s):
+             gh pr comment $(<PRurl) -b "Linter failed, fix the error(s):
              \`\`\`
              $(cat error.log)
              \`\`\`"
              gh pr edit $(<PRurl) --add-label "linter error"
            else
-             gh pr review $(<PRurl) -a

[SethFalco]

Suggested change

	fpb-lint ./books/ &>> output.log || echo "Analyzing..."
	fpb-lint ./casts/ &>> output.log || echo "Analyzing..."
	fpb-lint ./courses/ &>> output.log || echo "Analyzing..."
	fpb-lint ./more/ &>> output.log || echo "Analyzing..."
	fpb-lint books casts courses more &> output.log

The latest release of fpb-lint allows this to be done in one command.

I don't think it's worthwhile to echo Analyzing, since it will be performed after it's already finished anyway.

[SethFalco]

Suggested change

	fpb-lint ./books/
	fpb-lint ./casts/
	fpb-lint ./courses/
	fpb-lint ./more/
	fpb-lint books casts courses more

[SethFalco]

Suggested change

	- name: Clean output
	if: ${{ always() &&
	github.event_name == 'pull_request' }}
	uses: actions/github-script@v6
	with:
	script: |
	const fs = require('fs');
	const readline = require('readline');
	const file = readline.createInterface({
	input: fs.createReadStream('output.log'),
	output: process.stdout,
	terminal: false,
	});
	let lastLine = '';
	file.on('line', (line) => {
	if (lastLine) {
	fs.appendFile('error.log', lastLine, (err) => {
	if (err) {
	console.error(err);
	}
	});
	}
	if (line.includes('/home/runner/work/free-programming-books/')) {
	lastLine = line.replace('/home/runner/work/free-programming-books/', '') + "\r\n";
	} else if (line.includes('\u26a0')) {
	lastLine = '\r\n\r\n';
	} else if (line.includes('remark-lint')) {
	lastLine = line + '\r\n';
	} else {
	lastLine = null;
	}
	});
	file.on('close', () => {
	if (!lastLine || lastLine === '\r\n\r\n') {
	return;
	}
	fs.appendFile('error.log', lastLine, (err) => {
	if (err) {
	console.error(err);
	}
	});
	});
	- name: Upload artifact
	if: ${{ always() &&
	github.event_name == 'pull_request' }}
	run: |
	mkdir -p ./pr
	echo ${{ github.event.pull_request.html_url }} > ./pr/PRurl
	mv error.log ./pr/error.log
	- name: Clean output and create artifacts
	if: ${{ always() &&
	github.event_name == 'pull_request' }}
	run: |
	mkdir -p ./pr
	echo ${{ github.event.pull_request.html_url }} > ./pr/PRurl
	cat output.log | sed -E 's:/home/runner/work/free-programming-books/|⚠.+::' | uniq > ./pr/error.log

I think this would be a lot simpler with bash instead of JavaScript, what do you think? The suggestion should produce the same output. Furthermore, we can merge it with the Upload artifact task below now that, since the cleanup is just a single line.

Command	Description
cat output.log	Get the contents of output.log.
sed -E 's:/home/runner/work/free-programming-books/|⚠.+::'	Replace /home/runner/work/free-programming-books/ and the warning symbol (⚠) and all characters after it until the end of the line with nothing.
uniq	Remove redundant line breaks.

[SethFalco]

Suggested change

	upload:
	upload:
	permissions:
	pull-requests: write

This job would need to explicitly declare write access for pull-request here, I think?
Either that, or in the repo settings someone would have to enable write access by default for all workflows, but that's probably not a good idea for a public repo.

[SethFalco]

Suggested change

	touch error.log

If my other suggestions are merged, these line can be deleted.