Skip to content

OpenID Connect Core Basic エンドポイントの作成#53

Open
anko9801 wants to merge 14 commits intofeat/client-crudfrom
feat/oauth-oidc
Open

OpenID Connect Core Basic エンドポイントの作成#53
anko9801 wants to merge 14 commits intofeat/client-crudfrom
feat/oauth-oidc

Conversation

@anko9801
Copy link
Collaborator

@anko9801 anko9801 commented Jan 25, 2026

Confidential Client (サーバーサイドアプリ) 向けの Authorization Code Flow を実装しました。次のようなフローで認証します。

  1. /authorize: 認可コード取得
  2. /token: 認可コードをトークンに交換
  3. /userinfo: ユーザー情報取得

Summary by CodeRabbit

リリースノート

  • New Features

    • OAuth2/OpenID Connect認証機能を実装しました。ユーザーはログイン画面で認証可能になりました。
    • 標準的なOIDC discovery エンドポイントとトークンエンドポイントを提供します。
    • セッション管理とユーザー認証フローを追加しました。
  • Documentation

    • OIDC実装の検証手順を説明するマニュアルガイドを追加しました。

@anko9801 anko9801 requested a review from Pugma January 25, 2026 15:57
@anko9801 anko9801 changed the title feat: OAuth2/OIDC OpenID Connect Core Basic エンドポイントの作成 Jan 25, 2026
Copy link
Collaborator

@saitenntaisei saitenntaisei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

まあDB周りはあとあとでいいと思うのでLGTM

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Confidential Client(サーバーサイドアプリ)向けに、OIDC Authorization Code Flow を提供するためのエンドポイント(discovery/JWKS/authorize/token/userinfo)と、そのための永続ストレージ・認証導線(ログイン/セッション)を追加するPRです。

Changes:

  • OIDC discovery(/.well-known/openid-configuration)と JWKS(/.well-known/jwks.json)、OAuth2/OIDC主要エンドポイント(/oauth2/authorize,/oauth2/token,/oauth2/userinfo)を追加
  • fosite を用いたOAuth/OIDCサーバー実装と、認可コード・トークン永続化のためのDBスキーマ/SQLCを追加
  • Portal DB を参照したユーザー認証(ログイン/ログアウト)とセッション管理を追加

Reviewed changes

