Skip to content

fix: レビュー指摘の対応 - #4

Merged
laiso merged 1 commit into
payjp:mainfrom
laiso:fix/security-review-followup
May 1, 2026
Merged

fix: レビュー指摘の対応#4
laiso merged 1 commit into
payjp:mainfrom
laiso:fix/security-review-followup

Conversation

@laiso

@laiso laiso commented May 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

外部セキュリティレビューで上がった指摘のうち、妥当と判断した 5 件を 1 PR にまとめる。

カテゴリ 修正内容
HIGH webhook トークン比較を `crypto.timingSafeEqual` に変更 (タイミング攻撃耐性)
MEDIUM iOS Bundle ID typo `com.exmaple.` → `com.example.` (4 箇所)
MEDIUM `ALLOWED_ORIGINS` 未設定時に起動警告ログ
LOW `quantity` を整数 / 1-100 範囲でバリデーション
LOW README の Node 版数を CI 整合 (18 → 22)、CORS 注記追加

fork で 5/5 ジョブ green を確認済み (https://github.com/laiso/payjpv2-samples/pull/2)。

レビュー指摘から除外したもの

指摘 除外理由
iOS deployment target 26.1 が架空 2026-05 時点で iOS 26 は実在、Xcode 26.1 が対応
`sk_test_*` を Critical テスト環境専用キーで実害なし
`android/local.properties` tracked `.gitignore` line 7-8 で既にカバー、`git ls-files` も空
Android release が debug 署名 サンプル設計上の意図
`minSdkVersion = 16` サンプルとして広く動かす選択
iOS / Android テスト未整備 CI 拡張は別 PR スコープ
`PAYJP_WEBHOOK_SECRET` が `.env` に無い ローカル個人セットアップの問題

Test plan

  • `node --check server/index.js` 通過
  • webhook 異トークン (異長 / 同長) / ヘッダ無し → 401、正トークン → 200
  • quantity 0 / -1 / 1.5 / 999 → 400、1 → API 到達
  • iOS `exmaple` 文字列残存 0 件
  • laiso fork で 5 ジョブ green
  • upstream で 5 ジョブ green

外部レビューで上がった妥当な 5 件をまとめて対応:

- server: webhook トークン比較を crypto.timingSafeEqual に変更
  (タイミング攻撃耐性)
- server: quantity に整数 / 1-100 範囲のバリデーションを追加
- server: ALLOWED_ORIGINS 未設定時に警告ログを出力
- ios: Bundle ID typo "exmaple" を "example" に修正 (4 箇所)
- README: Node.js 版数を CI 整合 (18 → 22)、CORS 既定の注意書きを追加

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@laiso
laiso merged commit adc40d3 into payjp:main May 1, 2026
5 checks passed
@laiso laiso changed the title fix: セキュリティレビュー指摘の対応 fix: レビュー指摘の対応 May 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant