Skip to content

Conversation

jaspermayone
Copy link
Member

@jaspermayone jaspermayone commented Jul 20, 2025

Implements real-time domain feed monitoring from SinkingYachts API as requested in issue #64.

Summary

  • Enhanced SinkingYahtsService with WebSocket support for real-time updates
  • Added bulk import and incremental update capabilities via REST API
  • Implemented automatic reconnection with exponential backoff
  • Added admin management endpoints for feed control
  • Integrated auto-start configuration with environment variable controls

Test plan

  • Start the server and verify SinkingYachts feed monitoring automatically begins
  • Test admin endpoints for manual feed control
  • Verify WebSocket reconnection behavior during network interruptions
  • Check database for proper domain storage and updates

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Introduced admin endpoints for managing the SinkingYachts feed, including starting/stopping real-time monitoring, triggering bulk imports, and fetching recent domains.
    • Added support for real-time and bulk ingestion of malicious domains from SinkingYachts, with robust error handling and logging.
    • Enabled configuration of feed monitoring via environment variables.
  • Chores

    • Added WebSocket support and type definitions to project dependencies.

- Enhanced SinkingYahtsService with WebSocket support for real-time updates
- Added bulk import and incremental update capabilities via REST API
- Implemented automatic reconnection with exponential backoff
- Added admin management endpoints for feed control
- Integrated auto-start configuration with environment variable controls
- Added ws and @types/ws dependencies for WebSocket functionality

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-authored-by: Jasper Mayone <[email protected]>
@Copilot Copilot AI review requested due to automatic review settings July 20, 2025 16:00
@jaspermayone jaspermayone requested a review from a team as a code owner July 20, 2025 16:00
Copy link
Contributor

coderabbitai bot commented Jul 20, 2025

Walkthrough

The changes introduce integration with the SinkingYachts malicious domain feed, including both bulk import and real-time updates via WebSocket. New admin API endpoints are added for managing the feed, and the service logic for ingesting and synchronizing domain data is implemented. Required dependencies for WebSocket support are included.

Changes

File(s) Change Summary
package.json Added ws and @types/ws as dependencies for WebSocket support.
src/admin-routes/router.ts Imported and mounted new sinkingYachtsRouter under /sinking-yachts with request logging middleware.
src/admin-routes/routes/sinking-yachts.ts Introduced new Express router with endpoints for starting/stopping feed, bulk import, and recent fetch.
src/index.ts Added initialization logic for SinkingYachts feed monitoring at server startup, controlled by env vars.
src/services/SinkingYahts.ts Extended service to support SinkingYachts REST and WebSocket feed: bulk import, real-time sync, reconnection, and error handling.

Sequence Diagram(s)

sequenceDiagram
    participant Admin as Admin API Client
    participant Router as /admin/sinking-yachts Router
    participant Service as SinkingYahtsService
    participant SinkingYachts as SinkingYachts API/Feed
    participant DB as Database

    Admin->>Router: POST /start-feed (skipBulkImport?)
    Router->>Service: startFeedMonitoring(skipBulkImport)
    alt skipBulkImport = false
        Service->>SinkingYachts: GET /domains
        SinkingYachts-->>Service: [Domain List]
        Service->>DB: Bulk insert/update domains
    end
    Service->>SinkingYachts: Connect WebSocket
    SinkingYachts-->>Service: [Realtime domain events]
    Service->>DB: Insert/Update/Remove domain

    Admin->>Router: POST /stop-feed
    Router->>Service: stopRealtimeFeed()

    Admin->>Router: POST /bulk-import
    Router->>Service: initializeBulkImport()
    Service->>SinkingYachts: GET /domains
    SinkingYachts-->>Service: [Domain List]
    Service->>DB: Bulk insert/update domains

    Admin->>Router: GET /recent?since=...
    Router->>Service: getRecentDomains(since)
    Service->>SinkingYachts: GET /domains?since=...
    SinkingYachts-->>Service: [Filtered Domain List]
    Service->>Router: [Domain List]
    Router-->>Admin: [Domain List]
