traPtitech / traPtitech/Checkin

sanitizeRedirect が 2 本あり、片方だけが更新されると判定が分岐する

Open
#75 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

今の状態

sanitizeRedirect の実装は 2 つある。サーバー側が packages/api/src/auth/redirect.ts、クライアント側が apps/web/app/composables/useSanitizeRedirect.ts である(数え方: git grep -n 'export function sanitizeRedirect' -- packages apps)。

2026-09-18 に main、HEAD=74ee63c の作業ツリーで、この 2 つが同じ答えを返すことを 2 通りの方法で確かめた。

1 つ目はファイル全体の比較である。ファイルの先頭から末尾までを対象に、コメントだけの行を落としてから比べたところ、差分は 1 行も出なかった。数え方は diff <(sed -E '/^[[:space:]]*(\/\*|\*|\/\/)/d' packages/api/src/auth/redirect.ts) <(sed -E '/^[[:space:]]*(\/\*|\*|\/\/)/d' apps/web/app/composables/useSanitizeRedirect.ts) で、終了コードが 0 になる。落としたコメントは、両ファイルの冒頭の doc comment と、サーバー側の関数の中にだけある 2 行である。

実行される記述が同じなので、同じランタイムで走らせる限り、この 2 つはすべての入力に対して同じ答えを返す。ただし実際にはサーバー側は Node で、クライアント側はブラウザーで走る。どちらの実装も new URL() に依拠しているので、答えが一致することは、その 2 つのランタイムの URL の実装が一致することに依存する。両方のランタイムで測ってはいない。下の 43 件は Node で両方を呼んだ結果である。

2 つ目は実際に呼んだ結果の比較で、ファイル全体の比較の裏取りである。Node で両方のファイルから関数を import し、同じ入力を渡して返り値を文字列として比べた。入力は 43 件で、返り値が食い違ったものは 0 件だった。確かめた入力の範囲は次のとおりである。

  • 絶対 URL。https:http: のもの、スキームを大文字にしたもの。
  • プロトコル相対。///////@ で始まるもの。
  • バックスラッシュ。先頭が /\\\\//\/ のものと、パスの途中に \ を含むもの。
  • 制御文字と空白。前後と途中に空白・タブ・改行・NUL を置いたもの、\r\n を含むもの。
  • javascript: スキーム。そのまま、大文字小文字を混ぜたもの、先頭に空白を置いたもの、javascript の間にタブを入れたもの。data: スキームも同じ形で確かめた。
  • パーセントエンコーディング。%2f%2f/%2f%2f%2F//%2e%2e//path%00、クエリの中に %2F%2F を置いたもの。
  • 空文字列、undefinednull/、通常のパス、クエリとフラグメント付きのパス、/./../../admin

したがってこれは、今の答えが食い違っているという不具合ではなく、実装が 2 本あることそのものを扱う issue である

呼び出し側は分かれている。サーバー側の sanitizeRedirectapps/web/server/routes/login.get.tsapps/web/server/routes/login/callback.get.tsapps/web/server/routes/verify-email/confirm.get.tsapps/web/server/routes/dev/login.get.tspackages/api/src/auth/router.ts から呼ばれる。クライアント側の sanitizeRedirectapps/web/app/pages/verify-email.vue から呼ばれる(数え方: git grep -n 'sanitizeRedirect' -- packages apps)。

テストはサーバー側にしかない。packages/api/src/auth/auth.test.tsdescribe('redirect: sanitizeRedirect') がそれで、apps/web/app/composables/useSanitizeRedirect のテストファイルは無い(数え方: ls apps/web/app/composables/)。

2 本に分けた理由と、どちらが基準かは、apps/web/app/composables/useSanitizeRedirect.ts のコメントに書いてある。クライアントがサーバーの実装ではなく型だけを import するようにするためであり、コメントは mirroring the server's sanitizeRedirect (packages/api/src/auth/redirect.ts) と書いてサーバー側が基準であることを示している。実際に apps/web/app が workspace のパッケージから import しているのは @checkin/api-contract の型だけで、@checkin/api を import している箇所は無い(数え方: git grep -n "from '@checkin/" -- apps/web/app)。この境界を強制する lint の規則は eslint.config.mjs に無い。

誰が何を誤るか

片方だけを直した人が、両方が同じ答えを返し続けると読む。サーバー側にはテストがあり、クライアント側には無いので、クライアント側だけが古くなっても、テストは通ったままである。

逆にサーバー側の判定を厳しくした場合も、クライアント側は自動では変わらないので、同じ入力に対する答えが経路によって分かれる。apps/web/app/pages/verify-email.vue が受理したリダイレクト先をサーバー側が拒否する、あるいはその逆が起きる。コメントはサーバー側が基準であると書いているが、その対応関係を検査する手段が無いので、分岐したことはコメントを読み直すまで分からない。

取りうる手

  • 純粋関数を、クライアントから import してよい位置に 1 つ置き、両方がそれを使う。packages/api-contract は依存が @orpc/contractzod だけなので、置き場所の候補になる(packages/api-contract/package.json)。ただし apps/web/app がこのパッケージから import しているのは今のところ型だけで、型の import は変換の際に消えるので、クライアント側から実行時の値を import できるかは確かめていない。この案を採る場合は、まずそれを確かめる必要がある。この案を採ると、クライアントがサーバーの実装を import しないという今の境界は保たれる。
  • 2 本のままにする場合は、同じ入力表に対して両方が同じ答えを返すことを検査するテストを置く。今はサーバー側にしかテストが無いので、クライアント側が古くなったことを検出する手段が無い。
  • 2 本のままにする場合は、eslint.config.mjsno-restricted-imports を足して、クライアントが @checkin/api を import しないことを規則として明示する。今はコメントに書かれているだけで、機械的な検査が無い。

直さずに残した理由

技術選定の見直しで認証・セッションの基盤の作り直しを決めており、その結論は #72 にある。sanitizeRedirect は traQ OAuth とメール確認のリダイレクト先を決める関数で、その両方の経路が認証の一部である。置き場所を変えることは、クライアントとサーバーのどちらが何を import してよいかという境界の決め方を変えることでもあり、どちらもその作り直しと同じ範囲に入る。基盤が入れ替われば、リダイレクト先の検査をどの層に置くかもそこで決まるので、先に置き場所だけを動かすと二度動かすことになる。

スコープ外/関連

  • 認証・セッションの基盤をどう作るかの結論は #72。#72 は useSanitizeRedirect.ts を自作のまま残すと決めているので、この issue が扱う 2 本の分岐はその決定では解消しない。
  • sanitizeRedirect が受理と拒否をどう判定すべきかは、この issue の範囲ではない。扱うのは実装が 2 本あることだけである。
  • #16 は認証サブシステムを実装するための issue である。この issue が扱うのは、実装済みの sanitizeRedirect が 2 本あることなので、別の事項である。

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

Read the authentication redesign decision in issue #72 first, then inspect packages/api/src/auth/redirect.ts and apps/web/app/composables/useSanitizeRedirect.ts. Review their callers, packages/api/src/auth/auth.test.ts, packages/api-contract/package.json, and eslint.config.mjs before choosing an approach. Done means the chosen boundary or parity check is implemented so future changes cannot silently make the server and client decisions diverge.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
authentication, backend-api-design
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.