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

Aperta
#731 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
3/5
Tempo stimato
1-2 giorni
Idoneità per principianti
55/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
node.js, typescript

Direzione di ricerca

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.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

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.

Lingua principale
TypeScript
Stelle
2
Fork
90
Merge medio
31m
PR unite (30g)
20

Preparare l'ambiente

  • Include un Dockerfile o un file Docker Compose
  • Nessun modello di pull request
  • Nessuna guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di MergeFi/backend

Tutte le issue di MergeFi/backend

Issue simili

Altre issue su TypeScript

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.