Skip to content

support JSONC comments in config profile validation#154

Draft
chirizxc wants to merge 1 commit intoremnawave:mainfrom
chirizxc:_jsonc
Draft

support JSONC comments in config profile validation#154
chirizxc wants to merge 1 commit intoremnawave:mainfrom
chirizxc:_jsonc

Conversation

@chirizxc
Copy link
Copy Markdown

@snyk-io
Copy link
Copy Markdown

snyk-io bot commented Mar 27, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@what-the-diff
Copy link
Copy Markdown

what-the-diff bot commented Mar 27, 2026

PR Summary

  • Enhanced Configuration Property Validation in Creation Process
    The validation of the config property during the configuration profile creation process (create-config-profile.command.ts) has been improved. It can now accept both string and object types, thanks to the use of z.union.

  • Flexible Configuration Property Validation in Update Process
    Made similar changes to the configuration profile updating process (update-config-profile.command.ts). The config property can now accept either strings or objects, while maintaining its optional status.

  • Introduction of jsonc-parser
    A new dependency named jsonc-parser, version ^3.3.1, has been added to the package.json file.

  • Improved JSON Parsing
    The process of parsing JSON in xray-config.validator.ts has been enhanced by utilizing jsonc-parser. This change significantly improves error messages when invalid input is given.

@kastov
Copy link
Copy Markdown
Contributor

kastov commented Mar 27, 2026

This will not work, Remnawave stores JSON as JSONB in the database.

@chirizxc
Copy link
Copy Markdown
Author

This will not work, Remnawave stores JSON as JSONB in the database.

What if we added support for TOML? Xray can handle that type of configuration.

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 27, 2026

Greptile Summary

This PR adds support for JSONC (JSON with Comments) in config profile validation by replacing JSON.parse with jsonc-parser's parse() and widening the Zod contract schemas to accept either a JSONC string or a plain object.\n\nThe intent is correct and the approach is reasonable, but there are two implementation issues that need to be addressed before merging:\n\n- parse() from jsonc-parser does not throw on invalid input — unlike JSON.parse, it is intentionally fault-tolerant and communicates parse errors through an optional second errors array argument. The existing try-catch block in XRayConfig.prevValidateConfig is therefore dead code: invalid JSONC silently produces undefined (or a partial result), leading to a cryptic TypeError downstream rather than the intended informative error message.\n- Service method signatures are not updatedConfigProfileService.createConfigProfile and updateConfigProfile still declare config: object, but the Zod schema now infers config as string | Record<string, unknown>. Passing a string where TypeScript expects object is a type error that will break compilation.

Confidence Score: 3/5

Not safe to merge — invalid JSONC input silently bypasses error handling, and a TypeScript type mismatch between the new schema and the service signatures will break compilation.

Two P1 issues: (1) jsonc-parser's parse() is fault-tolerant and never throws, making the try-catch dead code and exposing a path where invalid input returns undefined causing a downstream TypeError; (2) the service method signatures still accept object while the Zod schemas now emit string | object, which is a TypeScript compilation error. Both need to be fixed before merging.

src/common/helpers/xray-config/xray-config.validator.ts (error handling logic) and src/modules/config-profiles/config-profile.service.ts (method signatures for createConfigProfile and updateConfigProfile).

Important Files Changed

Filename Overview
src/common/helpers/xray-config/xray-config.validator.ts Replaces JSON.parse with jsonc-parser's parse() to support JSONC comments, but parse() never throws — the try-catch is dead code and invalid input silently returns undefined, causing a downstream TypeError instead of a meaningful error message.
libs/contract/commands/config-profiles/create-config-profile.command.ts Widens the config field to z.union([z.string(), z.object({}).passthrough()]) to allow JSONC strings, but the downstream service method still expects config: object, creating a TypeScript type incompatibility.
libs/contract/commands/config-profiles/update-config-profile.command.ts Same widening as the create command — config now allows `string
package.json Adds jsonc-parser@^3.3.1 as a runtime dependency — a well-maintained Microsoft library with no concerns.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[HTTP Request with config] --> B{Zod schema validation}
    B -->|string JSONC| C[prevValidateConfig: string branch]
    B -->|object| D[prevValidateConfig: object branch]
    C --> E["jsonc-parser parse()"]
    E -->|Valid JSONC| F[Returns IXrayConfig]
    E -->|"Invalid JSONC - no throw!"| G["Returns undefined silently"]
    G --> H["this.config = undefined"]
    H --> I["validate() - TypeError on .inbounds"]
    F --> J[validate]
    D --> J
    J --> K[resolveInbounds]
    K --> L[Config accepted]
Loading

Reviews (1): Last reviewed commit: "support JSONC comments in config profile..." | Re-trigger Greptile

Comment on lines 64 to 68
try {
config = JSON.parse(configInput) as IXrayConfig;
config = parse(configInput) as IXrayConfig;
} catch (error) {
throw new Error(`Invalid JSON input or file path: ${error}`);
throw new Error(`Invalid JSON/JSONC input: ${error}`);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 jsonc-parser's parse() never throws — try-catch is dead code

According to the official jsonc-parser docs: "On invalid input, the parser tries to be as fault tolerant as possible, but still return a result. Therefore always check the errors list to find out if the input was valid."

parse() does not throw on invalid JSON/JSONC. It returns undefined (or a partial result) for completely invalid input and communicates errors through the optional second errors array argument. This means:

  1. The catch block is unreachable dead code for any malformed JSONC string
  2. If the input is completely invalid, parse() returns undefined, so config ends up as undefined, and the subsequent this.config.inbounds access in validate() causes a cryptic TypeError instead of the intended clear "Invalid JSON/JSONC input" message

The fix is to pass an errors array and check it explicitly:

import { parse, ParseError } from 'jsonc-parser';

if (typeof configInput === 'string') {
    const errors: ParseError[] = [];
    config = parse(configInput, errors) as IXrayConfig;
    if (errors.length > 0) {
        throw new Error(`Invalid JSON/JSONC input: error code ${errors[0].error} at offset ${errors[0].offset}`);
    }
}

'Name can only contain letters, numbers, underscores, dashes and spaces',
),
config: z.object({}).passthrough(),
config: z.union([z.string(), z.object({}).passthrough()]),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Service signature not updated to accept string

The Zod schema now infers config as string | Record<string, unknown>, but ConfigProfileService.createConfigProfile still declares config: object:

public async createConfigProfile(
    name: string,
    config: object,  // ← only object, not string | object
)

In TypeScript, string is not assignable to object, so passing createConfigProfileDto.config (which is string | Record<string, unknown>) to this method is a type error that will break compilation.

The service (and likewise updateConfigProfile) needs its signature updated:

public async createConfigProfile(
    name: string,
    config: string | object,
)

The same applies to src/modules/config-profiles/config-profile.service.ts line 133 (createConfigProfile) and line 192 (updateConfigProfile).

@chirizxc chirizxc marked this pull request as draft March 27, 2026 12:28
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.

2 participants