traPtitech / traPtitech/Checkin

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

Open
#69 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement
Dominant language
TypeScript
Stars
0
Forks
0
Avg merge
3d 5h
Merged PRs (30d)
7

Description

今の状態

packages/api/src/auth/crypto.tssafeEqual(a: string, b: string) => boolean という型を持ち、両方の入力を SHA-256 で固定長のダイジェストにしてから node:cryptotimingSafeEqual に渡す。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.tsexport * from './crypto' で再エクスポートし、packages/api/src/index.tsexport * from './auth' で再エクスポートする。packages/api/package.jsonexports"."./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.tsisCsrfValid。CSRF の cookie と x-csrf-token ヘッダーを照合する。
  • packages/api/src/auth/auth.test.tscrypto: safeEqual の 3 行。

apps 側の 2 箇所が比べているのは、いずれも同じファイルの generateTokennode:cryptorandomBytes から作る値である。generateToken の JSDoc はこれを "a high-entropy, URL-safe opaque token" と述べている。stategenerateToken(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_atdismissed_bydismissed_reasondismissed_comment はいずれも null である。この alert は dismiss していない

同じ日に gh api repos/traPtitech/Checkin/code-scanning/alerts/3/instances を実行すると、refs/pull/68/headrefs/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.tsmain に無く、統合の対象になったブランチ側で導入されたコードである。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 と呼び出しに波及するので、統合の差分とセキュリティ設計の判断を混ぜないために分けた。

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with packages/api/src/auth/crypto.ts, its exports through auth/index.ts and src/index.ts, and the callers in apps/web/server/routes/login/callback.get.ts and apps/web/server/utils/auth.ts. Review packages/api/src/auth/auth.test.ts and the package exports, then compare the naming, visibility, and branded-type options; done means one chosen design prevents unintended password use while preserving the token callers and passing the relevant tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
node.js, typescript
Domain
api, authentication, security
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.