Skip to content

Conversation

@Dongnyoung
Copy link
Contributor

Hello,

This commit updates the username sanitization logic in the profiling
example code to allow only alphanumeric characters. The original
pattern (/[!@#$%^&*]/g) was limited and could lead to inconsistent
behavior depending on input.

Changed:

  • From: username = username.replace(/[!@#$%^&*]/g, '');
  • To: username = username.replace(/[^a-zA-Z0-9]/g, '');

This change makes the input handling cleaner and more appropriate
for educational purposes, aligning better with common sanitization
practices.

Note: This change was initially merged into my forked repository by mistake during development. This PR now reflects the correct version intended for the original repository.

Relates to: #7867

Thank you for reviewing.

Hello,

This commit updates the username sanitization logic in the profiling
example code to allow only alphanumeric characters. The original
pattern (/[!@#$%^&*]/g) was limited and could lead to inconsistent
behavior depending on input.

Changed:
- From: username = username.replace(/[!@#$%^&*]/g, '')
- To:   username = username.replace(/[^a-zA-Z0-9]/g, '')

This change makes the input handling cleaner and more appropriate
for educational purposes, aligning better with common sanitization
practices.
Relates to: nodejs#7867

Signed-off-by: DongNyoung Lee <121621378+Dongnyoung@users.noreply.github.com>
feat: improve username sanitization in profiling example
@Dongnyoung Dongnyoung requested a review from a team as a code owner June 15, 2025 09:04
@vercel
Copy link

vercel bot commented Jun 15, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Jun 15, 2025 9:05am

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

In theory it's good. But I don't know what is the value behind this example

@Dongnyoung
Copy link
Contributor Author

Thanks for the feedback!

I suggested this change to make the example more aligned with common input sanitization practices. The original regex was quite limited, so I thought a more realistic pattern might help learners better understand typical use cases.

Happy to adjust further if needed!

@codecov
Copy link

codecov bot commented Jun 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.46%. Comparing base (1ccf19c) to head (a3e1ef3).
Report is 4 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7868   +/-   ##
=======================================
  Coverage   75.46%   75.46%           
=======================================
  Files         101      101           
  Lines        8306     8306           
  Branches      218      218           
=======================================
  Hits         6268     6268           
  Misses       2036     2036           
  Partials        2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Dongnyoung Dongnyoung requested a review from avivkeller June 16, 2025 05:28
@avivkeller avivkeller enabled auto-merge June 17, 2025 00:09
@avivkeller avivkeller added the github_actions:pull-request Trigger Pull Request Checks label Jun 17, 2025
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Jun 17, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jun 17, 2025

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 96 🟢 100 🟠 82 🔗
/en/about/previous-releases 🟢 99 🟢 96 🟢 100 🟠 83 🔗
/en/download 🟢 98 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 96 🟢 92 🔗

@avivkeller avivkeller added this pull request to the merge queue Jun 17, 2025
Merged via the queue into nodejs:main with commit 616d437 Jun 17, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants