The production boot guard for GITHUB_CLIENT_ID/GITHUB_CLIENT_SECRET is dead code — hardcoded `mock_*` fallbacks make both values permanently non-empty

オープン
#731 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る

メンテナーはふだん 1 日以内に返信

まだ誰も着手していません。

評価

難易度
3/5
見積もり時間
1〜2日
初心者へのやさしさ
55/100
issue の種類
バグ
明瞭さ
明確に書かれている
活発さ
活発
技術スタック
node.js, typescript

調査の方向性

Start with the config factory in src/config/configuration.ts and follow how its GitHub credentials reach collectConfigIssues() in src/config/validate-required-config.ts and the GithubStrategy constructor. Then inspect .env.example and the existing configuration validation specs. Done means missing or published placeholder credentials are rejected in production, the example file no longer supplies mock values, and tests cover the missing credentials; the issue also suggests checking local strategy registration behavior.

索引モデルが issue の本文から書いたものです。

説明

bug security

Location

  • src/config/configuration.ts:88-89 — the exported config factory
  • src/config/validate-required-config.ts:91-100 — collectConfigIssues()
  • src/auth/strategies/github.strategy.ts:16-23 — GithubStrategy constructor
  • .env.example:31-32

Problem

collectConfigIssues is meant to refuse a production boot when GitHub OAuth is not configured:

requireNonEmpty('GITHUB_CLIENT_ID',     config.github.clientId,     'GitHub OAuth login cannot be completed');
requireNonEmpty('GITHUB_CLIENT_SECRET', config.github.clientSecret, 'GitHub OAuth login cannot be completed');

requireNonEmpty only reports an issue when the value is falsy or whitespace (validate-required-config.ts:48-59). But the config factory never produces a falsy value for either key:

clientId:     process.env.GITHUB_CLIENT_ID     || 'mock_client_id_12345',
clientSecret: process.env.GITHUB_CLIENT_SECRET || 'mock_secret_key_67890',

So with GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET completely unset in production, config.github.clientId is the non-empty string 'mock_client_id_12345', requireNonEmpty passes, and both checks can never fire. They are unreachable branches.

The same two literals are hardcoded a second time inside GithubStrategy's constructor (github.strategy.ts:17-18), with a comment that states the intent explicitly: "If the config system returns an empty string or undefined, it automatically uses static string fallbacks so Passport NEVER crashes." That second layer guarantees that even a fixed config factory would not surface the problem at construction time.

.env.example:31-32 ships the same two mock values as if they were real settings, so an operator who copies .env.example to .env in production inherits them without ever seeing a blank to fill in.

This is the same class of defect as closed issue #288 (ENCRYPTION_KEY exempt from production enforcement because of a published fallback), applied to the OAuth client credentials instead.

Why it matters

The whole point of assertRequiredConfig running before NestFactory.create (see the comment block at src/main.ts:12-25) is that a misconfigured production pod must refuse to start rather than come up looking healthy. For GitHub OAuth that guarantee does not exist:

  • A production deploy that forgot GITHUB_CLIENT_ID/GITHUB_CLIENT_SECRET boots cleanly, reports healthy on GET /, and passes every readiness check.
  • The only way anyone discovers it is a user clicking "Sign in with GitHub" and being bounced by GitHub's own error page, or — once the callback runs — landing on /auth/callback?error=oauth_failed via AuthController.githubCallback's catch block. Since GitHub OAuth is the only authentication mechanism, the entire platform is unusable while reporting itself healthy.
  • Worse, the fallbacks are guessable, published values. 'mock_client_id_12345' is a real OAuth client id from GitHub's perspective — just one nobody controls. If anyone ever registers a GitHub OAuth app whose client id happens to collide with that literal, a production deploy with the env vars missing would complete an OAuth handshake against an app controlled by a third party, who would then receive the authorization code and the redirect_uri for this platform's users.

Reproduction / failure path

  1. Build the image and run it with NODE_ENV=production, a real DATABASE_URL, a real JWT_SECRET, GITHUB_WEBHOOK_SECRET, ESCROW_CONTRACT_ID and TREASURY_SECRET set — and GITHUB_CLIENT_ID / GITHUB_CLIENT_SECRET deliberately unset.
  2. Observed: assertRequiredConfig throws nothing, the app listens, and the startup banner prints normally.
  3. GET /api/auth/github redirects the browser to https://github.com/login/oauth/authorize?client_id=mock_client_id_12345&..., where GitHub replies with "The redirect_uri MUST match the registered callback URL for this application" / an unknown-client error.
  4. Expected: the process exits at boot with Refusing to start in production with an incomplete configuration: - GITHUB_CLIENT_ID is unset ....

Suggested fix

Two changes, both needed:

  1. In src/config/configuration.ts, stop manufacturing a value. Read the raw env var and let it be empty so the validator can see it:
clientId: process.env.GITHUB_CLIENT_ID ?? '',
clientSecret: process.env.GITHUB_CLIENT_SECRET ?? '',
oauthCallbackUrl: process.env.GITHUB_OAUTH_CALLBACK_URL ?? '',

collectConfigIssues already returns early outside production (validate-required-config.ts:46), so local development is unaffected by the stricter config — development simply gets an empty client id, which is the honest representation of "not configured".

  1. In src/auth/strategies/github.strategy.ts, remove the || 'mock_client_id_12345' / || 'mock_secret_key_67890' fallbacks and let the strategy throw when the credentials are absent in production. If "Passport must never crash in dev" is a requirement, gate the strategy's registration on env !== 'production' in AuthModule instead of substituting fake credentials silently.

Additionally, in src/config/validate-required-config.ts, add explicit "still the published placeholder" checks for both values — the same pattern already used for INSECURE_DEFAULT_JWT_SECRET and DEFAULT_LOCAL_DATABASE_URL — so a deploy that copied .env.example verbatim is also refused:

export const PLACEHOLDER_GITHUB_CLIENT_ID = 'mock_client_id_12345';
export const PLACEHOLDER_GITHUB_CLIENT_SECRET = 'mock_secret_key_67890';

Finally, replace the values in .env.example:31-32 with empty assignments (GITHUB_CLIENT_ID=) so the file reads as a form to fill in rather than as working defaults, and add a configuration.spec.ts / validate-required-config.spec.ts case asserting that a production config with both env vars unset produces two issues.

主要言語
TypeScript
スター
2
フォーク
90
平均マージ
31分
マージ済み PR(30日)
20

環境構築

  • Dockerfile または Docker Compose ファイルあり
  • プルリクエストのテンプレートなし
  • コントリビューションガイドなし

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

MergeFi/backend のほかの issue

MergeFi/backend の issue をすべて見る

似ている issue

TypeScript の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。