fix: add graceful error handling for VirusTotal invalid domains#111
Conversation
- Add null checks in parseData.ts to prevent crashes when VirusTotal returns null - Add domain validation before VirusTotal API calls using existing validateDomain utility - Enable proper error logging with informative warning messages - Apply consistent domain validation to both check and report methods Fixes #37 - prevents app crashes when domains are invalid or unresolvable Co-authored-by: Jasper Mayone <jaspermayone@users.noreply.github.com>
|
Claude finished @jaspermayone's task —— View job Code Review for VirusTotal Error Handling PRTodo List:
📋 Review SummaryOverall, this is a solid defensive programming improvement that addresses crash-prone scenarios with VirusTotal API integration. The changes are well-structured and follow security best practices. ✅ Code Quality & Best PracticesExcellent improvements:
Code style observations:
🐛 Potential IssuesMinor concerns:
⚡ Performance ConsiderationsPositive impacts:
Suggestions:
🔒 Security AssessmentSecurity improvements:
Security best practices followed:
🧪 Test Coverage RecommendationsThe PR mentions a test plan but I don't see automated tests. Consider adding:
🎯 Final Recommendations
🏆 Overall AssessmentThis PR successfully addresses the crash issues mentioned in #37. The implementation is defensive, secure, and maintains backward compatibility. The changes follow established patterns in the codebase and improve system reliability. Approval Status: ✅ Recommended - This is a solid improvement that makes the system more robust without introducing new risks. |
|
@claude do a dns lookup as part of the domain validation |
WalkthroughThe changes add defensive checks and domain validation before making VirusTotal API calls. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant VirusTotalService
participant Logger
Client->>VirusTotalService: check(domain)
VirusTotalService->>VirusTotalService: sanitizeDomain(domain)
VirusTotalService->>VirusTotalService: validateDomain(domain)
alt Domain invalid
VirusTotalService->>Logger: log warning
VirusTotalService-->>Client: return null
else Domain valid
VirusTotalService->>VirusTotalService: call VirusTotal API
VirusTotalService-->>Client: return result
end
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 significantly enhances the stability and error handling of the VirusTotal integration. It introduces proactive domain validation to prevent unnecessary API calls for invalid inputs and adds defensive programming with null checks to avoid crashes when processing potentially incomplete data from the VirusTotal API. The changes ensure more graceful degradation and clearer logging for problematic scenarios.
Highlights
- Improved Robustness: Added null and undefined checks within
parseData.tsto gracefully handle cases where VirusTotal data might be incomplete or missing, preventing potential crashes when accessing nested properties likelast_analysis_stats. - Pre-API Call Domain Validation: Implemented domain validation using the existing
validateDomainutility in theVirusTotalService'scheckandreportmethods. This ensures that API calls to VirusTotal are only made for valid domains, skipping calls and logging warnings for malformed inputs. - Graceful Error Handling & Logging: Enhanced error handling in
VirusTotalServiceto log informative warning messages for invalid domains or API errors, rather than throwing exceptions or silently failing. This provides better transparency and prevents the application from crashing due to external service issues or bad input. - Consistent Application of Validation: Applied the new domain validation logic consistently across both the
checkandreportfunctionalities within theVirusTotalServicefor uniform behavior.
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
-
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. ↩
|
Claude finished @jaspermayone's task —— View job Enhanced Domain Validation with DNS Lookup ✅I've successfully enhanced the domain validation to include DNS lookup functionality as requested. Changes Made:1. Enhanced Domain Validation (
2. Updated VirusTotal Service (
3. Code Quality Improvements:
How It Works:When a domain is validated, it now:
This prevents unnecessary API calls and provides better error handling for invalid/non-existent domains. The enhancement has been committed to the PR branch at commit |
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the robustness and error handling of the VirusTotal integration. The addition of explicit null checks in parseData.ts prevents potential crashes from incomplete API responses, and the proactive domain validation in VirusTotal.ts for both check and report methods is a crucial improvement, preventing unnecessary external API calls for invalid inputs. The refined error logging also contributes to better transparency and debugging.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // FIXME: this is terrible practice, handle ratelimits better after issue addressed. | ||
| // console.error(`VirusTotal API error for domain ${domain}:`, error); | ||
| // Log the error for transparency, but don't throw to prevent crashes | ||
| console.warn(`VirusTotal API error for domain "${domain}":`, error instanceof Error ? error.message : error); |
Check failure
Code scanning / CodeQL
Use of externally-controlled format string High
This autofix suggestion was applied.
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 months ago
To fix the issue, the format string in the console.warn statement on line 54 of src/services/VirusTotal.ts should be updated to use the %s specifier for the domain parameter. This ensures that the domain is treated as a string and prevents any format specifiers in the user-controlled input from being interpreted.
Steps:
- Replace the format string
`VirusTotal API error for domain "${domain}":`with"VirusTotal API error for domain '%s':". - Pass the
domainparameter as an argument to the format string.
No additional imports or dependencies are required for this fix.
| @@ -53,3 +53,3 @@ | ||
| // Log the error for transparency, but don't throw to prevent crashes | ||
| console.warn(`VirusTotal API error for domain "${domain}":`, error instanceof Error ? error.message : error); | ||
| console.warn("VirusTotal API error for domain '%s':", domain, error instanceof Error ? error.message : error); | ||
| return null; |
jaspermayone
committed this autofix suggestion 11 months ago.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances error resilience by validating and sanitizing domains before VirusTotal API calls and preventing null-related crashes in data parsing.
- Add and apply
validateDomainwith graceful skips and warnings for invalid domains in bothcheckandreportmethods - Switch
sanitizeDomaincalls from async to sync (removedawait) - Add null checks in
parseData.tsto guard against missing VirusTotal data
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/services/VirusTotal.ts | Introduced validateDomain checks, adjusted sanitizeDomain usage, and improved error logging |
| src/func/domain/parseData.ts | Added nested null checks around virusTotalData to prevent crashes |
Comments suppressed due to low confidence (2)
src/services/VirusTotal.ts:22
sanitizeDomainpreviously returned a promise (used withawait). RemovingawaitmeanssanitizedDomainis now a Promise instead of a string. Add backawaitbeforesanitizeDomain(domain).
const sanitizedDomain = sanitizeDomain(domain);
src/services/VirusTotal.ts:67
- Same issue here:
sanitizeDomainreturns a promise and needsawaitto produce a string. Please restoreawait sanitizeDomain(domain).
const sanitizedDomain = sanitizeDomain(domain);
| virusTotalData && | ||
| virusTotalData.data && | ||
| virusTotalData.data.attributes && | ||
| virusTotalData.data.attributes.last_analysis_stats && | ||
| (virusTotalData.data.attributes.last_analysis_stats.malicious > 0 || | ||
| virusTotalData.data.attributes.last_analysis_stats.suspicious > 0) |
There was a problem hiding this comment.
[nitpick] This long chain of null checks can be simplified using optional chaining (e.g., virusTotalData?.data?.attributes?.last_analysis_stats?.malicious > 0).
| virusTotalData && | |
| virusTotalData.data && | |
| virusTotalData.data.attributes && | |
| virusTotalData.data.attributes.last_analysis_stats && | |
| (virusTotalData.data.attributes.last_analysis_stats.malicious > 0 || | |
| virusTotalData.data.attributes.last_analysis_stats.suspicious > 0) | |
| virusTotalData?.data?.attributes?.last_analysis_stats?.malicious > 0 || | |
| virusTotalData?.data?.attributes?.last_analysis_stats?.suspicious > 0 |
…rolled format string Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Jasper Mayone <me@jaspermayone.com>
|
Claude finished @jaspermayone's task —— View job Code Review for VirusTotal Error Handling PRTodo List:
📋 Review SummaryOverall, this is a solid defensive programming improvement that addresses crash-prone scenarios with VirusTotal API integration. The changes are well-structured and follow security best practices. ✅ Code Quality & Best PracticesExcellent improvements:
Code style observations:
🐛 Potential IssuesMinor concerns:
⚡ Performance ConsiderationsPositive impacts:
Suggestions:
🔒 Security AssessmentSecurity improvements:
Security best practices followed:
🧪 Test Coverage RecommendationsThe PR mentions a test plan but I don't see automated tests. Consider adding:
🎯 Final Recommendations
🏆 Overall AssessmentThis PR successfully addresses the crash issues mentioned in #37. The implementation is defensive, secure, and maintains backward compatibility. The changes follow established patterns in the codebase and improve system reliability. Approval Status: ✅ Recommended - This is a solid improvement that makes the system more robust without introducing new risks. |
![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
🔭 Outside diff range comments (1)
src/services/VirusTotal.ts (1)
110-120: Use/votesendpoint instead of/commentsfor vote submissionsThe VirusTotal API v3 requires a POST to
/domains/{domain}/voteswhen casting a vote, not/comments. Update the second axios call accordingly:• File:
src/services/VirusTotal.ts
Lines: ~110–120- axios.post( - `https://www.virustotal.com/api/v3/domains/${sanitizedDomain}/comments`, - voteData, - { headers: headersWithVirusTotal } - ) + axios.post( + `https://www.virustotal.com/api/v3/domains/${sanitizedDomain}/votes`, + voteData, + { headers: headersWithVirusTotal } + )Reference: https://docs.virustotal.com/reference/domain-votes-post
🧹 Nitpick comments (1)
src/func/domain/parseData.ts (1)
50-55: Simplify with optional chaining for better readability.The defensive null checks are excellent for preventing runtime errors when VirusTotal returns incomplete data. However, this can be simplified using optional chaining for better readability.
Apply this diff to use optional chaining:
- } else if ( - virusTotalData && - virusTotalData.data && - virusTotalData.data.attributes && - virusTotalData.data.attributes.last_analysis_stats && - (virusTotalData.data.attributes.last_analysis_stats.malicious > 0 || - virusTotalData.data.attributes.last_analysis_stats.suspicious > 0) - ) { + } else if ( + (virusTotalData?.data?.attributes?.last_analysis_stats?.malicious > 0 || + virusTotalData?.data?.attributes?.last_analysis_stats?.suspicious > 0) + ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/func/domain/parseData.ts(1 hunks)src/services/VirusTotal.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/services/VirusTotal.ts (1)
src/utils/sanitizeDomain.ts (3)
sanitizeDomain(40-40)validateDomain(40-40)DomainValidationError(40-40)
🪛 Biome (1.9.4)
src/func/domain/parseData.ts
[error] 50-53: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: CodeQL
src/services/VirusTotal.ts
[failure] 54-54: Use of externally-controlled format string
Format string depends on a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: claude-review
- GitHub Check: Codacy Security Scan
🔇 Additional comments (3)
src/services/VirusTotal.ts (3)
6-6: LGTM: Clean import of domain validation utilities.The import properly brings in the necessary utilities for domain sanitization and validation with appropriate error handling.
22-33: Excellent domain validation implementation with proper error handling.The validation logic correctly sanitizes domains first, then validates them before making API calls. The graceful handling of
DomainValidationErrorby logging warnings and returningnullprevents unnecessary API calls for invalid domains.
67-78: Consistent domain validation implementation.Good implementation that mirrors the validation logic from the
checkmethod, ensuring consistent behavior across both API methods.
- Add DNS lookup to validateDomain using Node.js dns.promises.resolve() - Make validateDomain async to handle DNS resolution properly - Update VirusTotal service to await async domain validation - Fix incorrect await usage with synchronous sanitizeDomain across services - Improve error handling for DNS resolution failures - Add informative warnings when domains don't resolve This enhancement prevents unnecessary API calls for non-existent domains and provides better validation by checking if domains actually exist. Co-authored-by: Jasper Mayone <jaspermayone@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
src/services/IpQualityScore.ts (1)
42-46: Fix duplicate database insert.The same data is being inserted twice into the database, which appears to be unintentional and could cause data duplication issues.
- await db.insert(rawAPIData).values({ - sourceAPI: "IpQualityScore", - domain: dbDomain!.id!, - data: data, - });
♻️ Duplicate comments (1)
src/services/VirusTotal.ts (1)
54-54: Fix format string vulnerability in logging.Despite past review comments indicating this was addressed, the static analysis tool correctly identifies a format string vulnerability. The domain variable is embedded directly in the template string, which could allow format string attacks.
- console.warn(`VirusTotal API error for domain "${domain}":`, error instanceof Error ? error.message : error); + console.warn(`VirusTotal API error for domain:`, { domain, error: error instanceof Error ? error.message : error });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/services/AbuseCh.ts(1 hunks)src/services/GoogleSafebrowsing.ts(1 hunks)src/services/IpQualityScore.ts(1 hunks)src/services/PhishReport.ts(1 hunks)src/services/SecurityTrails.ts(1 hunks)src/services/SinkingYahts.ts(1 hunks)src/services/UrlScan.ts(1 hunks)src/services/VirusTotal.ts(4 hunks)src/services/Walshy.ts(2 hunks)src/utils/sanitizeDomain.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
src/services/AbuseCh.ts (1)
src/utils/sanitizeDomain.ts (1)
sanitizeDomain(49-49)
src/services/Walshy.ts (1)
src/utils/sanitizeDomain.ts (1)
sanitizeDomain(49-49)
src/services/UrlScan.ts (1)
src/utils/sanitizeDomain.ts (1)
sanitizeDomain(49-49)
src/services/SinkingYahts.ts (1)
src/utils/sanitizeDomain.ts (1)
sanitizeDomain(49-49)
src/services/SecurityTrails.ts (1)
src/utils/sanitizeDomain.ts (1)
sanitizeDomain(49-49)
src/services/PhishReport.ts (1)
src/utils/sanitizeDomain.ts (1)
sanitizeDomain(49-49)
src/services/VirusTotal.ts (1)
src/utils/sanitizeDomain.ts (3)
sanitizeDomain(49-49)validateDomain(49-49)DomainValidationError(49-49)
🪛 GitHub Check: CodeQL
src/services/VirusTotal.ts
[failure] 54-54: Use of externally-controlled format string
Format string depends on a user-provided value.
🔇 Additional comments (9)
src/utils/sanitizeDomain.ts (2)
1-1: DNS import looks good.The import of the DNS promises API is appropriate for the new DNS resolution functionality.
12-12: Function signature change correctly reflects the new async behavior.The change from synchronous to asynchronous is necessary for the DNS resolution functionality.
src/services/SinkingYahts.ts (1)
21-21: sanitizeDomain is synchronous—removal of await is correct
- In src/utils/sanitizeDomain.ts (line 44),
sanitizeDomainis defined as
const sanitizeDomain = (domain: string): string => { … },
confirming it returns a string synchronously.- All service calls now correctly invoke it without
await.No further changes required; approving.
src/services/SecurityTrails.ts (1)
22-22: LGTM - consistent with synchronous sanitizeDomain refactor.This change aligns with the coordinated refactor to make
sanitizeDomainsynchronous across all service files.src/services/IpQualityScore.ts (1)
23-23: LGTM - consistent with synchronous sanitizeDomain refactor.This change aligns with the coordinated refactor to make
sanitizeDomainsynchronous across all service files.src/services/GoogleSafebrowsing.ts (1)
22-22: LGTM - consistent with synchronous sanitizeDomain refactor.This change aligns with the coordinated refactor to make
sanitizeDomainsynchronous across all service files.src/services/VirusTotal.ts (3)
6-6: LGTM - proper imports for domain validation.The imports correctly bring in the necessary validation utilities to implement graceful error handling for invalid domains.
22-33: Excellent domain validation implementation.The synchronous sanitization followed by asynchronous validation with proper error handling is well-implemented. Invalid domains are logged and gracefully skipped rather than causing crashes.
67-78: Consistent domain validation in report method.The validation implementation in the report method follows the same pattern as the check method, providing consistent error handling across both methods.
| // Perform DNS lookup to verify domain exists | ||
| try { | ||
| await dns.resolve(domain); | ||
| } catch (error) { | ||
| throw new DomainValidationError(`Domain does not resolve: ${domain}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding timeout and improving error handling for DNS resolution.
The DNS resolution logic could be more robust:
- DNS lookups can fail due to temporary network issues or DNS server problems
- No timeout is specified, which could lead to long delays
- Consider whether DNS validation should be mandatory or optional
// Perform DNS lookup to verify domain exists
try {
- await dns.resolve(domain);
+ await Promise.race([
+ dns.resolve(domain),
+ new Promise((_, reject) =>
+ setTimeout(() => reject(new Error('DNS timeout')), 5000)
+ )
+ ]);
} catch (error) {
- throw new DomainValidationError(`Domain does not resolve: ${domain}`);
+ throw new DomainValidationError(`Domain does not resolve or DNS timeout: ${domain}`);
}Let me verify how this validation is being used across the codebase:
🏁 Script executed:
#!/bin/bash
# Check how validateDomain is being used across the codebase
echo "=== validateDomain usage ==="
rg -A 3 "validateDomain" --type ts
echo "=== DomainValidationError handling ==="
rg -A 5 "DomainValidationError" --type tsLength of output: 4661
Add DNS lookup timeout and enhance error handling in sanitizeDomain.ts
To fail fast on hanging DNS queries and ensure downstream code handles timeouts correctly, update the DNS resolution block in src/utils/sanitizeDomain.ts as shown below. We’ve verified that validateDomain and its DomainValidationError are only used in src/services/VirusTotal.ts, where errors are already caught and logged.
src/utils/sanitizeDomain.ts
// Perform DNS lookup to verify domain exists
try {
- await dns.resolve(domain);
+ await Promise.race([
+ dns.resolve(domain),
+ new Promise<never>((_, reject) =>
+ setTimeout(() => reject(new Error('DNS timeout')), 5000)
+ )
+ ]);
} catch (error) {
- throw new DomainValidationError(`Domain does not resolve: ${domain}`);
+ throw new DomainValidationError(
+ `Domain does not resolve or DNS timeout: ${domain}`
+ );
}
return true;No additional changes are needed—every invocation of validateDomain already catches DomainValidationError and skips invalid domains.
📝 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.
| // Perform DNS lookup to verify domain exists | |
| try { | |
| await dns.resolve(domain); | |
| } catch (error) { | |
| throw new DomainValidationError(`Domain does not resolve: ${domain}`); | |
| } | |
| // Perform DNS lookup to verify domain exists | |
| try { | |
| await Promise.race([ | |
| dns.resolve(domain), | |
| new Promise<never>((_, reject) => | |
| setTimeout(() => reject(new Error('DNS timeout')), 5000) | |
| ) | |
| ]); | |
| } catch (error) { | |
| throw new DomainValidationError( | |
| `Domain does not resolve or DNS timeout: ${domain}` | |
| ); | |
| } | |
| return true; |
🤖 Prompt for AI Agents
In src/utils/sanitizeDomain.ts around lines 34 to 40, the DNS lookup currently
lacks a timeout and has basic error handling. Modify the DNS resolution to
include a timeout mechanism that rejects the lookup if it takes too long,
ensuring the function fails fast on hanging queries. Also, enhance the error
handling to differentiate timeout errors from other DNS errors, throwing a
DomainValidationError with an appropriate message in both cases.
| ); | ||
|
|
||
| let sanitizedDomain = await sanitizeDomain(domain); | ||
| let sanitizedDomain = sanitizeDomain(domain); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct removal of await, but consider adding domain validation.
The removal of await is correct since sanitizeDomain is synchronous. However, unlike the VirusTotal service, this doesn't validate domains before making API calls.
Consider adding domain validation to prevent unnecessary API calls with invalid domains:
+import { validateDomain, DomainValidationError } from "src/utils/sanitizeDomain";
check: async (domain: string) => {
// metrics.increment("services.abusech.domain.check");
+ let sanitizedDomain = sanitizeDomain(domain);
+
+ try {
+ await validateDomain(sanitizedDomain);
+ } catch (error) {
+ if (error instanceof DomainValidationError) {
+ console.warn(`Skipping AbuseCh check for invalid domain: ${sanitizedDomain}`, error.message);
+ return { error: "Invalid domain" };
+ }
+ throw error;
+ }
let response = await axios.get(
`https://urlhaus.abuse.ch/api/v1/host/${domain}`,📝 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.
| let sanitizedDomain = sanitizeDomain(domain); | |
| // at top of src/services/AbuseCh.ts, alongside your other imports | |
| import { validateDomain, DomainValidationError } from "src/utils/sanitizeDomain"; | |
| export const AbuseChService = { | |
| check: async (domain: string) => { | |
| // metrics.increment("services.abusech.domain.check"); | |
| let sanitizedDomain = sanitizeDomain(domain); | |
| try { | |
| await validateDomain(sanitizedDomain); | |
| } catch (error) { | |
| if (error instanceof DomainValidationError) { | |
| console.warn( | |
| `Skipping AbuseCh check for invalid domain: ${sanitizedDomain}`, | |
| error.message | |
| ); | |
| return { error: "Invalid domain" }; | |
| } | |
| throw error; | |
| } | |
| let response = await axios.get( | |
| `https://urlhaus.abuse.ch/api/v1/host/${domain}` | |
| ); | |
| // …rest of your logic… | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In src/services/AbuseCh.ts at line 29, after removing the incorrect await from
sanitizeDomain, add a domain validation step before proceeding with API calls.
Implement a function or use a utility to check if the domain string is valid,
and only continue with the API call if the domain passes validation. This
prevents unnecessary requests with invalid domains and aligns with the
VirusTotal service's approach.
| // metrics.increment("services.phishreport.domain.check"); | ||
|
|
||
| const sanitizedDomain = await sanitizeDomain(domain); | ||
| const sanitizedDomain = sanitizeDomain(domain); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct removal of await, but missing domain validation.
The removal of await is correct since sanitizeDomain is synchronous. However, this service lacks domain validation before making API calls, which could result in unnecessary requests for invalid domains.
Consider adding domain validation consistent with the VirusTotal service approach:
+import { validateDomain, DomainValidationError } from "src/utils/sanitizeDomain";
check: async (domain: string) => {
// metrics.increment("services.phishreport.domain.check");
const sanitizedDomain = sanitizeDomain(domain);
+
+ try {
+ await validateDomain(sanitizedDomain);
+ } catch (error) {
+ if (error instanceof DomainValidationError) {
+ console.warn(`Skipping PhishReport check for invalid domain: ${sanitizedDomain}`, error.message);
+ return { error: "Invalid domain" };
+ }
+ throw error;
+ }📝 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.
| const sanitizedDomain = sanitizeDomain(domain); | |
| // At the top of src/services/PhishReport.ts | |
| import { validateDomain, DomainValidationError } from "src/utils/sanitizeDomain"; | |
| export const PhishReport = { | |
| check: async (domain: string) => { | |
| // metrics.increment("services.phishreport.domain.check"); | |
| const sanitizedDomain = sanitizeDomain(domain); | |
| try { | |
| await validateDomain(sanitizedDomain); | |
| } catch (error) { | |
| if (error instanceof DomainValidationError) { | |
| console.warn( | |
| `Skipping PhishReport check for invalid domain: ${sanitizedDomain}`, | |
| error.message | |
| ); | |
| return { error: "Invalid domain" }; | |
| } | |
| throw error; | |
| } | |
| // …rest of existing logic… | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In src/services/PhishReport.ts at line 22, after sanitizing the domain, add
domain validation to ensure the domain is valid before proceeding with API
calls. Implement a validation check similar to the VirusTotal service, and if
the domain is invalid, handle it appropriately by skipping the API call or
returning an error. This prevents unnecessary requests for invalid domains.
| check: async (domain: string) => { | ||
| // metrics.increment("services.walshy.domain.check"); | ||
| const sanitizedDomain = await sanitizeDomain(domain); | ||
| const sanitizedDomain = sanitizeDomain(domain); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct removal of await, but both methods lack domain validation.
The removal of await is correct since sanitizeDomain is synchronous. However, both check and report methods should validate domains before making API calls to prevent unnecessary requests with invalid domains.
Consider adding domain validation to both methods:
+import { validateDomain, DomainValidationError } from "src/utils/sanitizeDomain";
check: async (domain: string) => {
// metrics.increment("services.walshy.domain.check");
const sanitizedDomain = sanitizeDomain(domain);
+
+ try {
+ await validateDomain(sanitizedDomain);
+ } catch (error) {
+ if (error instanceof DomainValidationError) {
+ console.warn(`Skipping Walshy check for invalid domain: ${sanitizedDomain}`, error.message);
+ return { error: "Invalid domain" };
+ }
+ throw error;
+ }
report: async (domain: string) => {
// metrics.increment("services.walshy.domain.report");
const sanitizedDomain = sanitizeDomain(domain);
+
+ try {
+ await validateDomain(sanitizedDomain);
+ } catch (error) {
+ if (error instanceof DomainValidationError) {
+ console.warn(`Skipping Walshy report for invalid domain: ${sanitizedDomain}`, error.message);
+ return { error: "Invalid domain" };
+ }
+ throw error;
+ }Also applies to: 52-52
🤖 Prompt for AI Agents
In src/services/Walshy.ts at lines 21 and 52, both the check and report methods
call sanitizeDomain without validating the domain input. Add domain validation
logic before these calls to ensure the domain is valid and prevent unnecessary
API requests. Implement a validation function or use existing validation
utilities to verify the domain format and handle invalid domains appropriately
before proceeding.
| check: async (domain: string) => { | ||
| // metrics.increment("services.urlscan.domain.check"); | ||
| const sanitizedDomain = await sanitizeDomain(domain); | ||
| const sanitizedDomain = sanitizeDomain(domain); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct removal of await, but validation is especially important for UrlScan.
The removal of await is correct since sanitizeDomain is synchronous. However, domain validation is particularly important for UrlScan since it performs actual web scanning, which is resource-intensive and should not be wasted on invalid domains.
Add domain validation before the expensive scanning operations:
+import { validateDomain, DomainValidationError } from "src/utils/sanitizeDomain";
check: async (domain: string) => {
// metrics.increment("services.urlscan.domain.check");
const sanitizedDomain = sanitizeDomain(domain);
+
+ try {
+ await validateDomain(sanitizedDomain);
+ } catch (error) {
+ if (error instanceof DomainValidationError) {
+ console.warn(`Skipping UrlScan check for invalid domain: ${sanitizedDomain}`, error.message);
+ return { error: "Invalid domain" };
+ }
+ throw error;
+ }📝 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.
| const sanitizedDomain = sanitizeDomain(domain); | |
| // at the top of the file, next to the existing import of sanitizeDomain | |
| import { sanitizeDomain } from "src/utils/sanitizeDomain"; | |
| import { validateDomain, DomainValidationError } from "src/utils/sanitizeDomain"; | |
| export const UrlScan = { | |
| check: async (domain: string) => { | |
| // metrics.increment("services.urlscan.domain.check"); | |
| const sanitizedDomain = sanitizeDomain(domain); | |
| try { | |
| await validateDomain(sanitizedDomain); | |
| } catch (error) { | |
| if (error instanceof DomainValidationError) { | |
| console.warn( | |
| `Skipping UrlScan check for invalid domain: ${sanitizedDomain}`, | |
| error.message | |
| ); | |
| return { error: "Invalid domain" }; | |
| } | |
| throw error; | |
| } | |
| // ... existing scanning logic ... | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In src/services/UrlScan.ts at line 21, after sanitizing the domain with
sanitizeDomain, add a validation step to check if the sanitized domain is valid
before proceeding with any scanning operations. Implement a domain validation
function or use an existing one to verify the domain format and reject or handle
invalid domains early to avoid unnecessary resource usage during scanning.
Summary
Test plan
Fixes #37
Generated with Claude Code
Summary by CodeRabbit