Skip to content

fix(analyzer): make E2 whitespace-tolerant and detect all os.environ read forms - #331

Open
weed33834 wants to merge 1 commit into
NVIDIA:mainfrom
weed33834:fix/e2-whitespace-tolerant-env-harvesting
Open

fix(analyzer): make E2 whitespace-tolerant and detect all os.environ read forms#331
weed33834 wants to merge 1 commit into
NVIDIA:mainfrom
weed33834:fix/e2-whitespace-tolerant-env-harvesting

Conversation

@weed33834

@weed33834 weed33834 commented Aug 2, 2026

Copy link
Copy Markdown

Summary

The E2 (Env Variable Harvesting) regex patterns are spell-checkers, not behavior detectors. os\.environ without optional whitespace between os and . means that inserting PEP8-irrelevant whitespace — os . environ . copy () — bypasses detection entirely. A skill scoring DO_NOT_INSTALL with canonical syntax can be rewritten to SAFE by adding spaces. The same blind spot lets dict(os.environ) and {**os.environ} pass clean, even though they do exactly what the rule is meant to catch.

Changes

  1. Whitespace-tolerant os.environ matching\s* between os and \. in all Python E2 patterns, so whitespace variants no longer bypass detection.

  2. New patternsdict(os.environ) and {**os.environ} (the two most common alternative forms of reading the full environment mapping) are now detected.

  3. 5 regression tests added to test_patterns.py.

Testing

$ python -m pytest tests/unit/test_patterns.py tests/nodes/analyzers/test_static_patterns.py -v
157 passed in 2.07s
$ make lint && make format
All checks passed!
157 files left unchanged

Fixes #329

rng1995
rng1995 previously approved these changes Aug 5, 2026

@rng1995 rng1995 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.

[Automated SkillSpector Review]

Approved. The E2 additions cover whitespace-obfuscated os.environ access plus full-environment reads through dict() and mapping unpacking without changing the rule schema. Focused E2 regressions passed (7 tests), as did Ruff lint and format checks.

@weed33834
weed33834 force-pushed the fix/e2-whitespace-tolerant-env-harvesting branch from 998d750 to 77a6ce5 Compare August 6, 2026 14:30
Comment thread src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py Outdated

@rng1995 rng1995 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.

[Automated SkillSpector Review]

Re-review: requesting changes on the new head. The 19 focused E2 tests and Ruff lint pass, but the conflict-resolution commit broadens the malformed-Python fallback from mapping unpacking to bare ** os.environ. Exact-head verification shows 2 ** os.environ is then reported as E2 when another syntax error triggers fallback. Restore unpacking boundaries or equivalent parsing and cover the malformed negative case. GitHub also reports merge conflicts.

@weed33834
weed33834 force-pushed the fix/e2-whitespace-tolerant-env-harvesting branch from 77a6ce5 to a92654b Compare August 11, 2026 11:28
@rng1995

rng1995 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

@weed33834 - Please address review comments and resolve merge conflicts. Resolve any review comments that have been addressed

@weed33834
weed33834 force-pushed the fix/e2-whitespace-tolerant-env-harvesting branch from a92654b to f950c35 Compare August 12, 2026 18:51
weed33834 added a commit to weed33834/SkillSpector that referenced this pull request Aug 13, 2026
weed33834 added a commit to weed33834/SkillSpector that referenced this pull request Aug 13, 2026
…sting (NVIDIA#331)

Signed-off-by: badhope <game33834@outlook.com>
@weed33834
weed33834 force-pushed the fix/e2-whitespace-tolerant-env-harvesting branch from 247e544 to 903d4c2 Compare August 13, 2026 08:23
@weed33834

Copy link
Copy Markdown
Author

Hi @rng1995 and maintainers — the DCO issue is now fixed. The branch was rewritten so all commits in this PR carry a proper Signed-off-by line (unsigned helper commits were dropped, and the merge commit with upstream main 2.9.4 is signed too).

The only remaining blocker is that CI runs on this fork PR require maintainer approval (currently action_required). Could you approve the workflow run so DCO + lint + tests can execute? In the last actual CI run (a92654b), lint, unit tests, and docker-smoke all passed — only DCO failed.

@rng1995 if possible, could you re-review the latest head (903d4c2)? The previous CHANGES_REQUESTED was against an older commit.

@rng1995

rng1995 commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

@weed33834 - Fix the CI issues please

…read forms

The E2 regex fallback (used when Python source cannot be parsed by AST)
was missing several common os.environ access patterns and was not
whitespace-tolerant for the patterns it did cover.

Add fallback patterns for:
- os.environ['KEY'] / os.environ["KEY"] (whitespace-tolerant)
- os.environ.get('KEY') (whitespace-tolerant)

All existing patterns (items(), copy(), dict(), {**} spread) retain
whitespace tolerance. The dict-spread regex explicitly requires braces
({**os.environ}) so bare exponentiation (2 ** os.environ) is not
flagged as environment harvesting.

AST-level detection (used when Python parses successfully) now also
covers:
- os.environ['KEY'] / os.environ["KEY"] via ast.Subscript handling
- os.environ.get('KEY') by adding 'get' and 'setdefault' to the
  _ENVIRONMENT_MAPPING_METHOD_CONFIDENCE mapping

This closes the gap where whitespace-obfuscated access
(e.g. `os . environ [ 'API_KEY' ]`) was parsed by the AST but not
emitted as a finding because Subscript nodes were not checked and
the 'get' method was not in the confidence table.

Add regression tests:
- Whitespace-obfuscated environ access is detected (>= 2 findings)
- 2 ** os.environ (exponentiation) is NOT flagged as E2
- {**os.environ} (dict spread) IS flagged as E2

Signed-off-by: badhope <weed33834@users.noreply.github.com>
@weed33834
weed33834 force-pushed the fix/e2-whitespace-tolerant-env-harvesting branch from 26e6371 to 5387c3e Compare August 14, 2026 23:06
@weed33834

Copy link
Copy Markdown
Author

Thanks for the heads-up @rng1995. Both CI issues are now fixed.

DCO Check

The original branch had 5 merge commits without Signed-off-by. I rebased onto the latest upstream/main so the branch now contains a single clean commit with a proper Signed-off-by trailer:

Signed-off-by: badhope <weed33834@users.noreply.github.com>

test-unit: test_e2_whitespace_tolerant_environ_access

Root cause: The test input x = os . environ [ 'API_KEY' ] is valid Python that the AST parser handles, but the AST analyzer in _analyze_python_environment_reads was missing two cases:

  1. ast.Subscript nodesos.environ['KEY'] was not detected because the AST walk only checked ast.Call, ast.Dict, and ast.For/ast.comprehension nodes, not ast.Subscript.
  2. .get() methodos.environ.get('SECRET') was not detected because get was not in _ENVIRONMENT_MAPPING_METHOD_CONFIDENCE (only copy, items, keys, values).

Since the AST path succeeded (the source is valid Python), the regex fallback was never reached — so the whitespace-tolerant regex patterns also didn't help.

Fix:

  • Added "get": 0.7 and "setdefault": 0.6 to _ENVIRONMENT_MAPPING_METHOD_CONFIDENCE.
  • Added an ast.Subscript branch to the AST walk: when the subscripted value is os.environ (resolved through import aliases), it emits an E2 finding.

Verification: The test now expects ≥ 2 findings (subscript + .get()), and both are emitted. I also verified that the exponentiation_not_flagged test still passes — 2 ** os.environ is malformed Python (indentation error in the test case), so it falls through to the regex path, where the dict-spread pattern requires braces and doesn't match.

The force-push is live — CI should re-trigger once approved.

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.

E2 "Env Variable Harvesting" matches one spelling, so whitespace alone flips DO_NOT_INSTALL to SAFE

2 participants