Skip to content
Merged
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
33 changes: 26 additions & 7 deletions assistant/src/__tests__/extension-id-sync-guard.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,15 @@ function parseCanonicalConfig(): AllowlistConfig {
const parsed = JSON.parse(raw) as Partial<AllowlistConfig>;

if (!Number.isInteger(parsed.version) || (parsed.version ?? 0) <= 0) {
throw new Error("Invalid canonical config: version must be a positive integer");
throw new Error(
"Invalid canonical config: version must be a positive integer",
);
}

if (!Array.isArray(parsed.allowedExtensionIds)) {
throw new Error("Invalid canonical config: allowedExtensionIds must be an array");
throw new Error(
"Invalid canonical config: allowedExtensionIds must be an array",
);
}

if (parsed.allowedExtensionIds.length === 0) {
Expand Down Expand Up @@ -95,6 +99,7 @@ function listTextFilesRecursively(root: string): string[] {
".turbo",
".idea",
".vscode",
".codex-worktrees",
]);

const allowedExtensions = new Set([
Expand Down Expand Up @@ -196,12 +201,20 @@ describe("Chrome extension allowlist guard", () => {
expect(new Set(ALLOWED_EXTENSION_ORIGINS)).toEqual(expectedOrigins);
});

test("concrete extension IDs appear only in canonical config", () => {
test("concrete extension IDs appear only in canonical config or CWS URLs", () => {
// The canonical extension ID may also appear in Chrome Web Store URLs
// (e.g. chromewebstore.google.com/detail/.../ID) or in documentation
// referencing the published extension. Those are acceptable — they
// reference the published extension, not duplicate config. We flag
// files where the ID appears outside of a CWS URL context.
const config = parseCanonicalConfig();
const allFiles = listTextFilesRecursively(repoRoot);

const CWS_URL_PATTERN =
/chromewebstore\.google\.com\/detail\/[^/]+\/[a-p]{32}/;
Comment on lines +213 to +214

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.

🟡 Missing g flag on CWS_URL_PATTERN causes only the first CWS URL to be stripped

The comment on line 231 says "Strip all CWS URLs" but CWS_URL_PATTERN at assistant/src/__tests__/extension-id-sync-guard.test.ts:214 lacks the g (global) flag. String.replace() without g only replaces the first match. If a file contains the extension ID in two or more CWS URLs, the second occurrence won't be stripped, and stripped.includes(extensionId) will be true, causing a false-positive test failure. Currently no file has multiple CWS URLs, but the code doesn't match its stated intent.

Suggested change
const CWS_URL_PATTERN =
/chromewebstore\.google\.com\/detail\/[^/]+\/[a-p]{32}/;
const CWS_URL_PATTERN =
/chromewebstore\.google\.com\/detail\/[^/]+\/[a-p]{32}/g;
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


for (const extensionId of config.allowedExtensionIds) {
const matches: string[] = [];
const unexpectedMatches: string[] = [];
for (const absPath of allFiles) {
const relPath = absPath.replace(`${repoRoot}/`, "");
let content: string;
Expand All @@ -212,11 +225,17 @@ describe("Chrome extension allowlist guard", () => {
if ((err as NodeJS.ErrnoException).code === "ENOENT") continue;
throw err;
}
if (content.includes(extensionId)) {
matches.push(relPath);
if (!content.includes(extensionId)) continue;
if (relPath === CANONICAL_CONFIG_REL_PATH) continue;

// Strip all CWS URLs and check if the ID still appears — if it
// does, the file is using the ID as a standalone config value.
const stripped = content.replace(CWS_URL_PATTERN, "");
if (stripped.includes(extensionId)) {
unexpectedMatches.push(relPath);
}
}
expect(matches).toEqual([CANONICAL_CONFIG_REL_PATH]);
expect(unexpectedMatches).toEqual([]);
}
});
});
4 changes: 2 additions & 2 deletions clients/chrome-extension/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,10 @@ cd clients/chrome-extension/native-host
bun install
```

2. Export your extension ID(s). Include both the CWS ID and your dev ID if you want both to work:
2. Export your extension ID(s). Include both the CWS ID (from the canonical allowlist) and your dev ID if you want both to work:

```bash
export CWS_EXTENSION_ID=hphbdmpffeigpcdjkckleobjmhhokpne
export CWS_EXTENSION_ID=$(cat ../../meta/browser-extension/chrome-extension-allowlist.json | grep -oE '[a-p]{32}')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Fix allowlist path in manual setup command

This command is documented in a section that explicitly runs from clients/chrome-extension/native-host, but ../../meta/browser-extension/chrome-extension-allowlist.json resolves to clients/meta/... (which does not exist). In that flow CWS_EXTENSION_ID won't be populated correctly, so users can generate a manifest with broken allowed_origins and fail to connect from the published extension. The path should point to repo root (e.g. ../../../meta/...) or use a path-independent lookup.

Useful? React with 👍 / 👎.

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.

🔴 Wrong relative path in README: ../../meta/... should be ../../../meta/...

The manual setup instructions tell the user to cd clients/chrome-extension/native-host in step 1, and step 2 runs cat ../../meta/browser-extension/chrome-extension-allowlist.json from that directory. From clients/chrome-extension/native-host/, ../../ resolves to clients/, making the full path clients/meta/browser-extension/chrome-extension-allowlist.json — which does not exist. The correct relative path to the repo root is ../../../meta/browser-extension/chrome-extension-allowlist.json. Anyone following these instructions will get a "No such file or directory" error.

Suggested change
export CWS_EXTENSION_ID=$(cat ../../meta/browser-extension/chrome-extension-allowlist.json | grep -oE '[a-p]{32}')
export CWS_EXTENSION_ID=$(cat ../../../meta/browser-extension/chrome-extension-allowlist.json | grep -oE '[a-p]{32}')
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

export DEV_EXTENSION_ID=<id from chrome://extensions>
```

Expand Down
Loading