Skip to content

Fix admin-key comparison timing vulnerability by extracting auth logic - #608

Open
whitezaddy wants to merge 5 commits into
Heliobond:mainfrom
whitezaddy:fix/issue-526-admin-key-comparison
Open

Fix admin-key comparison timing vulnerability by extracting auth logic#608
whitezaddy wants to merge 5 commits into
Heliobond:mainfrom
whitezaddy:fix/issue-526-admin-key-comparison

Conversation

@whitezaddy

Copy link
Copy Markdown

Description

Closes #526

This PR addresses the admin-key comparison vulnerability where a plain === operator was incorrectly used, exposing the ADMIN_API_KEY to potential timing side-channel attacks.

Instead of fixing the vulnerable checks individually and leaving duplicated code, we extracted the entire authorization header parsing, admin key verification, and consumer key validation logic into a new centralized helper (src/lib/authHelper.ts).

Changes included:

  • Created resolveAuthFromHeaders in src/lib/authHelper.ts to cleanly extract and validate API keys from headers.
  • Updated src/middleware/apiKeyAuth.ts and src/graphql/schema.ts to consume the new helper, eliminating the duplicate code across REST and GraphQL implementations.
  • Implemented constant-time string comparison (timingSafeCompare) uniformly for the admin key check.
  • Added comprehensive unit tests for the new authHelper.ts utility.

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Testing checklist

  • Tests pass locally
  • Added new tests (if applicable)
  • Code follows project conventions

Screenshots (if applicable)

@drips-wave

drips-wave Bot commented Sep 1, 2026

Copy link
Copy Markdown

@whitezaddy Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@whitezaddy
whitezaddy force-pushed the fix/issue-526-admin-key-comparison branch 3 times, most recently from 1b0959e to 15f8a02 Compare September 1, 2026 20:26
@whitezaddy
whitezaddy force-pushed the fix/issue-526-admin-key-comparison branch 4 times, most recently from ac395e6 to 36ed1da Compare September 1, 2026 23:20
@whitezaddy

Copy link
Copy Markdown
Author

Kindly review

@whitezaddy

Copy link
Copy Markdown
Author

Conflicts resolved. Please review.

Comment thread src/lib/webhooks.ts
}

export function triggerWebhooks(payload: unknown): void {
for (const wh of webhooks.values()) {
@whitezaddy

Copy link
Copy Markdown
Author

Conflict resolved

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.

Admin-key comparison uses plain === instead of the codebase's own timing-safe compare, duplicated in two files

2 participants