diff --git a/scripts/activate-skills.sh b/scripts/activate-skills.sh index d471f3eb..8d0c5fb6 100755 --- a/scripts/activate-skills.sh +++ b/scripts/activate-skills.sh @@ -26,6 +26,10 @@ find_copy_dirs() { mkdir -p "$dest_dir" while IFS= read -r -d '' item; do + if [[ -L "$item" ]] && ! is_safe_dir_symlink "$src_dir" "$item"; then + echo " ! Skipping unsafe symlink outside source root: $(basename "$item")" + continue + fi cp -RP "$item" "$dest_dir/" done < <(find "$src_dir" -mindepth 1 -maxdepth 1 \( -type d -o -type l \) -print0 2>/dev/null) } @@ -60,6 +64,39 @@ resolve_python() { return 1 } +is_safe_dir_symlink() { + local root_dir="$1" + local item="$2" + local python_path="" + + if ! python_path="$(resolve_python 2>/dev/null)"; then + return 1 + fi + + "$python_path" - "$root_dir" "$item" <<'PY' +from pathlib import Path +import sys + +root_dir = Path(sys.argv[1]).resolve() +item = Path(sys.argv[2]) + +try: + target = item.resolve(strict=True) +except FileNotFoundError: + raise SystemExit(1) + +if not target.is_dir(): + raise SystemExit(1) + +try: + target.relative_to(root_dir) +except ValueError: + raise SystemExit(1) + +raise SystemExit(0) +PY +} + is_safe_skill_id() { [[ "$1" =~ ^[A-Za-z0-9._-]+$ ]] } diff --git a/tools/scripts/apply_skill_optimization.cjs b/tools/scripts/apply_skill_optimization.cjs index af8374e8..afd534a4 100644 --- a/tools/scripts/apply_skill_optimization.cjs +++ b/tools/scripts/apply_skill_optimization.cjs @@ -5,6 +5,8 @@ const path = require('node:path'); const { execFileSync } = require('node:child_process'); const REVIEW_HEADING = '## Tessl Skill Review'; +const TRUSTED_AUTHOR_ASSOCIATIONS = new Set(['OWNER', 'MEMBER', 'COLLABORATOR']); +const DEFAULT_ALLOWED_REVIEW_BOT_LOGINS = ['github-actions[bot]']; function getRequiredEnv(name) { const value = process.env[name]; @@ -88,7 +90,46 @@ function ensureRepoRelative(filePath) { return resolved; } -async function findLatestReviewComment(owner, repo, prNumber) { +function parseAllowedReviewBotLogins(envValue = process.env.APPLY_OPTIMIZE_ALLOWED_REVIEW_BOT_LOGINS) { + if (!envValue) { + return DEFAULT_ALLOWED_REVIEW_BOT_LOGINS; + } + + const logins = envValue + .split(',') + .map((item) => item.trim()) + .filter(Boolean); + + return logins.length > 0 ? logins : DEFAULT_ALLOWED_REVIEW_BOT_LOGINS; +} + +function isTrustedReviewComment(comment, allowedBotLogins = DEFAULT_ALLOWED_REVIEW_BOT_LOGINS) { + if (!comment?.body || !comment.body.includes(REVIEW_HEADING)) { + return false; + } + + if (TRUSTED_AUTHOR_ASSOCIATIONS.has(comment.author_association)) { + return true; + } + + const login = comment.user?.login; + const userType = comment.user?.type; + return Boolean(login && userType === 'Bot' && allowedBotLogins.includes(login)); +} + +function selectLatestTrustedReviewComment(comments, allowedBotLogins = DEFAULT_ALLOWED_REVIEW_BOT_LOGINS) { + let latest = null; + + for (const comment of comments) { + if (isTrustedReviewComment(comment, allowedBotLogins)) { + latest = comment; + } + } + + return latest; +} + +async function findLatestTrustedReviewComment(owner, repo, prNumber, allowedBotLogins) { let page = 1; let latest = null; @@ -97,11 +138,7 @@ async function findLatestReviewComment(owner, repo, prNumber) { `/repos/${owner}/${repo}/issues/${prNumber}/comments?per_page=100&page=${page}`, ); - for (const comment of comments) { - if (comment.body && comment.body.includes(REVIEW_HEADING)) { - latest = comment; - } - } + latest = selectLatestTrustedReviewComment(comments, allowedBotLogins) ?? latest; if (comments.length < 100) { return latest; @@ -133,6 +170,7 @@ async function main() { } const pr = await githubRequest(`/repos/${owner}/${repo}/pulls/${prNumber}`); + const allowedReviewBotLogins = parseAllowedReviewBotLogins(); if (pr.head.repo.full_name !== repoSlug) { throw new Error( @@ -140,9 +178,16 @@ async function main() { ); } - const reviewComment = await findLatestReviewComment(owner, repo, prNumber); + const reviewComment = await findLatestTrustedReviewComment( + owner, + repo, + prNumber, + allowedReviewBotLogins, + ); if (!reviewComment || !reviewComment.body) { - throw new Error('No Tessl skill review comment found on this PR. Run the review first.'); + throw new Error( + 'No trusted Tessl skill review comment found on this PR. Run the review first.', + ); } const optimizedFiles = extractOptimizedContent(reviewComment.body); @@ -205,7 +250,18 @@ async function main() { await postComment(owner, repo, prNumber, body); } -main().catch((error) => { - console.error(error instanceof Error ? error.stack : String(error)); - process.exitCode = 1; -}); +if (require.main === module) { + main().catch((error) => { + console.error(error instanceof Error ? error.stack : String(error)); + process.exitCode = 1; + }); +} + +module.exports = { + DEFAULT_ALLOWED_REVIEW_BOT_LOGINS, + REVIEW_HEADING, + findLatestTrustedReviewComment, + isTrustedReviewComment, + parseAllowedReviewBotLogins, + selectLatestTrustedReviewComment, +}; diff --git a/tools/scripts/tests/activate_skills_shell.test.js b/tools/scripts/tests/activate_skills_shell.test.js index a180c30b..8a397034 100644 --- a/tools/scripts/tests/activate_skills_shell.test.js +++ b/tools/scripts/tests/activate_skills_shell.test.js @@ -10,6 +10,7 @@ const scriptPath = path.join(repoRoot, "scripts", "activate-skills.sh"); const root = fs.mkdtempSync(path.join(os.tmpdir(), "activate-skills-shell-")); const baseDir = path.join(root, "antigravity"); const repoSkills = path.join(root, "repo-skills"); +const outsideDir = path.join(root, "outside-skill-root"); function makeSkill(skillId) { const skillDir = path.join(repoSkills, skillId); @@ -25,6 +26,9 @@ try { makeSkill("brainstorming"); makeSkill("systematic-debugging"); makeSkill("custom-skill"); + fs.mkdirSync(outsideDir, { recursive: true }); + fs.writeFileSync(path.join(outsideDir, "secret.txt"), "outside", "utf8"); + fs.symlinkSync(outsideDir, path.join(repoSkills, "escape-link"), "dir"); const result = spawnSync( "bash", @@ -54,6 +58,14 @@ try { fs.existsSync(path.join(baseDir, "skills_library", "brainstorming", "SKILL.md")), "repo skills should be synced into the backing library", ); + assert.ok( + !fs.existsSync(path.join(baseDir, "skills_library", "escape-link")), + "repo sync must not copy symlinked skills that point outside the source root", + ); + assert.ok( + !fs.existsSync(path.join(baseDir, "skills", "escape-link")), + "unsafe symlinked skills must never become active", + ); assert.match( result.stdout, /Done! Antigravity skills are now activated\./, diff --git a/tools/scripts/tests/apply_skill_optimization_security.test.js b/tools/scripts/tests/apply_skill_optimization_security.test.js new file mode 100644 index 00000000..fdc6d7df --- /dev/null +++ b/tools/scripts/tests/apply_skill_optimization_security.test.js @@ -0,0 +1,43 @@ +const assert = require('node:assert'); +const path = require('node:path'); + +const repoRoot = path.resolve(__dirname, '..', '..', '..'); +const { + selectLatestTrustedReviewComment, +} = require(path.join(repoRoot, 'tools', 'scripts', 'apply_skill_optimization.cjs')); + +const trustedMaintainerComment = { + body: '## Tessl Skill Review\ntrusted maintainer content', + author_association: 'MEMBER', + user: { login: 'maintainer-user', type: 'User' }, +}; + +const spoofedUserComment = { + body: '## Tessl Skill Review\nspoofed content', + author_association: 'NONE', + user: { login: 'random-user', type: 'User' }, +}; + +const trustedBotComment = { + body: '## Tessl Skill Review\nbot content', + author_association: 'NONE', + user: { login: 'github-actions[bot]', type: 'Bot' }, +}; + +assert.strictEqual( + selectLatestTrustedReviewComment([trustedMaintainerComment, spoofedUserComment]), + trustedMaintainerComment, + 'the apply script must ignore later untrusted review comments', +); + +assert.strictEqual( + selectLatestTrustedReviewComment([spoofedUserComment, trustedBotComment]), + trustedBotComment, + 'the apply script should accept review comments from the configured trusted bot', +); + +assert.strictEqual( + selectLatestTrustedReviewComment([spoofedUserComment]), + null, + 'the apply script must reject untrusted review comments entirely', +); diff --git a/tools/scripts/tests/run-test-suite.js b/tools/scripts/tests/run-test-suite.js index 52174efc..b35efed4 100644 --- a/tools/scripts/tests/run-test-suite.js +++ b/tools/scripts/tests/run-test-suite.js @@ -11,6 +11,7 @@ const LOCAL_TEST_COMMANDS = [ [path.join(TOOL_TESTS, "activate_skills_shell.test.js")], [path.join(TOOL_TESTS, "activate_skills_batch_security.test.js")], [path.join(TOOL_TESTS, "automation_workflows.test.js")], + [path.join(TOOL_TESTS, "apply_skill_optimization_security.test.js")], [path.join(TOOL_TESTS, "build_catalog_bundles.test.js")], [path.join(TOOL_TESTS, "claude_plugin_marketplace.test.js")], [path.join(TOOL_TESTS, "installer_antigravity_guidance.test.js")],