Fix: prevent crash when parsing invalid JSON in analytics handler#5906
Fix: prevent crash when parsing invalid JSON in analytics handler#5906TeamInfinixdevcom wants to merge 1 commit intoFlowiseAI:mainfrom
Conversation
Summary of ChangesHello, 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 improves the application's resilience by implementing defensive JSON parsing. By introducing a Highlights
Changelog
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![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 a safeJsonParse utility. The safeJsonParse utility has been modified to prevent a Denial of Service vulnerability by adding robust type validation and ensuring consistent handling of nullish and non-string inputs, aligning with repository idioms for nullish checks. Additionally, the implementation in google-utils.ts has been reviewed to prevent deferred runtime errors by ensuring that only valid, parsed objects are assigned to authOptions.credentials.
| else if (!googleApplicationCredentialFilePath && googleApplicationCredential) { | ||
| const parsedCredential = safeJsonParse(googleApplicationCredential) | ||
| authOptions.credentials = parsedCredential ?? googleApplicationCredential | ||
| } |
There was a problem hiding this comment.
The current implementation parsedCredential ?? googleApplicationCredential can lead to runtime errors. If safeJsonParse fails and returns null, the original (and likely malformed) googleApplicationCredential string is assigned to authOptions.credentials. The credentials property expects an object, not a string. This would replace an immediate crash with a deferred one. To fix this, we should only assign the parsed value if it's a valid object, similar to the validation added in handler.ts.
| else if (!googleApplicationCredentialFilePath && googleApplicationCredential) { | |
| const parsedCredential = safeJsonParse(googleApplicationCredential) | |
| authOptions.credentials = parsedCredential ?? googleApplicationCredential | |
| } | |
| else if (!googleApplicationCredentialFilePath && googleApplicationCredential) { | |
| const parsedCredential = safeJsonParse(googleApplicationCredential); | |
| if (parsedCredential && typeof parsedCredential === 'object' && !Array.isArray(parsedCredential)) { | |
| authOptions.credentials = parsedCredential; | |
| } | |
| } |
| export function safeJsonParse(input: string) { | ||
| if (!input || input.trim() === '') return null |
There was a problem hiding this comment.
The safeJsonParse function is vulnerable to a Denial of Service (DoS) attack because it does not validate the type of the input parameter before calling the .trim() method. If the function is called with a non-string value (such as an object, array, number, or boolean), it will throw a TypeError. This can lead to a server crash if the input is derived from untrusted or malformed configuration data, such as the options.analytic or googleApplicationCredential parameters used in this pull request.
To ensure consistent handling of nullish and non-string inputs, and to align with the repository's idiom for nullish checks, the function should return null for null, undefined, empty strings, and any other non-string inputs that cannot be parsed as JSON. This also prevents the TypeError.
if (input == null || (typeof input === 'string' && input.trim() === '')) return null;
if (typeof input !== 'string') return null;References
- In JavaScript/TypeScript, use loose equality (
== null) as a standard idiom for a 'nullish' check that covers bothnullandundefined.
There was a problem hiding this comment.
Pull request overview
This PR adds defensive JSON parsing to prevent runtime crashes when analytics (and related credential) configuration contains empty or malformed JSON, by introducing a safe JSON parsing helper and replacing direct JSON.parse usage in analytics handling.
Changes:
- Added
safeJsonParsehelper to returnnullinstead of throwing on invalid/empty JSON input. - Updated analytics handler paths (
additionalCallbacks,AnalyticHandler.init) to usesafeJsonParseand validate the parsed config is an object. - Updated Google credentials builder to use
safeJsonParseinstead of directJSON.parse.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
packages/components/src/utils.ts |
Introduces safeJsonParse helper used to prevent JSON parsing crashes. |
packages/components/src/handler.ts |
Switches analytics config parsing to safeJsonParse and adds object-shape validation before iterating providers. |
packages/components/src/google-utils.ts |
Uses safeJsonParse for Google credential JSON to avoid throwing on malformed input. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const analytic = safeJsonParse(options.analytic ?? '') | ||
| if (!analytic || typeof analytic !== 'object' || Array.isArray(analytic)) return [] | ||
| const callbacks: any = [] |
There was a problem hiding this comment.
There are existing Jest tests for handler.ts, but the new defensive analytics parsing/validation path isn’t covered. Please add tests that verify malformed/empty options.analytic (and this.options.analytic) no longer throw and instead return [] / short-circuit initialization, to prevent regressions of the crash this PR is fixing.
| const analytic = safeJsonParse(this.options.analytic ?? '') | ||
| if (!analytic || typeof analytic !== 'object' || Array.isArray(analytic)) return | ||
| for (const provider in analytic) { |
There was a problem hiding this comment.
Same as above: please add coverage for invalid JSON in this.options.analytic to ensure AnalyticHandler.init() doesn’t throw and correctly exits early when the config is malformed (and that valid JSON still initializes providers).
| export function safeJsonParse(input: string) { | ||
| if (!input || input.trim() === '') return null | ||
| try { | ||
| return JSON.parse(input) |
There was a problem hiding this comment.
safeJsonParse can still throw when input is not a string (e.g., an object from getCredentialParam), because input.trim() is executed before the try/catch. That undermines the goal of preventing crashes. Consider changing the signature to accept unknown and guarding with typeof input === 'string' (and/or moving the trim check inside the try) so non-string values safely return null (or pass through if you want to support already-parsed objects).
| export function safeJsonParse(input: string) { | |
| if (!input || input.trim() === '') return null | |
| try { | |
| return JSON.parse(input) | |
| export function safeJsonParse(input: unknown) { | |
| if (typeof input !== 'string') return null | |
| const trimmed = input.trim() | |
| if (!trimmed) return null | |
| try { | |
| return JSON.parse(trimmed) |
| authOptions.credentials = JSON.parse(googleApplicationCredential) | ||
| else if (!googleApplicationCredentialFilePath && googleApplicationCredential) { | ||
| const parsedCredential = safeJsonParse(googleApplicationCredential) | ||
| authOptions.credentials = parsedCredential ?? googleApplicationCredential |
There was a problem hiding this comment.
On parse failure, this now assigns authOptions.credentials to the raw googleApplicationCredential value (likely a string). Downstream libraries typically expect an object for credentials, so this can turn a clear JSON parse error into a later, harder-to-debug runtime/config error. Consider validating that the parsed value is a non-null object (and not an array) and either throw a user-friendly error when invalid, or avoid setting credentials unless it parsed to the expected shape.
| authOptions.credentials = parsedCredential ?? googleApplicationCredential | |
| if (parsedCredential && typeof parsedCredential === 'object' && !Array.isArray(parsedCredential)) { | |
| authOptions.credentials = parsedCredential | |
| } else { | |
| throw new Error('Invalid Google Application Credential JSON; expected a JSON object.') | |
| } |
This PR introduces defensive JSON parsing for analytics configuration
to prevent runtime crashes when JSON.parse receives empty or malformed input.
Changes:
This preserves existing behavior for valid JSON while preventing crashes.