Author SHA1 Message Date
Jeffery 43ad2b56d2 chore(ai-review 狀態): 回寫本輪 findings 與排除事項
node-actions/template: CI / BUILD (pull_request) Successful in 4s
CI / TEST (Claude) (pull_request) Successful in 33s
CI / TEST (Antigravity) (pull_request) Successful in 49s
CI / TEST (Codex) (pull_request) Successful in 3m38s
2026-07-21 16:26:30 +08:00
Jeffery 8e18bbacc2 style(action manifest): 統一中文斜線標點 2026-07-21 16:26:30 +08:00
Jeffery 1f012c6cfb refactor(gitref): 將分支名稱驗證改為正式模組 2026-07-21 16:26:30 +08:00
6 changed files with 53 additions and 125 deletions
+11
View File
@@ -1989,5 +1989,16 @@
"endLine": 188, "endLine": 188,
"problem": "`resolveMergeBase` 新增了多階段 fetch 補歷史流程:先 fetch base、首次 merge-base、再 deepen base、deepen PR HEAD、必要時 unshallow,且每個策略成功後要立即重試並短路返回。現有測試只驗證不安全 `baseRef` 會在 fetch 前被拒絕,沒有測到淺層 checkout、首次失敗後補抓成功、策略失敗繼續下一個、全部失敗時診斷訊息等邊界。這正是容易 off-by-one 或順序錯的流程,現在還沒有測試保護。", "problem": "`resolveMergeBase` 新增了多階段 fetch 補歷史流程:先 fetch base、首次 merge-base、再 deepen base、deepen PR HEAD、必要時 unshallow,且每個策略成功後要立即重試並短路返回。現有測試只驗證不安全 `baseRef` 會在 fetch 前被拒絕,沒有測到淺層 checkout、首次失敗後補抓成功、策略失敗繼續下一個、全部失敗時診斷訊息等邊界。這正是容易 off-by-one 或順序錯的流程,現在還沒有測試保護。",
"reason": "Paladin:可排除(重複)。resolveMergeBase 多階段 fetch、淺層與非淺層路徑、策略停止條件、全部失敗診斷與 cause 缺少測試,已由歷史 findings 涵蓋。" "reason": "Paladin:可排除(重複)。resolveMergeBase 多階段 fetch、淺層與非淺層路徑、策略停止條件、全部失敗診斷與 cause 缺少測試,已由歷史 findings 涵蓋。"
},
{
"addedAt": "2026/07/21 16:26:12",
"prNumber": 6,
"reviewer": "Assassin",
"severity": "嚴重",
"file": "src/index.js",
"startLine": 105,
"endLine": 116,
"problem": "這裡把審查結果 commit/push 失敗吞掉並回傳 `false`,而本次變更又把主流程改成「本輪審查不因嚴重問題直接 exit 1,靠下一輪讀到 `[failure]` commit 才失敗」。攻擊者只要讓結果 commit 推不上去,例如在 PR head 競態推送、讓 token 沒有 push 權限、或讓來源分支拒絕 bot push,就能讓嚴重安全 finding 已產生但沒有 failure commit、也沒有下一輪失敗檢查,等同把必要檢查繞過。",
"reason": "人工裁示排除:主流程已在 result === failure 且 commitFindings 回傳 false 時直接回傳 1;本 finding 針對舊位置的描述已由現有收尾防線涵蓋。"
} }
] ]
@@ -7,84 +7,7 @@
"version": "codex-cli 0.144.6", "version": "codex-cli 0.144.6",
"model": "gpt-5.5" "model": "gpt-5.5"
}, },
"findings": [ "findings": [],
{
"reviewer": "Assassin",
"focus": "security",
"badge": "🗡️",
"severity": "嚴重",
"file": "src/index.js",
"startLine": 105,
"endLine": 116,
"problem": "這裡把審查結果 commit/push 失敗吞掉並回傳 `false`,而本次變更又把主流程改成「本輪審查不因嚴重問題直接 exit 1,靠下一輪讀到 `[failure]` commit 才失敗」。攻擊者只要讓結果 commit 推不上去,例如在 PR head 競態推送、讓 token 沒有 push 權限、或讓來源分支拒絕 bot push,就能讓嚴重安全 finding 已產生但沒有 failure commit、也沒有下一輪失敗檢查,等同把必要檢查繞過。",
"suggestion": "嚴重問題存在時,結果 commit/push 失敗必須直接讓本輪 workflow 失敗;只有 success 結果或無變更時才可降級不阻擋。呼叫端應檢查 `commitFindings` 回傳值,或讓 `commitFindings` 在 `result === 'failure'` 時重拋錯誤。",
"suggestedCode": "const committed = commitFindings({ cwd, ctx, files: filesToCommit, result });\nif (result === 'failure' && !committed) {\n log('收尾', 'ERR', '存在嚴重問題,但無法推送 failure 結果 commit;本輪直接失敗以避免繞過檢查。');\n return 1;\n}\nreturn 0;",
"id": "F001",
"verdicts": {
"Paladin": {
"exclude": false,
"reason": "保留(成立)。已知排除事項與歷史 findings 未涵蓋「嚴重 finding 依賴 failure commit,但 failure commit 推送失敗時本輪仍可能通過」這個繞過風險;證據不足以排除。"
}
}
},
{
"reviewer": "Bard",
"focus": "style",
"badge": "🎼",
"severity": "建議",
"file": "action.yml",
"startLine": 18,
"endLine": 19,
"problem": "中文敘述裡混入 `PR/issue`、`findings/exclusions` 這種半形斜線寫法,但同一份變更其他地方大量使用 `PR/issue`、`警告+建議` 這類全形符號。標點像節拍器,這裡忽然換拍,讓 manifest 的文字風格不夠一致。",
"suggestion": "統一中文文件與註解中的分隔符號,建議在中文語境使用全形斜線:`PR/issue`、`findingsexclusions`;若是程式路徑或指令片段才保留半形 `/`。",
"suggestedCode": "",
"id": "F004",
"verdicts": {
"Paladin": {
"exclude": false,
"reason": "保留(成立)。未命中已知排除事項,也未見歷史 finding 涵蓋 action.yml 中文標點半形/全形風格不一致;依現有資料不能判為重複或誤報。"
}
}
},
{
"reviewer": "Bard",
"focus": "style",
"badge": "🎼",
"severity": "建議",
"file": "src/lib/gitrepo.js",
"startLine": 299,
"endLine": 344,
"problem": "`pushWithCredential` 的 JSDoc 幾乎把安全設計、CI 觸發語意、checkout extraheader 行為全部寫成一篇短文。資訊本身有價值,但集中在函式註解裡會壓過函式簽名,讀者想找參數與責任邊界時,得先穿過一大段敘事。",
"suggestion": "保留函式層級的摘要與關鍵安全不變式,其餘背景可移到較短的段落或專門文件。JSDoc 建議聚焦在「做什麼、為何不能改、參數怎麼用」,避免把完整決策紀錄塞進 API 註解。",
"suggestedCode": "",
"id": "F005",
"verdicts": {
"Paladin": {
"exclude": false,
"reason": "保留(成立)。歷史 finding 雖有 resolveMergeBase 閱讀密度問題,但未涵蓋 pushWithCredential JSDoc 過長且混入決策紀錄;目前不能排除。"
}
}
},
{
"reviewer": "Leo",
"focus": "maintainability",
"badge": "🧰",
"severity": "建議",
"file": "src/lib/gitrepo.js",
"startLine": 367,
"endLine": 369,
"problem": "為了測試把內部函式掛在 `module.exports.__test`,會讓 production module 的公開形狀混入測試專用 API。未來其他程式碼可能誤用 `__test.assertSafeBranchRef`,而維護者也得在重構時顧慮這個非正式出口,模組邊界會慢慢變模糊。",
"suggestion": "把分支名稱驗證抽到獨立小模組並正常匯出,例如 `src/lib/gitref.js`,讓 production code 與測試都依賴同一個正式 API;或若它只屬於 gitrepo 內部,就改由測試 `resolveMergeBase``commitAndPushFindings` 的外部行為覆蓋,不暴露 `__test`。",
"suggestedCode": "",
"id": "F009",
"verdicts": {
"Paladin": {
"exclude": false,
"reason": "保留(成立)。目前提供的排除事項與歷史 findings 未涵蓋 module.exports.__test 暴露測試專用 API 的模組邊界問題;證據不足以排除。"
}
}
}
],
"excluded": [ "excluded": [
{ {
"reviewer": "Bard", "reviewer": "Bard",
+2 -2
View File
@@ -16,11 +16,11 @@ author: 'Jeffery'
# 輸入參數區塊:呼叫端 workflow 以 `with:` 傳入, # 輸入參數區塊:呼叫端 workflow 以 `with:` 傳入,
# runner 會自動注入為 INPUT_* 環境變數(例如 INPUT_TOKEN、INPUT_MODEL、INPUT_CREATE-ISSUE)供主程式讀取。 # runner 會自動注入為 INPUT_* 環境變數(例如 INPUT_TOKEN、INPUT_MODEL、INPUT_CREATE-ISSUE)供主程式讀取。
inputs: inputs:
# Gitea API token:用於對 PRissue 留言審查結果,以及 push 審查結果檔(findings/exclusions)回 repo。 # Gitea API token:用於對 PRissue 留言審查結果,以及 push 審查結果檔(findingsexclusions)回 repo。
token: token:
# 參數用途說明:secrets/vars context 在 action 內不可用,故由呼叫端 workflow 以 secrets 傳入。 # 參數用途說明:secrets/vars context 在 action 內不可用,故由呼叫端 workflow 以 secrets 傳入。
# 建議傳入能觸發 CI 的 PAT;自動 token 推送結果 commit 時可能不會再觸發 workflow。 # 建議傳入能觸發 CI 的 PAT;自動 token 推送結果 commit 時可能不會再觸發 workflow。
description: 'Gitea API tokenPR/issue 留言與 push findings 用;建議以能觸發 CI 的 PAT 由 secrets 傳入)' description: 'Gitea API tokenPRissue 留言與 push findings 用;建議以能觸發 CI 的 PAT 由 secrets 傳入)'
# 必填:缺少 token 無法呼叫 Gitea APIaction 無法運作。 # 必填:缺少 token 無法呼叫 Gitea APIaction 無法運作。
required: true required: true
# 指定 AI 工具使用的模型名稱。 # 指定 AI 工具使用的模型名稱。
+32
View File
@@ -0,0 +1,32 @@
'use strict';
const { execFileSync } = require('child_process');
/**
* 驗證遠端分支名稱可安全用於 refspec 與 refs/remotes/origin/*。
*
* @param {string} refName - 使用者或事件 payload 提供的分支名稱。
* @param {string} fieldName - 錯誤訊息中的欄位名稱。
* @returns {string} 原樣回傳通過驗證的分支名稱。
* @throws {Error} 分支名稱空白、含路徑穿越,或不符合 git 分支 ref 規則時拋出。
*/
function assertSafeBranchRef(refName, fieldName) {
const value = String(refName || '').trim();
if (!value) throw new Error(`${fieldName} 不可為空。`);
if (value.includes('..') || value.startsWith('/') || value.endsWith('/') || value.includes('\\')) {
throw new Error(`${fieldName} 不是安全的分支名稱:${value}`);
}
try {
execFileSync('git', ['check-ref-format', '--branch', value], {
encoding: 'utf8',
stdio: ['ignore', 'pipe', 'pipe'],
});
} catch {
throw new Error(`${fieldName} 不是合法的 git 分支名稱:${value}`);
}
return value;
}
module.exports = {
assertSafeBranchRef,
};
+4 -43
View File
@@ -1,37 +1,10 @@
'use strict'; 'use strict';
const { execFileSync } = require('child_process'); const { execFileSync } = require('child_process');
const { assertSafeBranchRef } = require('./gitref');
// git 操作工具:一律以 execFileSync 呼叫 git(不經 shell,避免注入),輸出以 UTF-8 回傳。 // git 操作工具:一律以 execFileSync 呼叫 git(不經 shell,避免注入),輸出以 UTF-8 回傳。
/**
* 驗證遠端分支名稱可安全用於 refspec 與 refs/remotes/origin/*。
*
* @param {string} refName - 使用者或事件 payload 提供的分支名稱。
* @param {string} fieldName - 錯誤訊息中的欄位名稱。
* @returns {string} 原樣回傳通過驗證的分支名稱。
* @throws {Error} 分支名稱空白、含路徑穿越,或不符合 git 分支 ref 規則時拋出。
* @remarks
* 使用情境:`resolveMergeBase` 的 `baseRef` 與 `commitAndPushFindings` 的
* `headRef` 會被組進 refspec;先驗證可避免惡意 payload 影響本地 refs 路徑。
*/
function assertSafeBranchRef(refName, fieldName) {
const value = String(refName || '').trim();
if (!value) throw new Error(`${fieldName} 不可為空。`);
if (value.includes('..') || value.startsWith('/') || value.endsWith('/') || value.includes('\\')) {
throw new Error(`${fieldName} 不是安全的分支名稱:${value}`);
}
try {
execFileSync('git', ['check-ref-format', '--branch', value], {
encoding: 'utf8',
stdio: ['ignore', 'pipe', 'pipe'],
});
} catch {
throw new Error(`${fieldName} 不是合法的 git 分支名稱:${value}`);
}
return value;
}
/** /**
* 同步執行 git 指令並回傳原始 stdout 輸出。 * 同步執行 git 指令並回傳原始 stdout 輸出。
* *
@@ -308,18 +281,9 @@ function commitAndPushFindings(cwd, { headRef, headSha, message, files, token, s
/** /**
* 以帶認證的方式推送到指定遠端,認證資訊只經環境變數傳入、不進命令列 argv。 * 以帶認證的方式推送到指定遠端,認證資訊只經環境變數傳入、不進命令列 argv。
* *
* 認證方式:等同 `https://ai-review-bot:<secret>@host/...` 的 HTTP Basicgit 會把 * 以 `GIT_CONFIG_*` 注入本次 HTTP Basic extraheader,避免憑證出現在 argv
* URL 帳密轉成相同的 `Authorization: Basic` 標頭送出),但改以 git 的 * 同時先清空 checkout 持久化的自動 token extraheader,確保本次 push 使用呼叫端 token。
* `GIT_CONFIG_*` 環境變數注入 `http.<serverUrl>/.extraheader`,使 base64 憑證**不出現在 argv** * 推送失敗時改拋固定訊息,避免原始例外帶出遠端 URL 或認證資訊。
* (避免程序清單/例外回顯洩漏);推送目標 URL 亦不含帳密。
*
* 觸發 CI 關鍵:`actions/checkout` 會把「自動 Actions token」持久化在同一個
* `http.<serverUrl>/.extraheader` scope;若沿用它推送,Gitea 會視為「自動 token 觸發」而
* **不再觸發 workflow**(防遞迴)。故本函式對這次 push 於該 scope**先以空值重置**(清掉自動
* token——git 對 extraHeader 給空值即清空既有清單),**再注入 PAT 的 Authorization**,讓推送以
* PAT 身分進行、觸發 PR 的 synchronize;作用範圍僅限本次 push 的環境變數,不影響 action 其他
* 仰賴 checkout 持久化憑證的 fetch(如 {@link resolveMergeBase})。
* 推送失敗時**不重拋原始例外**(其 message 會含命令列與遠端 URL),改拋固定訊息。
* *
* @param {string} cwd - git 工作目錄(repo 的 checkout 路徑)。 * @param {string} cwd - git 工作目錄(repo 的 checkout 路徑)。
* @param {string} remoteUrl - 不含帳密的遠端 URL(形如 `https://host/owner/repo.git`)。 * @param {string} remoteUrl - 不含帳密的遠端 URL(形如 `https://host/owner/repo.git`)。
@@ -367,7 +331,4 @@ module.exports = {
fileDiff, fileDiff,
fileLastUpdatedIso, fileLastUpdatedIso,
commitAndPushFindings, commitAndPushFindings,
__test: {
assertSafeBranchRef,
},
}; };
+3 -2
View File
@@ -4,14 +4,15 @@ const assert = require('node:assert/strict');
const test = require('node:test'); const test = require('node:test');
const gitrepo = require('../src/lib/gitrepo'); const gitrepo = require('../src/lib/gitrepo');
const gitref = require('../src/lib/gitref');
test('assertSafeBranchRef 接受一般分支名稱', () => { test('assertSafeBranchRef 接受一般分支名稱', () => {
assert.equal(gitrepo.__test.assertSafeBranchRef('feature/review-123', 'baseRef'), 'feature/review-123'); assert.equal(gitref.assertSafeBranchRef('feature/review-123', 'baseRef'), 'feature/review-123');
}); });
test('assertSafeBranchRef 拒絕路徑穿越分支名稱', () => { test('assertSafeBranchRef 拒絕路徑穿越分支名稱', () => {
assert.throws( assert.throws(
() => gitrepo.__test.assertSafeBranchRef('../../hooks/pre-push', 'baseRef'), () => gitref.assertSafeBranchRef('../../hooks/pre-push', 'baseRef'),
/不是安全的分支名稱/, /不是安全的分支名稱/,
); );
}); });