Skip to content

feat(integrations): open GitHub issues through an installable GitHub App instead of a personal token - #75

Open
YJack0000 wants to merge 10 commits into
developfrom
feat/github-app-integration
Open

YJack0000 wants to merge 10 commits into
developfrom
feat/github-app-integration

Conversation

@YJack0000

@YJack0000 YJack0000 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Description

GitHub 整合(#31,建工單時開 issue)原本用某個人的 Personal Access Token,現在改成可安裝的 GitHub App「Pathors Inbox」。administrator 在整合頁按連接,到 GitHub 把 App 裝到自己的 org,回來後從下拉選單挑 repo、可選填 label,之後 issue 都由 App 開。系統裡不存任何長期的 GitHub token,PAT 的設定方式整個移除。對應票 pathorsAI/pathors#3521。

流程:

  1. 整合頁 → github.com/apps/<slug>/installations/new?state=<JWT>,state 用 HS256 簽 {sub: account_id, exp: +15min},驗證時一定要帶 exp
  2. GitHub 導回 GET /github/callback。這裡不綁定,只驗 state,再把 code、installation_id、state 轉給 /app/accounts/<id>/settings/integrations/github
  3. 設定頁在 administrator 自己的登入狀態下呼叫 POST /api/v1/accounts/:id/integrations/github:state 的 account 必須等於 Current.account → 用 code 換 user token → GET /user/installations 必須包含這個 installation_id → 寫進 hook
  4. 開 issue:App private key 簽 JWT(RS256)→ 換 installation token(只限選定的 repo、issues: write,存在 Rails.cache,到期前 5 分鐘換新)→ POST /repos/{repo}/issues

為什麼不在 callback 綁定:如果在 callback 綁定,account X 的 administrator 可以把自己的安裝連結(裡面有合法的 state)丟給別的 org 的 owner V;V 一裝,V 的 installation 就被綁到 X,X 就能列出 V 的 private repo、在那邊開 issue。改成在設定頁綁定之後,V 不是 X 的成員,在他的瀏覽器上這一步做不了。

新增:

  • Integrations::Github::AppClient:App JWT、installation token、repo 清單、OAuth code exchange、/user/installations
  • Github::CallbacksController、Api::V1::Accounts::Integrations::GithubController(create / repositories / update / destroy,限 administrator)
  • POST /webhooks/github:驗 X-Hub-Signature-256。installation.deleted/suspend,或已選的 repo 被移出 installation → prompt_reauthorization!;unsuspend → reauthorized!
  • hook 的形狀:reference_id 存 installation_id,settings 只有 {repository, label}
  • 設定頁 Github.vue:未連接 / 等 org owner 核准 / 選 repo / 已連接 / 需要重新連接
  • installation config:GITHUB_APP_ID、GITHUB_APP_SLUG、GITHUB_APP_CLIENT_ID、GITHUB_APP_CLIENT_SECRET、GITHUB_APP_PRIVATE_KEY(textarea;密碼欄位會吃掉換行,PEM 會壞掉)、GITHUB_APP_WEBHOOK_SECRET。六個都要有,整合卡片才會出現

行為:

  • 連接連結(state)有效一小時。過期的話,callback 會帶 error=state_expired 導回該 account 的 GitHub 設定頁,提示再按一次連接(GitHub 那邊可能已經裝好了);簽章不對的 state 一律導回首頁
  • 舊的 PAT hook 只能刪除,不能列 repo 或改設定(回 404)
  • 開 issue 失敗不會讓工單建立失敗。401、404,還有沒帶限流 header 的 403 → hook 標成需要重新連接;被限流的 403 只記 log
  • 舊的 PAT hook(沒有 reference_id)會顯示成「未連接」,processor 直接跳過、不呼叫 GitHub。重新連接時會蓋掉 settings,把 access_token 清掉

已知限制:

  • /user/installations 只要使用者看得到 installation 裡任何一個 repo 就會列出,不限 org owner。所以 org 的一般成員也能把自己 org 的 installation 綁到他管理的 account;org 以外的人擋得住。GitHub 這個 endpoint 沒有提供 org 角色可以判斷
  • 需要重新連接時沒有寄信通知(Reauthorizable 只對 Slack、Dialogflow 寄信),只有設定頁的提示

上線前

  1. 以 pathorsAI org owner 身分建 GitHub App:任何帳號都能安裝;權限只開 Issues: Read & write、Metadata: Read;打開「安裝時要求使用者授權 (OAuth)」;Callback URL https://inbox.pathors.com/github/callback;Webhook https://inbox.pathors.com/webhooks/github,Content type 選 application/json(GitHub 預設是 form-urlencoded,簽章仍會通過,但解析會失敗,每次都回 500,解除安裝就不會被發現),只訂閱 Installation 和 Installation repositories
  2. 六個設定值放進 Infisical,再填到 super admin
  3. 我們自己的 account:把 App 裝到 pathorsAI/pathors → 在整合頁重新連接 → 到 GitHub 撤銷舊的 PAT

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • rspec(容器內,RAILS_ENV=test):386 examples, 0 failures;修完 review 後再跑 GitHub 相關與 integrations 全部,260 examples, 0 failures。涵蓋 spec/lib/integrations/github、spec/controllers/github、webhook controller、spec/controllers/api/v1/accounts/integrations 全部、spec/models/integrations、spec/helpers、hook listener / job
    • 偽造 installation_id 不會寫入 hook;把驗證那段故意改成永遠通過,這個測試會失敗(1 failure),確認它真的有在檢查
    • 簽章錯的 webhook 回 401;installation.deleted 會讓 hook 需要重新連接;GitHub 回 404 時標記重新連接、不丟例外;沒選 repo 時不發任何 request;選了不在 installation 裡的 repo 回 422
  • vitest:API client + 設定頁,4 files / 32 tests passed
  • rubocop、eslint clean
  • PEM 用 CRLF 存進 InstallationConfig,再從 GlobalConfigService 讀出來,能 parse 回同一把 key
  • 沒驗證到的:spec/controllers/super_admin/app_config_controller_spec.rb 在本機容器跑到一半被 OOM kill,交給 CI;設定頁還沒在瀏覽器實際打開過;GitHub App 還沒建,所以還沒對真的 GitHub 跑過

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

🤖 Generated with Claude Code

YJack0000 and others added 8 commits October 6, 2026 16:53
Signs the app JWT with the private key, mints installation tokens scoped to
one repository with issues:write, and caches them until five minutes before
they expire. Also lists the installation's repositories, exchanges an OAuth
code for a user token, and lists the installations that user can see.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The processor now authenticates with an installation token instead of a
personal access token pasted into the hook settings. It skips hooks that
have no installation (left over from the token setup), no repository, or a
pending reconnect. When GitHub answers 401, 403 without rate-limit headers,
or 404, it flags the hook for reconnect instead of raising.

The hook settings schema drops access_token and the generic settings form;
reference_id holds the installation id.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The integration card links to the app's install page with a signed,
15-minute state naming the account. GitHub returns the browser to
/github/callback, which only checks the state and forwards code,
installation_id and state to the settings page. The page posts them to
POST /integrations/github from the admin's own session.

Binding happens there, not in the callback, for two reasons. The state
must name the current account, so an admin cannot hand their state to
another organization's owner and capture that installation. The user
token from the code must list the installation, because installation_id
comes through a browser redirect and anyone can forge it.

Reconnecting keeps the label, and keeps the repository only if the new
installation still grants it. Administrators can list the installation's
repositories, choose one plus an optional label (422 for a repository
outside the installation), and unbind the installation. Hook JSON now
reports reauthorization_required.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
POST /webhooks/github verifies X-Hub-Signature-256 against
GITHUB_APP_WEBHOOK_SECRET and answers 401 without it. An installation that
is deleted or suspended, or that loses the hook's repository, marks every
hook bound to it as needing a reconnect; unsuspend clears the flag. Replays
converge on the same state, and other events are acknowledged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The GitHub card now opens a dedicated page instead of the generic hook
form. The page derives one status from the account's hook (not
connected, reconnect, choose repository, connected), links Connect to
the GitHub App install URL, lets an administrator pick the repository
and optional issue label, and shows the install redirect's
setup_action/error notice once before clearing the query.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The install callback no longer binds the installation, because doing it
server-side allowed a cross-account CSRF. The settings page now posts
the redirect's code, installation_id and state to the integrations
endpoint itself, clears the single-use query, and then loads the hook.
A refused install shows the notice for the returned reason.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adds GITHUB_APP_ID, GITHUB_APP_SLUG, GITHUB_APP_CLIENT_ID,
GITHUB_APP_CLIENT_SECRET, GITHUB_APP_PRIVATE_KEY and
GITHUB_APP_WEBHOOK_SECRET as installation configs with a GitHub card in
super admin. The private key is a code field, because a password input
drops the PEM's newlines. The card stays hidden until all six are set; a
missing webhook secret would leave uninstalls unnoticed. The integration
description now says to connect the GitHub App.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@YJack0000 YJack0000 self-assigned this Oct 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

✅ SonarQube Quality Gate passed — pathorsAI_inbox

0 open issues on this PR.

@claude

claude Bot commented Oct 6, 2026

Copy link
Copy Markdown

Review

This is a strong PR. The security design is the standout: binding the installation from the admin's authenticated session instead of the callback (with the cross-account capture scenario documented in the code), verifying installation_id against GET /user/installations because it arrives via a browser redirect, HMAC-verified webhooks with secure_compare, per-repository tokens scoped to issues: write, and no long-lived secrets stored anywhere. The specs actually test the adversarial paths (forged installation id, state minted for another account, wrong webhook signature), not just happy paths. The diff is also upstream-friendly — almost everything is new files, and the touches to shared files (app.rb case branches, one line in _hook.json.jbuilder, routes, HookPolicy, super admin configs) follow the exact patterns Linear already uses.

Findings, roughly by importance:

1. Stale state strands the user on the dashboard root after a successful install

STATE_TTL is 15 minutes (app/helpers/github/integration_helper.rb:5), but the state is minted when the integrations page renders. If the admin leaves the tab open a while before clicking Connect, or the GitHub side takes long (org-permission detours, 2FA, picking repos), the install completes on GitHub but the callback rejects the expired state and silently redirects to the bare FRONTEND_URL (app/controllers/github/callbacks_controller.rb:11) — no account, no error, and the admin has no idea the connection wasn't finished.

