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

オープン
  • 何か問題がありますか？
#69 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る

まだ誰も着手していません。

評価

難易度
5/5
見積もり時間
1週間以上
初心者へのやさしさ
35/100
issue の種類
リファクタリング
明瞭さ
説明が足りない
活発さ
活発
技術スタック
node.js, typescript
領域
api, authentication, security

調査の方向性

packages/api/src/auth/crypto.ts、その auth/index.ts および src/index.ts 経由のエクスポート、さらに apps/web/server/routes/login/callback.get.ts と apps/web/server/utils/auth.ts の呼び出し元から始めてください。packages/api/src/auth/auth.test.ts とパッケージのエクスポートを確認し、その後、命名、可視性、branded type の各オプションを比較してください。選択した設計によって意図しないパスワードの使用を防ぎ、トークンの呼び出し元を維持し、関連するテストに合格できれば完了です。

索引モデルが issue の本文から書いたものです。

説明

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 と呼び出しに波及するので、統合の差分とセキュリティ設計の判断を混ぜないために分けた。

主要言語
TypeScript
スター
0
フォーク
0
30日以内にマージされた PR はありません

環境構築

  • Dockerfile または Docker Compose ファイルあり
  • プルリクエストのテンプレートなし
  • コントリビューションガイドなし

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

traPtitech/Checkin のほかの issue

traPtitech/Checkin の issue をすべて見る

似ている issue

TypeScript の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。