safeEqual が公開 API でありながら、型も名前も用途を制約していない

Abierto
  • ¿Algo no está bien?
#69 0 comentarios 0 reacciones 0 asignados Ver en GitHub

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
5/5
Tiempo estimado
Más de una semana
Aptitud para principiantes
35/100
Tipo de issue
Refactorización
Claridad
Necesita aclaración
Estado de actividad
Activo
Stack tecnológico
node.js, typescript
Área
api, authentication, security

Línea de trabajo

Empieza por packages/api/src/auth/crypto.ts, sus exportaciones a través de auth/index.ts y src/index.ts, y los callers en apps/web/server/routes/login/callback.get.ts y apps/web/server/utils/auth.ts. Revisa packages/api/src/auth/auth.test.ts y las exportaciones del paquete; después compara las opciones de nomenclatura, visibilidad y tipos branded; el trabajo estará terminado cuando un diseño elegido impida el uso involuntario de contraseñas, mantenga los callers de tokens y supere las pruebas pertinentes.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

enhancement

今の状態

packages/api/src/auth/crypto.ts の safeEqual は (a: string, b: string) => boolean という型を持ち、両方の入力を SHA-256 で固定長のダイジェストにしてから node:crypto の timingSafeEqual に渡す。SHA-256 を挟むのは保存のためではない。timingSafeEqual は長さの違う入力に対して例外を投げる（Node.js の crypto のドキュメントが "An error is thrown if a and b have different byte lengths." と述べている）ので、それを避けるために長さを揃えている。長さで早期に分岐すると長さがタイミングから漏れる、という理由が同じ関数の JSDoc に書いてある。

この関数はパッケージの公開 API になっている。packages/api/src/auth/index.ts が export * from './crypto' で再エクスポートし、packages/api/src/index.ts が export * from './auth' で再エクスポートする。packages/api/package.json の exports は "." を ./src/index.ts に対応付けているので、@checkin/api を import できる場所からはどこからでも safeEqual を呼べる。

現在の呼び出しは次のとおりである。2026-09-18 に git fetch origin pull/68/head のうえ git grep -n 'safeEqual(' FETCH_HEAD -- apps packages を実行して数えた。定義の 1 行を含めて 6 行が一致し、定義を除いた 5 行の内訳は apps が 2 行、packages のテストが 3 行である。

  • apps/web/server/routes/login/callback.get.ts の OAuth コールバックのハンドラー。クエリの state と、リダイレクトの前に cookie へ入れておいた値を照合する。
  • apps/web/server/utils/auth.ts の isCsrfValid。CSRF の cookie と x-csrf-token ヘッダーを照合する。
  • packages/api/src/auth/auth.test.ts の crypto: safeEqual の 3 行。

apps 側の 2 箇所が比べているのは、いずれも同じファイルの generateToken が node:crypto の randomBytes から作る値である。generateToken の JSDoc はこれを "a high-entropy, URL-safe opaque token" と述べている。state は generateToken(16)、CSRF トークンは generateToken の既定の 32 バイトで、どちらもパスワードではない。したがって、現在の使われ方では CodeQL の指摘は当たらない。

起こりうること

型が用途を制約していない。(a: string, b: string) => boolean は任意の文字列を受けるので、パスワードを渡すことを型が拒めない。名前も用途を限っていない。JSDoc の先頭行は Constant-time string comparison. で、文字列一般に使える定数時間比較として読める。

この関数に行き当たった実装者が、「定数時間で安全に比較する関数」と読んでパスワードの照合に使うと、ソルトを加えず、計算量を増やす処理も挟まない素の SHA-256 での照合になる。CodeQL の js/insufficient-password-hash が指しているのはその状態で、規則の説明は Creating a hash of a password with low computational effort makes the hash vulnerable to password cracking attacks.、タグは external/cwe/cwe-916 である。

今はそう使っている箇所が無いので症状は起きていない。ただし、それを止めているのは型でも名前でもなく、呼び出し元が上に挙げた 2 箇所しかないという現在の事実だけである。

CodeQL の alert への対応

PR #68 で CodeQL が js/insufficient-password-hash の alert を 1 件出している。2026-09-18 に gh api repos/traPtitech/Checkin/code-scanning/alerts/3 を実行して確かめた内容は次のとおりである。

  • 番号は 3、ツールは CodeQL、security severity は high。
  • 指している場所は packages/api/src/auth/crypto.ts、本文は Password from a call to getOAuthCookies is hashed insecurely.
  • dismissed_at・dismissed_by・dismissed_reason・dismissed_comment はいずれも null である。この alert は dismiss していない。

同じ日に gh api repos/traPtitech/Checkin/code-scanning/alerts/3/instances を実行すると、refs/pull/68/head と refs/pull/47/head の 2 件が open で並ぶ。ref を指定しない gh api 'repos/traPtitech/Checkin/code-scanning/alerts?state=open&per_page=100' は、同じ日に 0 件を返した。

現在の使われ方では当たらないと判断した根拠は、上に挙げた 2 箇所が比べているものが randomBytes 由来のトークンであることと、SHA-256 を挟むのが長さを揃えるためで保存のためではないことの 2 点である。この issue が扱うのは、その判断そのものではなく、型も名前もその判断を将来にわたって保証していないことである。判断が今は正しくても、型が同じままなら、次にこの関数を使う人が同じ判断に至る保証はない。

取りうる手

どれを採るかはこの issue では決めない。

  • 名前を用途に寄せる。比べてよいものを名前に出す。利点は変更が小さく、読んだ人が用途を取り違えにくくなること。費用は名前の変更が import している箇所に波及することと、型では依然として誤用を止められないこと。
  • 公開範囲を狭める。@checkin/api の入口から safeEqual を外す。利点はパッケージの外から呼べなくなること。費用は apps/web の 2 箇所が実際に使っているので、照合そのものを行う関数を代わりに公開するなど、それらに届く別の入口を用意する必要があること。
  • トークンに branded type を与えて型で縛る。generateToken の戻り値を専用の型にし、safeEqual の引数をその型にする。利点は誤用が型チェックで止まること。費用は generateToken の戻り値を受ける全経路と、cookie やヘッダーから読んだ文字列をその型へ持ち上げる箇所に、型の付け替えが要ること。

直さずに残した理由

packages/api/src/auth/crypto.ts は main に無く、統合の対象になったブランチ側で導入されたコードである。2026-09-18 に git cat-file -e main:packages/api/src/auth/crypto.ts が失敗すること、および git log --oneline FETCH_HEAD -- packages/api/src/auth/crypto.ts が 1 件だけ返すことで確かめた。

PR #68 は、統合の対象になったコードの振る舞いを変えることになる修正を、この差分では行わずに後続の issue へ回している。PR の本文の「既知の穴」の節が、isDuplicateKeyError の 3 実装の統一について「統一は auth 由来のコードの振る舞いの変更になるためである」と書いて見送り、#60 へ回しているのがその例である。safeEqual も、名前か型か公開範囲を変えれば上に挙げた import と呼び出しに波及するので、統合の差分とセキュリティ設計の判断を混ぜないために分けた。

Lenguaje dominante
TypeScript
Estrellas
0
Forks
0
Sin PR fusionados en 30 d

Preparar el entorno

  • Incluye un Dockerfile o un archivo de Docker Compose
  • Sin plantilla de pull request
  • Sin guía de contribución

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de traPtitech/Checkin

Todos los issues de traPtitech/Checkin

Issues similares

Más issues de TypeScript

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.