Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add CodeQL Action #4315

Merged
merged 2 commits into from
Nov 12, 2022
Merged

Add CodeQL Action #4315

merged 2 commits into from
Nov 12, 2022

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Dec 20, 2021

  • See if everyone's OK with the cron job
  • Tackle the errors

About this error: https://github.com/nodejs/nodejs.org/security/code-scanning/1?query=ref%3Arefs%2Fpull%2F4315%2Fmerge, I still do not get why we need this client side. I'm pretty sure I expressed my objection in the relevant PR, but due to lack of time, I couldn't spend more time then.

Does anybody recall why we need this client side and why we don't generate the Edit on GitHub links on build time?

EDIT: I see now it was done #3971. I still don't quite get what the issue was and why we can't fix it on build time...

@XhmikosR XhmikosR marked this pull request as ready for review December 20, 2021 09:52
@XhmikosR XhmikosR requested a review from a team as a code owner December 20, 2021 09:52
@XhmikosR XhmikosR added the CI label Dec 28, 2021
@Trott
Copy link
Member

Trott commented Mar 8, 2022

Does anybody recall why we need this client side and why we don't generate the Edit on GitHub links on build time?

I agree that there's no evident reason for it to be client side and not done at build time. If you want to move it to a build step, I'd 👍 that.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Mar 8, 2022

TBH I was hoping someone else would make the changes since I didn't really follow them and I don't have a lot of time this period :/

Copy link
Contributor

@SEWeiTung SEWeiTung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for me

SEWeiTung added a commit that referenced this pull request Oct 9, 2022
We don't need to implement it through the client side but serve side only by generating them together.

Refs: #4315.
@SEWeiTung
Copy link
Contributor

SEWeiTung commented Oct 9, 2022

This alert won't accept the input from the browser, and anyway, it will convert each word splitted by '-', so it cannot be a risk here. I ignored it and merge it.

@SEWeiTung SEWeiTung merged commit 1880e89 into main Nov 12, 2022
@SEWeiTung SEWeiTung deleted the codeql branch November 12, 2022 07:50
@Trott
Copy link
Member

Trott commented Nov 12, 2022

This alert won't accept the input from the browser, and anyway, it will convert each word splitted by '-', so it cannot be a risk here. I ignored it and merge it.

We're now going to get that alert on every pull request, aren't we?

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Nov 12, 2022 via email

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants