Skip to content

fix(security): normalize concealed instruction text - #408

Open
Spectorian wants to merge 11 commits into
mainfrom
codex/security-text-normalization
Open

fix(security): normalize concealed instruction text#408
Spectorian wants to merge 11 commits into
mainfrom
codex/security-text-normalization

Conversation

@Spectorian

@Spectorian Spectorian commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • normalize security-analysis text with version-pinned default-ignorable handling
  • reconstruct bounded inter-character separator runs with exact gap signatures and raw source-offset attribution
  • add a scored analysis-evasion signal for security-relevant condensed terms that cannot be reconstructed safely
  • treat confirmed inter-character obfuscation as a nonfatal interpretation limit so strict CLI and MCP consumers fail closed
  • preserve benign multilingual text, emoji, punctuation, notation, URLs, e-mail addresses, tables, and code through negative controls

Validation

  • sanitized static-only coverage for ASCII whitespace, punctuation, symbols, middle dot, and bullet separators
  • analyzer, SARIF location, cross-window, resource-limit, and false-positive regressions
  • completeness parity across reports, default and strict CLI modes, and MCP install verdicts
  • branch-wide non-integration/non-provider suite
  • Ruff lint, format, targeted mypy, and git diff --check

@rng1995
rng1995 marked this pull request as ready for review August 24, 2026 18:15

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

Requesting changes because the normalizer still has two fail-open concealed-instruction paths. Each current-head repro yields only the raw view, no P1/AE6 finding, risk 0, SAFE, complete analysis, and safe_to_install=True.

Comment thread src/skillspector/artifacts.py Outdated
start = offset
while offset < len(text) and _is_token_gap_character(text[offset]):
offset += 1
if (

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] Normalize default-ignorables at token boundaries too. Requiring word characters on both sides misses ignore\u034f previous instructions. (and the inverse boundary form), so the concealed directive reaches a SAFE install verdict with no normalized view. Remove contextual default-ignorables when they adjoin a word boundary while preserving source-offset mapping and emoji-sensitive cases.

break

next_letter_end = cursor + 1
if next_letter_end < len(text) and text[next_letter_end].isalpha():

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] The fail-closed AE6 path reuses an exact recognizer that rejects common adversarial spacing. i g n o r eall previous instructions. stops here because the next character is alphabetic; i.-g.-n.-o.-r.-e previous instructions. is rejected by the single-marker signature, and per-letter line breaks are excluded earlier. These inputs all receive a complete SAFE verdict. Use a broader, separately bounded detector for AE6/partial coverage instead of treating exact-compaction rejection as proof that no concealed run exists.

Signed-off-by: Nir Paz <npaz@nvidia.com>
@mohgupta-ship-it

mohgupta-ship-it commented Aug 25, 2026

Copy link
Copy Markdown
Member

Powered by Codex: PR council review result.

This is a triage signal, not a maintainer approval.

  • Rating: critical fix
  • Confidence: medium-high
  • Status read: Changes requested, green checks
  • Review method: fresh GitHub metadata/body/files/reviews/checks plus selected diffs; council lenses were spec fit, dead-code/reachability, YAGNI/scope, design/coupling, and code standards/tests.
  • Council assessment: Addresses concealed-instruction normalization misses that could yield SAFE verdicts, but review remains unresolved.
  • Recommended action: Request fresh reviewer clearance before merge.

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.

3 participants