Skip to content

fix(deploy/functions): improve list param parsing robustness #8895

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix(deploy/functions): improve list param parsing robustness #8895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f51f761
fix(deploy/functions): parsing list param
CorieW Jul 24, 2025
87ae2c9
fix(deploy/functions): improve list param parsing robustness
CorieW Jul 24, 2025
960ef0f
Merge branch 'master' of https://github.com/firebase/firebase-tools i…
CorieW Jul 24, 2025
91e6fb9
fix(deploy/functions): make more robust
CorieW Jul 24, 2025
22ff3f9
chore: format
CorieW Aug 6, 2025
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
137 changes: 137 additions & 0 deletions src/deploy/functions/params.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@
];
input.resolves("baz");
await params.resolveParams(paramsToResolve, fakeConfig, {});
expect(input.getCall(1).args[0].default).to.eq("baz");

Check warning on line 217 in src/deploy/functions/params.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe member access .default on an `any` value
});

it("can resolve a CEL expression containing only identities", async () => {
Expand All @@ -234,7 +234,7 @@
];
input.resolves("baz");
await params.resolveParams(paramsToResolve, fakeConfig, {});
expect(input.getCall(1).args[0].default).to.eq("baz/quox");

Check warning on line 237 in src/deploy/functions/params.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe member access .default on an `any` value
});

it("can resolve a CEL expression depending on the internal params", async () => {
Expand All @@ -260,9 +260,9 @@
];
input.resolves("baz");
await params.resolveParams(paramsToResolve, fakeConfig, {});
expect(input.getCall(0).args[0].default).to.eq("https://foo.firebaseio.com/quox");

Check warning on line 263 in src/deploy/functions/params.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe member access .default on an `any` value
expect(input.getCall(1).args[0].default).to.eq("projectID: foo");

Check warning on line 264 in src/deploy/functions/params.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe member access .default on an `any` value
expect(input.getCall(2).args[0].default).to.eq(

Check warning on line 265 in src/deploy/functions/params.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe member access .default on an `any` value
"http://foo.appspot.com.storage.googleapis.com/",
);
});
Expand Down Expand Up @@ -299,3 +299,140 @@
await expect(params.resolveParams(paramsToResolve, fakeConfig, {})).to.eventually.be.rejected;
});
});

describe("ParamValue.asList()", () => {
describe("basic usage", () => {
it("should parse JSON-style array with double quotes", () => {
const paramValue = new params.ParamValue('["1", "2", "3"]', false, { list: true });
expect(paramValue.asList()).to.deep.equal(["1", "2", "3"]);
});

it("should split comma-separated values", () => {
const paramValue = new params.ParamValue("a,b,c", false, { list: true });
expect(paramValue.asList()).to.deep.equal(["a", "b", "c"]);
});

it("should handle empty array", () => {
const paramValue = new params.ParamValue("[]", false, { list: true });
expect(paramValue.asList()).to.deep.equal([]);
});

it("should handle a single value", () => {
const paramValue = new params.ParamValue("foo", false, { list: true });
expect(paramValue.asList()).to.deep.equal(["foo"]);
});

it("should handle whitespace around values", () => {
const paramValue = new params.ParamValue(" a , b , c ", false, { list: true });
expect(paramValue.asList()).to.deep.equal([" a ", " b ", " c "]);
});
});

describe("delimiter handling", () => {
it("should use custom delimiter", () => {
const paramValue = new params.ParamValue("a;b;c", false, { list: true });
paramValue.setDelimiter(";");
expect(paramValue.asList()).to.deep.equal(["a", "b", "c"]);
});
});

describe("robustness", () => {
it("should handle values with commas inside JSON array", () => {
const paramValue = new params.ParamValue('["hello, world", "foo, bar"]', false, {
list: true,
});
expect(paramValue.asList()).to.deep.equal(["hello, world", "foo, bar"]);
});

it("should fall back to delimiter splitting for malformed JSON", () => {
const paramValue = new params.ParamValue("[a, b, c]", false, { list: true });
expect(paramValue.asList()).to.deep.equal(["a", " b", " c"]);
});

it("should handle special characters", () => {
const paramValue = new params.ParamValue('["@#$%", "^&*()"]', false, { list: true });
expect(paramValue.asList()).to.deep.equal(["@#$%", "^&*()"]);
});

it("should work with fromList static method", () => {
const originalList = ["apple", "banana", "cherry"];
const paramValue = params.ParamValue.fromList(originalList);
expect(paramValue.asList()).to.deep.equal(originalList);
});

it("should handle square brackets in the middle of the string", () => {
const paramValue = new params.ParamValue('["]", "[", "]"]', false, { list: true });
expect(paramValue.asList()).to.deep.equal(["]", "[", "]"]);
});

it("should handle quotes, apostrophes, and backticks in the middle of the string", () => {
const paramValue = new params.ParamValue('["a\\"b", "c`d", "e\'f"]', false, { list: true });
expect(paramValue.asList()).to.deep.equal(['a"b', "c`d", "e'f"]);
});

it("should handle empty string", () => {
const paramValue = new params.ParamValue("", false, { list: true });
expect(paramValue.asList()).to.deep.equal([""]);
});

it("should handle whitespace-only string", () => {
const paramValue = new params.ParamValue(" ", false, { list: true });
expect(paramValue.asList()).to.deep.equal([" "]);
});

it("should handle JSON array with numbers", () => {
const paramValue = new params.ParamValue("[1, 2, 3]", false, { list: true });
expect(paramValue.asList()).to.deep.equal(["1", "2", "3"]);
});

it("should handle JSON array with booleans", () => {
const paramValue = new params.ParamValue("[true, false, true]", false, { list: true });
expect(paramValue.asList()).to.deep.equal(["true", "false", "true"]);
});

it("should handle JSON array with nulls", () => {
const paramValue = new params.ParamValue('["a", null, "b"]', false, { list: true });
expect(paramValue.asList()).to.deep.equal(["a", "null", "b"]);
});

it("should handle trailing delimiter", () => {
const paramValue = new params.ParamValue("a,b,c,", false, { list: true });
expect(paramValue.asList()).to.deep.equal(["a", "b", "c", ""]);
});

it("should handle leading delimiter", () => {
const paramValue = new params.ParamValue(",a,b,c", false, { list: true });
expect(paramValue.asList()).to.deep.equal(["", "a", "b", "c"]);
});

it("should handle consecutive delimiters", () => {
const paramValue = new params.ParamValue("a,,b", false, { list: true });
expect(paramValue.asList()).to.deep.equal(["a", "", "b"]);
});

it("should handle only delimiters", () => {
const paramValue = new params.ParamValue(",,,", false, { list: true });
expect(paramValue.asList()).to.deep.equal(["", "", "", ""]);
});

it("should handle JSON array with empty strings", () => {
const paramValue = new params.ParamValue('["", "a", ""]', false, { list: true });
expect(paramValue.asList()).to.deep.equal(["", "a", ""]);
});

it("should handle JSON array with whitespace strings", () => {
const paramValue = new params.ParamValue('[" ", "a", " "]', false, { list: true });
expect(paramValue.asList()).to.deep.equal([" ", "a", " "]);
});

it("should handle JSON array with nested arrays", () => {
const paramValue = new params.ParamValue('[["a", "b"], ["c"]]', false, { list: true });
expect(paramValue.asList()).to.deep.equal(['["a","b"]', '["c"]']);
});

it("should handle JSON array with objects", () => {
const paramValue = new params.ParamValue('[{"foo":1}, {"bar":2}]', false, { list: true });
expect(paramValue.asList()).to.deep.equal(['{"foo":1}', '{"bar":2}']);
});
});
});
34 changes: 28 additions & 6 deletions src/deploy/functions/params.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@
return pv;
}

