Skip to content
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ All notable changes to SkillEvaluator are documented in this file.

### Fixed

- Schema, frontmatter, quality parsing, and security PII scanning accept a leading
UTF-8 BOM, matching the unicode scanner's "benign BOM" note
([#91](https://github.com/NVIDIA/SkillEvaluator/issues/91)).
- Windows personal-path PII now flags `C:\Users\...` usernames that start with
`s` (for example `steve`), matching the intended whitespace class rather than
excluding the letter `s` ([#87](https://github.com/NVIDIA/SkillEvaluator/issues/87)).
Expand Down
4 changes: 2 additions & 2 deletions src/skillevaluator/validators/frontmatter_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

# Regex pattern for extracting frontmatter between --- markers
FRONTMATTER_PATTERN = re.compile(
r"^---[^\S\r\n]*\r?\n(.*?)\r?\n---[^\S\r\n]*(?=\r?\n|\Z)(?:\r?\n)?(.*)",
r"^\ufeff?---[^\S\r\n]*\r?\n(.*?)\r?\n---[^\S\r\n]*(?=\r?\n|\Z)(?:\r?\n)?(.*)",
Comment thread
rng1995 marked this conversation as resolved.
re.DOTALL,
)

Expand Down Expand Up @@ -47,7 +47,7 @@ def parse_frontmatter(file_path: Path) -> tuple[ParsedFrontmatter | None, Valida
result = ValidationResult()

try:
content = file_path.read_text(encoding="utf-8")
content = file_path.read_text(encoding="utf-8-sig")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P1] Make security scanning honor BOM-aware frontmatter

I took another final review pass and found one remaining issue that should be fixed before merge. This utf-8-sig read makes BOM-prefixed frontmatter valid here, but _scan_file_for_pii() still reads the raw file with UTF-8 and _frontmatter_author_emails() requires the first raw line to equal ---. In a fresh real-CLI reproduction, byte-identical plain files exit 0 while BOM-prefixed files exit 1 with a HIGH finding for the valid metadata.author email, for both LF and CRLF. Can we make the security path BOM-aware too and add regression coverage for both line endings before we merge?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Frontmatter and quality parsing accept BOM now, but the security scan path still reads the raw first line in _frontmatter_author_emails(). I will wire that through the same BOM-aware read and add LF/CRLF regression coverage next.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed at c4e249d. _scan_file_for_pii now reads with utf-8-sig and _is_frontmatter_delimiter treats a BOM-prefixed opener as valid, so metadata.author emails stay exempt. Added LF and CRLF regression coverage in test_security.

except Exception as e:
result.add_error(f"Failed to read {file_path}: {e}")
return None, result
Expand Down
7 changes: 4 additions & 3 deletions src/skillevaluator/validators/quality_score.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
from skillevaluator.models.result import Finding, Severity, ValidationResult
from skillevaluator.models.skill import XML_TAG_RE
from skillevaluator.validators.base import ValidatorBase
from skillevaluator.validators.frontmatter_parser import FRONTMATTER_PATTERN
from skillevaluator.validators.markdown import markdown_link_targets

logger = get_logger(__name__)
Expand Down Expand Up @@ -281,7 +282,7 @@ def _validate_single_skill(self, skill_path: Path) -> ValidationResult:
result.metadata["quality_scores"] = qs.to_dict()
return result

content = manifest.read_text(encoding="utf-8")
content = manifest.read_text(encoding="utf-8-sig")
lines = content.split("\n")

frontmatter_data = self._parse_frontmatter(content)
Expand Down Expand Up @@ -333,7 +334,7 @@ def _validate_single_skill(self, skill_path: Path) -> ValidationResult:

@staticmethod
def _parse_frontmatter(content: str) -> dict | None:
fm_match = re.match(r"^---\s*\n(.*?)\n---", content, re.DOTALL)
fm_match = FRONTMATTER_PATTERN.match(content)
if not fm_match:
return None
try:
Expand Down Expand Up @@ -758,7 +759,7 @@ def _check_efficiency(

# Token estimates
qs.total_tokens = len(content) // 4
fm_match = re.match(r"^---\s*\n(.*?)\n---", content, re.DOTALL)
fm_match = FRONTMATTER_PATTERN.match(content)
if fm_match:
qs.frontmatter_tokens = len(fm_match.group(1)) // 4
inst_start = content.find("---", 3) + 3
Expand Down
6 changes: 3 additions & 3 deletions src/skillevaluator/validators/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ def _validate_frontmatter(self, skill_md: Path) -> ValidationResult:
file_path = str(skill_md)

try:
content = skill_md.read_text(encoding="utf-8")
content = skill_md.read_text(encoding="utf-8-sig")
except Exception as e:
result.add_finding(
Finding(
Expand Down Expand Up @@ -439,7 +439,7 @@ def _validate_line_count(self, skill_md: Path) -> ValidationResult:
file_path = str(skill_md)

try:
line_count = len(skill_md.read_text(encoding="utf-8").splitlines())
line_count = len(skill_md.read_text(encoding="utf-8-sig").splitlines())
if line_count > MAX_SKILL_MD_LINES:
result.add_finding(
Finding(
Expand Down Expand Up @@ -490,7 +490,7 @@ def _validate_body_content(self, skill_md: Path) -> ValidationResult:
file_path = str(skill_md)

try:
content = skill_md.read_text(encoding="utf-8")
content = skill_md.read_text(encoding="utf-8-sig")
except Exception:
return result

Expand Down
13 changes: 10 additions & 3 deletions src/skillevaluator/validators/security.py
Original file line number Diff line number Diff line change
Expand Up @@ -1590,6 +1590,11 @@ def _passes_luhn(digits: str) -> bool:
total += n
return total % 10 == 0

@staticmethod
def _is_frontmatter_delimiter(line: str) -> bool:
"""True when a line is a YAML frontmatter fence, including BOM-prefixed openers."""
return line.strip().removeprefix("\ufeff").strip() == "---"

def _scan_file_for_pii(self, file_path: Path, protected_usernames: set[str] | None = None) -> list[dict]:
"""Scan a single file for PII patterns, yielding findings with full context.

Expand All @@ -1601,7 +1606,7 @@ def _scan_file_for_pii(self, file_path: Path, protected_usernames: set[str] | No
protected_usernames = self._protected_home_usernames(file_path.parent)

try:
content = file_path.read_text(encoding="utf-8", errors="ignore")
content = file_path.read_text(encoding="utf-8-sig", errors="ignore")
except Exception as e:
logger.warning(f"Could not read {file_path}: {e}")
return []
Expand Down Expand Up @@ -1630,10 +1635,12 @@ def _scan_file_for_pii(self, file_path: Path, protected_usernames: set[str] | No

def _frontmatter_author_emails(self, file_path: Path, lines: list[str]) -> dict[int, str]:
"""Map valid frontmatter author lines to the public contributor email."""
if file_path.name not in SKILL_MANIFEST_VARIANTS or not lines or lines[0].strip() != "---":
if file_path.name not in SKILL_MANIFEST_VARIANTS or not lines or not self._is_frontmatter_delimiter(lines[0]):
return {}
try:
frontmatter_end = next(index for index, line in enumerate(lines[1:], 1) if line.strip() == "---")
frontmatter_end = next(
index for index, line in enumerate(lines[1:], 1) if self._is_frontmatter_delimiter(line)
)
except StopIteration:
return {}

Expand Down
19 changes: 19 additions & 0 deletions tests/validators/test_frontmatter_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,25 @@ def test_first_body_line_indentation_is_preserved(self, tmp_path: Path, newline:
assert result.passed
assert parsed.content == " [code](false.md)\n"

def test_utf8_bom_is_accepted(self, tmp_path: Path):
"""A valid frontmatter file that starts with a UTF-8 BOM still parses (#91)."""
test_file = tmp_path / "bom.md"
body = """---
title: Bom
description: A file whose only extra is a leading UTF-8 BOM
---

# Content
"""
test_file.write_bytes(b"\xef\xbb\xbf" + body.encode("utf-8"))

parsed, result = parse_frontmatter(test_file)

assert parsed is not None
assert result.passed
assert parsed.yaml_data["title"] == "Bom"
assert parsed.content.strip() == "# Content"

def test_missing_frontmatter(self, tmp_path: Path):
"""Test file without frontmatter markers."""
test_file = tmp_path / "test.mdc"
Expand Down
27 changes: 27 additions & 0 deletions tests/validators/test_quality_score.py
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,33 @@ def test_xml_tags_in_description_remain_quality_error(self, tmp_path):

assert any("Description contains XML tags" in finding.message for finding in result.findings)

def test_bom_prefixed_manifest_still_parses_frontmatter(self, tmp_path):
"""A UTF-8 BOM must not hide frontmatter from the quality parser."""
skill_dir = tmp_path / "bom-xml-desc"
skill_dir.mkdir()
body = (
"---\n"
"name: bom-xml-desc\n"
"description: \"A skill <script>alert('xss')</script> with injected tags\"\n"
"metadata:\n"
" author: Test User <test@nvidia.com>\n"
"---\n\n"
"# XML Description\n\n"
"## Instructions\n\n1. Inspect frontmatter quality findings.\n\n"
"## Examples\n\n"
"```text\n"
"Validate the skill.\n"
"```\n"
)
(skill_dir / "SKILL.md").write_bytes(b"\xef\xbb\xbf" + body.encode("utf-8"))

result = QualityScoreValidator(min_score=0).validate(skill_dir)
scores = result.metadata["quality_scores"]

assert scores["metrics"]["has_frontmatter"] is True
assert scores["metrics"]["frontmatter_tokens"] > 0
assert any("Description contains XML tags" in finding.message for finding in result.findings)

def test_unclosed_xml_tag_in_description_remains_quality_error(self, tmp_path):
"""Unclosed tag-like descriptions remain covered by XML-tag detection."""
skill_dir = tmp_path / "unclosed-xml-desc"
Expand Down
28 changes: 28 additions & 0 deletions tests/validators/test_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,34 @@ def test_all_required_sections_present_passes(self, tmp_path: Path):

assert result.passed, f"Skill with complete body should pass. Errors: {result.errors}"

def test_utf8_bom_skill_md_passes_schema(self, tmp_path: Path):
"""A valid SKILL.md that only adds a UTF-8 BOM must still pass schema (#91)."""
skill_dir = tmp_path / "bom-skill"
skill_dir.mkdir()
body = """---
name: bom-skill
description: Valid skill whose SKILL.md starts with a UTF-8 BOM
metadata:
author: Bom User <bomuser@example.com>
---

# BOM Skill

## Instructions

1. Open the file in an editor that writes a BOM.

## Examples

Example usage.
"""
(skill_dir / "SKILL.md").write_bytes(b"\xef\xbb\xbf" + body.encode("utf-8"))

result = SchemaValidator().validate(skill_dir)

assert result.passed, f"BOM-only difference should still pass schema. Errors: {result.errors}"
assert all(f.check_name != "frontmatter_format" for f in result.findings)

def test_canonical_support_dirs_accepted(self, tmp_path: Path):
"""Canonical public skill support directories must not be flagged.

Expand Down
39 changes: 39 additions & 0 deletions tests/validators/test_security.py
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,45 @@ def test_valid_public_author_email_is_exempt_only_in_frontmatter(self, tmp_path:
assert len(email_findings) == 1
assert email_findings[0].line_content == "Contact contributor@contributors.invalid for private support."

@pytest.mark.parametrize("newline", ["\n", "\r\n"])
def test_bom_prefixed_frontmatter_author_email_not_flagged_in_security_scan(
self, tmp_path: Path, newline: str
):
"""UTF-8 BOM must not turn a valid frontmatter author email into a PII finding."""
skill_dir = tmp_path / "bom-public-author-skill"
skill_dir.mkdir()
skill_md = skill_dir / "SKILL.md"
body = newline.join(
[
"---",
"name: bom-public-author-skill",
"description: A public skill with contributor metadata and a body contact leak",
"metadata:",
" author: Example Contributor <contributor@contributors.invalid>",
"---",
"",
"# Public Author Skill",
"",
"## Instructions",
"",
"Contact contributor@contributors.invalid for private support.",
"",
"## Examples",
"",
"Run the documented workflow.",
"",
]
)
skill_md.write_bytes(b"\xef\xbb\xbf" + body.encode("utf-8"))

schema_result = SchemaValidator().validate(skill_dir)
pii_result = SecurityValidator(submitter_usernames=[]).validate_pii_only(skill_dir)

assert not [finding for finding in schema_result.findings if finding.check_name == "author_format"]
email_findings = [finding for finding in pii_result.findings if finding.check_name == "emails"]
assert len(email_findings) == 1
assert email_findings[0].line_content == "Contact contributor@contributors.invalid for private support."

def test_unrelated_home_roots_not_flagged(self, tmp_path: Path):
"""Unrelated /home roots stay unflagged without an organization allowlist."""
skill_dir = tmp_path / "shared-home-skill"
Expand Down