Pre-publication fixes: close guard-p4 chain bypass, add license, trademark disclaimer - #1
Merged
Merged
Conversation
…emark disclaimer Fixes the four blocking findings from the pre-publication security review (2026-08-20) plus the should-fix items, in preparation for making this repo public. - guard-p4.sh: the awk verb extraction stopped at the first `p4` token, so `p4 info && p4 obliterate ...` and `p4 -ztag info; p4 obliterate ...` both bypassed the guard entirely. Ported the sibling guards' per-token approach so every p4 invocation in a chained command is checked. - Add LICENSE (MIT, matching upstream ButterStack/agents), add "license": "MIT" to all five plugin.json files, drop CONTRIBUTING.md's unenforceable retroactive-license clause, update the README License section. - Add a Trademarks / non-affiliation section to the README. - Should-fix: document the Unity CLI's undocumented install.ps1 with a verified hash and a download-then-inspect pattern instead of a bare `| iex`; soften the Blueprint/Verse deprecation claim in unreal/NOTES.md to Epic's actual public position; add an honest "shell-string matcher, not a sandbox" limitation note to every guard-shipping plugin's README/NOTES; add SECURITY.md; attribute ParrotGameSample in unreal/fixtures/README.md; genericize the internal-identifier examples in DISTILLING.md's checklist and final sweep command. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The genericize-machine-names example used real personal rig names (beast, wanda, beast-wsl), which violated the rule it was teaching. Swapped in obviously fictional placeholders (rig-01, rig-02, workstation-b) while keeping the arrow-to-replacement pattern intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Applies the four blocking findings plus the should-fix items from Shuri's pre-publication security review (
ai/team/agents/shuri/reports/2026-08-20-gamedev-agents-prepublication-review.mdin butter_stack). Review verdict: publishable after these changes, no secrets anywhere in history, fixture-codename concern refuted.This does not merge, flip visibility, or change repo settings. Ryan reviews and merges when ready.
BLOCK-2: guard-p4.sh chained-command bypass (the only real engineering item)
guard-p4.shextracted the p4 verb with an awk loop that stopped at the firstp4token in the command, so a benign p4 call ahead of a destructive one defeated the guard completely.Reproduced before the fix:
After the fix, all three:
Fix ports
guard-lore.sh's per-token approach: emit a verb candidate for everyp4token in the command instead of stopping at the first, and check each one. Also tested||, pipes, newline separators, backgrounding (&), a verb-first-then-benign ordering, a value-taking global flag before the destructive verb, and the original depot-path false-positive case (p4 print //depot/archive/foo.txt) - all behave correctly (destructive chains blocked, benign commands and depot paths pass).I then ran a systematic bypass sweep across the other four guards (
guard-jenkins.sh,guard-unreal.sh,guard-unity.sh,guard-lore.sh) with direct,&&,;, and pipe-chained forms of their respective blocked operations. None of the other four guards have the p4 guard's bug - they already scan every token or reset state per statement separator, so a benign prefix doesn't defeat them. One related but separate finding: the review's SF-3 called outguard-unity.sh's.metaprotection as best-effort. I could not reproduce the review's literal example (find Assets -name "*.meta" -exec rm {} +is actually blocked in this repo's current script, becauseAssetsitself is a protected path), but a real bypass exists in a different shape:find Assets -name '*.meta' | xargs rmandgit clean -xfd Assetsboth pass (exit 0). Documented honestly rather than "fixed," per the review's own framing (no shell-string matcher can catch every path to a deletion) - see the should-fix section below.BLOCK-1: license (three artifacts, not one)
/LICENSE: MIT,Copyright (c) 2026 ButterStack, matching the upstreamButterStack/agentsrepo. Ryan's call: the copyright line currently names the product/brand ("ButterStack") to match upstream exactly. If there's a formal legal entity that should be the actual grantor of a public MIT license, swap the name inLICENSEbefore merging - I did not have the authority to decide that."license": "MIT"to all five*/.claude-plugin/plugin.jsonfiles.CONTRIBUTING.md's unenforceable retroactive-license clause; replaced with a standard inbound-equals-outbound statement now that a real license exists.BLOCK-4: trademark and non-affiliation disclaimer
Added a
## Trademarkssection to the README naming Perforce/Helix Core, Unreal Engine/Epic Games/Lore, Unity, and Jenkins as marks of their respective owners, and stating plainly that ButterStack is not affiliated with, endorsed by, or sponsored by any of them.BLOCK-3: not in this PR
The GitHub repo description advertises a Steam plugin that does not exist and omits unity and lore. That's a repo-settings change, not a commit, so it's not in this diff. Exact command for Ryan to run:
Should-fix items
unity/README.mdandunity/skills/unity-cli/SKILL.md: the Windows Unity CLI install line piped an undocumented CDN script straight intoiexwith no published hash. Replaced with a download-then-inspect pattern and a SHA-256 I computed and verified against the live file today (2026-08-21:3b5b42c066f04a43aaa587cfa50c5873e0148b80138a8befc610f7a7477b58e6), with a note that Unity can change this file without notice so a different hash later isn't itself a red flag.unreal/NOTES.md: softened the flat claim that Epic is "sunsetting Blueprints in UE6 in favor of Verse" to Epic's actual public position (fully supported through UE6 Early Access and initial releases, deprecation only once the Verse-based Scene Graph framework matures, conversion tooling to ship first).SECURITY.mdwith a reporting address (hello@butterstack.com - swap for a dedicated alias if you'd rather) and scope.unreal/fixtures/README.md: attributed ParrotGameSample to Epic Games and Secret Dimension with links to the Fab listing and Epic's announcement.DISTILLING.md: genericized the four literal internal-identifier examples in the sanitization checklist and the final grep-sweep command (subdomains, container-name pattern, Rails path pattern, founders' first names) so the document doesn't itself publish a map of live internal identifiers, while keeping the checklist's instructional structure intact.Not touched (per review / task scope)
.github/workflows/claude.ymlstays deleted and out of history - the review found no secret in it and confirmed it was a real prompt-injection-to-write-access risk on a public repo.Test plan
||, pipes, newlines, backgrounding, reordered verb-first chains, value-taking global flags, and the depot-path false-positive case