[] Add LICENSE, Config branding fields, MetaHandler (/api/meta + /api/licenses) with unit tests#265
Conversation
![orange-codens[bot]](https://avatars.githubusercontent.com/in/4028936?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍊 Orange Codens レビュー
全体的に実装は仕様に沿っており、大きな問題はない。ただし TestLoadBrandingDefaults で SOURCE_URL を空文字セット→空文字期待しているアサーションが常に成功する無意味なテスト (confidence: 0.92) と、TestGetMetaOK でキーの存在確認のみで値を検証していない (confidence: 0.88) の2点は修正推奨。NewMetaHandler 内の nil チェックは Go の慣用句に反するため修正が望ましい。その他は設計上の soft な指摘。
🔒 セキュリティ: 今回の変更で追加された /api/meta および /api/licenses エンドポイントは認証ガードなしに公開されており、git_commit などのビルドメタデータが認証なしで取得可能になる懸念があります。また LICENSES_FILE_PATH を os.ReadFile へ無検証で渡す実装はパストラバーサルの潜在的な攻撃面を形成しますが、オペレーター設定値であるため実際のリスクは限定的です。致命的な認証バイパスや SQL インジェクションなどの高深刻度の問題は検出されませんでした。
🟡 medium: 3 / 🔵 low: 5
その他の指摘 (8 件)
- 🟡 [medium]
backend/internal/config/config_test.go:255—SourceURLのデフォルト値テストが常に成功する無意味なアサーション (confidence: 0.92,code.logic.test_source_url_trivial) - 🟡 [medium]
backend/internal/config/config_test.go:244— 環境変数を空文字にセットするとデフォルト値が返らない可能性がある (confidence: 0.72,code.logic.env_default_override) - 🟡 [medium]
backend/internal/handler/meta_handler.go:61— /api/meta および /api/licenses エンドポイントに認証・認可ガードがない (confidence: 0.70,sec.sast.authz.unauthenticated_info_disclosure) - 🔵 [low]
backend/internal/handler/meta_handler.go:31—thirdParty != nilチェックは不要（appendは nil スライスを受け付ける） (confidence: 0.95,code.style.unnecessary_nil_check) - 🔵 [low]
backend/internal/handler/meta_handler_test.go:43—TestGetMetaOKでキーの存在確認のみでレスポンス値を検証していない (confidence: 0.88,maintainability.test.missing_value_assertion) - 🔵 [low]
backend/internal/handler/meta_handler.go:52—licensesResponse型が unexported なため外部テストから直接参照できない (confidence: 0.65,maintainability.naming.response_type_unexported) - 🔵 [low]
backend/internal/handler/meta_handler.go:63—RegisterRoutesが*echo.Echoを直接受け取りグループ対応していない (confidence: 0.62,maintainability.design.route_registration_in_constructor) - 🔵 [low]
backend/cmd/server/main.go:681— LICENSES_FILE_PATH 環境変数による任意ファイル読み取りの可能性 (confidence: 0.55,sec.sast.path_traversal.config_file_path)
head: 89d039d | Orange Codens (P1)
There was a problem hiding this comment.
🍊 Orange Codens レビュー
全体的に実装は仕様通りで大きな問題はありません。ただし GetMeta の "license" キーと GetLicenses の "app_license" キーが同一概念を指しているにもかかわらず名前が異なる点 (medium) は API の一貫性を損なうため修正を推奨します。また config_test.go のデフォルト値テストは環境変数を空文字列にセットして検証しており、getenv の実装によっては「デフォルト値が返る」ことを証明できていない可能性があります。その他は命名・テスト品質に関する軽微な指摘です。
🔒 セキュリティ: 今回の変更はメタ情報・ライセンス情報を提供する読み取り専用エンドポイントの追加であり、重大な脆弱性は見当たりません。主な懸念点は (1) LICENSES_FILE_PATH 環境変数由来のパスをサニタイズせずに os.ReadFile に渡している点（パストラバーサルリスク・medium）、(2) エラーログにファイルパスを完全形で出力している点（情報漏洩・low）、(3) /api/meta / /api/licenses が認証なしのルートに登録されている点（設計確認推奨・info）です。いずれも直ちに悪用可能なケースは限定的ですが、パス検証の追加とログの最小化を推奨します。
🟡 medium: 3 / 🔵 low: 4 / ⚪ info: 1
その他の指摘 (8 件)
- 🟡 [medium]
backend/internal/handler/meta_handler.go:43—GetMetaのレスポンスキー名"license"がフィールド名LicenseName/GetLicensesの"app_license"と不統一 (confidence: 0.90,code.logic.map_key_consistency) - 🟡 [medium]
backend/internal/config/config_test.go:244— 空文字列セットでデフォルト値を検証しているがgetenvの実装次第では意図通りに動かない (confidence: 0.75,code.logic.test_default_value) - 🟡 [medium]
backend/cmd/server/main.go:682— LicensesFilePath が環境変数由来でパストラバーサルの可能性 (confidence: 0.60,sec.sast.injection.path_traversal) - 🔵 [low]
backend/internal/handler/meta_handler.go:31—NewMetaHandlerでmake([]LicenseEntry, 0)してから条件付きappendするのは冗長 (confidence: 0.85,maintainability.dead_alloc.unnecessary_make) - 🔵 [low]
backend/internal/handler/meta_handler_test.go:42—TestGetMetaOKでキーの存在確認のみで値を検証していない (confidence: 0.85,maintainability.test.key_presence_only) - 🔵 [low]
backend/cmd/server/main.go:685— 警告ログにファイルパスをそのまま出力している (confidence: 0.75,sec.sast.info_disclosure.verbose_path) - 🔵 [low]
backend/internal/config/config.go:93—LicensesFilePathのデフォルト値"./licenses.json"はサーバー起動ディレクトリ依存で脆弱 (confidence: 0.70,maintainability.naming.licenses_file_path_default) - ⚪ [info]
backend/internal/handler/meta_handler.go:62— /api/meta および /api/licenses が認証なしで公開されている (confidence: 0.65,sec.sast.authz.unauthenticated_endpoint)
head: 787c12c | Orange Codens (P1)
…/api/licenses) with unit tests
There was a problem hiding this comment.
🍊 Orange Codens レビュー
PR の主目的（LICENSE 追加・Config ブランディングフィールド・MetaHandler 実装）は概ね適切に実装されている。重大な問題として workflow_job_exec_handler.go の fmt.Errorf で %v: %w を使った二重エラーラップが3箇所あり、ログが冗長になる。また trigger_workflow_usecase.go で CreateBatch をループ Create に変更した結果、マルチジョブのワークフローで途中失敗時に部分挿入が残るトランザクション安全性の問題がある。SSH 認証変更で DB エラーが無音で飲み込まれる点も要確認。config_test.go の SourceURL テストは空文字列セット→空文字列期待で検証が無意味になっているため t.Unsetenv への修正を推奨する。
🔒 セキュリティ: 今回の PR はメタ情報・ライセンス公開エンドポイント (/api/meta, /api/licenses) の追加が主な変更です。最も注意すべき点は /api/meta が認証なしで git_commit などビルド詳細を公開することと、LICENSES_FILE_PATH を無検証でファイル読み込みに使用するパス・トラバーサルリスクです。また SSH 認証リファクタリングでストアエラーが無音で握りつぶされる点も可用性・監査性の観点でリスクがあります。SQL インジェクション・SSRF など重大な脆弱性は diff の範囲では確認されませんでした。
🟠 high: 1 / 🟡 medium: 8 / 🔵 low: 3 / ⚪ info: 1
その他の指摘 (12 件)
- 🟡 [medium]
backend/internal/infrastructure/queue/workflow_job_exec_handler.go:97—fmt.Errorfでエラーを二重にラップしている (confidence: 0.90,code.logic.workflow_job_exec_error_format) - 🟡 [medium]
backend/internal/infrastructure/queue/workflow_job_exec_handler.go:101—parse run idでも%v: %wの二重ラップ (confidence: 0.90,code.logic.workflow_job_exec_error_format) - 🟡 [medium]
backend/internal/infrastructure/queue/workflow_job_exec_handler.go:105—parse job idでも%v: %wの二重ラップ (confidence: 0.90,code.logic.workflow_job_exec_error_format) - 🟡 [medium]
backend/internal/infrastructure/ssh/ssh_server.go:109— SSH 公開鍵認証でエラー詳細が握りつぶされ認証失敗の理由が不透明に (confidence: 0.75,sec.sast.authz.ssh_key_auth_bypass) - 🟡 [medium]
backend/internal/usecase/workflow/trigger_workflow_usecase.go:345—CreateBatchをループCreateに変更したことで途中失敗時に部分挿入が残る (confidence: 0.72,code.logic.workflow_job_batch_to_serial) - 🟡 [medium]
backend/internal/config/config_test.go:244—getenvのデフォルト値は空文字列セット時に返らない可能性がある (confidence: 0.65,code.logic.test_default_override) - 🟡 [medium]
backend/cmd/server/main.go:682— 環境変数 LICENSES_FILE_PATH を検証せずにファイル読み込みに使用 (confidence: 0.60,sec.sast.path_traversal.licenses_file) - 🟡 [medium]
backend/internal/usecase/workflow/get_run_usecase.go:34—CancelRunUsecaseが削除されたが呼び出し側の対応が確認できない (confidence: 0.55,maintainability.dead_code.cancel_run_usecase_removed) - 🔵 [low]
backend/internal/config/config_test.go:258—SourceURLのデフォルト値テストが意味を持たない (confidence: 0.85,maintainability.test.config_default_source_url) - 🔵 [low]
backend/internal/handler/meta_handler.go:51—licensesResponseが unexported だがテストから構造体リテラルで直接使用されている (confidence: 0.70,maintainability.naming.meta_handler_response_struct_unexported) - 🔵 [low]
backend/internal/handler/meta_handler.go:40— /api/meta エンドポイントがビルド詳細情報を認証なしで公開 (confidence: 0.65,sec.sast.info_disclosure.build_meta) - ⚪ [info]
backend/internal/handler/meta_handler.go:63—RegisterRoutesが*echo.Echoを受け取り Group に対応していない (confidence: 0.60,maintainability.design.meta_handler_route_registration)
head: f6c50bd | Orange Codens (P1)
| return true | ||
| } | ||
|
|
||
| func (h *SSHServer) handleSession(s gossh.Session) { |
There was a problem hiding this comment.
🟠 [high] authenticateKey のシグネチャ変更で gossh.Permissions が渡せなくなった
gliderlabs/ssh の PublicKeyHandler は func(ctx Context, key PublicKey) bool を受け取る。変更後のシグネチャは正しいが、以前の実装では *gossh.Permissions を返すシグネチャ（PublicKeyHandlerWithPermissions）を使っていた可能性がある。
問題の核心は、以前は認証エラー時に gossh.ErrKeyRejected を返して具体的なエラーを通知していたが、変更後は err || stored == nil を単一の false に集約しているため、ルックアップエラー（DB 障害等）と鍵不存在が区別できなくなったこと。DB 障害時も黙って false を返し認証失敗扱いになる。
該当箇所:
func (h *SSHServer) authenticateKey(ctx gossh.Context, key gossh.PublicKey) bool {
fingerprint := xssh.FingerprintSHA256(key)
stored, err := h.keyStore.FindByFingerprint(ctx, fingerprint)
if err != nil || stored == nil {
return false
}
return true
}DB エラーをログに出力するか、エラー種別を区別して適切に処理するべき。
@@ -114,5 +114,8 @@
fingerprint := xssh.FingerprintSHA256(key)
stored, err := h.keyStore.FindByFingerprint(ctx, fingerprint)
- if err != nil || stored == nil {
- return false
+ if err != nil {
+ logger.Global().Error().Err(err).Msg("ssh key lookup failed")
+ return false
+ }
+ if stored == nil {
return false
}code.logic.ssh_auth_info_loss | confidence: 0.80
# Conflicts: # LICENSE # backend/internal/config/config.go # backend/internal/domain/repository/i_workflow_job_repository.go # backend/internal/infrastructure/git/git_service.go # backend/internal/infrastructure/git/init_service.go # backend/internal/infrastructure/queue/workflow_job_exec_handler.go # backend/internal/infrastructure/queue/workflow_job_exec_handler_test.go # backend/internal/infrastructure/queue/workflow_schedule_handler.go # backend/internal/infrastructure/ssh/ssh_server.go # backend/internal/infrastructure/workflow/parser.go # backend/internal/usecase/workflow/trigger_workflow_usecase.go # backend/internal/usecase/workflow/trigger_workflow_usecase_test.go # go.work.sum
There was a problem hiding this comment.
🍊 Orange Codens レビュー
Purple 計画どおり Config 拡張・MetaHandler・main 配線・基本ユニットテストは diff 上整合しており、致命的なロジックバグは見当たりません。NewMetaHandler の nil スライス正規化や空配列 JSON 保証など設計は妥当です。改善余地は主にテストで、loadLicensesFromFile のエラーパス未カバーと、既存 handler テストの値アサーション不足が中心です。
🔒 セキュリティ: 本 PR は公開向けメタデータ API（/api/meta, /api/licenses）と設定駆動のライセンスファイル読み込みを追加する変更です。SQL injection・SSRF・認可 bypass・暗号誤用など exploit 可能な論理脆弱性は diff 上確認できませんでした。/api/meta による git_commit 等の公開は reconnaissance 観点で low 程度の情報露出として留意してください。
🟡 medium: 1 / 🔵 low: 3
その他の指摘 (4 件)
- 🟡 [medium]
backend/cmd/server/main.go:938— loadLicensesFromFile のエラーパスがテストされていない (confidence: 0.78,maintainability.test.missing_coverage) - 🔵 [low]
backend/internal/handler/meta_handler_test.go:39— TestGetMetaOK がキー存在のみ確認で値を検証していない (confidence: 0.82,maintainability.test.weak_assertion) - 🔵 [low]
backend/internal/handler/meta_handler_test.go:80— TestGetLicensesWithEntries が third_party の一部フィールドのみ検証 (confidence: 0.80,maintainability.test.partial_field_assertion) - 🔵 [low]
backend/internal/handler/meta_handler.go:63— 未認証の /api/meta が git_commit 等のビルド情報を公開 (confidence: 0.55,sec.sast.info_disclosure.build_metadata)
head: 2402858 | Orange Codens (P1)
1 participant
Summary
Type of Change
Testing
Checklist
Task Goal
Target Repository
Corevice/open-gitAll work for this task belongs in this repository. Do the implementation inside this repository's directory in the workspace; do not search or edit other repositories unless this task explicitly requires a cross-repo change.
Context
Create the root LICENSE file, extend the Config struct with AppName/LicenseName/SourceURL/LicensesFilePath, implement MetaHandler serving /api/meta and /api/licenses, wire it into main.go, and cover it with unit tests — all in one sub-task so tests and implementation land together.
Steps
open-git/LICENSE— paste Apache-2.0 SPDX text (Copyright 2025 OpenGit Contributorsheader + full Apache-2.0 boilerplate)open-git/backend/internal/config/config.go— in theConfigstruct add:AppName string,LicenseName string,SourceURL string,LicensesFilePath string; inLoad()add:AppName: getenv("APP_NAME", "OpenGit"),LicenseName: getenv("LICENSE_NAME", "Apache-2.0"),SourceURL: getenv("SOURCE_URL", ""),LicensesFilePath: getenv("LICENSES_FILE_PATH", "./licenses.json"); add corresponding assertions inconfig_test.goto verify the defaults load correctlyopen-git/backend/licenses.jsonwith content[]open-git/backend/internal/handler/meta_handler.go— defineBuildInfostruct with fieldsAppName,Version,GitCommit,BuildDate,LicenseName,SourceURL(all string); defineLicenseEntrystruct with json-tagged fieldsName,Version,License,URL(all string); defineMetaHandlerstruct; implementNewMetaHandler(info BuildInfo, thirdParty []LicenseEntry) *MetaHandler(store copy of thirdParty, default to empty slice if nil); implementGetMeta(c echo.Context) errorreturningc.JSON(http.StatusOK, map[string]string{"app_name": h.info.AppName, "version": h.info.Version, "git_commit": h.info.GitCommit, "build_date": h.info.BuildDate, "license": h.info.LicenseName, "source_url": h.info.SourceURL}); implement `GeChanges Made
Implementing license
/branding backend
changes
.
Reading the
files we're
allowed
to modify
.
Checking
an existing handler test
for
patterns and how zer
olog is used in
main
.
Implementing the required
files
.
Implemented
license and branding backend
support for Open
Git.
Summary
LICENSE— Apache-2.0 full
text with
Copyright 2025 OpenGit Contributors.config.go— AddedApp Name,License Name,Source URL, andLicensesFilePathwith envdefaults (
Open Git,Apache -2.0,"",./ licenses.json).
config_test.go— Added
Test LoadBr andingDefaultstoverify
default
loading
.
licenses.json— Emptythird
-party license
list
(
[]).5
.
meta _handler.go—Build Info,License Entry,Meta Handlerwith
GetMeta,GetLic enses, andRegisterRoutesforGET /api /metaandGET /api/licenses.main.go—Added
loadLicenses FromFile(zerolog warning on
read
/parse errors
, returns empty slice
),
wired
MetaHandlerinregisterHandlers.meta_handler_test.go— Four tests: meta OK
, licenses
with entries, empty
licenses
(
[]not null),dev
version echo
.
[PURP
LE-MODIFIED
-FILES]
LICENSE
backend
/internal
/config/config.go
backend/internal/config/config
_test.go
backend
/licenses.json
backend
/internal/handler/meta
_handler.go
backend
/internal/handler/meta
_handler_test.go
backend/cmd/server/main
.go
[/P
URPLE-M
ODIFIED-FILES
]
Task ID:
019f0d7d-8ab8-794a-ae77-3530cd05c72dGenerated by Purple Codens