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

Open
#731 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
3/5
Estimated time
1-2 days
Newbie friendliness
55/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
node.js, typescript

Research direction

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.

Written by the indexing model from the issue text.

Description

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.

Dominant language
TypeScript
Stars
2
Forks
90
Avg merge
31m
Merged PRs (30d)
20

Getting set up

  • Ships a Dockerfile or Docker Compose file
  • No pull request template
  • No contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from MergeFi/backend

All issues in MergeFi/backend

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.