Skip to content

test: expand tls-check-server-identity coverage - #60002

Closed
diangogav wants to merge 1 commit into
nodejs:mainfrom
diangogav:diangogav/test-tls-hostname-matching
Closed

test: expand tls-check-server-identity coverage#60002
diangogav wants to merge 1 commit into
nodejs:mainfrom
diangogav:diangogav/test-tls-hostname-matching

Conversation

@diangogav

Copy link
Copy Markdown
Contributor
  • Falsy pattern via SAN: DNS: (triggers if (!pattern) return false)
  • CN array with empty string: CN: ['']
  • Empty label in pattern: bad..example.com (CN and SAN)
  • Non-ASCII/blank label: caf\u00E9.example.com and bad label.com
  • Multiple wildcards in first label: b**.a.com (CN and SAN)
  • Prefix mismatch with single wildcard: ab*cd.a.com vs xbcd.a.com (CN and SAN)

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 24, 2025
@Hritikk999

Copy link
Copy Markdown

Waiting for approval;
Load approved;
Load approval in Phase T BLOCK; Block released by Developer Node;
Markdown supported in OS

@RafaelGSSRafaelGSS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you expand on why these additional checks are important? Any coverage info?

@diangogav

diangogav commented Sep 25, 2025

Copy link
Copy Markdown
ContributorAuthor

Hellooo @RafaelGSS ! I saw that some validations in the check function were not covered when looking at the coverage, and that is what this PR covers.

https://app.codecov.io/gh/nodejs/node/blob/main/lib%2Ftls.js

@RafaelGSS

This comment was marked as resolved.

@diangogav

This comment was marked as resolved.

@RafaelGSSRafaelGSS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Never mind, the link you originally sent is enough. LGTM with green ci!

@RafaelGSSRafaelGSS added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 25, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecovBot commented Sep 25, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.45%. Comparing base (d2ff9da) to head (fd96c23).
⚠️ Report is 27 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #60002 +/- ##
=======================================
Coverage 88.45% 88.45% =======================================
Files 703 703 Lines 207545 207529 -16 Branches 40013 40010 -3 =======================================
- Hits 183578 183572 -6 + Misses 15957 15940 -17 - Partials 8010 8017 +7 

see 44 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Hritikk999

This comment was marked as spam.

@Hritikk999

This comment was marked as spam.

addaleax pushed a commit that referenced this pull request Sep 30, 2025
PR-URL: #60002
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax

Copy link
Copy Markdown
Member

Landed in 93ee08c

@diangogav
diangogav deleted the diangogav/test-tls-hostname-matching branch September 30, 2025 23:59
targos pushed a commit that referenced this pull request Oct 6, 2025
PR-URL: #60002
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
aduh95 pushed a commit that referenced this pull request Oct 22, 2025
PR-URL: #60002
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@diangogav@Hritikk999@RafaelGSS@nodejs-github-bot@addaleax