Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSpring Boot + SQLite で動くローカル向けブックマークアプリの初回実装。URL からタイトル/OGP をサーバー側で取得し SQLite に保存、一覧表示する Web UI とビルド設定を含みます。 Changesブックマーク管理アプリケーション
sequenceDiagram
participant Browser
participant Controller as BookmarkController
participant Fetcher as BookmarkMetadataFetcher
participant Repo as BookmarkRepository
participant DB as SQLite
Browser->>Controller: GET /
Controller->>Repo: findAll()
Repo->>DB: SELECT bookmarks
DB-->>Repo: rows
Repo-->>Controller: bookmarks
Controller-->>Browser: render index.html
Browser->>Controller: POST /bookmarks?url=...
Controller->>Fetcher: fetch(url)
Fetcher->>Fetcher: normalizeUrl & rejectDisallowedHost
Fetcher->>Fetcher: Jsoup connect & parse
Fetcher-->>Controller: BookmarkMetadata
Controller->>Repo: add(title,url,description,tags,ogp_image_url)
Repo->>DB: INSERT bookmark
DB-->>Repo: OK
Repo-->>Controller: OK
Controller-->>Browser: redirect /
推定レビュー負荷 🎯 4 (Complex) | ⏱️ ~45 分
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gradlew.bat (1)
1-16:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winWindows バッチファイルに Unix 行末記号（LF）が使用されています。
Windows バッチファイルは CRLF 行末記号を必要としますが、このファイルは LF のみを使用しています。これにより、GOTO/CALL ラベルの解析が失敗し、Windows でスクリプトが正常に動作しない可能性があります。
以下のコマンドで Windows 行末記号に変換してください：
#!/bin/bash # Unix から Windows 行末記号への変換 unix2dos gradlew.bat # または Git の設定で自動変換を有効化 git config core.autocrlf true静的解析ツールの指摘による検出です。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gradlew.bat` around lines 1 - 16, The gradlew.bat file currently uses Unix LF line endings which breaks Windows batch parsing; convert gradlew.bat to use CRLF line endings (e.g., run a line-ending conversion like unix2dos on gradlew.bat or enable Git automatic conversion via core.autocrlf) and commit the change so the batch file (containing DEFAULT_JVM_OPTS, JAVA_EXE and the GradleWrapperMain invocation) is stored with CRLF endings; verify the script runs on Windows after conversion.
🧹 Nitpick comments (2)
src/main/resources/application.properties (1)
4-4: ⚡ Quick win
spring.sql.init.mode=alwaysは条件付き初期化を前提に見直し推奨
schema.sqlはCREATE TABLE IF NOT EXISTSを使っているため、再起動時にテーブル作成でエラーになる可能性は低いです。一方でspring.sql.init.mode=alwaysは毎回schema.sqlを実行するため、起動の無駄が増えます。運用（特に本番）を見据えるならnever/embeddedや必要時のみ実行する設定への変更を検討してください。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/application.properties` at line 4, 現在の spring.sql.init.mode=always は起動ごとに schema.sql を毎回実行してしまい無駄や本番でのリスクがあるため、プロパティ spring.sql.init.mode の値を環境に応じて見直してください（例: 本番では never、埋め込みDBのみ初期化するなら embedded など）。schema.sql は CREATE TABLE IF NOT EXISTS を使っているため再作成エラーは起きにくいが、必要時のみ実行する運用方針に合わせて spring.sql.init.mode を変更するか、環境ごとに別プロファイルで上書きする（参照: spring.sql.init.mode, schema.sql）。src/main/resources/static/styles.css (1)
173-181: ⚡ Quick winセレクタの重複を解消してください。
.bookmark-urlセレクタが 2 回定義されています（Line 173 と Line 177）。overflow-wrap: anywhere;を含むすべてのプロパティを 1 つのルールセットにまとめることで、コードの保守性が向上します。♻️ 提案される修正
-.bookmark-url { - overflow-wrap: anywhere; -} - .bookmark-url { + overflow-wrap: anywhere; margin: 4px 0 10px; color: var(--muted); font-size: 14px; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/static/styles.css` around lines 173 - 181, Merge the duplicated .bookmark-url rules into a single rule containing all properties (overflow-wrap, margin, color, font-size) so the selector is defined only once; update the .bookmark-url rule (remove the second/first duplicate) to include overflow-wrap: anywhere; along with margin: 4px 0 10px, color: var(--muted), and font-size: 14px in the same declaration block to eliminate redundancy and preserve styling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.gradle`:
- Line 4: The sqlite-jdbc dependency declaration implementation
'org.xerial:sqlite-jdbc:3.50.3.0' is not available on Maven Central and must be
changed: update the dependency in build.gradle to a version that actually exists
on Maven Central (e.g., replace 'org.xerial:sqlite-jdbc:3.50.3.0' with a valid
release such as 'org.xerial:sqlite-jdbc:3.50.4.0' or another verified Maven
Central version), then run the Gradle dependency resolution (./gradlew
dependencies) to confirm it resolves; leave Spring Boot at '3.5.14'
(firstPatchedVersion = 3.5.14) and do not change the jsoup declaration as it is
outside the reported vulnerable range.
In `@src/main/java/com/example/bookmark/BookmarkController.java`:
- Around line 37-44: BookmarkController currently calls
bookmarkRepository.add(...) without handling persistence errors, causing a 500
on save failure; wrap the call to bookmarkRepository.add(...) in a try-catch
inside the POST handler (the method in class BookmarkController) and on
exception catch (Exception e) add a flash error (e.g., using
RedirectAttributes.addFlashAttribute("error", "...")) with a user-friendly
message (optionally include e.getMessage() for logs only), then return
"redirect:/" so the flow falls back to the flash error page instead of returning
500.
In `@src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java`:
- Around line 34-42: normalizeUrl currently only ensures a scheme and non-empty
host but allows localhost/private IPs, enabling SSRF; update normalizeUrl to (1)
enforce URI.getScheme() is "http" or "https", (2) resolve the host via
InetAddress.getAllByName(uri.getHost()) and reject any address where
isLoopbackAddress(), isAnyLocalAddress(), isSiteLocalAddress() or matches known
private/reserved ranges (including IPv6 equivalents), and (3) throw
IllegalArgumentException with a clear message when the host is disallowed; keep
using requireText and StringUtils.hasText but add these network-level checks
around the URI creation/validation to block internal endpoints before returning
uri.toString().
---
Outside diff comments:
In `@gradlew.bat`:
- Around line 1-16: The gradlew.bat file currently uses Unix LF line endings
which breaks Windows batch parsing; convert gradlew.bat to use CRLF line endings
(e.g., run a line-ending conversion like unix2dos on gradlew.bat or enable Git
automatic conversion via core.autocrlf) and commit the change so the batch file
(containing DEFAULT_JVM_OPTS, JAVA_EXE and the GradleWrapperMain invocation) is
stored with CRLF endings; verify the script runs on Windows after conversion.
---
Nitpick comments:
In `@src/main/resources/application.properties`:
- Line 4: 現在の spring.sql.init.mode=always は起動ごとに schema.sql
を毎回実行してしまい無駄や本番でのリスクがあるため、プロパティ spring.sql.init.mode の値を環境に応じて見直してください（例: 本番では
never、埋め込みDBのみ初期化するなら embedded など）。schema.sql は CREATE TABLE IF NOT EXISTS
を使っているため再作成エラーは起きにくいが、必要時のみ実行する運用方針に合わせて spring.sql.init.mode
を変更するか、環境ごとに別プロファイルで上書きする（参照: spring.sql.init.mode, schema.sql）。
In `@src/main/resources/static/styles.css`:
- Around line 173-181: Merge the duplicated .bookmark-url rules into a single
rule containing all properties (overflow-wrap, margin, color, font-size) so the
selector is defined only once; update the .bookmark-url rule (remove the
second/first duplicate) to include overflow-wrap: anywhere; along with margin:
4px 0 10px, color: var(--muted), and font-size: 14px in the same declaration
block to eliminate redundancy and preserve styling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58010437-f430-4d1b-ac7b-094c484d1d98
⛔ Files ignored due to path filters (1)
gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (17)
.gitignoreREADME.mdbuild.gradlegradle/wrapper/gradle-wrapper.propertiesgradlewgradlew.batsettings.gradlesrc/main/java/com/example/bookmark/Bookmark.javasrc/main/java/com/example/bookmark/BookmarkApplication.javasrc/main/java/com/example/bookmark/BookmarkController.javasrc/main/java/com/example/bookmark/BookmarkMetadata.javasrc/main/java/com/example/bookmark/BookmarkMetadataFetcher.javasrc/main/java/com/example/bookmark/BookmarkRepository.javasrc/main/resources/application.propertiessrc/main/resources/schema.sqlsrc/main/resources/static/styles.csssrc/main/resources/templates/index.html
| plugins { | ||
| id 'java' | ||
| id 'application' | ||
| id 'org.springframework.boot' version '3.5.14' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Spring Boot 3.5.14 の検証
echo "=== Spring Boot ==="
curl -s https://repo1.maven.org/maven2/org/springframework/boot/spring-boot/maven-metadata.xml | grep -E '<version>3\.5\.' | tail -5
# sqlite-jdbc のセキュリティアドバイザリ確認
echo -e "\n=== SQLite JDBC Security Advisories ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "org.xerial:sqlite-jdbc") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# jsoup のセキュリティアドバイザリ確認
echo -e "\n=== Jsoup Security Advisories ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "org.jsoup:jsoup") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: goofmint/bookmark-app-java
Length of output: 1390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== build.gradle（該当箇所）==="
ls -la
if [ -f build.gradle ]; then
sed -n '1,120p' build.gradle | nl -ba -w2 -s': '
else
echo "build.gradle not found in repo root"
fi
echo
echo "=== build.gradle から sqlite-jdbc / jsoup のバージョン抽出 ==="
if [ -f build.gradle ]; then
rg -n "org\.xerial:sqlite-jdbc|org\.jsoup:jsoup|sqlite-jdbc|jsoup" build.gradle || true
fi
echo
echo "=== GitHub Security Advisories（sqlite-jdbc: 取得件数増）==="
gh api graphql -f query='
{
securityVulnerabilities(first: 20, ecosystem: MAVEN, package: "org.xerial:sqlite-jdbc") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}' || true
echo
echo "=== GitHub Security Advisories（jsoup: 取得件数増）==="
gh api graphql -f query='
{
securityVulnerabilities(first: 20, ecosystem: MAVEN, package: "org.jsoup:jsoup") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}' || trueRepository: goofmint/bookmark-app-java
Length of output: 984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== build.gradle（先頭〜120行、行番号付き）==="
awk '{print NR ":" $0}' build.gradle | sed -n '1,120p'
echo
echo "=== build.gradle から該当依存の行を抽出（sqlite-jdbc / jsoup）==="
rg -n "sqlite-jdbc|org\.xerial:sqlite-jdbc|jsoup|org\.jsoup:jsoup" build.gradle || true
echo
echo "=== build.gradle から Spring Boot の指定（該当箇所）==="
rg -n "org\.springframework\.boot|spring-boot.*version" build.gradle || trueRepository: goofmint/bookmark-app-java
Length of output: 1558
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== GitHub Security Advisories（Spring Boot: org.springframework.boot:spring-boot）==="
gh api graphql -f query='
{
securityVulnerabilities(first: 20, ecosystem: MAVEN, package: "org.springframework.boot:spring-boot") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}' || true
echo
echo "=== GitHub Security Advisories（Spring Boot: org.springframework.boot:spring-boot-autoconfigure）==="
gh api graphql -f query='
{
securityVulnerabilities(first: 20, ecosystem: MAVEN, package: "org.springframework.boot:spring-boot-autoconfigure") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}' || trueRepository: goofmint/bookmark-app-java
Length of output: 4502
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Maven Central: sqlite-jdbc maven-metadata.xml に 3.50.3.0 が存在するか ==="
curl -s https://repo1.maven.org/maven2/org/xerial/sqlite-jdbc/sqlite-jdbc/maven-metadata.xml \
| rg -n "3\.50\.3\.0" || echo "not found"
echo
echo "=== Maven Central: jsoup maven-metadata.xml に 1.22.2 が存在するか ==="
curl -s https://repo1.maven.org/maven2/org/jsoup/jsoup/maven-metadata.xml \
| rg -n "1\.22\.2" || echo "not found"Repository: goofmint/bookmark-app-java
Length of output: 314
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== settings.gradle ==="
cat -n settings.gradle
echo
echo "=== build.gradle（repositories セクション付近を抜粋）==="
rg -n "repositories\s*\{" build.gradle || true
# repositories があればその直近を表示
if rg -n "repositories\s*\{" build.gradle >/dev/null 2>&1; then
# repositoriesブロック周辺を狙って表示（ファイルが小さい想定）
awk '
BEGIN{in=0; depth=0}
/repositories[[:space:]]*\{/ {in=1}
{
if(in){
print NR ":" $0
if(index($0,"{")>0){depth+=gsub(/{/,"&")}
if(index($0,"}")>0){depth-=gsub(/}/,"&")}
if(depth<=0 && /}/){in=0; exit}
}
}
' build.gradle
else
echo "No repositories block found in build.gradle"
fi
echo
echo "=== build.gradle 全体に mavenCentral / maven { があるか ==="
rg -n "mavenCentral\(\)|maven\s*\{|google\(\)" build.gradle || trueRepository: goofmint/bookmark-app-java
Length of output: 631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Maven Central: sqlite-jdbc（利用可能バージョンを3.50系中心に確認）==="
curl -s https://repo1.maven.org/maven2/org/xerial/sqlite-jdbc/sqlite-jdbc/maven-metadata.xml \
| rg -o "<version>[^<]+</version>" \
| sed 's#<version>##;s#</version>##' \
| rg "3\.50|^3\." | tail -n 30
echo
echo "=== Maven Central: sqlite-jdbc（3.50.3.0 が存在するか再チェック）==="
curl -s https://repo1.maven.org/maven2/org/xerial/sqlite-jdbc/sqlite-jdbc/maven-metadata.xml \
| rg -n "3\.50\.3\.0" || echo "3.50.3.0 not present"
echo
echo "=== GitHub Security Advisories（sqlite-jdbc: first 20）==="
gh api graphql -f query='
{
securityVulnerabilities(first: 20, ecosystem: MAVEN, package: "org.xerial:sqlite-jdbc") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}' | jq -r '.data.securityVulnerabilities.nodes[] | "\(.advisory.severity)\t\(.advisory.summary)\tRange=\(.vulnerableVersionRange)\tPatched=\(.firstPatchedVersion.identifier // "null")"'
echo
echo "=== GitHub Security Advisories（jsoup: first 20）==="
gh api graphql -f query='
{
securityVulnerabilities(first: 20, ecosystem: MAVEN, package: "org.jsoup:jsoup") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}' | jq -r '.data.securityVulnerabilities.nodes[] | "Range=\(.vulnerableVersionRange)\tPatched=\(.firstPatchedVersion.identifier // "null")\tSeverity=\(.advisory.severity)"'Repository: goofmint/bookmark-app-java
Length of output: 126
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "=== jq の有無 ==="
command -v jq >/dev/null 2>&1 && echo "jq: found" || echo "jq: not found"
echo
echo "=== Maven Central: sqlite-jdbc（3.50系の version タグ一覧を抽出）==="
curl -s https://repo1.maven.org/maven2/org/xerial/sqlite-jdbc/sqlite-jdbc/maven-metadata.xml \
| rg -n "<version>[0-9]+\.[0-9]+\.[0-9]+(\.[0-9]+)?</version>" \
| rg -n "3\.50" \
| head -n 50
echo
echo "=== Maven Central: sqlite-jdbc（3.50.3.0 の存在確認）==="
curl -s https://repo1.maven.org/maven2/org/xerial/sqlite-jdbc/sqlite-jdbc/maven-metadata.xml \
| rg -n "3\.50\.3\.0" || echo "3.50.3.0 not found in maven-metadata.xml"
echo
echo "=== GitHub Security Advisories（sqlite-jdbc: first 10, 生JSONで出す）==="
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: MAVEN, package: "org.xerial:sqlite-jdbc") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}' || true
echo
echo "=== GitHub Security Advisories（jsoup: first 10, 生JSONで出す）==="
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: MAVEN, package: "org.jsoup:jsoup") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}' || trueRepository: goofmint/bookmark-app-java
Length of output: 1440
build.gradle の sqlite-jdbc 指定が Maven Central に存在せず要修正（他は既知Advisory範囲外）
build.gradle（23-24行目）implementation 'org.xerial:sqlite-jdbc:3.50.3.0'は Maven Central のmaven-metadata.xmlに該当バージョンが見当たらず、mavenCentral()のみ参照設定のため依存解決に失敗する可能性があります。build.gradle（4行目）Spring Boot3.5.14は脆弱レンジ>= 3.5.0, < 3.5.14の修正済みバージョン（firstPatchedVersion =3.5.14）。build.gradle（24行目）jsoup1.22.2は返ってきた Advisory の脆弱レンジ（〜1.15.3など）に該当しません。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build.gradle` at line 4, The sqlite-jdbc dependency declaration
implementation 'org.xerial:sqlite-jdbc:3.50.3.0' is not available on Maven
Central and must be changed: update the dependency in build.gradle to a version
that actually exists on Maven Central (e.g., replace
'org.xerial:sqlite-jdbc:3.50.3.0' with a valid release such as
'org.xerial:sqlite-jdbc:3.50.4.0' or another verified Maven Central version),
then run the Gradle dependency resolution (./gradlew dependencies) to confirm it
resolves; leave Spring Boot at '3.5.14' (firstPatchedVersion = 3.5.14) and do
not change the jsoup declaration as it is outside the reported vulnerable range.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java (1)
18-24:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDNSリバインディングでSSRFフィルタを迂回できます。
rejectDisallowedHost()が見ているのは検証時の DNS 応答だけで、Line 20 のJsoup.connect(normalizedUrl)は実取得時にもう一度名前解決します。攻撃者管理ドメインなら、検証時は外部 IP・取得時は内部 IP を返して内部向けアクセスを通せるので、接続先を検証済み IP に固定するか、外向き通信を強制するプロキシ/allowlist 経由にしたいです。As per coding guidelines, "セキュリティ: ユーザー入力のURLをサーバー側で取得する処理（jsoup等）におけるSSRF対策を確認する。内部IP・プライベートアドレス・loopback・クラウドのメタデータエンドポイントへのアクセスがブロックされているか、スキーム(https/http)が制限されているか、リダイレクト追従の安全性を検証する。"
Also applies to: 55-64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java` around lines 18 - 24, The SSRF risk: DNS rebinding can bypass rejectDisallowedHost() because Jsoup.connect(normalizeUrl(url)) performs a separate DNS resolution at fetch time; update BookmarkMetadataFetcher so that you resolve and validate the URL’s IP once and use that validated endpoint for the HTTP request (or route through a forced outbound proxy/allowlist). Concretely: perform a hostname->InetAddress lookup before fetch, call rejectDisallowedHost() against those resolved addresses, enforce scheme restrictions (http/https) and disable or validate redirects (followRedirects=false and validate Location hosts on redirect), then perform the HTTP request using a client that lets you connect to a specific IP while preserving the original Host header (so the request cannot be redirected to an internal IP). Ensure the same logic is applied for the other fetch path referenced around lines 55-64.
🧹 Nitpick comments (1)
src/main/java/com/example/bookmark/BookmarkController.java (1)
45-49: ⚡ Quick win
catch (Exception)が広すぎて障害原因を隠します。保存失敗のハンドリング意図に対して捕捉範囲が広すぎます。
DataAccessExceptionなど永続化失敗に限定し、詳細はサーバーログに残してください（ユーザー向けメッセージは現状のままで問題ありません）。🔧 修正例
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.dao.DataAccessException; @@ public class BookmarkController { + private static final Logger logger = LoggerFactory.getLogger(BookmarkController.class); @@ - } catch (Exception ex) { + } catch (DataAccessException ex) { + logger.warn("Failed to save bookmark. url={}", url, ex); redirectAttributes.addFlashAttribute("error", "ブックマークを保存できませんでした。"); redirectAttributes.addFlashAttribute("url", url); return "redirect:/"; }As per coding guidelines, 「エラーハンドリング: 例外処理・エラー応答・リダイレクトの方針が一貫しているか確認する。」を重視しました。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/example/bookmark/BookmarkController.java` around lines 45 - 49, The catch-all catch (Exception ex) in BookmarkController should be narrowed to persistence-related exceptions: replace it with catching org.springframework.dao.DataAccessException (or your app's repository exception) in the save flow, log the exception details via the controller's logger (e.g., logger.error("Failed to save bookmark", ex)), keep the current user-facing flash attributes (error message and url) and redirect as-is; if you still want to guard against unexpected runtime errors, add a separate generic catch that logs and rethrows or returns a 500 handler, but do not swallow Exception broadly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java`:
- Around line 88-94: The check in BookmarkMetadataFetcher that uses "first ==
192 && second == 0" incorrectly blocks the entire 192.0.0.0/16 space; narrow
this to only the specific special-use prefixes you intend to block by replacing
that condition with explicit checks for the exact /24 (or other precise)
prefixes (for example match first == 192 && second == 0 && third == 0 for
192.0.0.0/24, and/or first == 192 && second == 0 && third == 2 for 192.0.2.0/24)
or otherwise convert to CIDR-aware logic to test against a whitelist/blacklist
of exact networks; update the boolean expression in the same method in
BookmarkMetadataFetcher and add/adjust unit tests and comments to document the
exact blocked prefixes.
---
Outside diff comments:
In `@src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java`:
- Around line 18-24: The SSRF risk: DNS rebinding can bypass
rejectDisallowedHost() because Jsoup.connect(normalizeUrl(url)) performs a
separate DNS resolution at fetch time; update BookmarkMetadataFetcher so that
you resolve and validate the URL’s IP once and use that validated endpoint for
the HTTP request (or route through a forced outbound proxy/allowlist).
Concretely: perform a hostname->InetAddress lookup before fetch, call
rejectDisallowedHost() against those resolved addresses, enforce scheme
restrictions (http/https) and disable or validate redirects
(followRedirects=false and validate Location hosts on redirect), then perform
the HTTP request using a client that lets you connect to a specific IP while
preserving the original Host header (so the request cannot be redirected to an
internal IP). Ensure the same logic is applied for the other fetch path
referenced around lines 55-64.
---
Nitpick comments:
In `@src/main/java/com/example/bookmark/BookmarkController.java`:
- Around line 45-49: The catch-all catch (Exception ex) in BookmarkController
should be narrowed to persistence-related exceptions: replace it with catching
org.springframework.dao.DataAccessException (or your app's repository exception)
in the save flow, log the exception details via the controller's logger (e.g.,
logger.error("Failed to save bookmark", ex)), keep the current user-facing flash
attributes (error message and url) and redirect as-is; if you still want to
guard against unexpected runtime errors, add a separate generic catch that logs
and rethrows or returns a 500 handler, but do not swallow Exception broadly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef5264fd-4b63-44c0-ad08-03a5c7dee2c3
⛔ Files ignored due to path filters (1)
gradlew.batis excluded by!gradlew.bat
📒 Files selected for processing (4)
.coderabbit.yamlsrc/main/java/com/example/bookmark/BookmarkController.javasrc/main/java/com/example/bookmark/BookmarkMetadataFetcher.javasrc/main/resources/static/styles.css
💤 Files with no reviewable changes (1)
- src/main/resources/static/styles.css
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java`:
- Around line 81-86: The isDisallowedAddress method currently misses IPv6 ULA
(fc00::/7) checks; update isDisallowedAddress(InetAddress address) to also
detect IPv6 addresses (instanceof Inet6Address) and reject ULA by inspecting the
raw address bytes (check the first byte: (addrBytes[0] & 0xFE) == 0xFC to match
both 0xFC and 0xFD). Add this ULA check alongside the existing
isAnyLocalAddress/isLoopbackAddress/isSiteLocalAddress/isLinkLocalAddress/isMulticastAddress
checks so any Inet6Address in fc00::/7 is treated as disallowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23a10e2f-a19a-42a1-bb07-4ddc4c3ab490
📒 Files selected for processing (1)
src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java (2)
41-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick win認証情報付き URL はここで拒否した方がいいです。
https://user:pass@example.com/...のような URL をそのままuri.toString()で返すと、BookmarkController.add→BookmarkRepository.add経由で資格情報ごと保存されます。uri.getUserInfo()が入っている入力はバリデーションエラーにしてください。As per coding guidelines, "入力検証: `@RequestParam` 等の外部入力に対するバリデーション（必須・形式・長さ）が網羅されているか確認する。"修正イメージ
if (!StringUtils.hasText(uri.getHost())) { throw new IllegalArgumentException("URL must include a host"); } + if (StringUtils.hasText(uri.getUserInfo())) { + throw new IllegalArgumentException("URL must not include user info"); + } rejectDisallowedHost(uri.getHost()); return uri.toString();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java` around lines 41 - 50, In BookmarkMetadataFetcher, reject inputs that include user credentials: after constructing URI (URI uri = new URI(withScheme)) check if uri.getUserInfo() is non-null/has text and if so throw an IllegalArgumentException (e.g. "URL must not contain user info/credentials") instead of returning uri.toString(); keep existing scheme/host checks and rejectDisallowedHost(...) unchanged so URLs with embedded credentials are treated as validation errors before BookmarkController.add / BookmarkRepository.add persist anything.
18-25:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSSRF対策（DNS rebinding/TOCTOU）とURLのuserinfo拒否を追加してください
normalizeUrl()のrejectDisallowedHost(uri.getHost())は検査時点の解決結果に基づきますが、実際の取得はJsoup.connect(normalizedUrl)で再解決されるため、検査後にDNSが変わる（DNS rebinding/TOCTOU）と内部アドレスへ到達し得ます。検査で得たInetAddressを接続先として固定する（同一結果を使うネットワーク層へ寄せる）対応が必要です。normalizeUrl()はuserinfo（https://user:pass@host/...）を拒否しておらず、資格情報付きURLがuri.toString()→Jsoup.connect()にそのまま渡り得ます。uri.getUserInfo()がある場合は明確に拒否してください。fetch()はIOExceptionを catch して空メタデータを返しますが、Controller 側ではそれを「保存不可」として扱わないため、失敗時の方針（保存しない/ユーザーへエラー理由）を揃えると安全です。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java` around lines 18 - 25, normalizeUrl currently only calls rejectDisallowedHost(uri.getHost()) at validation time and does not reject URLs with userinfo, and fetch() passes normalizedUrl into Jsoup.connect() causing TOCTOU/DNS rebinding risk; update normalizeUrl to explicitly reject any URI with uri.getUserInfo() (throw/return error), and change the fetch(String url) workflow to resolve the host to an InetAddress during normalization (use InetAddress.getAllByName or similar and run rejectDisallowedHost against those addresses), then make Jsoup connect to the resolved IP while sending the original host as the Host header (or use a custom connection/socket factory bound to that InetAddress) so the network layer uses the vetted address; finally, stop silently swallowing IOException in fetch()—either propagate the exception or return a failure result (e.g., Optional/Result or a BookmarkMetadata status) so the controller can treat fetch failures as non-saveable; refer to normalizeUrl, rejectDisallowedHost, fetch, and Jsoup.connect in your changes.
🧹 Nitpick comments (1)
src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java (1)
92-97: ⚡ Quick winこのビット演算は 1 行補足があると初学者に伝わりやすいです。
(bytes[0] & 0xfe) == 0xfcだけだと、なぜfc00::/7を判定できるのかが読み取りづらいです。fcとfdを上位 7 bit でまとめて見ている、という説明を添えると意図が追いやすくなります。As per coding guidelines, "可読性: 初学者を対象としているため、意図が伝わる十分なコメントがあるか確認し、不足していれば補足を提案する。"補足コメント例
private static boolean isIpv6UniqueLocalAddress(InetAddress address) { if (!(address instanceof Inet6Address)) { return false; } byte[] bytes = address.getAddress(); + // fc00::/7 は先頭 7bit が共通なので、0xfc と 0xfd をまとめて判定する。 return (bytes[0] & 0xfe) == 0xfc; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java` around lines 92 - 97, isIpv6UniqueLocalAddress の内部ビット演算が初学者には分かりにくいので、Inet6Address 判定後の ((bytes[0] & 0xfe) == 0xfc) の直前に短い説明コメントを追加してください（例: "fc00::/7 を判定するために上位7ビットをマスクして 0b11111100(0xfc) と比較している。これにより 0xfc と 0xfd の両方が一致する" の趣旨）。対象はメソッド isIpv6UniqueLocalAddress とローカル変数 bytes です。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java`:
- Around line 41-50: In BookmarkMetadataFetcher, reject inputs that include user
credentials: after constructing URI (URI uri = new URI(withScheme)) check if
uri.getUserInfo() is non-null/has text and if so throw an
IllegalArgumentException (e.g. "URL must not contain user info/credentials")
instead of returning uri.toString(); keep existing scheme/host checks and
rejectDisallowedHost(...) unchanged so URLs with embedded credentials are
treated as validation errors before BookmarkController.add /
BookmarkRepository.add persist anything.
- Around line 18-25: normalizeUrl currently only calls
rejectDisallowedHost(uri.getHost()) at validation time and does not reject URLs
with userinfo, and fetch() passes normalizedUrl into Jsoup.connect() causing
TOCTOU/DNS rebinding risk; update normalizeUrl to explicitly reject any URI with
uri.getUserInfo() (throw/return error), and change the fetch(String url)
workflow to resolve the host to an InetAddress during normalization (use
InetAddress.getAllByName or similar and run rejectDisallowedHost against those
addresses), then make Jsoup connect to the resolved IP while sending the
original host as the Host header (or use a custom connection/socket factory
bound to that InetAddress) so the network layer uses the vetted address;
finally, stop silently swallowing IOException in fetch()—either propagate the
exception or return a failure result (e.g., Optional/Result or a
BookmarkMetadata status) so the controller can treat fetch failures as
non-saveable; refer to normalizeUrl, rejectDisallowedHost, fetch, and
Jsoup.connect in your changes.
---
Nitpick comments:
In `@src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java`:
- Around line 92-97: isIpv6UniqueLocalAddress
の内部ビット演算が初学者には分かりにくいので、Inet6Address 判定後の ((bytes[0] & 0xfe) == 0xfc)
の直前に短い説明コメントを追加してください（例: "fc00::/7 を判定するために上位7ビットをマスクして 0b11111100(0xfc)
と比較している。これにより 0xfc と 0xfd の両方が一致する" の趣旨）。対象はメソッド isIpv6UniqueLocalAddress
とローカル変数 bytes です。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43fcc5c9-6e95-4fbd-8cb6-9bbf7fe6d0e6
📒 Files selected for processing (1)
src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/example/bookmark/BookmarkController.java (1)
28-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
urlの長さをサーバー側で上限チェックしてください。
BookmarkController.addが@RequestParam String urlをそのままmetadataFetcher.fetch(url)に渡しており、サーバー側にindex.htmlのmaxlength="1000"相当の制限がありません。DNS解決や外部取得の前に、metadataFetcher.normalizeUrlかコントローラで文字数上限（例: 1000）を検証して弾いてください。
（スキーム制限・リダイレクト追従無効・内部IPブロックは既に入っていますが、長さのみ未対応です。）🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/example/bookmark/BookmarkController.java` around lines 28 - 35, BookmarkController.add currently forwards the raw `@RequestParam` String url to metadataFetcher.fetch without enforcing the client-side maxlength; before calling metadataFetcher.fetch(url) validate the length (e.g. max 1000 chars) either by calling metadataFetcher.normalizeUrl(url) and checking its length or by checking the incoming url string directly, and if it exceeds the limit set the same redirectAttributes.addFlashAttribute("error", "URLを確認してください。") and redirect back without invoking fetch; reference BookmarkController.add, metadataFetcher.fetch and metadataFetcher.normalizeUrl when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/example/bookmark/BookmarkMetadataFetcher.java`:
- Around line 20-25: The code currently replaces the URL host with a resolved IP
in normalizeUrl() and then calls Jsoup.connect(normalizedUrl.fetchUrl()) while
restoring the original Host header, which breaks TLS SNI/hostname verification
for HTTPS. Replace the direct Jsoup.connect usage in BookmarkMetadataFetcher
(the fetch flow that calls normalizeUrl(), normalizedUrl.fetchUrl(), and sets
"Host") with an HTTP client that lets you connect to an IP but set SNI/hostname
verification to the original domain (for example: Java 11+ HttpClient or
OkHttp/Apache HttpClient with a custom Dns/SocketFactory/SSLParameters or
HostnameVerifier), perform the request through that client, and pass the
returned InputStream/response body to Jsoup.parse(...) (or
Jsoup.parseBodyFragment) so TLS SNI uses the original host while the TCP
connection goes to the pinned IP; ensure normalizeUrl() still provides both the
original host and the chosen IP so the fetch code can set SNI/hostname and Host
header appropriately.
---
Outside diff comments:
In `@src/main/java/com/example/bookmark/BookmarkController.java`:
- Around line 28-35: BookmarkController.add currently forwards the raw
`@RequestParam` String url to metadataFetcher.fetch without enforcing the
client-side maxlength; before calling metadataFetcher.fetch(url) validate the
length (e.g. max 1000 chars) either by calling metadataFetcher.normalizeUrl(url)
and checking its length or by checking the incoming url string directly, and if
it exceeds the limit set the same redirectAttributes.addFlashAttribute("error",
"URLを確認してください。") and redirect back without invoking fetch; reference
BookmarkController.add, metadataFetcher.fetch and metadataFetcher.normalizeUrl
when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76dade27-da0e-4dfe-b5e2-427d29b8923a
📒 Files selected for processing (2)
src/main/java/com/example/bookmark/BookmarkController.javasrc/main/java/com/example/bookmark/BookmarkMetadataFetcher.java
This was referenced May 23, 2026
Open
Open
Closed
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by CodeRabbit
New Features
UI
Documentation
Chores