Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ All notable changes to SkillEvaluator are documented in this file.

### Fixed

- SPDX headers keep the full license expression, so `MIT OR GPL-3.0` is
no longer truncated to MIT and allowed. Closing comment markers such as
`*/` and `-->` are not treated as part of the expression
([#86](https://github.com/NVIDIA/SkillEvaluator/issues/86)).
- 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
2 changes: 1 addition & 1 deletion src/skillevaluator/config/license_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ proprietary_indicators:

spdx_detection:
# Regex pattern to extract SPDX identifier
pattern: "SPDX-License-Identifier:\\s*([A-Za-z0-9.+-]+)"
pattern: "SPDX-License-Identifier:\\s*([^\\n]+)"

# Number of lines to scan from the beginning of each file
scan_lines: 50
Expand Down
2 changes: 1 addition & 1 deletion src/skillevaluator/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@
LICENSE_HEADER_EXTENSIONS = {".py", ".sh", ".js", ".ts", ".yaml", ".yml", ".md", ".txt", ".json"}

# SPDX header pattern for source files
SPDX_LICENSE_PATTERN = r"SPDX-License-Identifier:\s*([A-Za-z0-9.\-+]+)"
SPDX_LICENSE_PATTERN = r"SPDX-License-Identifier:\s*([^\n]+)"

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] Stop the capture before comment terminators

Capturing the entire line leaves closing comment syntax attached to the final SPDX symbol. For example, /* SPDX-License-Identifier: MIT OR GPL-3.0 */ in a scanned .js file becomes MIT OR GPL-3.0 */; GPL-3.0 */ is treated as unknown, and default (non-strict) validation passes with only a warning. HTML-comment headers have the same problem. Please parse a valid SPDX expression without *//--> (ideally with the existing SPDX expression parser) so the blocked component cannot be hidden.

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.

Yep, /* SPDX-License-Identifier: MIT OR GPL-3.0 */ was capturing GPL-3.0 */ as the last symbol, so default mode only warned.

I strip */ and --> after the capture, then evaluate the expression as before. Added JS block-comment and HTML-comment cases; both fail closed on GPL-3.0.

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.

Thanks for adding the block-comment and HTML cases. This still strips the terminator only when it is at the end of the line. With an inline block comment containing SPDX-License-Identifier: MIT OR GPL-3.0 followed by code on the same line, the GPL identifier retains the terminator and code suffix, so default validation passes with only an unknown-license warning. Please stop the capture at the first comment terminator wherever it occurs and add an inline-code regression.

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 on inline block comments. _strip_spdx_capture now truncates at the first */ or --> anywhere in the captured span, not only at end-of-line. Added a regression with trailing code on the same line.

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.

Inline */ truncation is in _strip_spdx_capture now, with a same-line code suffix regression.

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.

Merged main and pushed another fix here. _strip_spdx_capture now truncates at the first */ or --> anywhere in the captured span, not only at end-of-line. Added a regression for inline block comments that share a line with trailing source.

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.

Replying on your inline thread: inline */ truncation is in _strip_spdx_capture now, with a same-line code suffix regression. Merged main as well (2954bf9).



# =============================================================================
Expand Down
52 changes: 51 additions & 1 deletion src/skillevaluator/validators/license.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,18 @@
_SPDX_PATTERN = re.compile(SPDX_LICENSE_PATTERN, re.IGNORECASE)
_LICENSE_SUFFIX_PATTERN = re.compile(r"(-license|-licence)$")
_THE_PREFIX_PATTERN = re.compile(r"^(the-)?")
_SPDX_EXPRESSION_SPLIT = re.compile(r"\s+(?:OR|AND|WITH)\s+", re.IGNORECASE)


def _strip_spdx_capture(raw: str) -> str:
"""Drop inline comment terminators and trailing source after the SPDX value."""
raw = raw.split("#", 1)[0]
for marker in ("*/", "-->"):
idx = raw.find(marker)
if idx != -1:
raw = raw[:idx]
return raw.strip()


# File reference indicators in license field values
_FILE_REFERENCE_KEYWORDS = frozenset(["see ", "refer to ", "license.txt", "license.md", "copying"])
Expand Down Expand Up @@ -510,18 +522,30 @@ def _extract_spdx_from_file(file_path: Path) -> str | None:
with file_path.open(encoding="utf-8", errors="ignore") as f:
header = "".join(islice(f, LICENSE_HEADER_SCAN_LINES))
if match := _SPDX_PATTERN.search(header):
return match.group(1).strip()
raw = _strip_spdx_capture(match.group(1))
return raw or None
except Exception as e:
logger.debug("Could not scan %s: %s", file_path, e)
return None

@staticmethod
def _expression_symbols(license_id: str) -> list[str]:
"""Split an SPDX expression into identifiers, keeping hyphenated ids intact."""
stripped = license_id.replace("(", " ").replace(")", " ").strip()
return [part.strip() for part in _SPDX_EXPRESSION_SPLIT.split(stripped) if part.strip()]

def _validate_license(self, detection: LicenseDetection, result: ValidationResult) -> None:
"""Validate detected license against allowlist/blocklist."""
license_id = detection.license_id
if not license_id:
result.add_warning("License detection returned empty identifier")
return

symbols = self._expression_symbols(license_id)
if len(symbols) > 1:
self._validate_license_expression(detection, symbols, result)
return

normalized = self._normalize_license_id(license_id)

if normalized in self._normalized_allowlist:
Expand All @@ -534,6 +558,32 @@ def _validate_license(self, detection: LicenseDetection, result: ValidationResul
self._set_license_metadata(result, detection, "unknown")
self._handle_unknown_license(result, license_id, detection.file_path)

def _validate_license_expression(
self,
detection: LicenseDetection,
symbols: list[str],
result: ValidationResult,
) -> None:
"""Fail closed if any SPDX expression symbol is blocked or unknown when required."""
blocked = [symbol for symbol in symbols if self._normalize_license_id(symbol) in self._normalized_blocklist]
if blocked:
self._set_license_metadata(result, detection, "blocked")
result.add_message(
f"SPDX expression '{detection.license_id}' includes blocked license '{blocked[0]}'"
)
self._add_blocked_license_finding(result, blocked[0], detection.file_path)
return
if all(self._normalize_license_id(symbol) in self._normalized_allowlist for symbol in symbols):
self._set_license_metadata(result, detection, "allowed")
result.add_message(f"License: {detection.license_id} (ALLOWED - permissive)")
return
unknown = next(
(symbol for symbol in symbols if self._normalize_license_id(symbol) not in self._normalized_allowlist),
detection.license_id,
)
self._set_license_metadata(result, detection, "unknown")
self._handle_unknown_license(result, unknown, detection.file_path)

@staticmethod
def _set_license_metadata(result: ValidationResult, detection: LicenseDetection, status: str) -> None:
"""Set license metadata on validation result."""
Expand Down
124 changes: 124 additions & 0 deletions tests/validators/test_license.py
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,130 @@ def test_multiple_files_same_license(self, tmp_path: Path):
assert result.passed
assert "MIT" in result.metadata.get("license", "")

def test_spdx_or_expression_with_gpl_is_blocked(self, tmp_path: Path):
"""MIT OR GPL-3.0 in an SPDX header must not be truncated to allowed MIT (#86)."""
skill_dir = tmp_path / "spdx-or-gpl"
skill_dir.mkdir()
(skill_dir / "SKILL.md").write_text("""---
name: spdx-or-gpl
description: Skill whose only license signal is an SPDX OR expression
---

# SPDX OR
""")
(skill_dir / "script.py").write_text("""# SPDX-License-Identifier: MIT OR GPL-3.0
print("hi")
""")

result = LicenseValidator().validate(skill_dir)

assert not result.passed
assert result.metadata.get("license_status") == "blocked"
assert "GPL-3.0" in (result.metadata.get("license") or "")
assert any(f.check_name == "blocked_license" for f in result.findings)
assert not any("License: MIT (ALLOWED" in message for message in result.messages)

def test_spdx_or_expression_in_block_comment_is_blocked(self, tmp_path: Path):
"""`*/` after MIT OR GPL-3.0 must not turn the GPL half into an unknown id."""
skill_dir = tmp_path / "spdx-js-block"
skill_dir.mkdir()
(skill_dir / "SKILL.md").write_text("""---
name: spdx-js-block
description: Skill whose SPDX header is inside a JavaScript block comment
---

# SPDX JS
""")
(skill_dir / "script.js").write_text("/* SPDX-License-Identifier: MIT OR GPL-3.0 */\n")

result = LicenseValidator().validate(skill_dir)

assert not result.passed
assert result.metadata.get("license_status") == "blocked"
assert "GPL-3.0" in (result.metadata.get("license") or "")
assert "*/" not in (result.metadata.get("license") or "")
assert any(f.check_name == "blocked_license" for f in result.findings)

def test_spdx_inline_block_comment_with_code_suffix_is_blocked(self, tmp_path: Path):
"""Inline `*/` plus trailing code must not leave GPL-3.0 */ attached to the symbol."""
skill_dir = tmp_path / "spdx-js-inline"
skill_dir.mkdir()
(skill_dir / "SKILL.md").write_text("""---
name: spdx-js-inline
description: Skill whose SPDX header shares a line with trailing code
---

# SPDX inline
""")
(skill_dir / "script.js").write_text("/* SPDX-License-Identifier: MIT OR GPL-3.0 */ const x = 1;\n")

result = LicenseValidator().validate(skill_dir)

assert not result.passed
assert result.metadata.get("license_status") == "blocked"
assert "GPL-3.0" in (result.metadata.get("license") or "")
assert "*/" not in (result.metadata.get("license") or "")
assert any(f.check_name == "blocked_license" for f in result.findings)

def test_spdx_or_expression_in_html_comment_is_blocked(self, tmp_path: Path):
"""`-->` after MIT OR GPL-3.0 must not hide the GPL half."""
skill_dir = tmp_path / "spdx-html-comment"
skill_dir.mkdir()
(skill_dir / "SKILL.md").write_text("""---
name: spdx-html-comment
description: Skill whose SPDX header is inside an HTML comment
---

# SPDX HTML
""")
(skill_dir / "notes.md").write_text("<!-- SPDX-License-Identifier: MIT OR GPL-3.0 -->\n")

result = LicenseValidator().validate(skill_dir)

assert not result.passed
assert result.metadata.get("license_status") == "blocked"
assert "GPL-3.0" in (result.metadata.get("license") or "")
assert "-->" not in (result.metadata.get("license") or "")
assert any(f.check_name == "blocked_license" for f in result.findings)

def test_spdx_and_expression_with_gpl_is_blocked(self, tmp_path: Path):
"""Apache-2.0 AND GPL-3.0 must evaluate the copyleft half, not only Apache-2.0."""
skill_dir = tmp_path / "spdx-and-gpl"
skill_dir.mkdir()
(skill_dir / "SKILL.md").write_text("""---
name: spdx-and-gpl
description: Skill whose only license signal is an SPDX AND expression
---

# SPDX AND
""")
(skill_dir / "script.py").write_text("# SPDX-License-Identifier: Apache-2.0 AND GPL-3.0\n")

result = LicenseValidator().validate(skill_dir)

assert not result.passed
assert any(f.check_name == "blocked_license" for f in result.findings)
assert result.metadata.get("license_status") != "allowed"

def test_spdx_or_expression_of_allowed_licenses_passes(self, tmp_path: Path):
"""Compound expressions stay allowed when every symbol is permissive."""
skill_dir = tmp_path / "spdx-or-mit"
skill_dir.mkdir()
(skill_dir / "SKILL.md").write_text("""---
name: spdx-or-mit
description: Skill whose SPDX header is MIT OR MIT-0
---

# SPDX OR allowed
""")
(skill_dir / "script.py").write_text("# SPDX-License-Identifier: MIT OR MIT-0\n")

result = LicenseValidator().validate(skill_dir)

assert result.passed
assert result.metadata.get("license_status") == "allowed"
assert "MIT OR MIT-0" in (result.metadata.get("license") or "")


# =============================================================================
# NO LICENSE / UNKNOWN LICENSE TESTS
Expand Down