Loading

Poem

In the code’s blue sea, new yachts now float,
With WebSocket sails and API boat.
Domains are caught, both old and new,
Real-time or bulk, we process the queue.
The admin commands, the service replies—
SinkingYachts now watched by vigilant eyes!
🐇⛵

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

claude bot commented Jul 20, 2025

Claude finished @jaspermayone's task —— View job


Code Review Complete ✅

I've completed a comprehensive review of the SinkingYachts realtime feed integration implementation.

🟢 Strengths

  • Well-structured service architecture with clear separation of concerns
  • Comprehensive admin endpoints for feed management
  • Exponential backoff reconnection logic for resilient WebSocket connections
  • Environment variable controls for optional startup and configuration
  • Proper domain sanitization and database integration

🟡 Issues & Recommendations

Security Concerns

  1. Error Message Exposure (src/admin-routes/routes/sinking-yachts.ts:31, 55, 80, 119):

    error: error.message  // ⚠️ Exposes internal error details

    Fix: Sanitize error messages for API responses to prevent information leakage.

  2. Missing Input Validation (src/services/SinkingYahts.ts:181):

    const message: WebSocketMessage = JSON.parse(data.toString());

    Issue: No validation of WebSocket message structure or content
    Fix: Add schema validation for incoming WebSocket messages

