Skip to content

validate and remediate lowercase -f for susceptible gh fields (Pattern 2)#218

Merged
potiuk merged 1 commit into
apache:mainfrom
justinmclean:lowercase-f-flag
May 25, 2026
Merged

validate and remediate lowercase -f for susceptible gh fields (Pattern 2)#218
potiuk merged 1 commit into
apache:mainfrom
justinmclean:lowercase-f-flag

Conversation

@justinmclean

Copy link
Copy Markdown
Member

What

Adds a validate_lowercase_f_field check to the skill-validator and
remediates the two skills it flags.

skill-validator

New soft check (lowercase-f-field category) that fires when a skill's
fenced code block contains a gh call using lowercase -f with an
inline quoted value on a susceptible field (title, body,
description, name, label, milestone). These fields commonly
carry attacker-controlled content; passing them as inline shell arguments
exposes them to shell-tokeniser injection.

Safe fields (state, query, oid, type, sort) and the canonical
example in write-skill/security-checklist.md are excluded. Prose
mentions outside fenced blocks are ignored. The check is soft — it warns
by default and only fails under --strict.

Closes the automated coverage gap for Pattern 2 in
write-skill/security-checklist.md.

Skill remediations

security-issue-fix — milestone create call in § 9a now uses the
Write-tool-then--F field=@file pattern for title and description,
consistent with security-issue-import-from-pr.

security-issue-sync — both the core/chart and provider-wave
milestone create calls updated the same way. The indented fenced blocks
here were not caught by the validator (pre-existing _FENCED_CODE_RE
gap for indented fences), so this fix was applied manually after a
grep-based audit.

-f state=open and -f due_on= are left as -fstate is a static
framework value, due_on is a date string not on the susceptible-fields
list.

Tests

  • 13 new unit tests in TestLowercaseFField
  • Validator run across all 27 SKILL.md files — zero violations after
    remediation
  • skill-evals/security-issue-sync/step-2b-proposed-changes prompt
    construction verified: heading still resolves, updated -F pattern
    present, no fixture expected.json files reference the changed bash
    commands

Known gap

_FENCED_CODE_RE does not match indented fenced blocks (e.g. fences
inside list items). The four matches in security-issue-sync were found
via grep and fixed manually. A follow-up to generalise
_FENCED_CODE_RE to handle up to 3 spaces of indentation (CommonMark
spec) would let the validator catch these automatically.

@justinmclean

justinmclean commented May 18, 2026

Copy link
Copy Markdown
Member Author

Mac OS and Linux/CI are giving different prek results

@potiuk

potiuk commented May 20, 2026

Copy link
Copy Markdown
Member

Hi @justinmclean — heads-up: this PR currently shows as conflicting against main.

The conflicts are in tools/skill-validator/src/skill_validator/__init__.py and tools/skill-validator/tests/test_validator.py and overlap with merged #220 ("detect Pattern 4 injection-guard callout"), which reorganised the validator registry and the test suite in the same area.

I had a look at rebasing it on your behalf, but resolving how the new validator from this PR should coexist with the Pattern 4 infrastructure feels like a judgement call you should make rather than me guessing. Could you rebase against latest main and resolve when you get a moment? Happy to re-review once it's clean.

Thanks!

@justinmclean

Copy link
Copy Markdown
Member Author

Yep I will get to that sometime today

@justinmclean justinmclean self-assigned this May 20, 2026
@potiuk

potiuk commented May 25, 2026

Copy link
Copy Markdown
Member

Hi @justinmclean — heads-up first: main was just refreshed with pinned-action SHA bumps for actions/cache, github/codeql-action, zizmorcore/zizmor-action, and astral-sh/setup-uv. Those bumps had been blocked by a schema bug in .github/dependabot.yml (the github-actions ecosystem rejected semver-{major,minor,patch}-days cooldown keys), which is now fixed — see #257. A rebase will pull the new SHAs into your branch as a side effect; that's expected.

Now the actual reason for this ping: this branch conflicts with main after PR #257 + the injection-guard Pattern 4 work that landed. Could you rebase or merge main in and resolve?

Conflicting files:

  • tools/skill-validator/src/skill_validator/__init__.py — the SOFT_CATEGORIES frozenset and the surrounding constants block. Main now also has INJECTION_GUARD_CATEGORY, INJECTION_GUARD_TODO_CATEGORY, and BODY_INLINE_CATEGORY; your LOWERCASE_F_FIELD_CATEGORY should slot in alongside them, and the resolved set should include all of them (minus INJECTION_GUARD_CATEGORY, which is HARD by design).
  • tools/skill-validator/tests/test_validator.py — both the import block and two test sections. Your TestLowercaseFField class and main's TestValidateBodyInline / TestValidateInjectionGuard are independent additions; just keep both.

Both conflicts are mechanical (additive on both sides), no semantic reconciliation needed. Let me know if you'd prefer me to push the resolution to your branch instead — I have maintainer-edit access if "Allow edits by maintainers" is enabled.

@justinmclean

Copy link
Copy Markdown
Member Author

I can rebase for you. just working on it

@justinmclean

Copy link
Copy Markdown
Member Author

@potiuk should be all good for you

@choo121600

Copy link
Copy Markdown
Member

@justinmclean Another merge conflict here :)
may need a rebase.

@potiuk

potiuk commented May 25, 2026

Copy link
Copy Markdown
Member

Rebasing :)

@justinmclean

Copy link
Copy Markdown
Member Author

@choo121600 @potiuk should be good now

@potiuk potiuk merged commit f68064f into apache:main May 25, 2026
13 checks passed
@choo121600

Copy link
Copy Markdown
Member

Cool 👍

@justinmclean justinmclean deleted the lowercase-f-flag branch May 28, 2026 00:29
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