Skip to content

fix: dont double encode cel expression #959

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
+292 −6
Merged

fix: dont double encode cel expression #959

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
9 changes: 3 additions & 6 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 @@ -36,10 +36,7 @@ const listResources: AsyncTypedHandler<
const limit = rawLimit ?? 1000;
const offset = rawOffset ?? 0;

const decodedCel =
typeof cel === "string" ? decodeURIComponent(cel.replace(/\+/g, " ")) : cel;

const isValid = validResourceSelector(decodedCel);
const isValid = validResourceSelector(cel);
if (!isValid) {
res.status(400).json({ error: "Invalid resource selector" });
return;
Expand All @@ -51,8 +48,8 @@ const listResources: AsyncTypedHandler<
.where(eq(schema.resource.workspaceId, workspaceId));

const filteredResources = allResources.filter((resource) => {
if (decodedCel == null) return true;
const matches = evaluate(decodedCel, { resource });
if (cel == null) return true;
const matches = evaluate(cel, { resource });
return matches;
});

Expand Down
289 changes: 289 additions & 0 deletions e2e/tests/api/resources.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,295 @@ test.describe("Resource API", () => {
);
});

test("should filter resources with CEL containing + character", async ({
api,
workspace,
}) => {
const suffix = faker.string.alphanumeric(8);
const identifier = `res-plus-${suffix}`;
const kind = `Kind+Plus-${suffix}`;

await api.PUT(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier },
},
body: {
name: "Plus Resource",
kind,
version: "1.0.0",
config: {},
metadata: {},
},
},
);

const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", {
params: {
path: { workspaceId: workspace.id },
query: { cel: `resource.kind == "${kind}"` },
},
});

expect(listRes.response.status).toBe(200);
expect(listRes.data!.items.some((r) => r.identifier === identifier)).toBe(
true,
);
Comment on lines +318 to +321
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 assertion only checks that the matching resource is included. If the API accidentally ignores the cel query and returns all resources, this test would still pass. Consider also asserting total/items.length (e.g., equals 1) or creating a known non-matching resource and asserting it is excluded to ensure the filter is actually applied.

Copilot uses AI. Check for mistakes.

await api.DELETE(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier },
},
},
);
});

test("should filter resources with CEL containing % character", async ({
api,
workspace,
}) => {
const identifier = `res-pct-${faker.string.alphanumeric(8)}`;

await api.PUT(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier },
},
body: {
name: "100% Complete",
kind: "TestKind",
version: "1.0.0",
config: {},
metadata: {},
},
},
);

const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", {
params: {
path: { workspaceId: workspace.id },
query: { cel: 'resource.name == "100% Complete"' },
},
});

expect(listRes.response.status).toBe(200);
expect(listRes.data!.items.some((r) => r.identifier === identifier)).toBe(
true,
);
Comment on lines +362 to +365
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 only verifies that the matching identifier is present, which wouldn’t fail if the server ignored the cel filter and returned all resources. Add an exclusion assertion (or assert total/items.length) to make sure the filter is applied, not just parseable.

Copilot uses AI. Check for mistakes.

await api.DELETE(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier },
},
},
);
});

test("should filter resources with CEL containing literal %20", async ({
api,
workspace,
}) => {
const suffix = faker.string.alphanumeric(8);
const identifier = `res-pct20-${suffix}`;
const kind = `Kind%20Space-${suffix}`;

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

const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", {
params: {
path: { workspaceId: workspace.id },
query: { cel: `resource.kind == "${kind}"` },
},
});

expect(listRes.response.status).toBe(200);
expect(listRes.data!.items.some((r) => r.identifier === identifier)).toBe(
true,
);
Comment on lines +408 to +411
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 checks inclusion but not exclusion. To ensure the CEL filter is actually being enforced (and not ignored), consider asserting total/items.length or adding a second resource that should not match and asserting it’s absent.

Copilot uses AI. Check for mistakes.

await api.DELETE(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier },
},
},
);
});

