Skip to content

feat: add API key authentication middleware#5

Open
abhinavguptas wants to merge 1 commit intomainfrom
feat/auth-middleware
Open

feat: add API key authentication middleware#5
abhinavguptas wants to merge 1 commit intomainfrom
feat/auth-middleware

Conversation

@abhinavguptas
Copy link
Copy Markdown
Contributor

Adds x-api-key authentication to all task routes. Key is read from API_KEY env var, skipped when unset. Health endpoint stays public.

Gates all /tasks routes behind x-api-key header.
Key is read from API_KEY env var — skipped when unset (dev mode).
Health endpoint remains public.
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 8, 2026

🩺 Dr. Concret.io — 🔴 Changes Requested

This PR introduces API key authentication middleware for the /tasks routes. It correctly integrates the middleware into app.js and creates a dedicated auth.js file. However, a significant security vulnerability exists where authentication is bypassed if the API_KEY environment variable is not set, which could lead to unauthorized access in production.

Highlights

  • The PR correctly separates authentication logic into its own middleware file, promoting modularity and reusability.
  • The health endpoint remains publicly accessible as intended, demonstrating good consideration for public services.

Findings (2)

Severity Count
High 1
Medium 1

Missing Coding Standards

This repository has no coding standards files. Consider creating:

  • review-rules.md: No project-specific coding standards or review guidelines were found; this PR introduced a potential production security bypass that a clear rule on environment variable handling could have prevented.
  • .eslintrc.json: No ESLint configuration was found; this PR could benefit from linting rules to enforce consistent code style and identify potential issues like unused variables or insecure patterns.
  • .env.example: No example environment file was found; this PR introduces a new required environment variable (API_KEY) that should be documented for other developers.

🩺 Dr. Concret.io · Model: gemini-2.5-flash · Tokens: 1810 in, 1226 out · Cost: ~$.0067

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review: see individual comments below.

Comment thread middleware/auth.js
const expectedKey = process.env.API_KEY;

if (!expectedKey) {
// Auth not configured — skip in development
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 [HIGH] security: Unsafe Authentication Bypass in Production

The if (!expectedKey) condition allows authentication to be completely bypassed if the API_KEY environment variable is not set. While intended for development, this is a critical security vulnerability if deployed to production without the variable configured, making all /tasks routes publicly accessible. Authentication should fail loudly in production if the key is missing.

Suggestion:

Suggested change
// Auth not configured — skip in development
if (!expectedKey) {
if (process.env.NODE_ENV === 'production') {
console.error('API_KEY environment variable is not set in production. Authentication is disabled.');
return res.status(500).json({ error: 'Server configuration error' });
}
// Allow bypass in non-production environments for convenience
return next();
}

Comment thread middleware/auth.js
}

if (!apiKey || apiKey !== expectedKey) {
return res.status(401).json({ error: 'Unauthorized' });
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 [MEDIUM] security: Potential Timing Attack Vulnerability

The direct string comparison apiKey !== expectedKey could potentially be vulnerable to timing attacks, where an attacker might infer the API key character by character based on slight differences in response times. For sensitive comparisons like API keys, a constant-time string comparison function is recommended to mitigate this risk.

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.

1 participant