きっかけ
注文管理アプリの実装で、認可まわりに3つのバグを見つけました。
- 管理者用APIなのに、一般ユーザーでも叩けてしまう(ロールチェック漏れ)
- URLの
idを変えるだけで、他人の注文を見られてしまう(user_idの絞り込み忘れ / IDOR) - 画面でロールによる表示の出し分けができていない
バラバラのバグに見えて、原因は1つでした。
認可の判断が、あちこちのコードに手書きでコピペされていたこと。 手書きなので、どこかで必ず忘れてしまいます。
一番刺さった学び
今日学んだ中で一番刺さったのは 「アクセス制御の不備(IDOR)」 です。
理由は、攻撃者が特別なことを何もしていないからです。URLの id を 1つずらすだけで他人のデータに届いてしまいます。しかもコードは一見ちゃんと書けています。「正しく書いたつもり」なのに、1箇所の書き忘れで穴が空く。 だから「気をつける」では防げません。構造で防ぐしかない、と痛感しました。
自分が立てた仮説
バグを直すにあたって、まず自分はこう考えました。
-
user_idの付け忘れ → 検索条件(Criteria)をドメインとして作り、検索では必ずそれを経由させる。userIdとidを持つ Criteria を定義すれば、絞り込みの付け忘れを防げるのでは。 -
ロールチェック →
enumで役割を型安全に扱えばいいのでは。
方向性としては悪くないと思っていました。
AIと壁打ちして考えた設計方針
壁打ちしてみて、自分の仮説は方向性は合っているけれど「もう一歩」足りない、と気づきました。最終的に次の3つを柱に設計することにしました。それぞれ「なぜセキュリティ的に効くのか」も自分なりに整理します。
判断は1箇所(ドメイン)に集約する
最初は「Criteria や Actor を作ればいい」くらいに考えていましたが、本質は 認可の判断を書く場所を1つに絞ることでした。判断が各ハンドラに散っていたのが、そもそもの穴の原因だったからです。
入口を1つにすれば、守るべき場所が「全API」から「その1箇所」に減ります。セキュリティ的には、チェックの抜け漏れ(＝攻撃者の入口)が構造的に生まれなくなる、ということです。
不正な状態をそもそも作らせない(Always-Valid)
仮説では「Criteria に user_id を持たせる」でしたが、"持てる"だけだと結局セットし忘れます。なので 「所有者なしでは生成できない」ところまで寄せることにしました。
中途半端な状態を最初から存在させなければ、そこを突く攻撃も成立しません。**「チェックで弾く」のではなく「そもそも作らせない」**という発想です。
信用できない入力は入口で安全な型に変える(Parse, don't validate)
URLの id は生の文字列で、信用できない外部入力です。これを奥まで持ち回って使う直前に検証していると、検証を忘れる経路ができてしまいます。なので 入口で一度だけ検証して安全な型(OrderId など)に変換し、以降はその型だけを扱うことにしました。
どこまでが危険な生データで、どこからが安全かが型で見えるので、信頼の境界がはっきりします。
まとめると、入口で安全な型に変え(Parse, don't validate)、その型は常に正しく(Always-Valid)、使う入口は1つに絞る(ドメインに集約)。
こうすると「不正な状態を・変な場所で・うっかり作る」ことがどこでもできなくなります。
仮説にあった Criteria と enum は、この3つに沿って 「持てる→持たないと作れない」「enum は名前、判断はふるまいへ」 と一歩進めた形になりました。
実際にどう書いたか
1. Actor ― 「誰が」と「何ができるか」をまとめる
ログインユーザーを表すオブジェクトです。enum は役割の名前として使い、if (role !== 'admin') のような判断はこのクラスのメソッドに集約します。
enum Role {
Admin = 'admin',
Member = 'member',
}
class Actor {
constructor(
readonly userId: number,
readonly role: Role,
) {}
// 判断はハンドラに書かず、ここに集約する
canManageUsers() {
return this.role === Role.Admin
}
}
呼ぶ側は if (role !== 'admin') ではなく actor.canManageUsers() と意図の名前で尋ねます。ルールが変わってもこのメソッドだけ直せば済みます。
2. OrderCriteria ― 所有者なしでは作れない検索条件
class OrderCriteria {
private constructor(readonly ownerId: number, readonly orderId?: number) {}
// actor からしか作れない = user_id が必ず入る
static forOwner(actor: Actor, orderId?: number) {
return new OrderCriteria(actor.userId, orderId)
}
}
new を外から呼べないので、forOwner(actor) 経由でしか作れません。
= user_id の絞り込みを忘れようがありません。
3. OrderRepository ― DB アクセスを 1 箇所にまとめる
find は条件に合う注文を複数、findOne は1件だけ(なければ null)返します。
WHERE user_id = ... はここで必ず criteria.ownerId から付くので、呼び出し側が絞り込みを書くことはありません。
class OrderRepository {
constructor(private readonly pool: Pool) {}
async find(criteria: OrderCriteria) {
// user_id は criteria.ownerId から必ず付く。生SQLを外に出さない
const { rows } = await this.pool.query(
'SELECT * FROM orders WHERE user_id = $1 ORDER BY id DESC',
[criteria.ownerId],
)
return rows
}
// find の結果から 1 件だけ取り出すヘルパー
async findOne(criteria: OrderCriteria) {
const rows = await this.find(criteria)
return rows[0] ?? null
}
}
4. 使う側(ハンドラ)はこれだけです
orderRoutes.get('/:id', async (c) => {
const actor = c.get('actor')
const criteria = OrderCriteria.forOwner(actor, Number(c.req.param('id')))
const order = await orderRepo.findOne(criteria) // user_id は内部で必ず付く
return order ? c.json(order) : c.json({ error: 'not found' }, 404)
})
ロールチェックも同じ発想で、if (role !== 'admin') を各所に書くのではなく、requireRole('admin') を管理者ルートにまとめて適用すれば、書き忘れが消えます。
なお今回のトークンは署名なしで
roleを改ざんできる状態でした。
どれだけ設計を整えても、roleの出所が信用できなければ意味がありません(これも「信頼の境界」の話)。
これから意識する一行のチェック
「この生の string/number は、入口で“安全な型”に変換したか? そのまま奥へ流していないか?」
まとめ
- 認可バグの根っこは「判断があちこちに手書きでコピペされていた」こと
- 一番怖いのは IDOR。正しく書いたつもりでも、書き忘れ1つで穴が空いてしまいます
- 直す鍵は ドメインに集約 / Always-Valid / Parse, don't validate
- 「気をつける」ではなく「忘れても安全」な構造にします