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
28 changes: 27 additions & 1 deletion google-analytics/server/lib/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,30 @@ export const getGoogleAccessToken = (env: Env): string => {
return authorization.replace(/^Bearer\s+/i, "");
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
};

// Normalizes "1234567" → "properties/1234567"; already-prefixed IDs pass through.
function normalizePropertyId(id: string): string {

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.

P2: normalizePropertyId lacks validation for blank or malformed entries, which makes allowlist enforcement brittle and can cause accidental total lockout. The codebase already has a strict normalizeProperty function in ga-client.ts that validates property IDs and throws on malformed input; normalizePropertyId should align with that behavior.

For example, an allowlist entry of " " normalizes to "", and "abc" passes through unchanged. Because any non-empty allowlist array activates enforcement, a misconfigured list with only invalid entries silently denies all valid properties. Defensive validation (e.g., filtering out or rejecting invalid/blank entries during normalization) would prevent this operational failure mode.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At google-analytics/server/lib/env.ts, line 14:

<comment>`normalizePropertyId` lacks validation for blank or malformed entries, which makes allowlist enforcement brittle and can cause accidental total lockout. The codebase already has a strict `normalizeProperty` function in `ga-client.ts` that validates property IDs and throws on malformed input; `normalizePropertyId` should align with that behavior.

For example, an allowlist entry of `"   "` normalizes to `""`, and `"abc"` passes through unchanged. Because any non-empty allowlist array activates enforcement, a misconfigured list with only invalid entries silently denies all valid properties. Defensive validation (e.g., filtering out or rejecting invalid/blank entries during normalization) would prevent this operational failure mode.</comment>

<file context>
@@ -10,20 +10,20 @@ export const getGoogleAccessToken = (env: Env): string => {
-// Normalizes "1234567" → "accounts/1234567"; already-prefixed IDs pass through.
-function normalizeAccountId(id: string): string {
+// Normalizes "1234567" → "properties/1234567"; already-prefixed IDs pass through.
+function normalizePropertyId(id: string): string {
   const clean = String(id).trim();
-  return /^\d+$/.test(clean) ? `accounts/${clean}` : clean;
</file context>

const clean = String(id).trim();
return /^\d+$/.test(clean) ? `properties/${clean}` : clean;
}

/**
* Returns the normalized allowlist of property IDs for this installation,
* or null if no allowlist is configured (meaning all properties are allowed).
*/
export const getAllowedPropertyIds = (env: Env): string[] | null => {
const ids = env.MESH_REQUEST_CONTEXT?.state?.allowedPropertyIds;
if (!ids || ids.length === 0) return null;
return ids.map(normalizePropertyId);
};

/**
* Resolves the GA4 property to use for a request.
*
* Prefers the `property` passed to the tool; when omitted, falls back to the
* `propertyId` configured on the MCP installation state.
*
* When allowedPropertyIds is configured, the resolved property is validated
* against the allowlist — any property not in the list is rejected.
*/
export const resolveProperty = (env: Env, property?: string | null): string => {
const resolved = property ?? env.MESH_REQUEST_CONTEXT?.state?.propertyId;
Expand All @@ -23,5 +42,12 @@ export const resolveProperty = (env: Env, property?: string | null): string => {
"No GA4 property provided. Pass a `property` argument or configure a default `propertyId` in this integration's settings.",
);
}
return resolved;
const normalized = normalizePropertyId(resolved);
const allowed = getAllowedPropertyIds(env);
if (allowed && !allowed.includes(normalized)) {
throw new Error(
`Access denied: property '${normalized}' is not in the allowlist configured for this integration.`,
);
}
return normalized;
};
32 changes: 31 additions & 1 deletion google-analytics/server/tools/accounts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { z } from "zod";
import { createPrivateTool } from "@decocms/runtime/tools";
import type { Env } from "../../shared/deco.gen.ts";
import { GaClient } from "../lib/ga-client.ts";
import { getAllowedPropertyIds } from "../lib/env.ts";
import { AccountSummariesResponseSchema } from "../lib/schemas.ts";

export const getAccountSummariesTool = (env: Env) =>
Expand All @@ -15,7 +16,36 @@ export const getAccountSummariesTool = (env: Env) =>
const client = GaClient.fromEnv(env);

try {
const result = await client.listAccountSummaries();
const result = (await client.listAccountSummaries()) as {
accountSummaries?: Array<{
name: string;
account?: string;
displayName?: string;
propertySummaries?: Array<{
property: string;
displayName: string;
propertyType?: string;
parent?: string;
}>;
}>;
};
const allowed = getAllowedPropertyIds(env);

if (allowed) {
const filtered = (result.accountSummaries ?? [])
.map((account) => ({
...account,
propertySummaries: (account.propertySummaries ?? []).filter((p) =>
allowed.includes(p.property),
),
}))
.filter((account) => account.propertySummaries.length > 0);

return AccountSummariesResponseSchema.parse({
response: { accountSummaries: filtered },
});
}

return AccountSummariesResponseSchema.parse({ response: result });
} catch (error) {
throw new Error(
Expand Down
6 changes: 6 additions & 0 deletions google-analytics/shared/deco.gen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ export const StateSchema = z.object({
.describe(
"Default GA4 Property identifier — 'properties/1234567' or just '1234567'. Used as a fallback for tools when their `property` argument is omitted.",
),
allowedPropertyIds: z
.array(z.string())
.nullish()
.describe(
"Optional allowlist of GA4 property IDs this installation may access. Accepts 'properties/1234567' or just '1234567'. When set, get-account-summaries only returns matching properties (and the accounts that contain them), and all other tools reject any property not in the list. Leave empty to allow all properties the authenticated user can access.",
),
});

export interface MeshRequestContext {
Expand Down
Loading