Add URL validation to AddBlog function#18
Conversation
- Added InvalidURLError type for invalid URL errors - Validate blog URL has valid format and http/https scheme - Validate feed URL if provided - Added comprehensive tests for valid and invalid URLs Fixes issue where invalid URLs could be stored, causing failures during scanning.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded explicit URL validation to AddBlog (renamed param to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (1)
internal/controller/controller.go (1)
49-68: Extract duplicated URL validation logic into a helper function.The URL validation logic is duplicated for
urlStrandfeedURL. Consider extracting to a helper:func validateURL(urlStr string) error { if _, err := url.ParseRequestURI(urlStr); err != nil { return InvalidURLError{URL: urlStr} } parsedURL, err := url.Parse(urlStr) if err != nil || (parsedURL.Scheme != "http" && parsedURL.Scheme != "https") { return InvalidURLError{URL: urlStr} } return nil }Also, consider enhancing
InvalidURLErrorto include aReasonfield (e.g., "empty", "unsupported scheme", "malformed") to provide more actionable feedback to CLI users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/controller.go` around lines 49 - 68, The AddBlog function duplicates URL validation for urlStr and feedURL; extract that logic into a helper function (e.g., validateURL(urlStr string) error) and call it from AddBlog for both urlStr and feedURL, returning InvalidURLError on failure; also modify the InvalidURLError type to include an optional Reason field ("empty", "unsupported scheme", "malformed") and populate it in validateURL so callers of AddBlog get clearer error details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/controller_test.go`:
- Around line 44-61: The test table in controller_test.go contains an invalid
case named "empty feed URL allowed" that expects an error but should succeed;
locate the testCases slice used by the failing test (the loop invoking
AddBlog(ctx, db, "Test"+tc.name, tc.url, tc.feedURL, "")) and remove or relocate
the entry {"empty feed URL allowed", "https://example.com", ""} from this
error-expecting test to the positive-test suite (e.g., TestAddBlogValidURL) so
AddBlog is asserted to succeed for an empty feedURL instead of asserting an
InvalidURLError.
---
Nitpick comments:
In `@internal/controller/controller.go`:
- Around line 49-68: The AddBlog function duplicates URL validation for urlStr
and feedURL; extract that logic into a helper function (e.g., validateURL(urlStr
string) error) and call it from AddBlog for both urlStr and feedURL, returning
InvalidURLError on failure; also modify the InvalidURLError type to include an
optional Reason field ("empty", "unsupported scheme", "malformed") and populate
it in validateURL so callers of AddBlog get clearer error details.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6eb13a7-a890-4f65-9a1e-dba3bed0f9d8
📒 Files selected for processing (2)
internal/controller/controller.gointernal/controller/controller_test.go
|
Hey @sroecker thanks! Looking into this right now, just came back from vacation :) |
With AddBlog now rejecting non-http(s) URLs, a single bad entry in an OPML file would halt the whole import with earlier entries already committed. Treat InvalidURLError like BlogAlreadyExistsError: count it as skipped and keep going. Also fix the "empty feed URL allowed" case that was asserting an error on a valid input, and switch the errors.As assertion to ErrorAs per testifylint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Hey @sroecker — thanks for this! I pushed one small commit directly (e95da1e) to fix a behavior regression I caught while reviewing: With the new validation in place, a single OPML entry with a non-http(s) URL would abort the whole import mid-loop (with earlier entries already committed). I extended While I was in there I also fixed two small things in your test:
There are still a couple of follow-ups I'd like to land in a separate PR of my own — Thanks so much for your contribution! |
2 participants
Summary
Adds URL validation to prevent invalid URLs from being stored in the database, which could cause failures during scanning.
Changes
Validation Rules
Test Coverage
Added two new test functions:
Fixes
Resolves the security concern where invalid URLs could be stored, causing potential failures during the scanning process.
Summary by CodeRabbit
Bug Fixes
Tests