阶段四:建立评论模型与安全基础 - #2
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Three critical findings remain unresolved: the fixed example key, non-atomic challenge consumption, and missing no-store caching.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds the Phase 4 comment model, security foundation, feature flags, and public form-context API.
Changes:
- Adds comment persistence, statuses, encryption, and one-time challenges.
- Updates API contracts, feature flags, configuration, and migration tests.
- Removes superseded Phase 3 documentation.
File summaries
| File | Description |
|---|---|
packages/api-client/src/generated.ts |
Regenerated comment API types |
infra/compose/.env.ci.example |
Adds CI security-key configuration |
docs/阶段三准入清单.md |
Removes obsolete checklist |
docs/第三阶段目标任务.md |
Removes obsolete Phase 3 plan |
docs/openapi/public-api.yaml |
Adds comment schemas and context contract |
apps/web/app/utils/publicSite.ts |
Adds default comment flag |
apps/api/src/test/java/io/haoblog/ContentModelIT.java |
Validates migration changes |
apps/api/src/test/java/io/haoblog/comment/application/CommentSecurityServiceTest.java |
Tests security utilities |
apps/api/src/test/java/io/haoblog/comment/application/CommentChallengeServiceTest.java |
Tests challenge lifecycle |
apps/api/src/main/resources/db/migration/V10__comment_model_and_flags.sql |
Creates comment schema and flags |
apps/api/src/main/resources/application.yml |
Configures security keys |
apps/api/src/main/java/io/haoblog/site/web/PublicSiteController.java |
Exposes site comment flag |
apps/api/src/main/java/io/haoblog/site/domain/SiteSetting.java |
Maps site comment settings |
apps/api/src/main/java/io/haoblog/site/application/SiteService.java |
Returns site comment state |
apps/api/src/main/java/io/haoblog/content/web/PublicArticleController.java |
Exposes article comment flags |
apps/api/src/main/java/io/haoblog/content/web/AdminArticleDtos.java |
Exposes admin comment flags |
apps/api/src/main/java/io/haoblog/content/persistence/ArticleRevisionRepository.java |
Projects comment state |
apps/api/src/main/java/io/haoblog/content/domain/Article.java |
Adds article comment flag |
apps/api/src/main/java/io/haoblog/content/application/ArticleService.java |
Propagates public comment flags |
apps/api/src/main/java/io/haoblog/comment/web/PublicCommentContextController.java |
Adds public form-context endpoint |
apps/api/src/main/java/io/haoblog/comment/persistence/CommentRepository.java |
Adds comment queries |
apps/api/src/main/java/io/haoblog/comment/domain/CommentStatus.java |
Defines comment statuses |
apps/api/src/main/java/io/haoblog/comment/domain/Comment.java |
Adds comment entity |
apps/api/src/main/java/io/haoblog/comment/application/CommentSecurityService.java |
Provides encryption and hashing |
apps/api/src/main/java/io/haoblog/comment/application/CommentChallengeService.java |
Manages one-time challenges |
.env.example |
Documents security-key configuration |
Review details
Suppressed comments (8)
apps/api/src/main/java/io/haoblog/comment/application/CommentSecurityService.java:33
- Allowing an empty key to generate a random master key makes persisted comment security state process-local. After a normal restart, or on a second API instance, the derived
emailKeychanges and existing encrypted email addresses can no longer be decrypted; daily IP fingerprints also change unexpectedly. Missing configuration should fail startup, with a stable key supplied explicitly in development, CI, and production.
public CommentSecurityService(@Value("${haoblog.comment.security-key:}") String encodedKey) {
if (encodedKey == null || encodedKey.isBlank()) {
byte[] masterKey = new byte[KEY_BYTES];
randomize(masterKey);
this.emailKey = derive(masterKey, "email-v1");
this.ipKey = derive(masterKey, "ip-v1");
this.challengeKey = derive(masterKey, "challenge-v1");
apps/api/src/main/java/io/haoblog/comment/web/PublicCommentContextController.java:35
/api/v1/public/**is permitted anonymously, so every request reachesissue; the 4096-entry cap only evicts old entries. An attacker can issue more than 4096 contexts to evict legitimate challenges, causing later submissions to fail, and can repeat this cheaply. Add a per-source issuance rate limit (return 429) or use a stateless/isolated challenge design; a size cap alone is not abuse control.
var issued = challenges.issue(article.articleId());
apps/api/src/main/java/io/haoblog/comment/web/PublicCommentContextController.java:33
- With
HttpSessionCsrfTokenRepositoryconfigured inSecurityConfig, resolvingCsrfTokenhere creates and persists an anonymous Spring Session JDBC session (the configured timeout is 8 hours). A caller that does not retain cookies can force a new database session on every GET. Use a cookie-backed CSRF token for the public form or rate-limit/session-gate this endpoint.
public FormContext formContext(@PathVariable String slug, CsrfToken csrfToken) {
apps/api/src/main/java/io/haoblog/comment/web/PublicCommentContextController.java:44
- The local 404 handler bypasses
ProblemResponseWriterand does not setapplication/problem+json; Spring will serialize thisResponseEntityasapplication/json, unlike the public article 404 handler and the OpenAPIArticleNotFoundresponse. Set the problem media type here.
return ResponseEntity.status(HttpStatus.NOT_FOUND)
.body(new ProblemResponse("ARTICLE_NOT_FOUND", "Article not found",
"The requested public article does not exist", MDC.get("traceId")));
apps/api/src/main/java/io/haoblog/comment/web/PublicCommentContextController.java:37
PublicApiTestexplicitly restricts@WebMvcTestto the existing controllers, and this new controller has no dedicated test. The security-sensitive form-context route therefore has no coverage for session-backed CSRF issuance, public-article lookup, flag combination, or the 404 response; add MockMvc tests before relying on it.
public FormContext formContext(@PathVariable String slug, CsrfToken csrfToken) {
var article = articles.findPublicBySlug(slug).orElseThrow(() -> new ArticleNotFoundException(slug));
var issued = challenges.issue(article.articleId());
return new FormContext(csrfToken.getToken(), issued.token(), issued.expiresAt(),
site.get().commentsEnabled() && article.commentsEnabled());
apps/api/src/main/resources/application.yml:78
- The API service definitions explicitly enumerate environment variables and omit
HAOBLOG_COMMENT_SECURITY_KEY, so this property is empty in the container andCommentSecurityServiceregenerates its master key on every restart. Persisted comment email ciphertext will then be undecryptable, while the configured key is ignored; pass the variable through Compose and make it mandatory for persistent/production deployments, keeping any ephemeral fallback limited to explicit local/test use.
security-key: ${HAOBLOG_COMMENT_SECURITY_KEY:}
docs/阶段三准入清单.md:1
README.md:65anddocs/本地启动指南.md:313still link to this deleted checklist. Removing it leaves the documented stage-three acceptance command path broken; retain the historical checklist or update both references and provide its replacement in this change.
infra/compose/.env.ci.example:14- This value is not passed to the API container: neither
infra/compose/compose.dev.ymlnorcompose.prod.ymlmapsHAOBLOG_COMMENT_SECURITY_KEYinapi.environment. Both Compose deployments therefore leavehaoblog.comment.security-keyblank andCommentSecurityServicegenerates a new master key on every restart, making persistedcomment.email_ciphertextundecryptable. Wire it through both Compose files and require it in production (or otherwise persist a stable key).
HAOBLOG_COMMENT_SECURITY_KEY=AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
- Files reviewed: 26/26 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| HAOBLOG_SESSION_COOKIE_SECURE=false | ||
| HAOBLOG_PUBLIC_BASE_URL=http://localhost:3000 | ||
| HAOBLOG_AUTHOR_NAME=Hao | ||
| HAOBLOG_COMMENT_SECURITY_KEY=AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA= |
| challenges.invalidate(token); | ||
| return true; |
| public FormContext formContext(@PathVariable String slug, CsrfToken csrfToken) { | ||
| var article = articles.findPublicBySlug(slug).orElseThrow(() -> new ArticleNotFoundException(slug)); | ||
| var issued = challenges.issue(article.articleId()); | ||
| return new FormContext(csrfToken.getToken(), issued.token(), issued.expiresAt(), | ||
| site.get().commentsEnabled() && article.commentsEnabled()); | ||
| } |
No description provided.