Performance Issues

  1. Blocking Database Operations (src/services/SinkingYahts.ts:107-141):

    for (const item of domainData) {
      // Sequential database operations - blocks processing

    Issue: Sequential processing of domains blocks bulk operations
    Fix: Use Promise.allSettled() for concurrent processing with error handling

  2. Missing Batch Inserts (src/services/SinkingYahts.ts:131-136):

    await db.insert(rawAPIData).values({...}); // Individual inserts

    Issue: Individual database inserts are inefficient for bulk operations
    Fix: Batch inserts for better performance

Code Quality Issues

  1. Resource Leak Risk (src/services/SinkingYahts.ts:320-325):

    setTimeout(() => {
      this.startRealtimeFeed();
    }, this.reconnectDelay);

    Issue: Timer not stored/cleared, potential memory leak
    Fix: Store timer reference and clear on stop

  2. Hardcoded Configuration (src/services/SinkingYahts.ts:28-30):

    private maxReconnectAttempts = 5;
    private reconnectDelay = 5000;

    Fix: Move to environment variables for configurability

  3. Missing Dependency Type (package.json:51):

    "ws": "^8.18.0"

    Issue: Missing @types/ws in devDependencies
    Fix: Add proper TypeScript definitions

Error Handling

  1. Insufficient WebSocket Error Context (src/services/SinkingYahts.ts:193-195):

    this.wsConnection.on("error", (err: Error) => {
      error(`SinkingYachts: WebSocket error: ${err.message}`);
    });

    Issue: No reconnection triggered on WebSocket errors
    Fix: Consider reconnection on certain error types

  2. Silent Failure Risk (src/index.ts:38-41):

    .catch((error) => {
      logger.error(`Failed to start SinkingYachts feed monitoring: ${error.message}`);
      // Don't exit the process, let the server continue without feed monitoring
    });

    Issue: Server continues without feed monitoring but doesn't retry
    Suggestion: Consider a retry mechanism or health check endpoint

Missing Features

  1. No Health Monitoring: Missing endpoint to check WebSocket connection status
  2. No Metrics: No telemetry for connection health, message processing rate, etc.
  3. No Rate Limiting: WebSocket message processing isn't rate-limited

🔴 Critical Issues

Database Consistency (src/services/SinkingYahts.ts:262-282)

await db.update(domains)
  .set({ malicious: false, ... })  // ⚠️ Critical issue

Problem: Domain removal sets malicious: false but other services might still consider it malicious. This could cause inconsistent threat detection across the system.

Fix: Review domain status conflict resolution strategy across all threat intelligence sources.

📋 Missing Test Coverage

  • No unit tests for WebSocket connection handling
  • No integration tests for admin endpoints
  • No error scenario testing (network failures, malformed messages)
  • No load testing for bulk import operations

💡 Suggested Improvements

  1. Add configuration validation at startup
  2. Implement circuit breaker pattern for external API calls
  3. Add request/response logging for debugging
  4. Consider using TypeScript strict mode for better type safety
  5. Add JSDoc documentation for public methods
  6. Implement graceful shutdown for WebSocket connections

🎯 Verdict

This is a solid implementation with good architecture and proper separation of concerns. The main concerns are around security hardening, performance optimization, and test coverage. The WebSocket implementation is well-structured but needs some reliability improvements.

Recommendation: Address the security and performance issues before merging, particularly error message sanitization and database operation optimization.


Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @jaspermayone, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a robust real-time domain monitoring system by integrating with the SinkingYachts API. It establishes a persistent WebSocket connection for immediate updates on malicious domains, complemented by initial bulk data import and incremental update capabilities. The changes also include new administrative endpoints for manual feed control and an auto-start mechanism configurable via environment variables, ensuring comprehensive and automated domain intelligence.

Highlights

  • Real-time Feed Integration: Implemented a real-time domain feed monitoring system by integrating with the SinkingYachts API, fulfilling the requirements of issue #64.
  • WebSocket Support: Enhanced the SinkingYahtsService to establish and maintain a WebSocket connection for continuous, real-time updates on malicious domains, including automatic reconnection with exponential backoff.
  • Domain Data Management: Added capabilities for both initial bulk import of all known domains and incremental updates received via the real-time feed. Domains are processed, sanitized, and stored in the database, with their malicious status updated accordingly.
  • Admin Management Endpoints: Introduced new administrative API endpoints under /admin/sinking-yachts to manually control the feed, including starting/stopping monitoring, triggering bulk imports, and fetching recent domains.
  • Auto-Start Configuration: Integrated an auto-start mechanism for the SinkingYachts feed monitoring service upon server initialization, configurable via environment variables (ENABLE_SINKING_YACHTS_FEED and SKIP_BULK_IMPORT).
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements real-time domain feed monitoring from the SinkingYachts API, adding WebSocket-based live updates alongside existing batch processing capabilities. The implementation includes automatic startup configuration, admin management endpoints, and robust error handling with reconnection logic.

Key changes:

  • Enhanced SinkingYachtsService with WebSocket real-time monitoring and bulk import capabilities
  • Added admin API endpoints for manual feed control and monitoring
  • Integrated automatic feed startup with configurable environment variables

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/services/SinkingYachts.ts Core implementation of WebSocket feed monitoring, bulk import, and domain processing logic
src/index.ts Auto-start configuration for feed monitoring with environment variable controls
src/admin-routes/routes/sinking-yachts.ts Admin endpoints for feed management (start/stop/bulk-import/recent)
src/admin-routes/router.ts Registration of new SinkingYachts admin routes
package.json Added WebSocket dependencies (ws and @types/ws)

Comment on lines +27 to +31
} catch (error: any) {
return res.status(500).json({
success: false,
message: "Failed to start SinkingYachts feed monitoring",
error: error.message
Copy link
Preview

Copilot AI Jul 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using 'any' type for error handling reduces type safety. Consider using 'Error | unknown' or create a specific error type for better type checking.

Suggested change
} catch (error: any) {
return res.status(500).json({
success: false,
message: "Failed to start SinkingYachts feed monitoring",
error: error.message
} catch (error: unknown) {
const errorMessage = error instanceof Error ? error.message : "An unknown error occurred";
return res.status(500).json({
success: false,
message: "Failed to start SinkingYachts feed monitoring",
error: errorMessage

Copilot uses AI. Check for mistakes.

Comment on lines +89 to +94
const response = await axios.get<SinkingYachtsDomain[]>(
`https://phish.sinking.yachts/v2/recent/${since}`,
{
headers: headersWithSinkingYahts,
}
);

Check failure

Code scanning / CodeQL

Server-side request forgery Critical

The
URL
of this request depends on a
user-provided value
.
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds an integration with the SinkingYachts real-time domain feed. It includes WebSocket support for live updates, REST API endpoints for bulk import and feed control, and automatic reconnection logic. There's a high-severity bug in the bulk import logic that prevents existing domains from being updated, and another in the WebSocket reconnection logic that could cause the feed to restart unexpectedly after being stopped. I've also suggested some improvements for safer error handling and code clarity.

try {
info("SinkingYachts: Starting bulk import of all domains...");
const allDomains = await this.getAllDomains();
await this.processDomainData(allDomains, false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The processDomainData method is called with isUpdate = false. This means if a domain from the bulk import already exists in the database, it will not be updated to malicious: true. This can lead to data inconsistency, as the database will not reflect the latest information from the SinkingYachts feed. Pass true to ensure existing domain records are correctly updated during the bulk import.

Suggested change
await this.processDomainData(allDomains, false);
await this.processDomainData(allDomains, true);

Comment on lines +27 to +32
} catch (error: any) {
return res.status(500).json({
success: false,
message: "Failed to start SinkingYachts feed monitoring",
error: error.message
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error object in a catch block is of type unknown in modern TypeScript. Accessing error.message directly is unsafe as error might not be an Error instance. This can lead to runtime errors. It's safer to check if error is an instance of Error before accessing its message property.

    return res.status(500).json({
      success: false,
      message: "Failed to start SinkingYachts feed monitoring",
      error: error instanceof Error ? error.message : String(error)
    });

* @security BearerAuth
* @return {object} 200 - Success response
*/
router.post("/stop-feed", async (req: Request, res: Response) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The stopRealtimeFeed method is synchronous and does not return a promise. Using async for this route handler is unnecessary and can be slightly misleading. Removing it improves code clarity.

router.post("/stop-feed", (req: Request, res: Response) => {

logger.info("SinkingYachts feed monitoring started successfully");
})
.catch((error) => {
logger.error(`Failed to start SinkingYachts feed monitoring: ${error.message}`);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error object from a promise rejection can be of any type. Accessing error.message directly is unsafe and could lead to a runtime error if the rejected value is not an Error instance. It's best practice to check the type of the error before accessing properties on it.

Suggested change
logger.error(`Failed to start SinkingYachts feed monitoring: ${error.message}`);
logger.error(`Failed to start SinkingYachts feed monitoring: ${error instanceof Error ? error.message : String(error)}`);

Comment on lines +77 to +80
} catch (err: any) {
error(`SinkingYachts: Failed to fetch all domains: ${err.message}`);
throw err;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using err: any suppresses type checking for the caught error. It's safer to let TypeScript infer the type as unknown and then perform a type check before accessing properties like message. This prevents potential runtime errors if a non-Error object is thrown.

Suggested change
} catch (err: any) {
error(`SinkingYachts: Failed to fetch all domains: ${err.message}`);
throw err;
}
} catch (err) {
error(`SinkingYachts: Failed to fetch all domains: ${err instanceof Error ? err.message : String(err)}`);
throw err;
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (2)
src/index.ts (1)

32-34: Consider consistent environment variable handling

The pattern for checking environment variables differs between ENABLE_SINKING_YACHTS_FEED (checking for "false") and SKIP_BULK_IMPORT (checking for "true"). Consider using a consistent approach.

-  if (process.env.ENABLE_SINKING_YACHTS_FEED !== "false") {
+  const enableFeed = process.env.ENABLE_SINKING_YACHTS_FEED?.toLowerCase() !== "false";
+  const skipBulkImport = process.env.SKIP_BULK_IMPORT?.toLowerCase() === "true";
+  
+  if (enableFeed) {
     logger.info("Starting SinkingYachts realtime feed monitoring...");
-    sinkingYahtsService.startFeedMonitoring(process.env.SKIP_BULK_IMPORT === "true")
+    sinkingYahtsService.startFeedMonitoring(skipBulkImport)
src/services/SinkingYahts.ts (1)

107-141: Consider batch processing for improved performance

Processing domains sequentially could be slow for large datasets. Consider implementing batch processing for better performance.

The current implementation processes domains one by one, which could be inefficient for bulk imports. Would you like me to help implement a batch processing approach using database transactions and parallel processing?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 10b7549 and a453805.

📒 Files selected for processing (5)
  • package.json (2 hunks)
  • src/admin-routes/router.ts (2 hunks)
  • src/admin-routes/routes/sinking-yachts.ts (1 hunks)
  • src/index.ts (2 hunks)
  • src/services/SinkingYahts.ts (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/services/SinkingYahts.ts (7)
src/utils/axios.ts (1)
  • axios (27-27)
src/defs/headers.ts (1)
  • headersWithSinkingYahts (27-30)
src/utils/logger.ts (3)
  • info (61-61)
  • error (63-63)
  • warn (62-62)
src/utils/sanitizeDomain.ts (1)
  • sanitizeDomain (49-49)
src/func/db/domain.ts (1)
  • getDbDomain (7-15)
src/utils/db.ts (1)
  • db (5-8)
src/db/schema.ts (2)
  • domains (151-166)
  • rawAPIData (200-219)
🪛 GitHub Check: CodeQL
src/services/SinkingYahts.ts

[failure] 89-94: Server-side request forgery
The URL of this request depends on a user-provided value.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Codacy Security Scan
  • GitHub Check: claude-review
🔇 Additional comments (3)
src/admin-routes/router.ts (1)

17-17: LGTM!

The router integration follows the established pattern and is properly protected by authentication and admin permission checks.

Also applies to: 301-301

src/admin-routes/routes/sinking-yachts.ts (1)

1-123: Well-structured admin routes implementation

The admin routes are properly implemented with:

  • Comprehensive error handling
  • Clear API documentation
  • Input validation for the /recent endpoint
  • Consistent response structure
src/services/SinkingYahts.ts (1)

203-226: Well-implemented WebSocket message handler

The message handling logic is robust with proper sanitization, action dispatching, and error handling.

"response-time": "^2.3.3",
"ts-node": "^10.9.2"
"ts-node": "^10.9.2",
"ws": "^8.18.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify WebSocket library versions and security status

Please ensure these versions are the latest stable releases and free from known vulnerabilities.

Also applies to: 64-64


🏁 Script executed:

#!/bin/bash
# Description: Check for latest versions and security advisories for ws and @types/ws packages

# Check latest versions on npm
echo "Checking latest versions..."
curl -s https://registry.npmjs.org/ws | jq -r '.["dist-tags"].latest'
curl -s https://registry.npmjs.org/@types/ws | jq -r '.["dist-tags"].latest'

# Check for security advisories using GitHub Advisory Database
echo -e "\nChecking security advisories for ws..."
gh api graphql -f query='
{
  securityVulnerabilities(first: 5, ecosystem: NPM, package: "ws") {
    nodes {
      advisory {
        summary
        severity
        publishedAt
      }
      vulnerableVersionRange
      firstPatchedVersion {
        identifier
      }
    }
  }
}'

Length of output: 1813


Update ws and @types/ws to the latest stable releases

The ws package at ^8.18.0 already includes the DoS fix patched in 8.17.1, so it’s no longer vulnerable, but the current latest is 8.18.3. Similarly, @types/ws is at 8.5.13 while the latest is 8.18.1. For best compatibility and to pick up any minor fixes:

• package.json (line 51): update

-  "ws": "^8.18.0"
+  "ws": "^8.18.3"

• package.json (line 64): update

-  "@types/ws": "^8.5.13"
+  "@types/ws": "^8.18.1"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"ws": "^8.18.0"
"dependencies": {
- "ws": "^8.18.0"
+ "ws": "^8.18.3"
},
"devDependencies": {
- "@types/ws": "^8.5.13"
+ "@types/ws": "^8.18.1"
}
🤖 Prompt for AI Agents
In package.json at lines 51 and 64, update the versions of "ws" and "@types/ws"
dependencies to their latest stable releases, specifically change "ws" from
"^8.18.0" to "^8.18.3" and "@types/ws" from "8.5.13" to "8.18.1" to ensure best
compatibility and include minor fixes.

Comment on lines +87 to +102
async getRecentDomains(since: string): Promise<SinkingYachtsDomain[]> {
try {
const response = await axios.get<SinkingYachtsDomain[]>(
`https://phish.sinking.yachts/v2/recent/${since}`,
{
headers: headersWithSinkingYahts,
}
);

info(`SinkingYachts: Fetched ${response.data.length} recent domains since ${since}`);
return response.data;
} catch (err: any) {
error(`SinkingYachts: Failed to fetch recent domains: ${err.message}`);
throw err;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation for the date parameter

While the SSRF warning is a false positive (hardcoded base URL), it's good practice to validate the date format before using it in the URL path.

 async getRecentDomains(since: string): Promise<SinkingYachtsDomain[]> {
   try {
+    // Validate ISO date format
+    const dateRegex = /^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}:\d{2}(\.\d{3})?Z?)?$/;
+    if (!dateRegex.test(since)) {
+      throw new Error(`Invalid date format: ${since}. Expected ISO date string.`);
+    }
+    
     const response = await axios.get<SinkingYachtsDomain[]>(
       `https://phish.sinking.yachts/v2/recent/${since}`,
       {
         headers: headersWithSinkingYahts,
       }
     );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async getRecentDomains(since: string): Promise<SinkingYachtsDomain[]> {
try {
const response = await axios.get<SinkingYachtsDomain[]>(
`https://phish.sinking.yachts/v2/recent/${since}`,
{
headers: headersWithSinkingYahts,
}
);
info(`SinkingYachts: Fetched ${response.data.length} recent domains since ${since}`);
return response.data;
} catch (err: any) {
error(`SinkingYachts: Failed to fetch recent domains: ${err.message}`);
throw err;
}
}
async getRecentDomains(since: string): Promise<SinkingYachtsDomain[]> {
try {
// Validate ISO date format
const dateRegex = /^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}:\d{2}(\.\d{3})?Z?)?$/;
if (!dateRegex.test(since)) {
throw new Error(`Invalid date format: ${since}. Expected ISO date string.`);
}
const response = await axios.get<SinkingYachtsDomain[]>(
`https://phish.sinking.yachts/v2/recent/${since}`,
{
headers: headersWithSinkingYahts,
}
);
info(`SinkingYachts: Fetched ${response.data.length} recent domains since ${since}`);
return response.data;
} catch (err: any) {
error(`SinkingYachts: Failed to fetch recent domains: ${err.message}`);
throw err;
}
}
🧰 Tools
🪛 GitHub Check: CodeQL

[failure] 89-94: Server-side request forgery
The URL of this request depends on a user-provided value.

🤖 Prompt for AI Agents
In src/services/SinkingYahts.ts around lines 87 to 102, the method
getRecentDomains uses the since parameter directly in the URL without validating
its format. To fix this, add input validation to ensure the since parameter is a
valid date string (e.g., using a regex or Date parsing) before making the axios
request. If the validation fails, throw an error or handle it appropriately to
prevent malformed requests.

Comment on lines +161 to +166
startRealtimeFeed(): void {
if (this.wsConnection && this.wsConnection.readyState === WebSocket.OPEN) {
warn("SinkingYachts: WebSocket connection already active");
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve WebSocket connection state handling

The current check only verifies OPEN state. Consider handling all connection states to prevent potential multiple connections.

 startRealtimeFeed(): void {
-  if (this.wsConnection && this.wsConnection.readyState === WebSocket.OPEN) {
-    warn("SinkingYachts: WebSocket connection already active");
+  if (this.wsConnection) {
+    const state = this.wsConnection.readyState;
+    if (state === WebSocket.CONNECTING) {
+      warn("SinkingYachts: WebSocket connection is still connecting");
+      return;
+    } else if (state === WebSocket.OPEN) {
+      warn("SinkingYachts: WebSocket connection already active");
+      return;
+    } else if (state === WebSocket.CLOSING) {
+      warn("SinkingYachts: WebSocket connection is closing, please wait");
+      return;
+    }
+    // CLOSED state - proceed with new connection
+  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
startRealtimeFeed(): void {
if (this.wsConnection && this.wsConnection.readyState === WebSocket.OPEN) {
warn("SinkingYachts: WebSocket connection already active");
return;
}
startRealtimeFeed(): void {
if (this.wsConnection) {
const state = this.wsConnection.readyState;
if (state === WebSocket.CONNECTING) {
warn("SinkingYachts: WebSocket connection is still connecting");
return;
} else if (state === WebSocket.OPEN) {
warn("SinkingYachts: WebSocket connection already active");
return;
} else if (state === WebSocket.CLOSING) {
warn("SinkingYachts: WebSocket connection is closing, please wait");
return;
}
// CLOSED state - proceed with new connection
}
// ... existing code to establish a new WebSocket connection ...
🤖 Prompt for AI Agents
In src/services/SinkingYahts.ts around lines 161 to 166, the WebSocket
connection check only verifies if the connection is OPEN, which may allow
multiple connections in other states. Update the condition to handle all
relevant WebSocket states (CONNECTING, OPEN, CLOSING) to prevent starting a new
connection when one is already active or in the process of closing.

Comment on lines +311 to +325
private attemptReconnect(): void {
if (this.reconnectAttempts >= this.maxReconnectAttempts) {
error("SinkingYachts: Max reconnection attempts reached. Giving up.");
return;
}

this.reconnectAttempts++;
warn(`SinkingYachts: Attempting to reconnect WebSocket (${this.reconnectAttempts}/${this.maxReconnectAttempts}) in ${this.reconnectDelay}ms...`);

setTimeout(() => {
this.startRealtimeFeed();
}, this.reconnectDelay);

this.reconnectDelay *= 2; // Exponential backoff
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve reconnection logic with delay cap and reset

The exponential backoff should have a maximum delay cap, and the delay should be reset after successful reconnection.

 private attemptReconnect(): void {
   if (this.reconnectAttempts >= this.maxReconnectAttempts) {
     error("SinkingYachts: Max reconnection attempts reached. Giving up.");
+    this.reconnectDelay = 5000; // Reset for future attempts
     return;
   }

   this.reconnectAttempts++;
   warn(`SinkingYachts: Attempting to reconnect WebSocket (${this.reconnectAttempts}/${this.maxReconnectAttempts}) in ${this.reconnectDelay}ms...`);

   setTimeout(() => {
     this.startRealtimeFeed();
   }, this.reconnectDelay);

-  this.reconnectDelay *= 2; // Exponential backoff
+  // Exponential backoff with maximum delay of 60 seconds
+  this.reconnectDelay = Math.min(this.reconnectDelay * 2, 60000);
 }

Also update the startRealtimeFeed method to reset the delay on successful connection:

 this.wsConnection.on("open", () => {
   info("SinkingYachts: WebSocket connection established");
   this.reconnectAttempts = 0;
+  this.reconnectDelay = 5000; // Reset delay for future reconnections
 });

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/services/SinkingYahts.ts around lines 311 to 325, the exponential backoff
for reconnection delay lacks a maximum cap and does not reset after a successful
connection. Add a maximum delay cap constant and ensure the reconnectDelay does
not exceed this cap when doubling. Modify the startRealtimeFeed method to reset
reconnectDelay to its initial value upon a successful connection to prepare for
future reconnection attempts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant