feat: support HTML to RSS navigation actions - #833
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 5 minor |
| Security | 1 critical 1 high |
| Complexity | 40 medium |
🟢 Metrics 1575 complexity · 136 duplication
Metric Results Complexity 1575 Duplication 136
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several major features to FeedCraft, including a Web Monitor source for tracking webpage changes, an Inbox system for third-party HTTP data pushes with token authentication, semantic topic filtering and deduplication via embedding models, and a transition to a functional closure-based CraftOption processing pipeline. The code review highlights critical security vulnerabilities, including SSRF risks from removed private IP checks in the feed viewer and Stored XSS risks in the public inbox content endpoint. Additionally, the reviewer points out significant performance and stability issues, such as socket churn from repeated Redis client instantiations, data loss risks when updating inboxes or setting maximum item limits, and potential runtime panics in embedding batching and cosine similarity calculations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func validateFeedViewerURL(rawURL string) error { | ||
| parsedURL, err := url.Parse(rawURL) | ||
| if err != nil || parsedURL == nil { | ||
| return errors.New("Please enter a valid http(s) feed URL") | ||
| return errors.New(feedViewerInvalidURLError) | ||
| } | ||
| if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" { | ||
| return errors.New("Please enter a valid http(s) feed URL") | ||
| return errors.New(feedViewerInvalidURLError) | ||
| } | ||
| if parsedURL.Hostname() == "" { | ||
| return errors.New("Please enter a valid http(s) feed URL") | ||
| return errors.New(feedViewerInvalidURLError) | ||
| } | ||
|
|
||
| ips, err := net.LookupIP(parsedURL.Hostname()) | ||
| if err != nil { | ||
| return fmt.Errorf("Unable to resolve this URL: %w", err) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The private IP and loopback address checks have been removed from validateFeedViewerURL. This exposes the application to Server-Side Request Forgery (SSRF) vulnerabilities, allowing users to probe or access internal services (e.g., local databases, Redis, or internal APIs) via the preview endpoint. Consider restoring the private IP check by default, or introducing an environment variable (e.g., FC_ALLOW_PRIVATE_IPS) to safely toggle this behavior for local development.
| return | ||
| } | ||
|
|
||
| c.Data(http.StatusOK, "text/html; charset=utf-8", []byte(item.Content)) |
There was a problem hiding this comment.
Serving raw, user-supplied HTML content directly on the same domain without any restriction can lead to Stored Cross-Site Scripting (XSS) attacks. If an attacker pushes malicious JavaScript into an inbox, it will execute in the context of the FeedCraft domain when viewed, potentially hijacking admin sessions. To mitigate this, consider serving the content with a restrictive Content-Security-Policy: sandbox header to disable script execution and isolate the origin.
\tc.Header(\"Content-Security-Policy\", \"sandbox\")\n\tc.Data(http.StatusOK, \"text/html; charset=utf-8\", []byte(item.Content))| func tryGetRedisClient() (*redis.Client, error) { | ||
| envClient := GetEnvClient() | ||
| if envClient == nil { | ||
| return nil, errors.New("env client not available") | ||
| } | ||
| redisURI := strings.TrimSpace(envClient.GetString("REDIS_URI")) | ||
| if redisURI == "" { | ||
| return nil, errors.New("REDIS_URI is not configured") | ||
| } | ||
| opts, err := redis.ParseURL(redisURI) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| rdb := redis.NewClient(opts) | ||
| if rdb == nil { | ||
| return nil, errors.New("failed to create redis client") | ||
| } | ||
| return rdb, nil | ||
| } |
There was a problem hiding this comment.
tryGetRedisClient instantiates a new redis.Client on every single invocation. Since this function is called on every cache read and write, this will cause massive socket churn, connection pool exhaustion, and CPU overhead because connection pools are not reused. Consider caching the client instance globally using sync.Once.
var (\n\ttryRedisClient *redis.Client\n\ttryRedisOnce sync.Once\n\ttryRedisErr error\n)\n\nfunc tryGetRedisClient() (*redis.Client, error) {\n\ttryRedisOnce.Do(func() {\n\t\tenvClient := GetEnvClient()\n\t\tif envClient == nil {\n\t\t\ttryRedisErr = errors.New(\"env client not available\")\n\t\t\treturn\n\t\t}\n\t\tredisURI := strings.TrimSpace(envClient.GetString(\"REDIS_URI\"))\n\t\tif redisURI == \"\" {\n\t\t\ttryRedisErr = errors.New(\"REDIS_URI is not configured\")\n\t\t\treturn\n\t\t}\n\t\topts, err := redis.ParseURL(redisURI)\n\t\tif err != nil {\n\t\t\ttryRedisErr = err\n\t\t\treturn\n\t\t}\n\t\ttryRedisClient = redis.NewClient(opts)\n\t\tif tryRedisClient == nil {\n\t\t\ttryRedisErr = errors.New(\"failed to create redis client\")\n\t\t}\n\t})\n\treturn tryRedisClient, tryRedisErr\n}| data := dao.Inbox{ | ||
| ID: id, | ||
| Title: body.Title, | ||
| Description: body.Description, | ||
| MaxItems: body.MaxItems, | ||
| IsPublic: body.IsPublic, | ||
| } |
There was a problem hiding this comment.
Constructing a new dao.Inbox struct with only the request body fields and calling db.Save will overwrite the existing record's created_at field with a zero value (0001-01-01 00:00:00). To prevent this, retrieve the existing record first, update its fields, and then save it, or use GORM's Updates method on a model with the primary key.
\texisting, err := dao.GetInboxByID(db, id)\n\tif err != nil {\n\t\tif errors.Is(err, gorm.ErrRecordNotFound) {\n\t\t\tc.JSON(http.StatusNotFound, util.APIResponse[any]{Msg: \"Inbox not found\"})\n\t\t\treturn\n\t\t}\n\t\tc.JSON(http.StatusInternalServerError, util.APIResponse[any]{Msg: err.Error()})\n\t\treturn\n\t}\n\n\texisting.Title = body.Title\n\texisting.Description = body.Description\n\texisting.MaxItems = body.MaxItems\n\texisting.IsPublic = body.IsPublic\n\n\tif err := dao.UpdateInbox(db, existing); err != nil {\n\t\tc.JSON(http.StatusInternalServerError, util.APIResponse[any]{Msg: err.Error()})\n\t\treturn\n\t}| if maxItems <= 0 { | ||
| return db.Where("inbox_id = ?", inboxID).Delete(&InboxItem{}).Error | ||
| } |
There was a problem hiding this comment.
If maxItems is set to 0 or a negative value, DeleteOverflowInboxItems will delete all items in the inbox. This is highly counter-intuitive for users who might set max_items to 0 expecting 'unlimited' storage, leading to accidental data loss. Consider treating <= 0 as unlimited and skipping the deletion.
\tif maxItems <= 0 {\n\t\treturn nil\n\t}| if err != nil { | ||
| return nil, fmt.Errorf("embedding call failed after retries (batch [%d-%d]): %w", batchStart, batchEnd-1, err) | ||
| } |
There was a problem hiding this comment.
If the embedding service returns a different number of vectors than the requested batch size, iterating over result32 can either cause an out-of-bounds panic or leave nil vectors in allResults. Adding a length check prevents these potential runtime panics.
\t\tif err != nil {\n\t\t\treturn nil, fmt.Errorf(\"embedding call failed after retries (batch [%d-%d]): %w\", batchStart, batchEnd-1, err)\n\t\t}\n\n\t\tif len(result32) != len(batch) {\n\t\t\treturn nil, fmt.Errorf(\"embedding service returned %d vectors, expected %d\", len(result32), len(batch))\n\t\t}| // 零向量检查 | ||
| if normA == 0 || normB == 0 { | ||
| return 0.0 | ||
| } | ||
|
|
||
| return dotProduct / (math.Sqrt(normA) * math.Sqrt(normB)) |
There was a problem hiding this comment.
If normA or normB is extremely small (subnormal float), math.Sqrt(normA) * math.Sqrt(normB) can underflow to 0.0, leading to a division by zero and returning NaN or Inf. Checking if the denominator is 0 directly is safer.
\tdenom := math.Sqrt(normA) * math.Sqrt(normB)\n\tif denom == 0 {\n\t\treturn 0.0\n\t}\n\n\treturn dotProduct / denom| return | ||
| } | ||
|
|
||
| siteBaseURL := strings.TrimRight(util.GetEnvClient().GetString("SITE_BASE_URL"), "/") |
There was a problem hiding this comment.
If SITE_BASE_URL is not configured, siteBaseURL will be empty, causing the RSS feed to contain relative URLs. Relative URLs are a common cause of feed validation failures and reader compatibility issues. Consider dynamically resolving the base URL from the request headers as a fallback.
\tsiteBaseURL := strings.TrimRight(util.GetEnvClient().GetString(\"SITE_BASE_URL\"), \"/\")\n\tif siteBaseURL == \"\" {\n\t\tscheme := \"http\"\n\t\tif c.Request.TLS != nil {\n\t\t\tscheme = \"https\"\n\t\t}\n\t\tif proto := c.GetHeader(\"X-Forwarded-Proto\"); proto != \"\" {\n\t\t\tscheme = proto\n\t\t}\n\t\thost := c.Request.Host\n\t\tif fwdHost := c.GetHeader(\"X-Forwarded-Host\"); fwdHost != \"\" {\n\t\t\thost = fwdHost\n\t\t}\n\t\tsiteBaseURL = scheme + \"://\" + host\n\t}| if len(modelClients) == 0 { | ||
| return "", fmt.Errorf("all models failed, last error: %v", lastErr) | ||
| } |
There was a problem hiding this comment.
If modelClients is empty (e.g., due to empty or invalid model configuration), lastErr will be nil, resulting in a confusing error message: 'all models failed, last error: <nil>'. Consider returning a more descriptive error.
\tif len(modelClients) == 0 {\n\t\treturn \"\", fmt.Errorf(\"no models configured or initialized\")\n\t}
Co-authored-by: Colin <Colin_XKL@outlook.com>
Co-authored-by: Colin <Colin_XKL@outlook.com>
f010f13 to
c7d0b3a
Compare
2 participants
Summary
http_fetcher.navigation_actionsfor ordered browser actions before HTML capture.click,wait_for_selector, andwaitactions in Browserless REST/functionand CDP rendering paths.Walkthrough
html_to_rss_navigation_actions_demo.mp4
HTML to RSS navigation actions final state
Testing
go test ./internal/util -run 'Test(GetBrowserlessContentUsesFunctionEndpointForNavigationActions|ValidateBrowserNavigationActionsRejectsInvalidAction)'go test ./internal/util -run 'TestValidateBrowserNavigationActions(NormalizesActionFields|RejectsExcessiveWait)'gofmt -w ... && go test ./internal/util ./internal/source/fetcherpnpm run format && pnpm run type:checkinweb/admintask fixgo test ./...task backend-buildtask frontend-buildTo show artifacts inline, enable in settings.