Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 0 additions & 46 deletions npm-shrinkwrap.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@
"async-lock": "1.4.1",
"body-parser": "^1.19.0",
"chokidar": "^3.6.0",
"cjson": "^0.3.1",
"cli-table3": "0.6.5",
"colorette": "^2.0.19",
"commander": "^5.1.0",
Expand Down
13 changes: 13 additions & 0 deletions src/loadCJSON.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,26 @@

it("should return parsed JSON on success", () => {
const filePath = path.join(fixturesDir, "valid.cjson");
const result = loadCJSON(filePath);

Check warning on line 11 in src/loadCJSON.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unsafe assignment of an `any` value
expect(result).to.deep.equal({ key: "value" });
});

it("should correctly parse comments inside string values and block comments", () => {
const filePath = path.join(fixturesDir, "complex.cjson");
const result = loadCJSON(filePath);

Check warning on line 17 in src/loadCJSON.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unsafe assignment of an `any` value
expect(result).to.deep.equal({
url: "https://example.com/foo",
comment_in_string: "this is // not a comment",
block_in_string: "this is /* not a block */ comment",
nested: {
key: "value",
},
});
});

it("should throw FirebaseError on ENOENT", () => {
const filePath = path.join(fixturesDir, "nonexistent.cjson");
expect(() => loadCJSON(filePath)).to.throw(

Check warning on line 30 in src/loadCJSON.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unsafe return of an `any` typed value
FirebaseError,
"File " + filePath + " does not exist",
);
Expand All @@ -22,7 +35,7 @@

it("should throw FirebaseError on parse error", () => {
const filePath = path.join(fixturesDir, "invalid.cjson");
expect(() => loadCJSON(filePath)).to.throw(

Check warning on line 38 in src/loadCJSON.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unsafe return of an `any` typed value
FirebaseError,
new RegExp(`Parse Error in ${filePath}`),
);
Expand Down
18 changes: 16 additions & 2 deletions src/loadCJSON.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,30 @@
import * as fs from "fs";
import { FirebaseError } from "./error";
import * as cjson from "cjson";

/**
* Loads CJSON from given path.
*/
export function loadCJSON(path: string): any {

Check warning on line 7 in src/loadCJSON.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unexpected any. Specify a different type
let content: string;
try {
return cjson.load(path);
content = fs.readFileSync(path, "utf8");
} catch (e: any) {

Check warning on line 11 in src/loadCJSON.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unexpected any. Specify a different type
if (e.code === "ENOENT") {

Check warning on line 12 in src/loadCJSON.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unsafe member access .code on an `any` value
throw new FirebaseError(`File ${path} does not exist`);
}
throw e;
}

try {
const stripped = content.replace(
/("([^"\\]|\\.)*")|(\/\*[\s\S]*?\*\/|\/\/.*)/g,
(match, string) => {
if (string) return string;

Check warning on line 22 in src/loadCJSON.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unsafe return of an `any` typed value
return "";
},
);
Comment on lines +19 to +25

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

Issue: Critical Bug with Empty Strings ""

The current implementation of the comment-stripping parser has a critical bug when parsing empty strings "" in JSON.

In the replacement callback:

(match, string) => {
  if (string) return string;
  return "";
}

The parameter string corresponds to the first capture group ("([^"\\]|\\.)*"). When the parser encounters an empty string "", the captured value is "". Since "" is a falsy value in JavaScript, if (string) evaluates to false, causing the function to return "" (which is treated as a comment removal).

This results in the empty string being completely stripped from the JSON, leading to invalid JSON and a SyntaxError during JSON.parse. For example, {"key": ""} becomes {"key": }, which fails to parse.

Suggestion: Preserve Line Numbers & Fix Empty Strings

We can fix this bug by explicitly checking if quotedString is not undefined.

Additionally, we can improve error reporting by replacing comments with spaces/newlines instead of an empty string "". This preserves the exact line and column numbers of the original file, making any JSON parsing error messages much more accurate and helpful.

    const stripped = content.replace(
      /("([^"\\]|\\.)*")|(\/\*[\s\S]*?\*\/|\/\/.*)/g,
      (match, quotedString) => {
        if (quotedString !== undefined) {
          return quotedString;
        }
        return match.replace(/[^\r\n]/g, " ");
      },
    );

return JSON.parse(stripped);
} catch (e: any) {

Check warning on line 27 in src/loadCJSON.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unexpected any. Specify a different type
throw new FirebaseError(`Parse Error in ${path}:\n\n${e.message}`);

Check warning on line 28 in src/loadCJSON.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Invalid type "any" of template literal expression
}
}
11 changes: 11 additions & 0 deletions src/test/fixtures/loadCJSON/complex.cjson
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
// Line comment at start
"url": "https://example.com/foo",
"comment_in_string": "this is // not a comment",
"block_in_string": "this is /* not a block */ comment",
/* Multi-line
block comment */
"nested": {
"key": "value" // trailing line comment
}
}
Loading