Copilot reviewed 21 out of 22 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
internal/router/v1/oauth.go authorize/token/userinfo、discovery/JWKS ハンドラを追加
internal/router/v1/auth.go ログイン/ログアウトとCookieセッションによる認証導線を追加
internal/router/v1/handler.go Handler に oauth/provider・userRepo・session・config を追加
internal/repository/oauth_storage.go fosite.Storage 実装(認可コード/トークン永続化など)を追加
internal/repository/user.go Portal DB のユーザー認証リポジトリを追加
db/schema.sql / db/query/oidc.sql / internal/repository/oidc/* authorization_codes/tokens テーブルとSQLC生成物を追加
cmd/serve.go / cmd/oauth.go / cmd/config.go 2DB接続、RSA鍵、fosite provider、設定読み込み変更
api/openapi.yaml / internal/router/v1/gen/* OIDC関連API定義と生成コードを追加
go.mod / go.sum / compose.yaml / config.yaml 依存追加と実行環境設定の更新

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +146 to +157
issuer := h.config.Issuer
scopesSupported := []string{"openid", "profile", "email"}
claimsSupported := []string{"sub", "name", "preferred_username", "email", "email_verified"}
codeChallengeMethodsSupported := []string{"S256", "plain"}
tokenEndpointAuthMethodsSupported := []string{"client_secret_basic", "client_secret_post"}

return ctx.JSON(http.StatusOK, gen.OpenIDConfiguration{
Issuer: issuer,
AuthorizationEndpoint: issuer + "/oauth2/authorize",
TokenEndpoint: issuer + "/oauth2/token",
UserinfoEndpoint: issuer + "/oauth2/userinfo",
JwksUri: issuer + "/.well-known/jwks.json",
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issuer URL is concatenated with endpoint paths without normalizing trailing slashes, which can produce invalid URLs (e.g. "https://issuer/" + "/oauth2/token" => double slash). Trim trailing "/" from issuer before building endpoint URLs.

Copilot uses AI. Check for mistakes.
Comment on lines +127 to +132
func (h *Handler) GetJWKS(ctx echo.Context) error {
pubKey := &h.config.PrivateKey.PublicKey

hash := sha256.Sum256(pubKey.N.Bytes())
kid := base64.RawURLEncoding.EncodeToString(hash[:8])

Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetJWKS dereferences h.config.PrivateKey without checking it’s configured; a nil PrivateKey will panic at runtime. Consider validating config in NewHandler/newServer (fail fast) or returning 500 if the key is missing.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@Pugma Pugma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

まだレビューを終えられてないんですが、先にロジック以外で気になったところだけでもコメントをつけておきます…!

compose.yaml Outdated
Comment on lines +16 to +18
- OIDC_DATABASE_HOST=oidc
- OIDC_DATABASE_PORT=3306
- OIDC_PORTAL_DATABASE_HOST=portal
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こちらは config.yaml には追記しない感じなんでしょうか…?
現時点での環境変数と config.yaml の使い分けがあまりわかっておらず…
特に使い分けがないのであれば、いずれかにまとめてしまっていいのかなと思ったという感じです

Copy link
Collaborator

@Pugma Pugma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

追加で確認しました!
openapi.yamlrouter/v1/oauth.go については、改めてドキュメントを確認するのでもう少々お待ちいただきたいです

現状で気になったのは、 oauth_storage.go user.go です
このファイルで、

  • 構造体 + getter / setter の定義 (domain)
  • DB query のラップ (repository)
  • session の validation (usecase)

が混在している気がします
これらのファイルが分割されると、ファイルごとの責務が分かりやすくなって見通しが良くなるかと思います…!

@saitenntaisei
Copy link
Collaborator

提案
正常系は通っててこれをもとにportalの開発を進めるのは支障がないので一旦mergeして後からfixのprを出すのはどうですか?
image

// OAuthClient implements fosite.Client
type OAuthClient struct {
ID string
Secret []byte

Check failure

Code scanning / gosec

Exported struct field "SessionSecret" (JSON key "SessionSecret") matches secret pattern

Exported struct field "Secret" (JSON key "Secret") matches secret pattern
… subpackage

Split monolithic oauth_storage.go (445 lines) into focused files:
- storage.go: Storage struct, constructor, client management
- authcode.go: authorize code + PKCE sessions
- token.go: access/refresh token CRUD + revocation
- oidc.go: OpenID Connect in-memory sessions
- client.go: fosite.Client implementation
- session.go: fosite.Session/openid.Session implementation

Key improvements in the extracted code:
- Unexport Session fields (subject, username, expiresAt, extra)
- Add interface compliance assertions (var _ fosite.Storage, var _ fosite.Client)
- Use maps.Copy() instead of manual map copy loops
- Set AuthTime in Session constructor for OIDC conformance
- Enforce PKCE (EnforcePKCE: true, EnforcePKCEForPublicClients: true)

Assisted-by: Claude Code (model: claude-opus-4-6)
@coderabbitai
Copy link

coderabbitai bot commented Feb 15, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • ✅ Review completed - (🔄 Check again to review again)
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/oauth-oidc

Comment @coderabbitai help to get the list of available commands and usage tips.

- Update Go to 1.25.7 for security fixes
- Manage oapi-codegen via mise instead of go tool directive
- Use double underscore as koanf env separator
- Remove bearerFormat from OpenAPI security scheme

Assisted-by: Claude Code (model: claude-opus-4-6)
…orage

- Add domain types for AuthCode, Token, OIDCSession, and User
- Add repository interfaces for AuthCode, Token, Client, and OIDCSession
- Add OIDC session database schema with auth_time column
- Refactor OAuth storage to use repository interfaces with transaction support
- Implement compile-time interface checks for fosite storage contracts
- Add GetWithSecretHash to ClientRepository for OAuth client lookup
- Change NewSession to accept authTime parameter for OIDC conformance

Assisted-by: Claude Code (model: claude-opus-4-6)
- Extract user authentication logic into UserUseCase with GetByID
- Simplify UserRepository to pure data access layer
- Handle prompt=none (return login_required without redirect)
- Handle prompt=login (force re-authentication with loop prevention)
- Handle max_age parameter (re-authenticate if auth_time too old)
- Store auth_time in login cookie session for OIDC conformance
- Add return_url sanitization to prevent open redirects
- Harden session cookie with Secure flag and SameSite=Lax
- Return scope-based UserInfo claims (profile scope → name)
- Add POST method for /oauth2/authorize endpoint (OIDC Core 3.1.2.1)

Assisted-by: Claude Code (model: claude-opus-4-6)
Assisted-by: Claude Code (model: claude-opus-4-6)
Widen oidc_sessions.authorize_code to varchar(255) because fosite passes
the full HMAC code (~87 chars) to CreateOpenIDConnectSession.

Disable global PKCE enforcement while keeping it for public clients.
Support PKCS#8 key format in addition to PKCS#1.
Skip portal database connection in non-production environments.

Assisted-by: Claude Code (model: claude-opus-4-6)
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

🧹 Nitpick comments (4)
api/openapi.yaml (1)

209-276: 認可エンドポイントに追加の OIDC パラメータを検討してください。

コミットメッセージに prompt/max_age のハンドリングが記載されていますが、OpenAPI 仕様には含まれていません。以下のパラメータの追加を検討してください:

  • prompt: none, login, consent などの値をサポート
  • max_age: 認証からの最大経過時間(秒)

現在の実装で十分であれば、後続 PR での対応でも問題ありません。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/openapi.yaml` around lines 209 - 276, The OpenAPI spec for the
/oauth2/authorize GET operation (operationId: authorize) is missing OIDC
parameters referenced in commits; add two query parameters to that operation: 1)
prompt — schema type string with an enum that at minimum includes values like
none, login, consent (and allow others if desired) and an appropriate
description; 2) max_age — schema type integer (seconds) with minimum 0 and
description explaining it is the maximum elapsed authentication time; ensure
both are optional (required: false) and follow the same parameter structure as
existing query params (name, in: query, required, schema, description) so the
authorize operation correctly documents prompt and max_age.
internal/repository/user.go (1)

12-12: エラー定義の重複について検討してください。

ErrUserNotFoundinternal/usecase/user.go にも定義されています(行 22)。ErrClientNotFound と同様の重複パターンです。エラー定義の一貫した管理のため、共通パッケージへの統合を検討してください。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/repository/user.go` at line 12, ErrUserNotFound is defined in
multiple places (ErrUserNotFound in the repository and usecase layers);
consolidate the error into a single shared location (e.g., a common/errors or
internal/errors package) and replace duplicate declarations by importing and
referencing that single ErrUserNotFound symbol throughout the codebase; update
imports and remove the duplicate definitions (the existing ErrUserNotFound
declarations in repository and usecase) so all modules use the unified
ErrUserNotFound.
internal/repository/oauth/client.go (1)

32-32: GetAudience()nil を返す点を確認してください。

GetAudience()nil を返していますが、OIDC フローによっては aud クレームの検証に影響する可能性があります。現在の実装で問題なければこのままで構いませんが、将来的に audience 制限が必要になった場合は更新が必要です。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/repository/oauth/client.go` at line 32, GetAudience currently
returns nil which disables OIDC audience checks; confirm this is intentional,
otherwise update the Client.GetAudience method on the Client type to return a
fosite.Arguments containing the allowed audience values (e.g. from your client
config or a field on Client) so the OIDC `aud` validation can operate; locate
GetAudience and the Client construction/fields to source the audience values and
return them as fosite.Arguments instead of nil.
internal/repository/client.go (1)

15-15: エラー定義の重複を検討してください。

ErrClientNotFoundinternal/repository/client.gointernal/usecase/client.go の両方で定義されています。現在のコードは errors.Is() で比較しているため機能上の問題はありませんが、レイヤー間で同じエラーの定義が重複しているため、保守性の観点から共通のエラーパッケージへの統合を検討してください。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/repository/client.go` at line 15, There are duplicate error
definitions named ErrClientNotFound in different layers; consolidate them into a
single shared error constant (e.g., move ErrClientNotFound to a common package
like internal/errors or pkg/errors) and update all callers to import and
reference that single symbol instead of redeclaring it; change the declarations
in client.go and the other file to remove the local ErrClientNotFound and update
imports/usages to use the shared ErrClientNotFound so existing errors.Is
comparisons continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@db/schema.sql`:
- Around line 14-65: The oidc_sessions table lacks a foreign key to
authorization_codes causing orphaned sessions and the authorize_code column
length (varchar(255)) doesn’t match authorization_codes.code (varchar(64));
update oidc_sessions.authorize_code to varchar(64) to match
authorization_codes.code and add a foreign key constraint (e.g., CONSTRAINT
fk_oidc_sessions_authorize_code FOREIGN KEY (authorize_code) REFERENCES
authorization_codes(code) ON DELETE CASCADE) so sessions are removed when their
authorization code is deleted, or alternatively implement and document an
explicit deletion flow that removes oidc_sessions rows whenever
authorization_codes are deleted.

In `@go.mod`:
- Line 96: The go.mod currently pulls the vulnerable indirect dependency
golang.org/x/oauth2 v0.14.0; update this module to v0.27.0 or later by adding or
changing a require directive for golang.org/x/oauth2 to >= v0.27.0 (or run `go
get golang.org/x/[email protected]` and `go mod tidy`) and re-run builds/tests; if
the indirect requirement comes from github.com/ory/fosite, consider also
updating fosite to a version that depends on oauth2 v0.27.0+ or add an explicit
replace/require for golang.org/x/oauth2 to force the safe version.

In `@internal/repository/oauth/authcode.go`:
- Around line 50-51: AuthTime is not persisted into the AuthCode model so
sessions are reconstructed with time.Time{}; add an AuthTime time.Time field to
the AuthCode struct, set authCode.AuthTime when creating the auth code in
CreateAuthorizeCodeSession, and in GetAuthorizeCodeSession pass that stored
value to NewSession (e.g. NewSession(authCode.UserID, authCode.AuthTime)) so
idTokenClaims.AuthTime is initialized correctly; ensure any
persistence/serialization of AuthCode includes the new field.

In `@internal/repository/oauth/session.go`:
- Around line 59-76: The Clone method currently shallow-copies idTokenClaims via
idTokenClaimsClone := *s.idTokenClaims which leaves reference fields shared;
update Clone to deep-copy idTokenClaims.Extra (make a new map and copy entries)
and deep-copy idTokenClaims.Audience and
idTokenClaims.AuthenticationMethodsReferences (create new slices and copy
values), mirroring how idTokenHeaders.Extra is handled; also guard against nil
s.idTokenClaims/s.idTokenClaims.Extra/slices before copying so Clone returns
fully independent idTokenClaims and idTokenHeaders.

In `@internal/repository/oauth/storage.go`:
- Around line 144-150: The two stub methods ClientAssertionJWTValid and
SetClientAssertionJWT currently do nothing but return default values; either
remove them or make intent explicit with a TODO if JWT auth won't be supported,
or implement full JTI storage/expiration/replay-detection if you plan to enable
private_key_jwt/client_secret_jwt later. If keeping stubs, add clear TODO
comments referencing ClientAssertionJWTValid and SetClientAssertionJWT; if
implementing, add a persistent (or in-memory with TTL) JTI store, have
SetClientAssertionJWT save jti with exp, and make ClientAssertionJWTValid check
existence and expiration and return an appropriate error (and delete/expire
entries) to prevent replay.

In `@internal/repository/oauth/token.go`:
- Around line 81-99: In GetRefreshTokenSession, add an expiration check for
token.ExpiresAt (compare with time.Now()) after retrieving the token and before
creating the session; if the refresh token is expired, return fosite.ErrNotFound
(same behavior as access tokens) instead of returning a session—this involves
using token.ExpiresAt, time.Now(), and early-returning before calling
NewSession/newFositeRequest.
- Around line 60-75: CreateRefreshTokenSession is building a domain.Token
without populating the AccessToken field, causing the DB NOT NULL/UNIQUE
constraint to fail; update the domain.Token construction in
CreateRefreshTokenSession to set AccessToken to a unique non-empty value (for
example generate a UUID or otherwise derive a unique token string) before
calling s.getTokens(ctx).Create, ensuring AccessToken satisfies the NOT
NULL/UNIQUE requirements and remains consistent with your token design.
- Around line 77-79: RotateRefreshToken currently no-ops; implement it to
atomically remove the old refresh token and any related tokens by using the
storage transaction, calling DeleteByRefreshToken(refreshTokenSignature) and
DeleteByRequestID(requestID) inside the same transaction so both removals
succeed or roll back together, propagate and return any errors, and ensure the
method uses the provided ctx, requestID and refreshTokenSignature parameters and
the existing transaction helper on Storage.

In `@internal/router/v1/auth.go`:
- Around line 133-142: sanitizeReturnURL currently only rejects host and leading
'//' but allows scheme-prefixed inputs like "javascript:..."; update
sanitizeReturnURL to reject any non-empty parsed.Scheme and ensure the returned
path starts with '/' (e.g., require parsed.Scheme == "" and parsed.Path != ""
and strings.HasPrefix(parsed.RequestURI(), "/")). Also keep the existing checks
for empty input and leading '//' and return "/" for any invalid case; make the
change in the sanitizeReturnURL function.

In `@internal/router/v1/handler.go`:
- Around line 36-43: Add a minimum-length check for the session secret: validate
config.SessionSecret (recommend >=32 bytes) before constructing the cookie store
in handler.go and reject/startup-fail with a clear error if it's too short;
implement the check where the secret is converted to bytes in cmd/serve.go
(around the []byte(cfg.OAuth.Secret) conversion) so the application cannot start
with an insufficient secret. Reference symbols: config.SessionSecret,
sessions.NewCookieStore, cmd/serve.go conversion site
([]byte(cfg.OAuth.Secret)), and keep existing MaxAge and isMaxAgeExpired logic
unchanged.

In `@internal/router/v1/oauth.go`:
- Around line 21-62: In Authorize, change handling for prompt == "none" so that
if the user is authenticated but h.isMaxAgeExpired(ar, info.AuthTime) &&
!h.isReauthCompleted(ctx, info.AuthTime) you call
h.oauth2.WriteAuthorizeError(c, rw, ar, fosite.ErrLoginRequired) and return nil
instead of allowing the flow to reach redirectToLogin; i.e., in the "none" case
(using ar and info) add the max_age check and write the login_required error, or
alternatively check if prompt=="none" before the later generic max_age redirect
and convert that branch into a WriteAuthorizeError using fosite.ErrLoginRequired
(use the existing oauth2.WriteAuthorizeError and fosite.ErrLoginRequired
symbols).

In `@internal/usecase/user.go`:
- Around line 98-120: In verifyPBKDF2SHA512, add input validation before calling
pbkdf2.Key: ensure iterations (from iterationsStr) is > 0, ensure decoded salt
(salt) length > 0, and ensure decoded expectedHash length > 0; if any check
fails return a wrapped ErrUnsupportedHashAlgorithm (consistent with existing
error style) with a clear cause (e.g., "invalid iterations", "empty salt",
"empty hash") so pbkdf2.Key is only invoked with valid parameters and edge cases
are handled explicitly.

In `@TEST_MANUAL.md`:
- Line 60: The markdown header "## 7. 実行と結果確認" is incorrectly numbered and
should be "## 6. 実行と結果確認"; update the header text in TEST_MANUAL.md by replacing
the string "## 7. 実行と結果確認" with "## 6. 実行と結果確認" so the section numbering is
consecutive.

---

Nitpick comments:
In `@api/openapi.yaml`:
- Around line 209-276: The OpenAPI spec for the /oauth2/authorize GET operation
(operationId: authorize) is missing OIDC parameters referenced in commits; add
two query parameters to that operation: 1) prompt — schema type string with an
enum that at minimum includes values like none, login, consent (and allow others
if desired) and an appropriate description; 2) max_age — schema type integer
(seconds) with minimum 0 and description explaining it is the maximum elapsed
authentication time; ensure both are optional (required: false) and follow the
same parameter structure as existing query params (name, in: query, required,
schema, description) so the authorize operation correctly documents prompt and
max_age.

In `@internal/repository/client.go`:
- Line 15: There are duplicate error definitions named ErrClientNotFound in
different layers; consolidate them into a single shared error constant (e.g.,
move ErrClientNotFound to a common package like internal/errors or pkg/errors)
and update all callers to import and reference that single symbol instead of
redeclaring it; change the declarations in client.go and the other file to
remove the local ErrClientNotFound and update imports/usages to use the shared
ErrClientNotFound so existing errors.Is comparisons continue to work.

In `@internal/repository/oauth/client.go`:
- Line 32: GetAudience currently returns nil which disables OIDC audience
checks; confirm this is intentional, otherwise update the Client.GetAudience
method on the Client type to return a fosite.Arguments containing the allowed
audience values (e.g. from your client config or a field on Client) so the OIDC
`aud` validation can operate; locate GetAudience and the Client
construction/fields to source the audience values and return them as
fosite.Arguments instead of nil.

In `@internal/repository/user.go`:
- Line 12: ErrUserNotFound is defined in multiple places (ErrUserNotFound in the
repository and usecase layers); consolidate the error into a single shared
location (e.g., a common/errors or internal/errors package) and replace
duplicate declarations by importing and referencing that single ErrUserNotFound
symbol throughout the codebase; update imports and remove the duplicate
definitions (the existing ErrUserNotFound declarations in repository and
usecase) so all modules use the unified ErrUserNotFound.

Comment on lines +14 to +65
CREATE TABLE IF NOT EXISTS `authorization_codes` (
`code` varchar(64) NOT NULL,
`client_id` char(36) NOT NULL,
`user_id` char(36) NOT NULL,
`redirect_uri` text NOT NULL,
`scopes` text NOT NULL,
`code_challenge` varchar(128) NULL,
`code_challenge_method` varchar(10) NULL,
`nonce` varchar(255) NULL,
`used` BOOLEAN NOT NULL DEFAULT FALSE,
`expires_at` datetime(6) NOT NULL,
`created_at` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
PRIMARY KEY (`code`),
INDEX `idx_authorization_codes_client_id` (`client_id`),
INDEX `idx_authorization_codes_expires_at` (`expires_at`),
CONSTRAINT `fk_authorization_codes_client` FOREIGN KEY (`client_id`) REFERENCES `clients` (`client_id`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;

CREATE TABLE IF NOT EXISTS `tokens` (
`id` char(36) NOT NULL,
`request_id` varchar(64) NOT NULL,
`client_id` char(36) NOT NULL,
`user_id` char(36) NOT NULL,
`access_token` varchar(64) NOT NULL,
`refresh_token` varchar(64) NULL,
`scopes` text NOT NULL,
`expires_at` datetime(6) NOT NULL,
`created_at` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
PRIMARY KEY (`id`),
UNIQUE INDEX `idx_tokens_access_token` (`access_token`),
UNIQUE INDEX `idx_tokens_refresh_token` (`refresh_token`),
INDEX `idx_tokens_client_id` (`client_id`),
INDEX `idx_tokens_user_id` (`user_id`),
INDEX `idx_tokens_request_id` (`request_id`),
INDEX `idx_tokens_expires_at` (`expires_at`),
CONSTRAINT `fk_tokens_client` FOREIGN KEY (`client_id`) REFERENCES `clients` (`client_id`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;

CREATE TABLE IF NOT EXISTS `oidc_sessions` (
`authorize_code` varchar(255) NOT NULL,
`client_id` char(36) NOT NULL,
`user_id` char(36) NOT NULL,
`scopes` text NOT NULL,
`nonce` varchar(255) NULL,
`auth_time` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
`requested_at` datetime(6) NOT NULL,
`created_at` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
PRIMARY KEY (`authorize_code`),
INDEX `idx_oidc_sessions_client_id` (`client_id`),
CONSTRAINT `fk_oidc_sessions_client` FOREIGN KEY (`client_id`)
REFERENCES `clients` (`client_id`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;
Copy link

@coderabbitai coderabbitai bot Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for OIDC session and authorization code deletion patterns
rg -n "DeleteOIDCSession|DeleteOpenIDConnectSession|DeleteAuthorizationCode|authorization_codes" -t go -t java -t ts -t js

Repository: traPtitech/portal-oidc

Length of output: 2001


🏁 Script executed:

#!/bin/bash
# Also search for common delete patterns and cascade behaviors
rg -n "oidc_sessions|authorize_code" -t go -t java -t ts -t js -A 3 | head -100

Repository: traPtitech/portal-oidc

Length of output: 2047


🏁 Script executed:

#!/bin/bash
# Check the schema file more thoroughly
git ls-files | grep -E "schema|migration|db" | head -20

Repository: traPtitech/portal-oidc

Length of output: 545


🏁 Script executed:

#!/bin/bash
# Check the OAuth storage implementation
cat -n internal/repository/oauth/oidc.go | head -80

Repository: traPtitech/portal-oidc

Length of output: 2129


🏁 Script executed:

#!/bin/bash
# Check the authorization code deletion flow
rg -B 5 -A 10 "DeleteAuthorizationCode" internal/repository/authcode.go

Repository: traPtitech/portal-oidc

Length of output: 626


🏁 Script executed:

#!/bin/bash
# Check for transactional patterns and cleanup logic
rg -B 5 -A 10 "DeleteOIDCSession|DeleteAuthorizationCode" internal/handler -t go

Repository: traPtitech/portal-oidc

Length of output: 122


🏁 Script executed:

#!/bin/bash
# Search for authorization code exchange and deletion flows
rg -n "ExchangeAuthorizationCodeForToken|exchange" --type-list | head -20

Repository: traPtitech/portal-oidc

Length of output: 455


🏁 Script executed:

#!/bin/bash
# Look for where both DeleteAuthorizationCode and DeleteOpenIDConnectSession are used
rg -l "Delete" internal/ | xargs rg -l "authorization|oidc" | head -10

Repository: traPtitech/portal-oidc

Length of output: 406


🏁 Script executed:

#!/bin/bash
# Check the main entry points and handlers
fd -t f -e "go" "handler|handler|service" internal/ | head -20

Repository: traPtitech/portal-oidc

Length of output: 95


🏁 Script executed:

#!/bin/bash
# Examine the fosite callback implementations
rg -n "PersistAuthorizeCode|PersistRefreshTokenGrantToken|IntrospectToken" -t go -B 3 -A 10

Repository: traPtitech/portal-oidc

Length of output: 1035


🏁 Script executed:

#!/bin/bash
# Find fosite callback methods in Storage
rg -n "Persist|Revoke" internal/repository/oauth/ -t go

Repository: traPtitech/portal-oidc

Length of output: 310


🏁 Script executed:

#!/bin/bash
# Check the Storage type and its methods
cat -n internal/repository/oauth/token.go | head -100

Repository: traPtitech/portal-oidc

Length of output: 3694


🏁 Script executed:

#!/bin/bash
# Search for where authorization codes are persisted/exchanged
rg -n "PersistAuthorizeCode\|ExchangeCode\|exchange" internal/repository/oauth/ -B 3 -A 10

Repository: traPtitech/portal-oidc

Length of output: 48


🏁 Script executed:

#!/bin/bash
# Check all methods in the oauth Storage
rg -n "func \(s \*Storage\)" internal/repository/oauth/ -A 1

Repository: traPtitech/portal-oidc

Length of output: 6250


🏁 Script executed:

#!/bin/bash
# Look at the authcode.go file
cat -n internal/repository/oauth/authcode.go

Repository: traPtitech/portal-oidc

Length of output: 3452


🏁 Script executed:

#!/bin/bash
# Search for the interface definition fosite expects
rg -n "PersistAuthorizeCode" --type go

Repository: traPtitech/portal-oidc

Length of output: 48


🏁 Script executed:

#!/bin/bash
# Check if authorization codes are deleted or only marked as used
cat -n internal/repository/oidc/oidc.sql.go | grep -A 5 "DeleteAuthorizationCode\|deleteAuthorizationCode\|CleanupExpired\|cleanup"

Repository: traPtitech/portal-oidc

Length of output: 538


🏁 Script executed:

#!/bin/bash
# Look for background jobs or cleanup routines
fd -t f -e "go" | xargs rg -l "cleanup\|expired\|Cleanup" | head -10

Repository: traPtitech/portal-oidc

Length of output: 48


🏁 Script executed:

#!/bin/bash
# Check the query file for auth code operations
cat db/query/oidc.sql

Repository: traPtitech/portal-oidc

Length of output: 3051


🏁 Script executed:

#!/bin/bash
# Check if cleanup routines are called anywhere
rg -n "DeleteExpired\|cleanup\|Cleanup" -t go internal/

Repository: traPtitech/portal-oidc

Length of output: 48


🏁 Script executed:

#!/bin/bash
# Check for job/scheduler implementations
fd -t f -e "go" | xargs rg -l "job\|ticker\|schedule\|Cron" | head -10

Repository: traPtitech/portal-oidc

Length of output: 48


OIDC セッションの参照整合性が欠けています

oidc_sessions.authorize_codeauthorization_codes.code と紐付いておらず、認可コード削除時にセッションが孤児化する可能性があります。authorize_code の型長も 255 と 64 で不一致なので FK を張れません。FK + 型長統一、または明示的な削除フローのどちらかを必ず担保してください。

🔧 例: FK 追加と型長合わせ
-  `authorize_code` varchar(255) NOT NULL,
+  `authorize_code` varchar(64) NOT NULL,
   `client_id` char(36) NOT NULL,
   `user_id` char(36) NOT NULL,
   `scopes` text NOT NULL,
   `nonce` varchar(255) NULL,
   `auth_time` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
   `requested_at` datetime(6) NOT NULL,
   `created_at` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
   PRIMARY KEY (`authorize_code`),
   INDEX `idx_oidc_sessions_client_id` (`client_id`),
-  CONSTRAINT `fk_oidc_sessions_client` FOREIGN KEY (`client_id`)
-    REFERENCES `clients` (`client_id`) ON DELETE CASCADE
+  CONSTRAINT `fk_oidc_sessions_client` FOREIGN KEY (`client_id`)
+    REFERENCES `clients` (`client_id`) ON DELETE CASCADE,
+  CONSTRAINT `fk_oidc_sessions_auth_code` FOREIGN KEY (`authorize_code`)
+    REFERENCES `authorization_codes` (`code`) ON DELETE CASCADE
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
CREATE TABLE IF NOT EXISTS `authorization_codes` (
`code` varchar(64) NOT NULL,
`client_id` char(36) NOT NULL,
`user_id` char(36) NOT NULL,
`redirect_uri` text NOT NULL,
`scopes` text NOT NULL,
`code_challenge` varchar(128) NULL,
`code_challenge_method` varchar(10) NULL,
`nonce` varchar(255) NULL,
`used` BOOLEAN NOT NULL DEFAULT FALSE,
`expires_at` datetime(6) NOT NULL,
`created_at` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
PRIMARY KEY (`code`),
INDEX `idx_authorization_codes_client_id` (`client_id`),
INDEX `idx_authorization_codes_expires_at` (`expires_at`),
CONSTRAINT `fk_authorization_codes_client` FOREIGN KEY (`client_id`) REFERENCES `clients` (`client_id`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;
CREATE TABLE IF NOT EXISTS `tokens` (
`id` char(36) NOT NULL,
`request_id` varchar(64) NOT NULL,
`client_id` char(36) NOT NULL,
`user_id` char(36) NOT NULL,
`access_token` varchar(64) NOT NULL,
`refresh_token` varchar(64) NULL,
`scopes` text NOT NULL,
`expires_at` datetime(6) NOT NULL,
`created_at` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
PRIMARY KEY (`id`),
UNIQUE INDEX `idx_tokens_access_token` (`access_token`),
UNIQUE INDEX `idx_tokens_refresh_token` (`refresh_token`),
INDEX `idx_tokens_client_id` (`client_id`),
INDEX `idx_tokens_user_id` (`user_id`),
INDEX `idx_tokens_request_id` (`request_id`),
INDEX `idx_tokens_expires_at` (`expires_at`),
CONSTRAINT `fk_tokens_client` FOREIGN KEY (`client_id`) REFERENCES `clients` (`client_id`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;
CREATE TABLE IF NOT EXISTS `oidc_sessions` (
`authorize_code` varchar(255) NOT NULL,
`client_id` char(36) NOT NULL,
`user_id` char(36) NOT NULL,
`scopes` text NOT NULL,
`nonce` varchar(255) NULL,
`auth_time` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
`requested_at` datetime(6) NOT NULL,
`created_at` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
PRIMARY KEY (`authorize_code`),
INDEX `idx_oidc_sessions_client_id` (`client_id`),
CONSTRAINT `fk_oidc_sessions_client` FOREIGN KEY (`client_id`)
REFERENCES `clients` (`client_id`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;
CREATE TABLE IF NOT EXISTS `oidc_sessions` (
`authorize_code` varchar(64) NOT NULL,
`client_id` char(36) NOT NULL,
`user_id` char(36) NOT NULL,
`scopes` text NOT NULL,
`nonce` varchar(255) NULL,
`auth_time` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
`requested_at` datetime(6) NOT NULL,
`created_at` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
PRIMARY KEY (`authorize_code`),
INDEX `idx_oidc_sessions_client_id` (`client_id`),
CONSTRAINT `fk_oidc_sessions_client` FOREIGN KEY (`client_id`)
REFERENCES `clients` (`client_id`) ON DELETE CASCADE,
CONSTRAINT `fk_oidc_sessions_auth_code` FOREIGN KEY (`authorize_code`)
REFERENCES `authorization_codes` (`code`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@db/schema.sql` around lines 14 - 65, The oidc_sessions table lacks a foreign
key to authorization_codes causing orphaned sessions and the authorize_code
column length (varchar(255)) doesn’t match authorization_codes.code
(varchar(64)); update oidc_sessions.authorize_code to varchar(64) to match
authorization_codes.code and add a foreign key constraint (e.g., CONSTRAINT
fk_oidc_sessions_authorize_code FOREIGN KEY (authorize_code) REFERENCES
authorization_codes(code) ON DELETE CASCADE) so sessions are removed when their
authorization code is deleted, or alternatively implement and document an
explicit deletion flow that removes oidc_sessions rows whenever
authorization_codes are deleted.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こちらは対応いただきたいです

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Task: OIDCセッションの参照整合性を修正

  • oidc_sessions.authorize_codeの型をvarchar(255)からvarchar(64)に変更し、authorization_codes.codeと一致させる
  • authorization_codes.codeへの外部キー制約を追加し、ON DELETE CASCADEを設定

💡 For code changes across multiple files, use @coderabbitai plan in an issue comment

go.yaml.in/yaml/v3 v3.0.3 // indirect
golang.org/x/mod v0.31.0 // indirect
golang.org/x/net v0.48.0 // indirect
golang.org/x/oauth2 v0.14.0 // indirect
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, let's check if go.mod exists and examine it around line 96
if [ -f "go.mod" ]; then
  echo "=== go.mod line 96 context ==="
  sed -n '90,100p' go.mod
  echo ""
  echo "=== Checking for golang.org/x/oauth2 in go.mod ==="
  grep -n "oauth2" go.mod
fi

Repository: traPtitech/portal-oidc

Length of output: 656


🏁 Script executed:

#!/bin/bash
# Try to identify the dependency chain for oauth2
# Since this is a sandbox, we may not have go command available, so let's search the file
echo "=== Full go.mod content (showing require section) ==="
sed -n '/^require/,/^)/p' go.mod | head -50

Repository: traPtitech/portal-oidc

Length of output: 2356


🌐 Web query:

golang.org/x/oauth2 v0.14.0 vulnerability GO-2025-3488 GHSA-6v2p-p543-phr9

💡 Result:

golang.org/x/[email protected] is vulnerable to GO-2025-3488 / GHSA-6v2p-p543-phr9 (alias CVE-2025-22868), because the advisory affects all versions < v0.27.0. The issue is unexpected/unbounded memory consumption while parsing a malicious malformed token in golang.org/x/oauth2/jws, which can lead to denial of service (availability impact). [1][2][3]

Fix: upgrade to golang.org/x/[email protected] or later. [1][2]
Example:

go get golang.org/x/[email protected]
go mod tidy

Severity (GitHub / CISA-ADP CVSS v3.1): 7.5 HIGH (AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H). [2][3]

Sources:
[1] Go Vulnerability Database (GO-2025-3488)
[2] GitHub Advisory (GHSA-6v2p-p543-phr9)
[3] NVD entry for CVE-2025-22868


golang.org/x/oauth2 v0.14.0 に HIGH 深刻度の脆弱性が検出されています。

静的解析により以下の脆弱性が報告されています:

  • GO-2025-3488 / GHSA-6v2p-p543-phr9 (CVE-2025-22868): トークン解析時の予期しないメモリ消費により、サービス妨害の原因となる可能性があります。CVSS 7.5 (HIGH)

v0.14.0 は全て脆弱なバージョンです。v0.27.0 以降へのアップグレードが必須です。

これは github.com/ory/fosite の間接的な依存関係です。fosite の更新により解決される可能性がありますが、必要に応じて明示的に依存関係を更新してください。

🧰 Tools
🪛 OSV Scanner (2.3.3)

[HIGH] 96-96: golang.org/x/oauth2 0.14.0: Unexpected memory consumption during token parsing in golang.org/x/oauth2

(GO-2025-3488)


[HIGH] 96-96: golang.org/x/oauth2 0.14.0: golang.org/x/oauth2 Improper Validation of Syntactic Correctness of Input vulnerability

(GHSA-6v2p-p543-phr9)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@go.mod` at line 96, The go.mod currently pulls the vulnerable indirect
dependency golang.org/x/oauth2 v0.14.0; update this module to v0.27.0 or later
by adding or changing a require directive for golang.org/x/oauth2 to >= v0.27.0
(or run `go get golang.org/x/[email protected]` and `go mod tidy`) and re-run
builds/tests; if the indirect requirement comes from github.com/ory/fosite,
consider also updating fosite to a version that depends on oauth2 v0.27.0+ or
add an explicit replace/require for golang.org/x/oauth2 to force the safe
version.

Comment on lines +50 to +51
sess := NewSession(authCode.UserID, time.Time{})
sess.SetExpiresAt(fosite.AuthorizeCode, authCode.ExpiresAt)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

rg -n "AuthTime|auth_time" --type=go -C 3

Repository: traPtitech/portal-oidc

Length of output: 7941


🏁 Script executed:

# First, find the AuthCode struct definition
rg -n "type AuthCode" --type=go -A 10

# Then get the context of authcode.go around the problematic lines
fd "authcode.go" --type=go -x cat -n {}

Repository: traPtitech/portal-oidc

Length of output: 1640


🏁 Script executed:

# Read the authcode.go file
cat -n internal/repository/oauth/authcode.go | head -100

# Also check how GetAuthorizeCodeSession is used
rg -n "GetAuthorizeCodeSession" --type=go -B 2 -A 5

Repository: traPtitech/portal-oidc

Length of output: 4846


🏁 Script executed:

# Check if NewSession function and how AuthTime is used in idTokenClaims
rg -n "func NewSession" --type=go -A 15

# Check the idTokenClaims structure to understand AuthTime usage
rg -n "idTokenClaims" --type=go -B 2 -A 2

Repository: traPtitech/portal-oidc

Length of output: 3124


authTime が保持されていません。

AuthCode ドメインモデルに AuthTime フィールドがないため、認証時刻が失われています。NewSession(authCode.UserID, time.Time{}) でゼロ値が渡され、idTokenClaims.AuthTime が初期化されません。

CreateAuthorizeCodeSession では元の認証時刻にアクセス可能ですが、AuthCode に保存されていないため、GetAuthorizeCodeSession では復元できません。これは OIDC フロー (oidc.go 行 45) とは異なり、OIDC 仕様要件に違反しています。

修正するには、以下が必要です:

  • AuthCode 構造体に AuthTime フィールドを追加
  • CreateAuthorizeCodeSessionauthTime を保存
  • GetAuthorizeCodeSession で保存された authTime を使用
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/repository/oauth/authcode.go` around lines 50 - 51, AuthTime is not
persisted into the AuthCode model so sessions are reconstructed with
time.Time{}; add an AuthTime time.Time field to the AuthCode struct, set
authCode.AuthTime when creating the auth code in CreateAuthorizeCodeSession, and
in GetAuthorizeCodeSession pass that stored value to NewSession (e.g.
NewSession(authCode.UserID, authCode.AuthTime)) so idTokenClaims.AuthTime is
initialized correctly; ensure any persistence/serialization of AuthCode includes
the new field.

Comment on lines +59 to +76
func (s *Session) Clone() fosite.Session {
expiresAt := make(map[fosite.TokenType]time.Time)
maps.Copy(expiresAt, s.expiresAt)
extra := make(map[string]interface{})
maps.Copy(extra, s.extra)
idTokenClaimsClone := *s.idTokenClaims
idTokenHeadersClone := *s.idTokenHeaders
idTokenHeadersClone.Extra = make(map[string]interface{})
maps.Copy(idTokenHeadersClone.Extra, s.idTokenHeaders.Extra)
return &Session{
subject: s.subject,
username: s.username,
expiresAt: expiresAt,
extra: extra,
idTokenClaims: &idTokenClaimsClone,
idTokenHeaders: &idTokenHeadersClone,
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for IDTokenClaims struct definition in the codebase
find . -type f -name "*.go" | xargs grep -l "type IDTokenClaims struct" 2>/dev/null | head -20

Repository: traPtitech/portal-oidc

Length of output: 48


🏁 Script executed:

#!/bin/bash
# Search for jwt package imports and IDTokenClaims usage
rg "IDTokenClaims" -t go -B2 -A10 | head -100

Repository: traPtitech/portal-oidc

Length of output: 3615


🌐 Web query:

fosite jwt IDTokenClaims struct definition golang

💡 Result:

In fosite, the OIDC ID token claims type is github.com/ory/fosite/token/jwt.IDTokenClaims with this struct definition: (pkg.go.dev)

type IDTokenClaims struct {
    JTI                                 string                 `json:"jti"`
    Issuer                              string                 `json:"iss"`
    Subject                             string                 `json:"sub"`
    Audience                            []string               `json:"aud"`
    Nonce                               string                 `json:"nonce"`
    ExpiresAt                           time.Time              `json:"exp"`
    IssuedAt                            time.Time              `json:"iat"`
    RequestedAt                         time.Time              `json:"rat"`
    AuthTime                            time.Time              `json:"auth_time"`
    AccessTokenHash                     string                 `json:"at_hash"`
    AuthenticationContextClassReference string                 `json:"acr"`
    AuthenticationMethodsReferences     []string               `json:"amr"`
    CodeHash                            string                 `json:"c_hash"`
    Extra                               map[string]interface{} `json:"ext"`
}

Citations:


Clone メソッドで IDTokenClaims の深いコピーが不完全です。

idTokenClaimsClone := *s.idTokenClaims は構造体の浅いコピーを行うため、Extra マップと AudienceAuthenticationMethodsReferences スライスが元のセッションと共有されてしまいます。idTokenHeaders.Extra のように、これらの参照型フィールドも明示的に深くコピーする必要があります。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/repository/oauth/session.go` around lines 59 - 76, The Clone method
currently shallow-copies idTokenClaims via idTokenClaimsClone :=
*s.idTokenClaims which leaves reference fields shared; update Clone to deep-copy
idTokenClaims.Extra (make a new map and copy entries) and deep-copy
idTokenClaims.Audience and idTokenClaims.AuthenticationMethodsReferences (create
new slices and copy values), mirroring how idTokenHeaders.Extra is handled; also
guard against nil s.idTokenClaims/s.idTokenClaims.Extra/slices before copying so
Clone returns fully independent idTokenClaims and idTokenHeaders.

Comment on lines +144 to +150
func (s *Storage) ClientAssertionJWTValid(ctx context.Context, jti string) error {
return fosite.ErrNotFound
}

func (s *Storage) SetClientAssertionJWT(ctx context.Context, jti string, exp time.Time) error {
return nil
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for client_assertion and ClientAssertion references
rg -n "client_assertion|ClientAssertion|TokenEndpointAuthMethods|private_key_jwt|client_secret_jwt" -g '*.go' cmd internal

Repository: traPtitech/portal-oidc

Length of output: 570


🏁 Script executed:

cat -n internal/router/v1/oauth.go | sed -n '230,270p'

Repository: traPtitech/portal-oidc

Length of output: 1306


JWT ベースの認証が有効化されていない場合、このコードは現在使用されていません。

TokenEndpointAuthMethodsSupported は "client_secret_basic" と "client_secret_post" のみに設定されており、private_key_jwt や client_secret_jwt などの JWT ベースの認証方法は有効化されていません。そのため、ClientAssertionJWTValid と SetClientAssertionJWT は現在のところ呼び出されないスタブコードです。

ただし、将来 JWT ベースの認証方法をサポートする予定がある場合、これらのメソッドは JTI の再利用検知を適切に実装する必要があります。以下のいずれかを検討してください:

  • 今後実装予定がなければ、このコードを削除するか明確な TODO コメントを追加
  • JWT 認証サポートを追加する場合は、JTI の保存・期限管理・再利用検知を完全に実装
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/repository/oauth/storage.go` around lines 144 - 150, The two stub
methods ClientAssertionJWTValid and SetClientAssertionJWT currently do nothing
but return default values; either remove them or make intent explicit with a
TODO if JWT auth won't be supported, or implement full JTI
storage/expiration/replay-detection if you plan to enable
private_key_jwt/client_secret_jwt later. If keeping stubs, add clear TODO
comments referencing ClientAssertionJWTValid and SetClientAssertionJWT; if
implementing, add a persistent (or in-memory with TTL) JTI store, have
SetClientAssertionJWT save jti with exp, and make ClientAssertionJWTValid check
existence and expiration and return an appropriate error (and delete/expire
entries) to prevent replay.

Comment on lines +133 to +142
func sanitizeReturnURL(raw string) string {
if raw == "" {
return "/"
}
parsed, err := url.Parse(raw)
if err != nil || parsed.Host != "" || strings.HasPrefix(raw, "//") {
return "/"
}
return parsed.RequestURI()
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

return_url のサニタイズが不十分でオープンリダイレクトになる恐れ

javascript: などスキーム付きの URL が parsed.Host == "" をすり抜けるため、Location に危険な値が入る可能性があります。スキームの有無とパス先頭 / を厳密にチェックしてください。

🛡️ 修正例
 func sanitizeReturnURL(raw string) string {
 	if raw == "" {
 		return "/"
 	}
 	parsed, err := url.Parse(raw)
-	if err != nil || parsed.Host != "" || strings.HasPrefix(raw, "//") {
+	if err != nil ||
+		parsed.Host != "" ||
+		parsed.Scheme != "" ||
+		strings.HasPrefix(raw, "//") ||
+		!strings.HasPrefix(parsed.Path, "/") {
 		return "/"
 	}
 	return parsed.RequestURI()
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func sanitizeReturnURL(raw string) string {
if raw == "" {
return "/"
}
parsed, err := url.Parse(raw)
if err != nil || parsed.Host != "" || strings.HasPrefix(raw, "//") {
return "/"
}
return parsed.RequestURI()
}
func sanitizeReturnURL(raw string) string {
if raw == "" {
return "/"
}
parsed, err := url.Parse(raw)
if err != nil ||
parsed.Host != "" ||
parsed.Scheme != "" ||
strings.HasPrefix(raw, "//") ||
!strings.HasPrefix(parsed.Path, "/") {
return "/"
}
return parsed.RequestURI()
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/router/v1/auth.go` around lines 133 - 142, sanitizeReturnURL
currently only rejects host and leading '//' but allows scheme-prefixed inputs
like "javascript:..."; update sanitizeReturnURL to reject any non-empty
parsed.Scheme and ensure the returned path starts with '/' (e.g., require
parsed.Scheme == "" and parsed.Path != "" and
strings.HasPrefix(parsed.RequestURI(), "/")). Also keep the existing checks for
empty input and leading '//' and return "/" for any invalid case; make the
change in the sanitizeReturnURL function.

Comment on lines +36 to +43
store := sessions.NewCookieStore(config.SessionSecret)
store.Options = &sessions.Options{
Path: "/",
MaxAge: 600,
HttpOnly: true,
Secure: strings.HasPrefix(config.Issuer, "https://"),
SameSite: http.SameSiteLaxMode,
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Search for SessionSecret configuration and validation
rg -n "SessionSecret" --type=go -C 5

Repository: traPtitech/portal-oidc

Length of output: 1816


🏁 Script executed:

# Search for OAuthConfig struct definition
rg -n "type OAuthConfig" --type=go -A 10

Repository: traPtitech/portal-oidc

Length of output: 1301


🏁 Script executed:

# Search for where SessionSecret is set/initialized
rg -n "config.SessionSecret" --type=go -C 5

Repository: traPtitech/portal-oidc

Length of output: 767


🏁 Script executed:

# Check for session secret validation/length checks
rg -n "len.*SessionSecret|SessionSecret.*len" --type=go -C 3

Repository: traPtitech/portal-oidc

Length of output: 48


🏁 Script executed:

# Look for TestUserID usage
rg -n "TestUserID" --type=go -C 5

Repository: traPtitech/portal-oidc

Length of output: 3670


🏁 Script executed:

# Search for MaxAge and session timeout configurations
rg -n "MaxAge" --type=go -C 3

Repository: traPtitech/portal-oidc

Length of output: 1868


SessionSecret の最小長検証がありません。

config.SessionSecret に対する最小長検証(推奨32バイト以上)が実装されていません。cmd/serve.go[]byte(cfg.OAuth.Secret) 変換時に長さの検証を追加することを検討してください。

MaxAge: 600 はセッションクッキーの有効期限であり、OAuth フローの max_age パラメータ(別途 isMaxAgeExpired() で処理)とは独立しており、適切な設定です。

TestUserID は Environment != "production" の条件下でのみ使用されるため問題ありません。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/router/v1/handler.go` around lines 36 - 43, Add a minimum-length
check for the session secret: validate config.SessionSecret (recommend >=32
bytes) before constructing the cookie store in handler.go and
reject/startup-fail with a clear error if it's too short; implement the check
where the secret is converted to bytes in cmd/serve.go (around the
[]byte(cfg.OAuth.Secret) conversion) so the application cannot start with an
insufficient secret. Reference symbols: config.SessionSecret,
sessions.NewCookieStore, cmd/serve.go conversion site
([]byte(cfg.OAuth.Secret)), and keep existing MaxAge and isMaxAgeExpired logic
unchanged.

Comment on lines +21 to +62
func (h *Handler) Authorize(ctx echo.Context, params gen.AuthorizeParams) error {
c := ctx.Request().Context()
rw := ctx.Response()
req := ctx.Request()

ar, err := h.oauth2.NewAuthorizeRequest(c, req)
if err != nil {
h.oauth2.WriteAuthorizeError(c, rw, ar, err)
return nil
}

prompt := ar.GetRequestForm().Get("prompt")
returnURL := req.URL.String()

if h.config.Environment != "production" {
return h.completeAuthorize(ctx, ar, h.config.TestUserID, time.Now())
}

info, authenticated := h.getAuthInfo(ctx)

switch prompt {
case "none":
if !authenticated {
h.oauth2.WriteAuthorizeError(c, rw, ar, fosite.ErrLoginRequired)
return nil
}
case "login":
if !authenticated || !h.isReauthCompleted(ctx, info.AuthTime) {
return h.redirectToLogin(ctx, returnURL)
}
default:
if !authenticated {
return ctx.Redirect(http.StatusFound, "/login?return_url="+url.QueryEscape(returnURL))
}
}

if h.isMaxAgeExpired(ar, info.AuthTime) && !h.isReauthCompleted(ctx, info.AuthTime) {
return h.redirectToLogin(ctx, returnURL)
}

return h.completeAuthorize(ctx, ar, info.UserID, info.AuthTime)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

prompt=none で max_age 超過時にリダイレクトしてしまいます。
prompt=none は UI なしが前提なので、再認証が必要な場合は login_required を返すべきです。現状は max_age 超過時に /login へリダイレクトされます。

🛠️ 修正案
-	if h.isMaxAgeExpired(ar, info.AuthTime) && !h.isReauthCompleted(ctx, info.AuthTime) {
-		return h.redirectToLogin(ctx, returnURL)
-	}
+	if h.isMaxAgeExpired(ar, info.AuthTime) && !h.isReauthCompleted(ctx, info.AuthTime) {
+		if prompt == "none" {
+			h.oauth2.WriteAuthorizeError(c, rw, ar, fosite.ErrLoginRequired)
+			return nil
+		}
+		return h.redirectToLogin(ctx, returnURL)
+	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/router/v1/oauth.go` around lines 21 - 62, In Authorize, change
handling for prompt == "none" so that if the user is authenticated but
h.isMaxAgeExpired(ar, info.AuthTime) && !h.isReauthCompleted(ctx, info.AuthTime)
you call h.oauth2.WriteAuthorizeError(c, rw, ar, fosite.ErrLoginRequired) and
return nil instead of allowing the flow to reach redirectToLogin; i.e., in the
"none" case (using ar and info) add the max_age check and write the
login_required error, or alternatively check if prompt=="none" before the later
generic max_age redirect and convert that branch into a WriteAuthorizeError
using fosite.ErrLoginRequired (use the existing oauth2.WriteAuthorizeError and
fosite.ErrLoginRequired symbols).

Comment on lines +98 to +120
func verifyPBKDF2SHA512(password, iterationsStr, saltB64, hashB64 string) error {
iterations, err := strconv.Atoi(iterationsStr)
if err != nil {
return fmt.Errorf("%w: invalid iterations: %w", ErrUnsupportedHashAlgorithm, err)
}

salt, err := base64.StdEncoding.DecodeString(saltB64)
if err != nil {
return fmt.Errorf("%w: invalid salt: %w", ErrUnsupportedHashAlgorithm, err)
}

expectedHash, err := base64.StdEncoding.DecodeString(hashB64)
if err != nil {
return fmt.Errorf("%w: invalid hash: %w", ErrUnsupportedHashAlgorithm, err)
}

computedHash := pbkdf2.Key([]byte(password), salt, iterations, len(expectedHash), sha512.New)

if subtle.ConstantTimeCompare(computedHash, expectedHash) != 1 {
return ErrInvalidPassword
}
return nil
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat internal/usecase/user.go | head -150 | tail -60

Repository: traPtitech/portal-oidc

Length of output: 1543


🏁 Script executed:

rg -n "pbkdf2|import" internal/usecase/user.go -B 2 -A 2

Repository: traPtitech/portal-oidc

Length of output: 571


🌐 Web query:

pbkdf2.Key panic invalid iterations empty hash golang

💡 Result:

In Go, pbkdf2.Key (from golang.org/x/crypto/pbkdf2) does not produce errors like “invalid iterations” or “empty hash” by itself—it just computes PBKDF2 using hmac.New(h, password) with whatever you pass in. (raw.githubusercontent.com)

So when you see panics around this area, they’re almost always caused by bad inputs you parsed/constructed, especially the hash function argument:

1) “empty hash” ⇒ you passed a nil/invalid hash constructor

If you map an algorithm name (e.g., from a stored string) and end up with h == nil, pbkdf2.Key(..., h) will panic inside HMAC creation. (Also, h must return a fresh hash each call, or hmac.New can panic about non-unique values.) (ref.gotd.dev)

Fix: map strings to concrete constructors, and reject unknown/empty values:

var h func() hash.Hash
switch alg {
case "sha256":
    h = sha256.New
case "sha512":
    h = sha512.New
default:
    return fmt.Errorf("unsupported/empty hash algorithm: %q", alg)
}

2) “invalid iterations” ⇒ your own validation (or a wrapper library), not PBKDF2

PBKDF2 requires a positive iteration count; many apps/libraries validate this and return/raise “invalid iterations”. In x/crypto/pbkdf2, an iter <= 0 won’t inherently error—it just results in effectively doing only the first PRF step (which is insecure), so you should validate it yourself. (raw.githubusercontent.com)

Fix:

if iter <= 0 {
    return fmt.Errorf("invalid iterations: %d", iter)
}

If you share the exact panic text + the code line calling pbkdf2.Key, I can pinpoint which input (iter, h, salt, etc.) is becoming empty/zero.

Citations:


PBKDF2の入力検証を追加してください。

pbkdf2.Keyが直接panic することはありませんが、iterations <= 0 や len(expectedHash) == 0 のような無効な入力を検証することは重要です。iterations <= 0 はPBKDF2の安全性を損ない、0長のハッシュ値はエッジケースとして適切に処理すべきです。

修正案
 iterations, err := strconv.Atoi(iterationsStr)
-if err != nil {
+if err != nil || iterations <= 0 {
 	return fmt.Errorf("%w: invalid iterations: %w", ErrUnsupportedHashAlgorithm, err)
 }
@@
 expectedHash, err := base64.StdEncoding.DecodeString(hashB64)
 if err != nil {
 	return fmt.Errorf("%w: invalid hash: %w", ErrUnsupportedHashAlgorithm, err)
 }
+if len(expectedHash) == 0 {
+	return fmt.Errorf("%w: invalid hash length", ErrUnsupportedHashAlgorithm)
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func verifyPBKDF2SHA512(password, iterationsStr, saltB64, hashB64 string) error {
iterations, err := strconv.Atoi(iterationsStr)
if err != nil {
return fmt.Errorf("%w: invalid iterations: %w", ErrUnsupportedHashAlgorithm, err)
}
salt, err := base64.StdEncoding.DecodeString(saltB64)
if err != nil {
return fmt.Errorf("%w: invalid salt: %w", ErrUnsupportedHashAlgorithm, err)
}
expectedHash, err := base64.StdEncoding.DecodeString(hashB64)
if err != nil {
return fmt.Errorf("%w: invalid hash: %w", ErrUnsupportedHashAlgorithm, err)
}
computedHash := pbkdf2.Key([]byte(password), salt, iterations, len(expectedHash), sha512.New)
if subtle.ConstantTimeCompare(computedHash, expectedHash) != 1 {
return ErrInvalidPassword
}
return nil
}
func verifyPBKDF2SHA512(password, iterationsStr, saltB64, hashB64 string) error {
iterations, err := strconv.Atoi(iterationsStr)
if err != nil || iterations <= 0 {
return fmt.Errorf("%w: invalid iterations: %w", ErrUnsupportedHashAlgorithm, err)
}
salt, err := base64.StdEncoding.DecodeString(saltB64)
if err != nil {
return fmt.Errorf("%w: invalid salt: %w", ErrUnsupportedHashAlgorithm, err)
}
expectedHash, err := base64.StdEncoding.DecodeString(hashB64)
if err != nil {
return fmt.Errorf("%w: invalid hash: %w", ErrUnsupportedHashAlgorithm, err)
}
if len(expectedHash) == 0 {
return fmt.Errorf("%w: invalid hash length", ErrUnsupportedHashAlgorithm)
}
computedHash := pbkdf2.Key([]byte(password), salt, iterations, len(expectedHash), sha512.New)
if subtle.ConstantTimeCompare(computedHash, expectedHash) != 1 {
return ErrInvalidPassword
}
return nil
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/usecase/user.go` around lines 98 - 120, In verifyPBKDF2SHA512, add
input validation before calling pbkdf2.Key: ensure iterations (from
iterationsStr) is > 0, ensure decoded salt (salt) length > 0, and ensure decoded
expectedHash length > 0; if any check fails return a wrapped
ErrUnsupportedHashAlgorithm (consistent with existing error style) with a clear
cause (e.g., "invalid iterations", "empty salt", "empty hash") so pbkdf2.Key is
only invoked with valid parameters and edge cases are handled explicitly.

- Client Secret: 手順 3 で作成した `client_secret`
- Discovery URL: `http://host.docker.internal:8080/.well-known/openid-configuration`

## 7. 実行と結果確認
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

セクション番号が 5 から 7 に飛んでいます。

セクション 6 が欠落しています。「## 7. 実行と結果確認」を「## 6. 実行と結果確認」に修正してください。

📝 修正案
-## 7. 実行と結果確認
+## 6. 実行と結果確認
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## 7. 実行と結果確認
## 6. 実行と結果確認
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@TEST_MANUAL.md` at line 60, The markdown header "## 7. 実行と結果確認" is
incorrectly numbered and should be "## 6. 実行と結果確認"; update the header text in
TEST_MANUAL.md by replacing the string "## 7. 実行と結果確認" with "## 6. 実行と結果確認" so
the section numbering is consecutive.

@Pugma Pugma self-requested a review February 19, 2026 23:58
Copy link
Collaborator

@Pugma Pugma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

慌てて見たのでまだ途中ですが、次見れるまで時間が開くかもなので今のうちにここで送っておきます…!

Comment on lines +81 to +83
e.POST("/oauth2/authorize", func(c echo.Context) error {
return handler.Authorize(c, gen.AuthorizeParams{})
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

直接 HTML を返す login, logout 関連とは違い、こちらは通常通り openapi.yaml に追加していただいたほうが実装がラクかと思います
すでに get req については openapi.yaml に載っているので、 post もそこに追加するだけですね

Comment on lines +14 to +65
CREATE TABLE IF NOT EXISTS `authorization_codes` (
`code` varchar(64) NOT NULL,
`client_id` char(36) NOT NULL,
`user_id` char(36) NOT NULL,
`redirect_uri` text NOT NULL,
`scopes` text NOT NULL,
`code_challenge` varchar(128) NULL,
`code_challenge_method` varchar(10) NULL,
`nonce` varchar(255) NULL,
`used` BOOLEAN NOT NULL DEFAULT FALSE,
`expires_at` datetime(6) NOT NULL,
`created_at` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
PRIMARY KEY (`code`),
INDEX `idx_authorization_codes_client_id` (`client_id`),
INDEX `idx_authorization_codes_expires_at` (`expires_at`),
CONSTRAINT `fk_authorization_codes_client` FOREIGN KEY (`client_id`) REFERENCES `clients` (`client_id`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;

CREATE TABLE IF NOT EXISTS `tokens` (
`id` char(36) NOT NULL,
`request_id` varchar(64) NOT NULL,
`client_id` char(36) NOT NULL,
`user_id` char(36) NOT NULL,
`access_token` varchar(64) NOT NULL,
`refresh_token` varchar(64) NULL,
`scopes` text NOT NULL,
`expires_at` datetime(6) NOT NULL,
`created_at` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
PRIMARY KEY (`id`),
UNIQUE INDEX `idx_tokens_access_token` (`access_token`),
UNIQUE INDEX `idx_tokens_refresh_token` (`refresh_token`),
INDEX `idx_tokens_client_id` (`client_id`),
INDEX `idx_tokens_user_id` (`user_id`),
INDEX `idx_tokens_request_id` (`request_id`),
INDEX `idx_tokens_expires_at` (`expires_at`),
CONSTRAINT `fk_tokens_client` FOREIGN KEY (`client_id`) REFERENCES `clients` (`client_id`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;

CREATE TABLE IF NOT EXISTS `oidc_sessions` (
`authorize_code` varchar(255) NOT NULL,
`client_id` char(36) NOT NULL,
`user_id` char(36) NOT NULL,
`scopes` text NOT NULL,
`nonce` varchar(255) NULL,
`auth_time` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
`requested_at` datetime(6) NOT NULL,
`created_at` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
PRIMARY KEY (`authorize_code`),
INDEX `idx_oidc_sessions_client_id` (`client_id`),
CONSTRAINT `fk_oidc_sessions_client` FOREIGN KEY (`client_id`)
REFERENCES `clients` (`client_id`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こちらは対応いただきたいです

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.

4 participants