Readplace

Add Chrome extension for saving URLs to Hutch reading list by FagnerMartinsBrack · Pull Request #104 · Readplace/readplace.com

GitHub ~3 min read
View original
Crawl details
  • current
Summary (TL;DR)
A code review identifies critical security issues in a Chrome extension project. The S3 bucket serving extension builds is publicly readable with no access controls, allowing anyone to list and download all objects. The review also flags unsafe 'as' type assertions in background.ts and popup.ts that bypass TypeScript's type system, recommending Zod schema validation at message boundaries. Other issues include missing project scopes in commit messages, use of innerHTML instead of replaceChildren for DOM clearing, a broad content script match pattern (<all_urls>) for a keyboard shortcut, forceDestroy:true on the S3 bucket risking accidental permanent deletion, hardcoded port 3001 in E2E tests, and direct use of process.env instead of requireEnv in build scripts. High priority: fix S3 bucket access controls.

High Priority Issues (Must Fix)

1. S3 bucket is publicly readable with no access controls

projects/chrome-extension/src/infra/index.ts:12-43 — The S3 bucket is configured with all public access blocks disabled and a bucket policy granting s3:GetObject to Principal: "*" plus s3:ListBucket to Principal: "*". This means anyone on the internet can list every object in the bucket and download any file. If extension builds contain source maps, debug symbols, or are uploaded alongside internal artifacts, those become publicly enumerable and downloadable. The ListBucket permission is particularly risky — it lets anyone discover every key in the bucket.

If this bucket is meant to serve extension downloads publicly, at minimum remove the PublicListBucket statement so directory listing is not exposed, and consider using CloudFront with an OAI instead of raw public S3.

Fix this →

Medium Priority Issues

1. as type assertions in background.ts bypass the type system

projects/chrome-extension/src/runtime/background/background.ts:31,46,210,214 — Per CLAUDE.md, as type assertions should be avoided because they bypass the compiler and hide type mismatches. The background message listener casts raw using as { type: string }, as { target?: string }, and as PopupMessage without any runtime validation. If the message shape changes upstream, these casts will silently hide the mismatch.

Consider using a Zod schema at the message boundary to validate incoming messages:

const PopupMessageSchema = z.discriminatedUnion("type", [...]);
const parsed = PopupMessageSchema.safeParse(raw);

The as OAuthTokens cast in tokenStorage.getTokens() (line 31) is similarly unsafe for data coming from browser storage.

Fix this →

2. as type assertions in popup.ts bypass the type system

projects/chrome-extension/src/runtime/popup/popup.ts:148-151,170,212-215,230-234,247,276-279,303 — Similar to background.ts, the popup uses as GuardedResult<...> and as HTMLInputElement casts extensively. These are at system boundaries (messages from the background script) and should use Zod or at least runtime checks. The as HTMLInputElement on line 170 could use assert(filterInput instanceof HTMLInputElement) instead.

3. Commit messages missing project scope per Conventional Commits

Per the git-commit skill, commit scopes must be the name property from package.json. The fix commits use bare fix: without a scope:

  • fix: resolve CI failure (attempt #2)
  • fix: skip Chrome extension E2E tests in CI (attempt #1)

These should be fix(chrome-extension): since the changes are scoped to that project. The feat commit feat: add Chrome browser extension (Manifest V3) could reasonably omit scope since it adds an entirely new project, though feat(chrome-extension): would be more precise.

Low Priority Suggestions

1. innerHTML = "" for clearing DOM elements

projects/chrome-extension/src/runtime/popup/popup.ts:35,94 — Using innerHTML = "" to clear children works but replaceChildren() (no arguments) is the modern alternative that doesn't involve the HTML parser.

2. Content script matches <all_urls> for keyboard shortcut

projects/chrome-extension/src/runtime/manifest.json:41-42 — The content script (shortcut.ts) is injected into every page the user visits just to intercept Ctrl+D. This adds a small overhead to every page load. Consider narrowing the match pattern if there are specific domains where this is most useful, or document why <all_urls> is necessary.

3. forceDestroy: true on S3 bucket

projects/chrome-extension/src/infra/index.ts:9 — forceDestroy: true allows Pulumi to delete the bucket even when it contains objects. This is convenient for dev but risky for production — an accidental pulumi destroy will permanently delete all extension builds with no recovery. Consider removing this for the prod stack.

4. E2E test uses hardcoded port 3001

projects/chrome-extension/src/e2e/login-flow/run.e2e-local.ts:16 — The test server port is hardcoded. If another process uses port 3001, the test will fail with an opaque error. Consider detecting a free port or documenting this dependency.

5. process.env.HUTCH_SERVER_URL used directly in build scripts

projects/chrome-extension/scripts/build-extension.js:9 and projects/chrome-extension/package.json:8 — CLAUDE.md says to use requireEnv/getEnv instead of process.env directly. However, these are Node.js build scripts (not runtime), so this may be an acceptable exception. Worth noting for consistency.

This PR has issues that should be addressed before merging.