Skip to content

[Aikido] AI Fix for A timing attack might allow hackers to bruteforce passwords - #756

Closed
aikido-autofix[bot] wants to merge 1 commit into
mainfrom
fix/aikido-security-sast-20868821-bhmx
Closed

[Aikido] AI Fix for A timing attack might allow hackers to bruteforce passwords#756
aikido-autofix[bot] wants to merge 1 commit into
mainfrom
fix/aikido-security-sast-20868821-bhmx

Conversation

@aikido-autofix

Copy link
Copy Markdown

This patch mitigates timing attack vulnerabilities in password and string comparison operations across multiple modules by replacing direct string equality checks with constant-time comparison using SHA-256 digests and crypto.timingSafeEqual().

Aikido used AI to generate this PR.

High confidence: Aikido has a robust set of benchmarks for similar fixes, and they are proven to be effective.

@github-actions

Copy link
Copy Markdown

Thanks for your contribution, @aikido-autofix[bot], @web-flow!

Before we can merge this PR, we need you to accept our Contributor License Agreement (CLA).

Missing CLA entries for: @aikido-autofix[bot], @web-flow.

How to sign: Add contributor entry lines in CONTRIBUTORS.md in this PR:

Your Name (@aikido-autofix[bot])
Your Name (@web-flow)

By adding your name, you agree to the CLA. This is a one-time step.

If you are contributing on behalf of a company, please see our Corporate CLA.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 14ab2d209c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@@ -1,3 +1,5 @@
import crypto from "node:crypto";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep auth template runtime-agnostic

Replacing the Web Crypto global with import crypto from "node:crypto" makes the generated lib/auth.ts module Node-specific, so projects running this template in non-Node runtimes (edge/worker-style deployments) will fail to load the module before login/signup handlers execute. This is a behavioral regression from the previous implementation, which only required crypto.subtle and stayed portable across runtimes.

Useful? React with 👍 / 👎.

@ariskemper

Copy link
Copy Markdown
Contributor

Review — Reject (False Positive)

cli/templates/features/auth/files/lib/auth.ts: The timing attack concern is marginal — the far larger issue is that this template uses raw SHA-256 for password hashing, which is unsuitable for passwords (no salt, no cost factor, brute-forceable at billions/sec on commodity hardware). The timingSafeEqual fix papers over a symptom while leaving the root cause. The template should use bcrypt or argon2id.

The fix also introduces a bug: import crypto from "node:crypto" shadows the global crypto already used by crypto.subtle.digest() in hashPassword(), which will break in Deno and edge runtimes.

src/transforms/esm/bundle-recovery.ts: This is a clear false positive. The tHash === hash comparison is between two internal build cache hashes (djb2 integer hashes of bundle URLs) used for loop prevention. Neither value is attacker-controlled, there is no secret, and no externally observable timing difference. Adding crypto.timingSafeEqual with double SHA-256 re-hashing here adds two unnecessary hash operations per loop iteration with zero security benefit. The import crypto from "node:crypto" is also unnecessary in a Deno codebase.

Recommendation: Close this PR. File a separate issue to replace SHA-256 with a proper KDF (bcrypt/argon2id) in the auth template.

@kojiwakayama

Copy link
Copy Markdown
Contributor

🐙 Claude Octopus Review — Merge Readiness: 10/100

Verdict: Reject — introduces bugs, partial false positive

Finding 1: auth.ts — Bug introduced

import crypto from "node:crypto" shadows the global crypto object used by crypto.subtle.digest() in hashPassword(). In Deno and edge runtimes, crypto.subtle is a global — the Node.js crypto module's default export does NOT expose .subtle. This will break password hashing at runtime.

Additionally, the underlying issue is far more serious than timing: this template uses raw SHA-256 for password hashing — no salt, no cost factor. Timing-safe comparison on an unsalted fast hash is security theater. The real fix is migrating to bcrypt or argon2id.

Finding 2: bundle-recovery.ts — Clear false positive

The tHash === hash comparison at line 83 compares two internal djb2 hash integers extracted from cached bundle filenames (http-<hash>.mjs). These are:

  • Not attacker-controlled (derived from internal URL hashing)
  • Not secrets (they are file name components)
  • Not externally observable (no timing side-channel exists)

Adding crypto.timingSafeEqual with double SHA-256 re-hashing here adds two unnecessary hash operations per loop iteration with zero security benefit. The import crypto from "node:crypto" also introduces an unnecessary Node.js dependency in a Deno codebase.

ariskemper Review Assessment

ariskemper's review is excellent — one of the strongest reviews in this batch:

  • Correctly identified the crypto shadowing bug ✓
  • Correctly identified the root cause (SHA-256 is wrong for passwords) ✓
  • Correctly identified bundle-recovery.ts as a clear false positive with precise reasoning ✓
  • Actionable recommendation (close PR, file separate issue for KDF migration) ✓

Recommendation: Close this PR. Open a separate issue to replace SHA-256 with bcrypt/argon2id in the auth template.

Score breakdown: Correctness 10 (introduces runtime bug) · Security improvement 5 (wrong fix for wrong problem) · False positive rate 50%


🐙 Review by Claude Octopus (Claude + source analysis)

@kojiwakayama

Copy link
Copy Markdown
Contributor

Closing: False positive + introduces runtime bug

  • auth.ts: The import crypto from "node:crypto" shadows the global crypto used by crypto.subtle.digest(), breaking hashPassword() at runtime. The real fix is migrating from SHA-256 to bcrypt/argon2id — filed separately.
  • bundle-recovery.ts: Clear false positive. Internal hash comparison between non-secret, non-attacker-controlled values with no timing side-channel.

See review comment for full analysis.

kojiwakayama added a commit that referenced this pull request Mar 27, 2026
The auth template used raw SHA-256 for password hashing with no salt and
no cost factor, making stored passwords trivially reversible via rainbow
tables. Password verification also used direct string comparison (===),
which is vulnerable to timing attacks.

Replace with PBKDF2-SHA256 (100k iterations, 16-byte random salt) using
the Web Crypto API — no external dependencies needed. Passwords are now
stored in "salt:hash" hex format. Verification uses constant-time byte
comparison to prevent timing side-channel attacks.

Addresses the real security issue behind PR #756 (Aikido review).
kojiwakayama added a commit that referenced this pull request Mar 27, 2026
#767)

* fix: replace SHA-256 with PBKDF2 for password hashing in auth template

The auth template used raw SHA-256 for password hashing with no salt and
no cost factor, making stored passwords trivially reversible via rainbow
tables. Password verification also used direct string comparison (===),
which is vulnerable to timing attacks.

Replace with PBKDF2-SHA256 (100k iterations, 16-byte random salt) using
the Web Crypto API — no external dependencies needed. Passwords are now
stored in "salt:hash" hex format. Verification uses constant-time byte
comparison to prevent timing side-channel attacks.

Addresses the real security issue behind PR #756 (Aikido review).

* style: fix line length formatting in demo user password hash
@ariskemper
ariskemper deleted the fix/aikido-security-sast-20868821-bhmx branch May 13, 2026 10:04
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.

2 participants