Skip to content

fix(pe3): treat ignore-list code fences in Markdown as ignore-file entries - #831

Open
jwcastillo wants to merge 2 commits into
NVIDIA:mainfrom
jwcastillo:fix/pe3-ignore-list-code-fences
Open

jwcastillo wants to merge 2 commits into
NVIDIA:mainfrom
jwcastillo:fix/pe3-ignore-list-code-fences

Conversation

@jwcastillo

Copy link
Copy Markdown
Contributor

Problem

#787 stopped PE3 from flagging single-pattern lines of real ignore files. The same lines shown in a Markdown code block labelled as an ignore list still raise HIGH PE3 (issue #830):

```gitignore
# Environment files
.env
.env.local
```

Fix

static_patterns_privilege_escalation.py now collects the body ranges of fenced blocks whose info string starts with an ignore-list language: gitignore, git-ignore, ignore (GitHub Linguist's "Ignore List" aliases) or dockerignore, case-insensitive, first word only. It does this for Markdown and text files only. A match inside such a block goes through the existing _is_ignore_file_exclusion_entry check, exactly like a line of a real ignore file.

Still flagged:

  • comments, ! negations, commands, host paths and any line that is not one pattern token;
  • unfenced lines and lines after the fence;
  • other fence languages (bash, text, yaml, no language, gitignore-like, ...);
  • the same fence inside a non-Markdown file, such as a Python docstring.

Tests

tests/nodes/analyzers/test_pe3_ignore_file_entries.py gets a new TestIgnoreListCodeFencesInMarkdown class with 33 tests:

on main @ 4c52fbb this branch
exempt cases (ignore-list fences in SKILL.md / README.md / references, tilde fence, unclosed fence) 21 fail pass
fail-closed controls (other languages, negation, comment, command, host paths, prose after fence, script file) 12 pass pass
  • make lint and make format pass.
  • pytest -m "not integration and not provider" tests/ gives 13702 passed, 20 skipped, 4 xfailed and 2 failed. The 2 failures are test_suppression.py::test_dump_baseline_concurrent_writers_publish_complete_documents[.yaml/.json], which failed under load in the full serial run. Rerun on their own, they pass on both this branch and main.

Real-world check

skillspector scan skills/git-workflow --no-llm on affaan-m/ECC:

  • main reports PE3 on lines 522 and 523 (.env and .env.local inside the ```gitignore block) and on line 669.
  • This branch reports only line 669, which is git add .env # Contains API keys in a "BAD" example. It is a command, so it should stay flagged.

Fixes #830. Refs #644.

Prepared with Claude Code; I reviewed and tested the change.

…tries

NVIDIA#787 stopped PE3 from flagging `.env` and other credential paths when they
are single-pattern lines of a real ignore file (.gitignore, .dockerignore,
...). The same lines shown in a Markdown code block labelled as an ignore
list (```gitignore, ```git-ignore, ```ignore, ```dockerignore) still raise
HIGH PE3, which is common in skills that document a project's .gitignore.

Fenced blocks whose info string starts with one of those languages now go
through the existing `_is_ignore_file_exclusion_entry` check, so only lines
that are one pattern token are exempt. Comments, negations (`!.env`),
commands, host paths, prose after the fence, other fence languages, and
unfenced lines keep their findings. Only Markdown and text files are
affected.

Fixes NVIDIA#830. Refs NVIDIA#644.

Signed-off-by: Wen <jwcastillo@gmail.com>

@yashrajp22 yashrajp22 left a comment

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.

One remaining integration issue: the ignore-list exemption is lost across production scan windows. Confirmed in source and a freshly built wheel; the inline comment includes the boundary case.

reference_material = is_reference_material(file_path, file_type)
ignore_pattern_file = _is_ignore_pattern_file(file_path)
ignore_list_fences = (
_markdown_ignore_list_fence_ranges(content) if file_type in {"markdown", "text"} else []

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.

Could we preserve the ignore-list language across scan windows? The production runner rebuilds an open fence using only its marker and width, so later windows lose the gitignore label this helper needs. I reproduced this with a 256,001-character Markdown file containing one gitignore block and .env near the end: direct analyze() returns no PE3, but the runner and CLI still report it at line 36,571, in both source and a fresh wheel. The 256,000-character control passes. Please carry the language into the window context and add a regression check through the production runner.

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.

Thanks @yashrajp22, confirmed and fixed in cdc7599.

Cause: _markdown_context_prefix rebuilt an open fence as marker * width + "\n", so every window after the first saw a bare fence and the gitignore label was gone.

Fix (static_runner.py): the fence state and the open/close transitions now also record the opening info string, and the synthetic opening line includes it (```gitignore). This applies to all three prefix cases: inside a fence, a window starting on the opening line, and a window starting on the closing line. Findings inside the synthetic prefix still map before the owned range, so the prefix itself does not add findings.

Regression checks through run_static_patterns (TestIgnoreListFenceAcrossScanWindows):

  • a ```gitignore fence longer than SECURITY_VIEW_WINDOW_CHARS with `.env` in a later window: no PE3 (failed before the fix at line 36574);
  • the same file with a ```bash fence: PE3 still reported on the .env line.

CLI check on a 256,001-character SKILL.md with one gitignore block and .env at the end: no PE3 on this branch.

…s a fence

Long files are scanned in windows. A window that starts inside a fenced
block gets a synthetic opening fence, which only carried the marker and
width. Analyzers that depend on the fence language, such as the PE3
ignore-list fence exemption, lost it after the first window, so `.env`
inside a long ```gitignore block was still reported by the runner and CLI.

Fence state and transitions now record the opening info string, and the
synthetic opening line includes it.

Signed-off-by: Wen <jwcastillo@gmail.com>
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.

PE3 flags .env entries inside ```gitignore code fences in Markdown

2 participants