setDelimiter(delimiter: string) {

Check warning on line 271 in src/deploy/functions/params.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing return type on function
this.delimiter = delimiter;
}

Expand All @@ -291,15 +291,37 @@
}

asList(): string[] {
// Handle something like "['a', 'b', 'c']"
if (this.rawValue.includes("[")) {
// Convert quotes to apostrophes
const unquoted = this.rawValue.replace(/'/g, '"');
return JSON.parse(unquoted);
let modifiedValue = this.rawValue;

// Handle something like "["a", "b", "c"]"
if (modifiedValue.startsWith("[") && modifiedValue.endsWith("]")) {
try {
const parsed = JSON.parse(modifiedValue);

Check warning on line 299 in src/deploy/functions/params.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe assignment of an `any` value
Copy link
Contributor

Choose a reason for hiding this comment

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

ha this is clever way to parse arrays.

if (Array.isArray(parsed)) {
// The return type is string[], so we must convert all elements to strings.
return parsed.map((elem: any) => {

Check warning on line 302 in src/deploy/functions/params.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unexpected any. Specify a different type
if (elem === null) {
// String(null) is "null", which is what we want.
return "null";
}
if (typeof elem === "object") {
// String(obj) is "[object Object]", JSON.stringify is more useful.
// Avoid inserting spaces after commas for objects/arrays
return JSON.stringify(elem);
}
return String(elem);
Comment on lines +303 to +312
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This block of code can be simplified. The if (elem === null) check is redundant because typeof null is 'object', so the subsequent if (typeof elem === 'object') block will handle null values correctly. JSON.stringify(null) produces the string 'null', which is the desired behavior as confirmed by the new test cases.

            if (typeof elem === "object") {
              // This correctly handles objects, arrays, and null.
              // For objects/arrays, JSON.stringify is more useful than the default String() conversion.
              // For null, JSON.stringify(null) results in "null", which is what we want.
              return JSON.stringify(elem);
            }
            return String(elem);

});
}
// It's valid JSON but not an array (e.g. a string literal "foo,bar").
// Fall through to splitting by delimiter.
} catch (e) {
// Malformed JSON, e.g. "[a, b, c]". Remove brackets and fall through.
modifiedValue = modifiedValue.slice(1, -1);
}
}

// Continue to handle something like "a,b,c"
return this.rawValue.split(this.delimiter);
return modifiedValue.split(this.delimiter);
}

asNumber(): number {
Expand Down Expand Up @@ -413,7 +435,7 @@
}
if (paramDefault && !canSatisfyParam(param, paramDefault)) {
throw new FirebaseError(
"Parameter " + param.name + " has default value " + paramDefault + " of wrong type",

Check warning on line 438 in src/deploy/functions/params.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Operands of '+' operation must either be both strings or both numbers. Consider using a template literal
);
}
paramValues[param.name] = await promptParam(param, firebaseConfig.projectId, paramDefault);
Expand Down Expand Up @@ -460,7 +482,7 @@
* to read its environment variables. They are instead provided through GCF's own
* Secret Manager integration.
*/
async function handleSecret(secretParam: SecretParam, projectId: string) {

Check warning on line 485 in src/deploy/functions/params.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing return type on function
const metadata = await secretManager.getSecretMetadata(projectId, secretParam.name, "latest");
if (!metadata.secret) {
const secretValue = await password({
Expand Down
Loading