test("should filter resources with CEL containing & and = characters", async ({
api,
workspace,
}) => {
const suffix = faker.string.alphanumeric(8);
const identifier = `res-amp-${suffix}`;
const name = `a&b=c-${suffix}`;

await api.PUT(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier },
},
body: {
name,
kind: "TestKind",
version: "1.0.0",
config: {},
metadata: {},
},
},
);

const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", {
params: {
path: { workspaceId: workspace.id },
query: { cel: `resource.name == "${name}"` },
},
});

expect(listRes.response.status).toBe(200);
expect(listRes.data!.items.some((r) => r.identifier === identifier)).toBe(
true,
);
Comment on lines +454 to +457
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 verifies the matching resource is included, but it won’t catch a regression where the API ignores cel and returns all resources. Add a negative assertion (non-matching resource not returned) or assert total/items.length to confirm filtering happens.

Copilot uses AI. Check for mistakes.

await api.DELETE(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier },
},
},
);
});

test("should return 400 for invalid CEL expression", async ({
api,
workspace,
}) => {
const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", {
params: {
path: { workspaceId: workspace.id },
query: { cel: "this is not valid cel ===!!!" },
},
});

expect(listRes.response.status).toBe(400);
});

test("should return empty results when CEL matches no resources", async ({
api,
workspace,
}) => {
const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", {
params: {
path: { workspaceId: workspace.id },
query: {
cel: `resource.kind == "NonExistentKind-${faker.string.alphanumeric(16)}"`,
},
},
});

expect(listRes.response.status).toBe(200);
expect(listRes.data!.items).toHaveLength(0);
expect(listRes.data!.total).toBe(0);
});

test("should filter resources with compound CEL expression", async ({
api,
workspace,
}) => {
const identifier1 = `res-comp-a-${faker.string.alphanumeric(8)}`;
const identifier2 = `res-comp-b-${faker.string.alphanumeric(8)}`;
const kind = `CompoundKind-${faker.string.alphanumeric(6)}`;

await api.PUT(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier: identifier1 },
},
body: {
name: "Compound Match",
kind,
version: "1.0.0",
config: {},
metadata: {},
},
},
);

await api.PUT(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier: identifier2 },
},
body: {
name: "Compound NoMatch",
kind,
version: "1.0.0",
config: {},
metadata: {},
},
},
);

const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", {
params: {
path: { workspaceId: workspace.id },
query: {
cel: `resource.kind == "${kind}" && resource.name == "Compound Match"`,
},
},
});

expect(listRes.response.status).toBe(200);
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: identifier1 },
},
},
);
await api.DELETE(
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}",
{
params: {
path: { workspaceId: workspace.id, identifier: identifier2 },
},
},
);
});
Comment on lines +287 to +574
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

Use YAML-backed entity fixtures for these new E2E cases.

The added tests in Line 287 onward manually create/delete entities via API calls. For this e2e path, please define entities in a sibling .spec.yaml and use importEntitiesFromYaml / cleanupImportedEntities instead of inline setup/teardown.

As per coding guidelines: **/e2e/**/*.spec.ts: E2E tests use YAML fixture files (.spec.yaml alongside .spec.ts) to declare test entities; use importEntitiesFromYaml to load them and cleanupImportedEntities to tear them down.

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

In `@e2e/tests/api/resources.spec.ts` around lines 287 - 574, The tests starting
at the new resource-filter cases create and delete entities inline using
api.PUT/api.DELETE; replace that manual setup/teardown by declaring the
resources in a sibling .spec.yaml fixture and loading them via
importEntitiesFromYaml before the tests and removing them with
cleanupImportedEntities after (or in an afterEach/afterAll). Update each test
that references created identifiers/kinds/names (the blocks using api.PUT with
identifier variables like identifier, identifier1, identifier2 and kinds like
kind/Kind+Plus-*/Kind%20Space-*) to instead rely on the imported fixture entity
identifiers and ensure tests call importEntitiesFromYaml and
cleanupImportedEntities in the same spec to manage lifecycle. Ensure the YAML
fixture names correspond to the generated identifiers used by the tests or make
the tests read identifiers from the imported fixture metadata.


test("should upsert a resource with variables", async ({
api,
workspace,
Expand Down
Loading