Skip to content

fix: handle missing metadata keys in CEL resource filter #960

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

Merged
adityachoudhari26 merged 1 commit into from
Apr 9, 2026
Merged

fix: handle missing metadata keys in CEL resource filter #960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions apps/api/src/routes/v1/workspaces/resources.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,11 @@ const listResources: AsyncTypedHandler<

const filteredResources = allResources.filter((resource) => {
if (cel == null) return true;
const matches = evaluate(cel, { resource });
return matches;
try {
return evaluate(cel, { resource });
} catch {
return false;
}
Comment on lines +52 to +56
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

The catch-all around evaluate() will silently treat any runtime evaluation error as a non-match. That prevents 500s for missing metadata keys, but it also masks unexpected server-side bugs or selectors that parse successfully but are not evaluable (e.g., unsupported functions/fields), returning a misleading 200/empty result. Consider narrowing this to the specific evaluation error(s) you expect for missing keys (or checking error type/message), and rethrow/500 (or 400) for other failures; at minimum, capture the error and add bounded logging/telemetry so issues aren’t silently hidden.

Copilot uses AI. Check for mistakes.
Comment on lines +52 to +56
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 9, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect selector validation and the new catch block side by side.
sed -n '1,220p' apps/api/src/routes/v1/valid-selector.ts
printf '\n--- resources.ts ---\n'
sed -n '39,60p' apps/api/src/routes/v1/workspaces/resources.ts

Repository: ctrlplanedev/ctrlplane

Length of output: 955

🏁 Script executed:

# Find the evaluate function definition and its documentation
rg -B 3 -A 10 "export.*evaluate|function evaluate" apps/api/src --type ts --type tsx

# Check for test cases around resource filtering and CEL evaluation
fd -e test.ts -e spec.ts apps/api/src | xargs rg -l "evaluate\|resources" | head -5

# Look for any comments or context around the try/catch addition
git log --oneline -S "catch {" apps/api/src/routes/v1/workspaces/resources.ts | head -3

Repository: ctrlplanedev/ctrlplane

Length of output: 530

🏁 Script executed:

# Find evaluate function definition
fd . apps/api --full-path | xargs rg -l "export.*evaluate|function evaluate" | head -3

# Search for evaluate in the codebase more broadly
rg "evaluate\(" apps/api/src --type ts -B 2 -A 8 | head -50

# Check if there's any specific error handling for missing keys elsewhere
rg "missing.*key|key.*missing|undefined.*field" apps/api/src --type ts

Repository: ctrlplanedev/ctrlplane

Length of output: 836

🏁 Script executed:

# Find where evaluate is imported from in resources.ts
head -40 apps/api/src/routes/v1/workspaces/resources.ts | grep -E "import|from"

# Find all files that export evaluate
rg "export.*evaluate" apps/ --type ts

# Find the evaluate function implementation
rg -B 5 -A 15 "const evaluate|function evaluate" apps/ --type ts

Repository: ctrlplanedev/ctrlplane

Length of output: 533

🏁 Script executed:

# Look for tests involving evaluate or CEL resource filtering
fd . apps/api --full-path -name "*.test.ts" -o -name "*.spec.ts" | xargs rg "evaluate|CEL|resource.*filter" 2>/dev/null | head -20

# Check if evaluate is used elsewhere in the codebase with different error handling
rg "evaluate\(" apps/ --type ts -B 3 -A 3

# Check for any comments or documentation about CEL evaluation errors
rg "cel-js|evaluate" apps/api/src --type ts -B 2 -A 2 | grep -i "error\|exception\|missing\|undefined"

# Look at the full resources.ts file around the filter to understand context
sed -n '30,75p' apps/api/src/routes/v1/workspaces/resources.ts

Repository: ctrlplanedev/ctrlplane

Length of output: 5747

🏁 Script executed:

# Find cel-js package info and version
rg "cel-js" apps/ packages/ --type json

# Look for any tests involving CEL evaluation errors
fd . apps/api --full-path -type f | xargs grep -l "evaluate" | xargs grep -l "test\|spec" | head -3

# Check workspace-engine for similar patterns (from learnings, there's CEL usage there)
rg "evaluate\|cel" apps/workspace-engine/pkg/workspace/store/relationship_indexes.go -B 2 -A 5

# Look for error types or error messages related to missing keys
rg "missing\|undefined\|field\|key" apps/api/src/routes/v1/workspaces/resources.ts

Repository: ctrlplanedev/ctrlplane

Length of output: 502

🏁 Script executed:

# Search for tests of the resources endpoint
fd . apps/api --full-path -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | xargs rg -l "resources" 2>/dev/null | head -5

# Look for any logging or error reporting around evaluate
rg "console\|log\|error" apps/api/src/routes/v1/workspaces/resources.ts

# Check if there are any comments or documentation in resources.ts about error handling
head -100 apps/api/src/routes/v1/workspaces/resources.ts | grep -E "//|/\*"

# Search entire resources.ts file for context
cat apps/api/src/routes/v1/workspaces/resources.ts

Repository: ctrlplanedev/ctrlplane

Length of output: 13718

Broaden scope or narrow error handling in CEL evaluation catch block.

The commit targets "missing metadata keys," but the current implementation catches all exceptions from evaluate(). Since validResourceSelector only validates CEL syntax (not runtime evaluation), expressions like resource.metadata.nonexistent == "value" will parse fine, fail at runtime when the field is missing, and silently return false instead of surfacing an error. The endpoint then returns 200 with partial or empty results instead of an error response. Either explicitly handle only missing-key errors, log unexpected failures, or update the PR scope.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/routes/v1/workspaces/resources.ts` around lines 52 - 56, The
catch in validResourceSelector currently swallows all exceptions from
evaluate(cel, { resource }) causing runtime missing-field errors (e.g.,
resource.metadata.nonexistent) to be treated as false; change the handler to
only catch/handle missing-key/runtime-lookup errors (or rethrow unexpected
exceptions) and/or log them: specifically, in the function that calls
evaluate(cel, { resource }) (validResourceSelector), detect the error
type/message for missing metadata keys and return false for that case, but for
any other exception rethrow or log and propagate an error response so the
endpoint does not silently succeed.

});

const total = filteredResources.length;
Expand Down
70 changes: 46 additions & 24 deletions e2e/tests/api/resources.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,9 +288,7 @@ test.describe("Resource API", () => {
api,
workspace,
}) => {
const suffix = faker.string.alphanumeric(8);
const identifier = `res-plus-${suffix}`;
const kind = `Kind+Plus-${suffix}`;
const identifier = `res-plus-${faker.string.alphanumeric(8)}`;

await api.PUT(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
Expand All @@ -300,18 +298,18 @@ test.describe("Resource API", () => {
},
body: {
name: "Plus Resource",
kind,
kind: "TestKind",
version: "1.0.0",
config: {},
metadata: {},
metadata: { tag: "a+b" },
},
},
);

const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", {
params: {
path: { workspaceId: workspace.id },
query: { cel: `resource.kind == "${kind}"` },
query: { cel: 'resource.metadata["tag"] == "a+b"' },
},
});

Expand Down Expand Up @@ -374,23 +372,38 @@ test.describe("Resource API", () => {
);
});

test("should filter resources with CEL containing literal %20", async ({
test("should skip resources where CEL references a missing metadata key", async ({
api,
workspace,
}) => {
const suffix = faker.string.alphanumeric(8);
const identifier = `res-pct20-${suffix}`;
const kind = `Kind%20Space-${suffix}`;
const identifier1 = `res-haskey-${faker.string.alphanumeric(8)}`;
const identifier2 = `res-nokey-${faker.string.alphanumeric(8)}`;

await api.PUT(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier },
path: { workspaceId: workspace.id, identifier: identifier1 },
},
body: {
name: "Pct20 Resource",
kind,
name: "Has Key",
kind: "MissingKeyTest",
version: "1.0.0",
config: {},
metadata: { env: "production" },
},
},
);

await api.PUT(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier: identifier2 },
},
body: {
name: "No Key",
kind: "MissingKeyTest",
version: "1.0.0",
config: {},
metadata: {},
Expand All @@ -401,20 +414,31 @@ test.describe("Resource API", () => {
const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", {
params: {
path: { workspaceId: workspace.id },
query: { cel: `resource.kind == "${kind}"` },
query: { cel: 'resource.metadata["env"] == "production"' },
},
Comment on lines 375 to 418
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

This test previously covered CEL filters containing a literal "%20" in the selector value, but it has been replaced with the missing-metadata-key scenario. There are no remaining "%20"-related assertions in this file, so the URL-decoding regression the prior test guarded against is no longer covered. If handling literal "%20" is still required, keep that case as a separate test (or add a new one) rather than overwriting it here.

Copilot uses AI. Check for mistakes.
});

expect(listRes.response.status).toBe(200);
expect(listRes.data!.items.some((r) => r.identifier === identifier)).toBe(
true,
);
expect(
listRes.data!.items.some((r) => r.identifier === identifier1),
).toBe(true);
expect(
listRes.data!.items.some((r) => r.identifier === identifier2),
).toBe(false);

await api.DELETE(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier },
path: { workspaceId: workspace.id, identifier: identifier1 },
},
},
);
await api.DELETE(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier: identifier2 },
},
},
);
Expand All @@ -424,9 +448,7 @@ test.describe("Resource API", () => {
api,
workspace,
}) => {
const suffix = faker.string.alphanumeric(8);
const identifier = `res-amp-${suffix}`;
const name = `a&b=c-${suffix}`;
const identifier = `res-amp-${faker.string.alphanumeric(8)}`;

await api.PUT(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
Expand All @@ -435,19 +457,19 @@ test.describe("Resource API", () => {
path: { workspaceId: workspace.id, identifier },
},
body: {
name,
name: "Amp Resource",
kind: "TestKind",
version: "1.0.0",
config: {},
metadata: {},
metadata: { "a&b": "x=y" },
},
},
);

const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", {
params: {
path: { workspaceId: workspace.id },
query: { cel: `resource.name == "${name}"` },
query: { cel: 'resource.metadata["a&b"] == "x=y"' },
},
});

Expand Down
Loading