Since the state's only security job is to name the account — the dangerous part (binding) is separately gated by the authenticated admin session plus the user-installation check in GithubController#create — a much longer TTL (an hour or more) wouldn't weaken anything and would mostly eliminate this trap. Alternatively (or additionally), redirect expired/invalid states to a page that explains the connect link expired, instead of the app root.

2. Deploy note: webhook content type must be JSON

Webhooks::GithubController#payload does JSON.parse(request.raw_post) (app/controllers/webhooks/github_controller.rb:50). If the GitHub App's webhook is created with the default application/x-www-form-urlencoded content type, the signature still verifies (HMAC over the raw body) but the parse raises and every delivery 500s — uninstalls would go unnoticed, which is exactly what the webhook exists to catch. Worth adding "Content type: application/json" to the pre-launch checklist in the PR description so whoever creates the App doesn't trip on the GitHub default.

3. Legacy PAT hook: small API-reachable edge + no UI escape hatch

  • A leftover token hook has reference_id: nil. The shipped UI never calls repositories/update for it (it reads as not-connected), but a direct API call would hit app_client.repositories(nil) → POST /app/installations//access_tokens → 404 → AuthorizationError → render_access_lost marks the legacy hook reauthorization_required (app/controllers/api/v1/accounts/integrations/github_controller.rb:20-33). Harmless in practice, but a guard in fetch_hook (404 or 422 when reference_id is blank) would keep the reauth flag meaningful.
  • In the NOT_CONNECTED state the page only offers Connect, so an admin can't remove a stale legacy hook without going through a full app install. Probably fine (reconnecting overwrites it and clears access_token), just confirming it's intentional.

4. Minor observations (no action strictly needed)

  • AppClient#installation_token (lib/integrations/github/app_client.rb:27-28): if GitHub ever returned an expires_at less than the 5-minute refresh margin away, expires_in goes non-positive and Rails.cache.write raises on Redis. Tokens are 1 hour today, so this is theoretical — flagging only because it's a tiny fix (skip the write unless expires_in.positive?) if you want it.
  • _hook.json.jbuilder now does a Redis GET per hook rendered (reauthorization_required?). Hooks-per-account counts are tiny, so this is negligible — just noting it's per-render I/O on a shared partial.
  • Reusing GITHUB_APP_CLIENT_SECRET as the HMAC key for the state JWT is fine (it's a secret shared only with GitHub), but it does mean rotating the OAuth client secret invalidates in-flight connect links. Acceptable trade-off for not adding a seventh config key.

5. CLAUDE.md: zh_TW locale edits

The repo guidelines say product/source-string changes should touch only en.yml/en.json, with other locales flowing through Crowdin. This PR hand-edits zh_TW in both trees. If this fork maintains zh_TW manually (which the history suggests), ignore this — otherwise these strings will be overwritten by the next Crowdin sync.

Test coverage

Good: request specs for connect (including forged ids and cross-account state), webhook signature/replay/scoping, client pagination + token caching under freeze_time, processor reauth paths, and Vue specs for the API client and the page states. The known gaps called out in the PR description (no run against real GitHub yet, settings page not opened in a browser) are the right things to verify before flipping the production account over — especially given finding 2.

Overall: 👍 well-scoped, carefully reasoned, and ready to merge once the deploy checklist picks up the webhook content-type note; finding 1 is the only code change I'd actually push for.

YJack0000 and others added 2 commits October 6, 2026 17:19
…bles

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…count

The connect state now lives an hour, and a genuine but expired one returns
the admin to the GitHub settings page with a notice to press Connect again
instead of the app root. A leftover personal-token hook can only be deleted,
and a non-positive token TTL is no longer written to the cache.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants