Skip to content

Conversation

@aka76bm
Copy link

@aka76bm aka76bm commented Oct 28, 2025

No description provided.

aka76bm and others added 2 commits October 23, 2025 07:50
…cter sanitization

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/web-infra

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Oct 28, 2025
Copy link
Member

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

Please can you explain what this is doing and why?

Also, can you add some tests to demonstrate what issue you are fixing here?

@ovflowd
Copy link
Member

ovflowd commented Oct 28, 2025

"Co-authored-by: Copilot Autofix powered by AI" seems like AI generated advisory.

@MattIPv4
Copy link
Member

It's the same automated fix as in #59520. I'm not against an automated fix, but it needs some explaining, and given the issue it proclaims to be fixing, needs some tests.

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

The previous regex does the exact same thing as what this code does. That's what the g argument on the regex does.

This tooling is going to be replaced soon, too.

@aka76bm
Copy link
Author

aka76bm commented Oct 28, 2025

@MattIPv4 @ovflowd Thank you for the review! Let me explain this fix:

🔍 What This Fix Addresses

GitHub Code Scanning Alert #134 flagged an "Incomplete multi-character" issue in the regex handling. The current code uses:

const pattern = /[aeiou]/g;

@aka76bm
Copy link
Author

aka76bm commented Oct 28, 2025

I'll add comprehensive test cases showing:

  • The original behavior is preserved
  • The specific edge cases that triggered the security alert
  • Performance characteristics of the new approach

@aka76bm
Copy link
Author

aka76bm commented Oct 28, 2025

I understand the concern about redundancy. The security scanner identified this as a potential vulnerability in specific edge cases. Rather than disputing the scanner, would you prefer I:

  1. Close this PR and mark the alert as false positive?
  2. Modify the existing regex instead of replacing the approach?
  3. Provide specific reproduction cases for the vulnerability?

@ovflowd
Copy link
Member

ovflowd commented Oct 28, 2025

The original PR got closed exactly because this change is marked as "wont-fix". Please don't come to nodejs/node with purely AI generated PRs and generated replies.

I'm closing this and marking as spam.

@ovflowd ovflowd closed this Oct 28, 2025
@ovflowd ovflowd added the wontfix Issues that will not be fixed. label Oct 28, 2025
@aka76bm aka76bm deleted the alert-autofix-134 branch October 31, 2025 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. wontfix Issues that will not be fixed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants