fix(bcos): strip .git suffix instead of char-set in _detect_repo_name#7955
Conversation
rstrip(".git") strips the trailing character SET {'.','g','i','t'}, not
the literal ".git" suffix, so any repo whose name ends in '.', 'g', 'i'
or 't' is silently mangled (e.g. audit->aud, rustkit->rustk, my-bot->my-bo,
toolkit->toolki). repo_name feeds report["repo_name"] which is hashed into
the BCOS commitment and cert_id, so the attestation certifies the wrong
repository identity for a large class of real repos.
Fix: rstrip('/') then remove an exact '.git' suffix. Add regression test
covering names ending in the .git charset (fails on main, passes here);
existing bcos suite stays green (19 passed).
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150) A maintainer will review your PR soon. Thanks for contributing! |
|
/claim Fixes the |
|
Maintainer validation: all 19 focused BCOS tests pass locally. Replacing |
RTC RewardThis merged PR earned 5 RTC — sent to |
FlintLeng
left a comment
There was a problem hiding this comment.
PR #7955 技术审查
标题: fix(bcos): strip .git suffix instead of char-set in _detect_repo_name
作者: Vyacheslav-Tomashevskiy
钱包地址: RTC019e78d600fb3131c29d7ba80aba8fe644be426e
文件变更: 2 files | +27 -1
技术评估
根本原因:str.rstrip(".git") 的语义被严重误解。rstrip 接收一个字符集(character set),而非一个字符串。因此 rstrip(".git") 会贪婪地移除所有属于集合 {'.', 'g', 'i', 't'} 的尾部字符——例如 "audit.git" 会被 strip 成 "au","rustkit.git" 变成 "rustk"。
影响范围:该 bug 影响所有名称以 .、g、i、t 结尾的仓库(这涵盖了绝大多数真实 Git 仓库名,因为 t 是极其常见的词尾字母)。_detect_repo_name() 的返回值会:
- 写入
report["repo_name"] - 进入
json.dumps→blake2b→ 生成链上commitment和cert_id
因此,错误的仓库名会被永久写入链上信任证明,这是一个数据完整性问题,影响 BCOS 信任认证体系的可信度。
修复方案:
# 修复前(错误)
name = url[len(prefix):].rstrip(".git").rstrip("/")
# 修复后(正确)
name = url[len(prefix):].rstrip("/")
if name.endswith(".git"):
name = name[:-len(".git")]
return name使用显式 endswith + 切片替代 rstrip,语义清晰正确。改动仅 +6/-1 行。
测试覆盖:
- 新增
test_repo_name_detection_preserves_names_ending_in_git_charset(+21 行),覆盖audit.git、rustkit.git、my-bot、toolkit、rustchain-client等 6 个用例 - 现有 BCOS 测试套件(19 个用例)全部保持绿色
- PR 提供了在 main 上失败的明确证据(
assert 'acme/aud' == 'acme/audit'会报错)
代码质量:修复精确,文档清晰,测试有针对性,不存在回归风险。
结论
✅ Approve
这是一个隐蔽但影响严重的 bug,涉及链上数据完整性。rstrip 的字符集语义是 Python 中常见的新手陷阱,本修复以最小的代码变更彻底解决问题,测试覆盖充分。强烈推荐合并。
认领地址: RTC019e78d600fb3131c29d7ba80aba8fe644be426e
Add test_detect_repo_name_strips_dotgit_suffix_not_chars to verify that
repos like audit, test, gigi are not truncated by the old rstrip('.git')
bug which stripped individual characters instead of the exact suffix.
Refs Scottcjn#7955
…ession Address @elianguitarra review feedback: - Add no-suffix URL cases ending in t/i/g (my-bot, toolkit) to ensure rstrip('/') + exact-suffix matching is preserved. - Add SSH remote cases (git@github.com:owner/repo.git) to confirm both URL branches are protected by the fix. Refs Scottcjn#7955
…ession Address @elianguitarra review feedback: - Add no-suffix URL cases ending in t/i/g (my-bot, toolkit) to ensure rstrip('/') + exact-suffix matching is preserved. - Add SSH remote cases (git@github.com:owner/repo.git) to confirm both URL branches are protected by the fix. Refs Scottcjn#7955
FlintLeng
left a comment
There was a problem hiding this comment.
PR Review: #7955 — Strip .git suffix instead of character-set in bcos_engine
Verdict: Approve (post-merge) ✅
Changes reviewed
tools/bcos_engine.py: Fixes repo name extraction to strip ".git" suffix properlytests/test_bcos_engine.py: +21 lines test coverage
Assessment
The previous code used a character-set based approach (likely removing all characters in ".git" as a set, which is wrong) instead of a literal ".git" suffix strip. This means repo names like "something.gitfoo" would incorrectly become "somethingfoo" instead of "something.gitfoo" or "something".
This affects chain-on commitment data and cert_id integrity — the BCOS engine uses repo names for on-chain commitments, so corrupted names would break attestation tracking.
Wallet address for bounty: RTC019e78d600fb3131c29d7ba80aba8fe644be426e
…epo_name (#7955) (#7998) * test(bcos): add regression test for .git suffix stripping Add test_detect_repo_name_strips_dotgit_suffix_not_chars to verify that repos like audit, test, gigi are not truncated by the old rstrip('.git') bug which stripped individual characters instead of the exact suffix. Refs #7955 * test(bcos): add no-suffix and SSH coverage for _detect_repo_name regression Address @elianguitarra review feedback: - Add no-suffix URL cases ending in t/i/g (my-bot, toolkit) to ensure rstrip('/') + exact-suffix matching is preserved. - Add SSH remote cases (git@github.com:owner/repo.git) to confirm both URL branches are protected by the fix. Refs #7955
Problem
BCOSEngine._detect_repo_name()(tools/bcos_engine.py) derives the repo name from the git remote with:str.rstrip(".git")does not remove the.gitsuffix — it strips every trailing character that is a member of the set{'.', 'g', 'i', 't'}, greedily. So any repository whose name ends in.,g,i, ortis silently mangled:github.com/acme/audit.gitacme/audacme/auditgithub.com/scottcjn/rustkit.gitscottcjn/rustkscottcjn/rustkitgithub.com/acme/my-botacme/my-boacme/my-botgithub.com/acme/toolkitacme/toolkiacme/toolkitgithub.com/acme/rustchain-clientacme/rustchain-clienacme/rustchain-clientThe trigger class is broad (
-kit,-bot,-client,-agent,-test,audit,commit,unit,circuit,orbit,toolkit, …).Impact
_detect_repo_name()setsreport["repo_name"]inrun_all(), and the full report is serialized and hashed into the on-chain commitment and certificate id:So a BCOS trust attestation certifies a repository under a corrupted name, and the wrong name is bound into the commitment /
cert_idfor a large fraction of real repos.Fix
Tests
Added
test_repo_name_detection_preserves_names_ending_in_git_charsetcovering names ending in the.gitcharset. It fails onmain(assert 'acme/aud' == 'acme/audit') and passes with the fix. The existing bcos suite stays green: