feat(skill-store): add GitHub Token setting to raise API rate limit (#108)#123
Merged
Merged
Conversation
…egeling#108) Users hitting the anonymous 60 req/h GitHub API limit saw an error message telling them to 'add a GitHub Token in settings', but the setting did not exist anywhere. This PR adds it. ### What the token does Once configured, the main process attaches 'Authorization: Bearer <token>' to outbound skill-store requests targeting api.github.com and raw.githubusercontent.com. That raises the limit from 60 req/h (unauthenticated) to 5 000 req/h (authenticated PAT) without needing any scope — a read-only classic or fine-grained token is enough for public skill repos. ### Security - The token is attached ONLY to hostnames in a small allow-list (api.github.com, raw.githubusercontent.com). Any 3xx redirect to a different host drops the token before the next request. - The main process re-validates every read: non-string types, empty strings, and values containing CR / LF / NUL / other control bytes are all treated as 'no token'. This is defence in depth against HTTP header injection via a tampered localStorage. - The renderer-side setter performs the same sanitization before writing to localStorage and pushing to the main DB. - The token value is never logged. ### Changes - packages/shared/types/settings.ts: add optional `githubToken` field. - apps/desktop/src/main/ipc/settings.ipc.ts: new `getGithubTokenSetting(db)` helper with sanitization. - apps/desktop/src/main/services/skill-installer-remote.ts: extend `fetchRemoteText` with an optional `githubToken` that is attached only on trusted GitHub hosts and dropped on cross-host redirects. Added tested `shouldAttachGithubAuth` helper. - apps/desktop/src/main/services/skill-installer.ts: `fetchRemoteContent` reads the token from DB and passes it to `fetchRemoteText`. - apps/desktop/src/renderer/stores/settings.store.ts: add `githubToken` state + `setGithubToken` action that strips control chars and syncs to the main DB. - apps/desktop/src/renderer/components/settings/SkillSettings.tsx: new 'GitHub Access Token' section with masked input, show/hide toggle, clear button, and a link to the GitHub PAT generation page. - i18n: added new keys to all 7 locales (en, zh, zh-TW, ja, fr, de, es). ### Tests - tests/unit/main/github-token-setting.test.ts (18 tests): covers missing row, valid token, trim, empty, wrong types, control characters, malformed JSON, legacy plain-string rows, isolation from other keys, and long fine-grained PATs. - tests/unit/stores/settings-github-token.test.ts (8 tests): covers happy path, trimming, control-character stripping, clear round-trip, and verifies the value is pushed to window.api.settings.set so the main process picks it up. - tests/unit/main/skill-installer-remote.test.ts: +11 new tests on `shouldAttachGithubAuth` covering trusted hosts (case-insensitive), untrusted hosts (including lookalikes / homograph-style attackers), and invalid inputs. ### Verification - pnpm lint → clean - pnpm test -- --run tests/unit/stores tests/unit/main/settings-startup.test.ts tests/unit/main/github-token-setting.test.ts tests/unit/main/skill-installer-remote.test.ts → 83/83 passing Closes legeling#108
|
@TuTouPower is attempting to deploy a commit to the legeling's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthrough此 PR 为 Skill Store 添加本地 GitHub PAT 支持：类型契约、后端安全读取与验证、按主机条件附加/移除认证头、前端持久化与设置 UI、多语言本地化及单元测试覆盖。 变更GitHub PAT 认证与本地配置
预估代码审查工作量🎯 3 (中等) | ⏱️ ~25 分钟 诗
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoAdd GitHub personal access token setting to raise API rate limit
WalkthroughsDescription• Add GitHub personal access token setting to raise API rate limit • Implement token sanitization and secure attachment to GitHub API requests • Add UI controls for token management with show/hide and clear buttons • Extend localization across 7 languages with new token-related strings • Add comprehensive unit tests for token validation and security Diagramflowchart LR
UI["UI Input<br/>SkillSettings.tsx"]
Store["Renderer Store<br/>setGithubToken"]
IPC["IPC Sync<br/>window.api.settings.set"]
DB["SQLite DB<br/>settings table"]
Main["Main Process<br/>getGithubTokenSetting"]
Fetch["fetchRemoteText<br/>shouldAttachGithubAuth"]
GitHub["GitHub API<br/>api.github.com"]
UI -- "token input" --> Store
Store -- "sanitize & persist" --> IPC
IPC -- "sync to main" --> DB
DB -- "read token" --> Main
Main -- "pass token" --> Fetch
Fetch -- "attach Bearer header<br/>if trusted host" --> GitHub
File Changes1. packages/shared/types/settings.ts
|
Code Review by Qodo
1. as assertions undocumented
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/ipc/settings.ipc.ts`:
- Around line 76-81: The catch block that handles reading the githubToken
setting currently logs only e.message which loses stack info; update the catch
for the githubToken read (the catch (e) around the githubToken logic) to log the
full Error object (e) or e.stack via console.error so the full stack is
preserved while still ensuring you do not print the token value itself; keep the
existing descriptive prefix ("Failed to read githubToken setting:") and either
re-throw or return/handle after logging according to surrounding flow to satisfy
the "no empty catch" guideline.
In `@apps/desktop/src/main/services/skill-installer.ts`:
- Around line 833-842: The catch block that swallows token loading failures
(catch (tokenError) in skill-installer.ts when reading the githubToken setting)
must not silently fall back to an unauthenticated request; change the behavior
to surface the failure to callers by either rethrowing a contextual Error (e.g.,
wrap tokenError with a message like "failed to load githubToken setting") or
returning a clear degradation status/value (e.g., { ok: false, reason:
'token_load_failed', error: tokenError }) from the function that attempts the
fetch; update any callers of that function (the code that performs the GitHub
fetch) to handle the new error/status instead of assuming anonymous requests.
Ensure you reference tokenError and the githubToken read logic so the change
affects all code paths that currently rely on the silent fallback.
In `@apps/desktop/src/renderer/stores/settings.store.ts`:
- Around line 426-431: The settings.store currently persists githubToken in the
renderer (the githubToken field and any zustand persist usage), which must be
removed: stop storing the actual PAT in the renderer persist/localStorage, keep
only a session-state flag like githubTokenConfigured or githubTokenSet in the
settings store, and route all get/set operations for the real token through the
main process secure storage (e.g., Electron safeStorage or system credential
store) via IPC handlers (implement IPC methods such as
getGithubToken/setGithubToken in the main process and call them from settings
actions instead of writing to persist), and remove any code in settings.store
that writes the raw token into zustand persist/localStorage (also update code
paths that read the token to use the IPC get call).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5ecf592-138c-4cab-b4e6-b0300e1f9911
📒 Files selected for processing (16)
apps/desktop/src/main/ipc/settings.ipc.tsapps/desktop/src/main/services/skill-installer-remote.tsapps/desktop/src/main/services/skill-installer.tsapps/desktop/src/renderer/components/settings/SkillSettings.tsxapps/desktop/src/renderer/i18n/locales/de.jsonapps/desktop/src/renderer/i18n/locales/en.jsonapps/desktop/src/renderer/i18n/locales/es.jsonapps/desktop/src/renderer/i18n/locales/fr.jsonapps/desktop/src/renderer/i18n/locales/ja.jsonapps/desktop/src/renderer/i18n/locales/zh-TW.jsonapps/desktop/src/renderer/i18n/locales/zh.jsonapps/desktop/src/renderer/stores/settings.store.tsapps/desktop/tests/unit/main/github-token-setting.test.tsapps/desktop/tests/unit/main/skill-installer-remote.test.tsapps/desktop/tests/unit/stores/settings-github-token.test.tspackages/shared/types/settings.ts
Comment on lines
+76
to
+81
| } catch (e) { | ||
| // Never log the token content, only the error class. | ||
| console.error( | ||
| 'Failed to read githubToken setting:', | ||
| e instanceof Error ? e.message : 'unknown', | ||
| ); |
There was a problem hiding this comment.
保留完整错误堆栈，避免排障信息丢失。
Line 78-81 目前只记录 e.message，会丢失关键堆栈信息。建议直接记录 Error 对象（仍不打印 token 内容）。
建议修改
- console.error(
- 'Failed to read githubToken setting:',
- e instanceof Error ? e.message : 'unknown',
- );
+ console.error('Failed to read githubToken setting:', e);As per coding guidelines "No empty catch blocks are allowed; every catch must either re-throw, log with full stack, or handle the error meaningfully".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (e) { | |
| // Never log the token content, only the error class. | |
| console.error( | |
| 'Failed to read githubToken setting:', | |
| e instanceof Error ? e.message : 'unknown', | |
| ); | |
| } catch (e) { | |
| // Never log the token content, only the error class. | |
| console.error('Failed to read githubToken setting:', e); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/ipc/settings.ipc.ts` around lines 76 - 81, The catch
block that handles reading the githubToken setting currently logs only e.message
which loses stack info; update the catch for the githubToken read (the catch (e)
around the githubToken logic) to log the full Error object (e) or e.stack via
console.error so the full stack is preserved while still ensuring you do not
print the token value itself; keep the existing descriptive prefix ("Failed to
read githubToken setting:") and either re-throw or return/handle after logging
according to surrounding flow to satisfy the "no empty catch" guideline.
Comment on lines
+833
to
+842
| } catch (tokenError) { | ||
| // DB may be unavailable during very early startup or in tests — | ||
| // fall back to an unauthenticated request without failing the | ||
| // fetch. | ||
| // DB 不可用时（启动初期或测试场景）回落到未登录请求。 | ||
| console.warn( | ||
| "Unable to load githubToken setting, continuing unauthenticated:", | ||
| tokenError instanceof Error ? tokenError.message : "unknown", | ||
| ); | ||
| } |
There was a problem hiding this comment.
避免静默降级为未鉴权请求。
Line 833-842 捕获 token 读取失败后继续请求，会把“配置/DB异常”伪装成“正常匿名请求”，后续只表现为 rate-limit 问题。建议向调用方显式暴露失败（抛出上下文错误或返回可判定的降级状态）。
As per coding guidelines "Functions must not swallow errors and return default values; if an operation can fail, the caller must know".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/services/skill-installer.ts` around lines 833 - 842,
The catch block that swallows token loading failures (catch (tokenError) in
skill-installer.ts when reading the githubToken setting) must not silently fall
back to an unauthenticated request; change the behavior to surface the failure
to callers by either rethrowing a contextual Error (e.g., wrap tokenError with a
message like "failed to load githubToken setting") or returning a clear
degradation status/value (e.g., { ok: false, reason: 'token_load_failed', error:
tokenError }) from the function that attempts the fetch; update any callers of
that function (the code that performs the GitHub fetch) to handle the new
error/status instead of assuming anonymous requests. Ensure you reference
tokenError and the githubToken read logic so the change affects all code paths
that currently rely on the silent fallback.
Comment on lines
+426
to
+431
| // GitHub personal access token for authenticated skill-store fetches | ||
| // Used to raise the GitHub API rate limit from 60 req/h (unauthenticated) | ||
| // to 5000 req/h (authenticated). Only sent to api.github.com and | ||
| // raw.githubusercontent.com. See #108. | ||
| // GitHub PAT，提升 Skill Store 的 GitHub API 请求限额 (#108)。 | ||
| githubToken: string; |
There was a problem hiding this comment.
不要在 renderer 持久化层长期明文保存 GitHub PAT。
当前实现把 githubToken 落到 zustand persist（localStorage）。这会扩大泄露面：一旦 renderer 侧被注入或被调试脚本读取，PAT 可直接外流。建议将 PAT 仅放在主进程安全存储（如 safeStorage/系统凭据库），renderer 仅持有会话态与“已配置”标记。
Also applies to: 651-652, 1272-1282
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/stores/settings.store.ts` around lines 426 - 431,
The settings.store currently persists githubToken in the renderer (the
githubToken field and any zustand persist usage), which must be removed: stop
storing the actual PAT in the renderer persist/localStorage, keep only a
session-state flag like githubTokenConfigured or githubTokenSet in the settings
store, and route all get/set operations for the real token through the main
process secure storage (e.g., Electron safeStorage or system credential store)
via IPC handlers (implement IPC methods such as getGithubToken/setGithubToken in
the main process and call them from settings actions instead of writing to
persist), and remove any code in settings.store that writes the raw token into
zustand persist/localStorage (also update code paths that read the token to use
the IPC get call).
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+47
to
+49
| const stmt = db.prepare('SELECT value FROM settings WHERE key = ?'); | ||
| const row = stmt.get('githubToken') as { value: string } | undefined; | ||
| if (!row) { |
There was a problem hiding this comment.
1. as assertions undocumented 📘 Rule violation ⚙ Maintainability
New as type assertions were added without an explanatory comment or clear necessity, weakening TypeScript strictness and potentially masking type errors. This violates the rule restricting type assertions unless justified with a comment.
Agent Prompt
## Issue description
New `as` assertions were introduced without justification comments, violating the strictness rule and potentially hiding type mismatches.
## Issue Context
Casts appear in main-process settings DB reads, renderer store sync, and tests. Prefer safe typing (generics, runtime guards, proper types) over assertions; if an assertion is truly required for interop, add a brief comment explaining why it is safe/necessary.
## Fix Focus Areas
- apps/desktop/src/main/ipc/settings.ipc.ts[47-49]
- apps/desktop/src/renderer/stores/settings.store.ts[1277-1282]
- apps/desktop/tests/unit/stores/settings-github-token.test.ts[44-46]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+23
to
+24
| import { initDatabase } from "../database"; | ||
| import { getGithubTokenSetting } from "../ipc/settings.ipc"; |
There was a problem hiding this comment.
2. Relative imports in skill-installer 📘 Rule violation ⚙ Maintainability
New imports in the main process use relative paths instead of the documented @/ alias for src/main, increasing coupling to folder structure and risking circularity/drift. This violates the import/path-alias compliance rule.
Agent Prompt
## Issue description
New code adds relative imports in a `src/main` module where the repository requires documented aliases.
## Issue Context
The compliance rule requires `@/` for main-process imports to keep paths stable and avoid drift.
## Fix Focus Areas
- apps/desktop/src/main/services/skill-installer.ts[23-24]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+23
to
+24
| import { initDatabase } from "../database"; | ||
| import { getGithubTokenSetting } from "../ipc/settings.ipc"; |
There was a problem hiding this comment.
4. Cli pulls electron dependency 🐞 Bug ≡ Correctness
SkillInstaller now imports getGithubTokenSetting from settings.ipc.ts, which has a top-level import from 'electron'. Because the CLI imports SkillInstaller and the CLI build config doesn’t externalize 'electron' (and electron is only a devDependency), the published CLI can fail to bundle or crash at startup with “Cannot find module 'electron'”.
Agent Prompt
## Issue description
`SkillInstaller` (used by the Node CLI) now imports `getGithubTokenSetting` from `main/ipc/settings.ipc.ts`, which has a top-level `import { ipcMain } from 'electron'`. This makes the CLI bundle/runtime require the `electron` module even though the published CLI package doesn’t ship it as a dependency.
## Issue Context
The helper `getGithubTokenSetting(db)` is electron-independent, but it currently lives in an Electron IPC module. The CLI build config also does not externalize `electron`, making this a high-risk transitive dependency.
## Fix Focus Areas
- apps/desktop/src/main/services/skill-installer.ts[20-24]
- apps/desktop/src/main/ipc/settings.ipc.ts[1-10]
### Suggested approach
- Move `getGithubTokenSetting` into a new electron-free module (e.g. `apps/desktop/src/main/settings/github-token.ts` or `apps/desktop/src/main/database/settings.ts`).
- Import that helper from both:
- `settings.ipc.ts` (for IPC usage)
- `skill-installer.ts` (for fetch usage)
- Keep `ipcMain` imports only in IPC registration code, not in modules shared with the CLI entry graph.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
AGENTS.md forbids hardcoded Chinese characters in source files outside the locale JSON resources. The review flagged every Chinese bilingual comment block this PR added. Removed them so only the English explanations remain.
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
2 participants
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
Fixes #108 — users hitting the anonymous 60 req/h GitHub API limit saw an error message telling them to "add a GitHub Token in settings", but the setting did not exist anywhere. This PR adds it, wired from UI to main-process HTTP layer.
What the token does
Once configured, the main process attaches `Authorization: Bearer ` to outbound skill-store requests targeting `api.github.com` and `raw.githubusercontent.com`. That raises the limit from 60 req/h (unauthenticated) to 5 000 req/h (authenticated PAT) without needing any scope — a read-only classic or fine-grained token is enough for public skill repos.
Security
Changes
Tests
Verification
Notes
Summary by CodeRabbit