Added a validation check for pr approval in the deploy-pr workflow

[devanshkansagra]

This Pull request adds an additional pr approval check in deploy-pr.yml workflow. This adds validation to ensure the deploy-pr workflow runs only after PRs are approved by collaborators or owners. The GitHub API is used to verify approval status, preventing unauthorized deployments from forked repositories.

Acceptance Criteria fulfillment

• Add an approval validation check in the deploy-pr workflow

Videos/Screenshots

N/A

[devanshkansagra]

Hey @Spiral-Memory, I have worked on this vulnerablity which btw was difficult to find out and finally I have found the fix after researching. I have tested using my other github account, you can also test it here for better clearence https://github.com/devanshkansagra/Embedded-Chat . Also please review this pull request

[Spiral-Memory]

Awesome work @devanshkansagra

Will discuss about this with you today.

[devanshkansagra]

Okay sure. Then let's finish up this as early as possible coz this was very long time awaited

[Spiral-Memory]

Still carries the same vulnerability as discussed.

[devanshkansagra]

Hey @Spiral-Memory, I have removed the part of transfering pr number as artifact and instead I have used github api to fetch the pr number in deploy-pr workflow

[devanshkansagra]

You can check and review whenever you are free

[Spiral-Memory]

Awesome work @devanshkansagra

Is it working ?
Also, can you explain a bit, how exactly are you getting PR number ? I mean how will this workflow know, which build-pr workflow triggered it ?

[devanshkansagra]

Yes @Spiral-Memory, this workflow is working. You can check it out here in my test repo https://github.com/devanshkansagra/Embedded-Chat by raising a demo pr
The thing I researched and found out that the URL for the GitHub API endpoint to list pull requests. It filters PRs by the head branch github.head_ref and repository owner github.repository_owner. jq -r '.[0].number': Pipes the JSON response to jq, which is a command-line JSON processor. It extracts the number of the first pull request in the list

This is based on the thing I researched on the internet and various AIs including ChatGPT, Github-Copilot etc. also I haven't gone in depth regarding this Github-API, and jq stuff

and same goes with the checking approval status

[Spiral-Memory]

No, what i am asking is, with this, it will check the PR permission for the first PR in the list, not the PR it is triggered on, have you tested this ?

[devanshkansagra]

Yea I have tested it by raising two different prs in parallel

[Spiral-Memory]

Okay then tomorrow, we will see how exactly is it working and then will merge it if testing succeeds.

[devanshkansagra]

Okay No problem

[devanshkansagra]

No, what i am asking is, with this, it will check the PR permission for the first PR in the list, not the PR it is triggered on, have you tested this ?

Hey you are right, yesterday I ran the approval check was shown the respective pr numbers but today I have tested very parallel, it was checking the top of the list pr number

[devanshkansagra]

Hey @Spiral-Memory, I have pushed the commit, you can check / review it

[devanshkansagra]

Hey @Spiral-Memory, I have one thing to ask, in build-and-lint workflow is the last step of building the project necessary or is it going to use further? if not then we can consider it unnecessary and remove it. also I think the major work of this workflow is to check format and linting issues in the pull request right?

[Spiral-Memory]

No not just that, it also tests if project is getting built correctly after the new change so it shouldn't break the project

[devanshkansagra]

ohh okay

[devanshkansagra]

One thing to share, I have observed currently that if you merge/close/approve multiple prs at same time or the small time diff then pages-build workflow will be skipped, this is currently there

[devanshkansagra]

To overcome this let the workflow get's fully completed then approve / merge next pr

[Spiral-Memory]

That's Okay... Apart from that, is it good to merge this?

[devanshkansagra]

Yea sure, you can go for it

[Spiral-Memory]

Getting a critial security alert, for now reverting the PR, You can take your time to research why codescan is giving this alert

[devanshkansagra]

But how it can cause, during this research I didn't got any this type of error, I have the duplicated the exact things and created a dummy test repo

[Spiral-Memory]

That's fine @devanshkansagra
Take your time...

Before reverting, let me atleast test that if it is working or not (ignoring security alert)

[devanshkansagra]

Okk

[Spiral-Memory]

Hi @devanshkansagra
It is working fine, just a security alert is present, what do you think, should i revert ?

or you will add a fix soon ?

[Spiral-Memory]

https://github.com/RocketChat/EmbeddedChat/security/code-scanning/14

[devanshkansagra]

You made me sweaty in winters after showing me that security alert error. Currently it is more secure than before, so I will add that fix in my next pr

[Spiral-Memory]

https://github.com/RocketChat/EmbeddedChat/actions/runs/12453376805

[Spiral-Memory]

This new change is something causing deploy error too
jq: error (at :5): Cannot index string with string "submitted_at"

[Spiral-Memory]

I need to revert this change, let me know once you test it well and working fine
No need to worry