traPtitech / traPtitech/Checkin
safeEqual が公開 API でありながら、型も名前も用途を制約していない
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 0
- Forks
- 0
- Avg merge
- 3d 5h
- Merged PRs (30d)
- 7
Description
今の状態
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 と呼び出しに波及するので、統合の差分とセキュリティ設計の判断を混ぜないために分けた。
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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