Conversation
- Add actual authentication enforcement in createActionRoute handler (previously only OpenAPI metadata, no runtime check) - Add requiredRole: "admin" to all notification management endpoints - Add SSRF protection to testWebhookAction blocking internal IPs, localhost, and dangerous ports 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ntication errors - Changed use_commit_signing from true to false in all 17 Claude workflow files - Fixes GitHub Actions OIDC token exchange 401 error - Related to anthropics/claude-code-action#522 - Workflow will work without commit signing until OIDC issue is resolved
- Enhanced SSRF protection with IPv6 ULA address checking (fc00::/7, fe80::/10) - Fixed leaderboard-view.tsx useEffect logic to prevent state reset issues - Replaced manual Cookie parsing with Hono's getCookie helper for better robustness
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 本次拉取请求主要引入了用户界面和后端安全性的改进。在用户界面方面，为供应商管理页面新增了排行榜入口，方便用户快速查看供应商排名。在安全性方面，通过实现 SSRF 防护机制，显著增强了 Webhook 功能的安全性，防止恶意请求访问内部资源。此外，还改进了排行榜页面的 URL 参数处理，并为 API 路由增加了更细粒度的认证和权限控制，提升了系统的整体健壮性和用户体验。 Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
本次 Pull Request 主要实现了两个功能：在供应商管理页面增加了排行榜的入口，并为 Webhook 测试功能增加了 SSRF（服务器端请求伪造）攻击防护。
整体来看，代码质量不错。主要改动点包括：
- 在多个语言的 i18n 文件中添加了“供应商排行榜”的翻译。
- 在供应商管理页面
src/app/[locale]/settings/providers/page.tsx添加了跳转到排行榜的按钮。 - 在
src/actions/notifications.ts中新增了isInternalUrl函数用于检测内部网络地址，并在testWebhookAction中使用它来防止 SSRF 攻击。这是一个重要的安全增强。 - 在排行榜视图
leaderboard-view.tsx中，实现了从 URL 查询参数同步scope和period状态，提升了页面的可链接性。
我提出了一些建议，主要集中在两个方面：
- 安全加固：当前的 SSRF 防护措施可以被 DNS 解析绕过，我建议通过增加 DNS 解析步骤来使其更加健壮。
- 功能完善：排行榜页面的 URL 同步目前是单向的，我建议实现双向同步，以便在用户切换视图时也能更新 URL。
请查看具体的评论以获取详细信息。修复这些问题后，代码将更加安全和完善。
| function isInternalUrl(urlString: string): boolean { | ||
| try { | ||
| const url = new URL(urlString); | ||
| const hostname = url.hostname.toLowerCase(); | ||
|
|
||
| // 阻止 localhost 和 IPv6 loopback | ||
| if (hostname === "localhost" || hostname === "127.0.0.1" || hostname === "::1") { | ||
| return true; | ||
| } | ||
|
|
||
| // 解析 IPv4 地址 | ||
| const ipv4Match = hostname.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/); | ||
| if (ipv4Match) { | ||
| const [, a, b, c] = ipv4Match.map(Number); | ||
| // 私有 IP 范围 | ||
| if (a === 10) return true; // 10.0.0.0/8 | ||
| if (a === 172 && b >= 16 && b <= 31) return true; // 172.16.0.0/12 | ||
| if (a === 192 && b === 168) return true; // 192.168.0.0/16 | ||
| if (a === 169 && b === 254) return true; // 169.254.0.0/16 (link-local) | ||
| if (a === 0) return true; // 0.0.0.0/8 | ||
| } | ||
|
|
||
| // 检查 IPv6 私有地址范围 | ||
| // 移除方括号（如果存在）用于 IPv6 地址检查 | ||
| const ipv6Hostname = hostname.replace(/^\[|\]$/g, ""); | ||
| // ULA (Unique Local Address): fc00::/7 | ||
| if (ipv6Hostname.startsWith("fc") || ipv6Hostname.startsWith("fd")) { | ||
| return true; | ||
| } | ||
| // Link-local: fe80::/10 | ||
| if (ipv6Hostname.startsWith("fe80:")) { | ||
| return true; | ||
| } | ||
|
|
||
| // 危险端口 | ||
| const dangerousPorts = [22, 23, 3306, 5432, 27017, 6379, 11211]; | ||
| if (url.port && dangerousPorts.includes(parseInt(url.port, 10))) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } catch { | ||
| return true; // 无效 URL 视为不安全 | ||
| } | ||
| } |
There was a problem hiding this comment.
这个 SSRF 防护函数 isInternalUrl 目前的实现存在一个安全隐患。它只检查了 URL 中的主机名是否为私有 IP 地址格式，但没有对域名进行 DNS 解析。攻击者可以通过将一个域名（例如 my-private-service.attacker.com）解析到内部网络地址（例如 127.0.0.1 或 10.0.0.1）来绕过此检查。
为了更有效地防止 SSRF 攻击，建议在检查之前，先将主机名解析为 IP 地址，然后对解析出的 IP 地址进行黑名单校验。
你可以使用 Node.js 的 dns.promises.resolve() 方法来实现。这将使函数变为异步函数，调用时需要使用 await。
例如：
import { promises as dns } from 'dns';
async function isInternalUrl(urlString: string): Promise<boolean> {
try {
const url = new URL(urlString);
const hostname = url.hostname;
// 首先获取所有 IP 地址
const addresses = await dns.resolve(hostname);
for (const address of addresses) {
// 接下来对每个 address 进行私有地址检查...
// ... (现有 IP 检查逻辑)
}
// ... (端口检查等)
return false;
} catch (error) {
// DNS 解析失败或 URL 无效等情况，都视为不安全
return true;
}
}考虑到 DNS Rebinding 攻击，最佳实践是仅接受解析出的第一个 IP 地址，或者确保所有解析出的 IP 地址都是安全的。
CHANGELOG.md
Outdated
|
|
||
| ### Changed | ||
|
|
||
| - 合并若干优化 (#162) @ding113 |
There was a problem hiding this comment.
🔴 Vague Changelog Entry: Change description is too generic and uninformative
Problem: The entry "合并若干优化 (#162)" (merge several optimizations) doesn't tell readers what was actually changed in this release. Changelogs should be specific enough for users to understand what improvements or fixes they're getting.
Impact: Users reading the changelog won't know:
- What features were added
- What bugs were fixed
- What performance improvements were made
- Whether they need to take any action
Suggested Fix:
## [Unreleased]
### Added
- 供应商管理页面新增排行榜入口按钮 (#168)
- Webhook 通知增加 SSRF 防护，阻止访问内部网络地址
### Changed
- 排行榜页面支持通过 URL 参数 \`scope\` 和 \`period\` 直达特定榜单
- 通知管理 API 端点增加管理员权限校验
### Security
- 修复 Webhook 测试接口的 SSRF 漏洞Reference: Follow Keep a Changelog format with specific categories (Added, Changed, Fixed, Security)
CHANGELOG.md
Outdated
|
|
||
| ### Changed | ||
|
|
||
| - 合并若干优化 (#162) @ding113 |
There was a problem hiding this comment.
🔴 Missing Link Formatting: PR reference is not formatted as a clickable link
Problem: The PR reference (#162) is written as plain text instead of a proper markdown link. While GitHub auto-links issue numbers in comments, changelogs should use explicit links for better portability and clarity.
Impact:
- If the changelog is read outside GitHub (e.g., in release notes, documentation sites), the link won't work
- Readers can't quickly navigate to the referenced PR
- Inconsistent with changelog best practices
Suggested Fix:
- 合并若干优化 ([#162](https://github.com/ding113/claude-code-hub/pull/162)) @ding113Or better yet, replace with specific changes as mentioned in the previous comment:
### Added
- 供应商管理页面新增排行榜入口按钮 ([#168](https://github.com/ding113/claude-code-hub/pull/168))Reference: Keep a Changelog - Link Conventions
ding113
left a comment
ding113
left a comment
There was a problem hiding this comment.
📝 Documentation Review
📊 Issues Summary
- Critical (🔴): 2 - Must fix before merge
- High (🟠): 0 - Should fix before merge
- Medium (🟡): 0 - Consider addressing
- Low (🟢): 0 - Optional improvements
⚡ Priority Fixes
-
CHANGELOG.md - Vague Change Description: The changelog entry "合并若干优化" (merge several optimizations) is too generic and doesn't describe what was actually changed. Users need to know specifically what features, fixes, or improvements are included.
-
CHANGELOG.md - Missing Link Format: The PR reference
(#162)should be formatted as a proper markdown link for better portability outside GitHub.
📋 Review Coverage
- Technical accuracy - 0 issues
- Completeness - 1 issue (vague description)
- Code examples - N/A (no code examples in docs)
- Links and references - 1 issue (missing link format)
- Clarity and organization - 0 issues
💡 General Observations
The new CHANGELOG.md file follows the correct structure (using Keep a Changelog format), but the content needs to be more specific. Based on the PR changes, this update includes:
- Added: 供应商管理页面的排行榜入口按钮
- Changed: 排行榜支持 URL 参数直达特定视图
- Security: Webhook 测试接口的 SSRF 防护
Consider breaking down the changelog entries by category (Added, Changed, Fixed, Security) and providing specific descriptions instead of generic phrases like "合并若干优化".
🤖 Automated docs review by Claude AI
|
Re-running CI - the formatting fix (commit e75dd91) is already in the dev branch. |
🔒 Security Scan Results - Inline Finding🟡 Medium Severity: 输入验证缺失文件: 问题描述: 安全影响: 如果 攻击场景:
修复建议: // 在函数开始处验证 requiredRole
if (requiredRole && !['admin', 'user'].includes(requiredRole)) {
logger.error(`[ActionAPI] Invalid requiredRole: ${requiredRole}`);
return c.json({ ok: false, error: '配置错误' }, 500);
}
// 在认证检查中使用更严格的逻辑
if (requiresAuth) {
const authToken = getCookie(c, 'auth-token');
if (!authToken) {
return c.json({ ok: false, error: '未认证' }, 401);
}
const session = await validateKey(authToken);
if (!session) {
return c.json({ ok: false, error: '认证无效或已过期' }, 401);
}
// 严格验证角色权限
if (requiredRole) {
if (requiredRole === 'admin' && session.user.role !== 'admin') {
logger.warn(`[ActionAPI] ${fullPath} 权限不足: 需要 admin 角色`, {
userId: session.user.id,
userRole: session.user.role,
});
return c.json({ ok: false, error: '权限不足' }, 403);
}
}
}参考: |
🔒 Security Scan Results - Inline Finding #2🟡 Medium Severity: 不完整的依赖数组（React Hooks 最佳实践）文件: 问题描述: useEffect 的依赖数组缺少 安全影响:
攻击场景:
修复建议: // 修改依赖数组，包含所有使用的状态变量
useEffect(() => {
const urlScope = searchParams.get('scope');
const normalizedScope = urlScope === 'provider' && isAdmin ? 'provider' : 'user';
if (normalizedScope !== scope) {
setScope(normalizedScope);
}
const urlPeriod = searchParams.get('period');
const normalizedPeriod = urlPeriod === 'monthly' ? 'monthly' : 'daily';
if (normalizedPeriod !== period) {
setPeriod(normalizedPeriod);
}
}, [isAdmin, searchParams, scope, period]); // ✅ 添加 scope 和 period或使用 useRef 避免循环依赖: const scopeRef = useRef(scope);
const periodRef = useRef(period);
useEffect(() => {
scopeRef.current = scope;
periodRef.current = period;
}, [scope, period]);
useEffect(() => {
const urlScope = searchParams.get('scope');
const normalizedScope = urlScope === 'provider' && isAdmin ? 'provider' : 'user';
if (normalizedScope !== scopeRef.current) {
setScope(normalizedScope);
}
const urlPeriod = searchParams.get('period');
const normalizedPeriod = urlPeriod === 'monthly' ? 'monthly' : 'daily';
if (normalizedPeriod !== periodRef.current) {
setPeriod(normalizedPeriod);
}
}, [isAdmin, searchParams]); // ✅ 正确的依赖数组参考: |
🔒 Security Scan Results🚨 Vulnerabilities Found
✅ Positive Security Findings1. ✅ SSRF Protection (src/actions/notifications.ts)Excellent implementation of SSRF (Server-Side Request Forgery) protection:
Reference: This is a CRITICAL security control that prevents attackers from accessing internal services. Well done! 2. ✅ Authentication & Authorization (src/lib/api/action-adapter-openapi.ts)Good implementation of authentication layer:
3. ✅ Commit Signing Changes (GitHub Workflows)
⚡ Medium Severity Issues (Must Address)🟡 Finding #1: 输入验证缺失 (Authorization Bypass Risk)Location: Issue: Risk: If Fix: Add strict validation: if (requiredRole && !['admin', 'user'].includes(requiredRole)) {
return c.json({ ok: false, error: '配置错误' }, 500);
}Reference: CWE-285: Improper Authorization 🟡 Finding #2: 不完整的依赖数组 (State Synchronization Issue)Location: Issue: Risk: Race condition where URL parameters update but component state doesn't sync, leading to users seeing incorrect permission scope data (e.g., user data displayed under provider mode). Fix: Use const scopeRef = useRef(scope);
useEffect(() => {
const normalizedScope = /* ... */;
if (normalizedScope !== scopeRef.current) {
setScope(normalizedScope);
}
}, [isAdmin, searchParams]); // Correct dependenciesReference: CWE-362: Race Condition 📋 OWASP Top 10 Coverage
Additional Checks:
🛡️ Security Posture: Strong with Minor Improvements NeededOverall Assessment: Recommendations:
Approval Recommendation: ✅ Safe to merge after addressing Finding #1 (critical for production security) 🤖 Automated security scan by Claude AI - OWASP Top 10 & CWE coverage |
ding113
left a comment
ding113
left a comment
There was a problem hiding this comment.
📋 Code Review Summary
This PR adds a provider leaderboard entry to the settings page and includes several GitHub workflow configuration changes (switching from OIDC to GH_PAT authentication). The PR also includes SSRF protection for webhook testing and adds authentication checks to notification API endpoints. While the feature implementation is straightforward, there are security concerns with the SSRF protection implementation.
🔍 Issues Found
- Critical (🔴): 0 issues
- High (🟠): 1 issue
- Medium (🟡): 2 issues
- Low (🟢): 0 issues
🎯 Priority Actions
-
SSRF Protection Bypass (🟠 High) -
src/actions/notifications.ts:30: TheisInternalUrlfunction is missing check for the full 127.0.0.0/8 loopback range. Currently only checks127.0.0.1as a string, but127.0.0.2,127.1.1.1, etc. would bypass validation. Addif (a === 127) return true;in the IPv4 validation section. -
Missing useEffect Dependencies (🟡 Medium) -
src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:47: The useEffect that syncs URL params with state referencesscopeandperiodin comparison logic but doesn't include them in the dependency array[isAdmin, searchParams]. This can cause stale closure bugs. Addscopeandperiodto the dependency array. -
IPv6-mapped IPv4 Bypass (🟡 Medium) -
src/actions/notifications.ts:41-45: The IPv6 checks don't account for IPv6-mapped IPv4 addresses like::ffff:127.0.0.1which could bypass the internal URL check. Add:if (hostname.startsWith("::ffff:")) return true;
💡 General Observations
- The workflow changes switching from
id-token: writepermission togithub_token: ${{ secrets.GH_PAT }}and disabling commit signing appear consistent across all workflow files. - The authentication additions to the notification management APIs (
requiredRole: "admin") are a good security practice. - The i18n additions for the leaderboard link are consistent across all supported locales.
- Consider adding network-level egress filtering as a defense-in-depth measure since client-side URL validation alone cannot fully prevent SSRF (DNS rebinding, redirects, etc.).
🤖 Automated review by Claude AI - focused on identifying issues for improvement
ding113
left a comment
ding113
left a comment
There was a problem hiding this comment.
📋 Code Review Summary
This PR adds a provider leaderboard entry to the settings page and includes several GitHub workflow configuration changes (switching from OIDC to GH_PAT authentication). The PR also includes SSRF protection for webhook testing and adds authentication checks to notification API endpoints. While the feature implementation is straightforward, there are security concerns with the SSRF protection implementation.
🔍 Issues Found
- Critical (🔴): 0 issues
- High (🟠): 1 issue
- Medium (🟡): 2 issues
- Low (🟢): 0 issues
🎯 Priority Actions
-
SSRF Protection Bypass (🟠 High) -
src/actions/notifications.ts:30: TheisInternalUrlfunction is missing check for the full 127.0.0.0/8 loopback range. Currently only checks127.0.0.1as a string, but127.0.0.2,127.1.1.1, etc. would bypass validation. Addif (a === 127) return true;in the IPv4 validation section. -
Missing useEffect Dependencies (🟡 Medium) -
src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:47: The useEffect that syncs URL params with state referencesscopeandperiodin comparison logic but doesn't include them in the dependency array[isAdmin, searchParams]. This can cause stale closure bugs. Addscopeandperiodto the dependency array. -
IPv6-mapped IPv4 Bypass (🟡 Medium) -
src/actions/notifications.ts:41-45: The IPv6 checks don't account for IPv6-mapped IPv4 addresses like::ffff:127.0.0.1which could bypass the internal URL check. Add:if (hostname.startsWith("::ffff:")) return true;
💡 General Observations
- The workflow changes switching from
id-token: writepermission togithub_token: ${{ secrets.GH_PAT }}and disabling commit signing appear consistent across all workflow files. - The authentication additions to the notification management APIs (
requiredRole: "admin") are a good security practice. - The i18n additions for the leaderboard link are consistent across all supported locales.
- Consider adding network-level egress filtering as a defense-in-depth measure since client-side URL validation alone cannot fully prevent SSRF (DNS rebinding, redirects, etc.).
🤖 Automated review by Claude AI - focused on identifying issues for improvement
🔒 Security Scan Results✅ No security vulnerabilities detected This PR has been scanned against OWASP Top 10, CWE Top 25, and common security anti-patterns. No security issues were identified in the code changes. 🛡️ Security Improvements DetectedThis PR actually enhances the application's security posture with the following fixes: ✅ Authentication Enforcement (src/lib/api/action-adapter-openapi.ts)
✅ SSRF Protection (src/actions/notifications.ts)
✅ Authorization Enhancement
📋 OWASP Top 10 Coverage
🔍 Additional Security Checks
📝 Non-Security Changes ReviewedThe following changes are configuration/UX improvements with no security impact:
🎯 Security AssessmentSecurity Posture: ✅ Strong - This PR significantly improves security Recommendation: ✅ Safe to merge - All changes enhance security without introducing new risks 🤖 Automated security scan by Claude AI - OWASP Top 10 & CWE coverage |
| if (ipv4Match) { | ||
| const [, a, b, c] = ipv4Match.map(Number); | ||
| // 私有 IP 范围 | ||
| if (a === 10) return true; // 10.0.0.0/8 |
There was a problem hiding this comment.
🟠 High: Missing 127.0.0.0/8 loopback range check
Why this is a problem: The IPv4 validation checks for 127.0.0.1 as a string at the beginning, but does not check the entire 127.0.0.0/8 loopback range when parsing IPv4 addresses. An attacker could use 127.0.0.2 or 127.1.1.1 to bypass the check and access internal services.
Suggested fix:
// 解析 IPv4 地址
const ipv4Match = hostname.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/);
if (ipv4Match) {
const [, a, b, c] = ipv4Match.map(Number);
// 私有 IP 范围
if (a === 10) return true; // 10.0.0.0/8
if (a === 127) return true; // 127.0.0.0/8 (loopback range) - ADD THIS
if (a === 172 && b >= 16 && b <= 31) return true; // 172.16.0.0/12
if (a === 192 && b === 168) return true; // 192.168.0.0/16
if (a === 169 && b === 254) return true; // 169.254.0.0/16 (link-local)
if (a === 0) return true; // 0.0.0.0/8
}| // 移除方括号（如果存在）用于 IPv6 地址检查 | ||
| const ipv6Hostname = hostname.replace(/^\[|\]$/g, ""); | ||
| // ULA (Unique Local Address): fc00::/7 | ||
| if (ipv6Hostname.startsWith("fc") || ipv6Hostname.startsWith("fd")) { |
There was a problem hiding this comment.
🟡 Medium: IPv6-mapped IPv4 addresses can bypass SSRF protection
Why this is a problem: The IPv6 checks don't account for IPv6-mapped IPv4 addresses like ::ffff:127.0.0.1 which represent IPv4 addresses in IPv6 format. An attacker could use this representation to bypass the internal URL check.
Suggested fix:
// 检查 IPv6 私有地址范围
// 移除方括号（如果存在）用于 IPv6 地址检查
const ipv6Hostname = hostname.replace(/^\[|\]$/g, "");
// 阻止 IPv6-mapped IPv4 addresses (::ffff:x.x.x.x)
if (ipv6Hostname.startsWith("::ffff:")) {
return true;
}
// ULA (Unique Local Address): fc00::/7
if (ipv6Hostname.startsWith("fc") || ipv6Hostname.startsWith("fd")) {
return true;
}|
|
||
| if (normalizedPeriod !== period) { | ||
| setPeriod(normalizedPeriod); | ||
| } |
There was a problem hiding this comment.
🟡 Medium: Missing dependencies in useEffect hook causes stale closure
Why this is a problem: The useEffect that syncs URL params with state references scope and period in its comparison logic but does not include them in the dependency array. This can cause the effect to use stale values when comparing, potentially leading to unexpected behavior or the state not updating correctly when URL params change while state was modified programmatically.
Suggested fix:
}, [isAdmin, searchParams, scope, period]);- 修改编辑密钥弹窗（edit-key-form.tsx）使用 FormGrid 双栏布局 - 修改添加密钥弹窗（add-key-form.tsx）使用 FormGrid 双栏布局 - 与编辑用户弹窗的限高、双栏设计保持一致 - 改善长表单的可读性和空间利用率
1 participant
close #115