Skip to content

Commit 1b69414

Browse files
committed
fix(security): harden authentication and secret handling
- Keep Memos credentials inside the background worker and fail closed when account verification is unavailable. - Prevent OAuth session races, restrict image downloads, and clear session-derived caches on sign-out. - Make store packaging reproducible and update browser support and reviewer documentation.
1 parent 278e86d commit 1b69414

29 files changed

Lines changed: 1086 additions & 370 deletions

.github/workflows/ci.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,5 @@ jobs:
2727
- run: pnpm lint
2828
- run: pnpm test
2929
- run: pnpm build
30+
- run: pnpm audit --prod
31+
- run: node scripts/package.mjs firefox

README.md

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# Memos Web Clipper
22

3-
Save pages, selections, and images directly to your Memos instance. Available for Chrome and Firefox.
3+
Save pages, selections, and images directly to your Memos instance. Available for Chromium-based browsers and Firefox.
44

55
## Install
66

@@ -32,14 +32,6 @@ Browser-owned pages, such as extension stores and internal browser URLs, may blo
3232

3333
## Browser support
3434

35-
- Google Chrome
35+
- Chromium-based browsers that support Chrome extensions, including Google Chrome, Microsoft Edge, Brave, and Arc
3636
- Mozilla Firefox 142 or later
3737
- Memos 0.26.0 or later in the 0.x series
38-
39-
## Privacy
40-
41-
Memos Web Clipper does not include analytics, advertising, or telemetry. It captures page content only after you open the extension or use its context-menu action.
42-
43-
The extension communicates with usememos.com for sign-in and with your Memos instance when saving a clip. OAuth session data, the clip template, visibility preference, and small connection caches are stored in browser-local extension storage. Signing out removes the local OAuth session.
44-
45-
The extension requests page access to capture the active page, work with selections, reach self-hosted Memos instances, and download selected images from their original hosts.

docs/FIREFOX_REVIEW.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ This extension is written in TypeScript and bundled with Vite, so the matching s
99
- pnpm 11.10.0 (the version pinned in `package.json`)
1010
- The system `zip` command
1111

12-
The source archive contains a `.env` file with only the public OAuth client ID, issuer URL, and web app URL used for the submitted package. No OAuth client secret or server-side Clerk secret is included or needed.
12+
The source archive contains a `.env` file with only the public OAuth client ID, issuer URL, and web app URL used for the submitted package. No OAuth client secret or server-side Clerk secret is included or needed. It also contains `.memos-amo-source.json`, a generated marker identifying the package version and source commit; this lets the extracted reviewer archive build without a `.git` directory.
1313

1414
## Reproduce the submitted Firefox package
1515

@@ -21,6 +21,8 @@ pnpm package:firefox
2121

2222
The package to compare with the AMO upload is written to `artifacts/memos-web-clipper-firefox-v<version>.zip`. The packaging command also runs Mozilla's `web-ext lint` before creating the ZIP.
2323

24+
When run from the reviewer archive, the command creates the Firefox binary only; it does not create another nested source archive. Publisher builds run from a clean Git checkout and additionally create `artifacts/memos-web-clipper-firefox-source-v<version>.zip`. A dirty publisher checkout is rejected before any store artifact is changed.
25+
2426
## Validator warnings
2527

2628
`web-ext lint` currently reports two `UNSAFE_VAR_ASSIGNMENT` warnings in the generated

docs/OAUTH_SETUP.md

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,12 @@ Create one OAuth application for the web clipper with these settings:
1717
unsafe metadata. The extension reads `unsafe_metadata.memos` from `/oauth/userinfo`; it never
1818
writes Clerk metadata. Connection changes remain on usememos.com.
1919

20+
The complete userinfo response is background-only. Popup and Options messages receive only a
21+
display identity or sanitized connection diagnostics, never `unsafe_metadata` or the Memos access
22+
token. Local OAuth session V2 persists only the OAuth access/refresh token set and expiry; an
23+
existing V1 session is migrated without its cached userinfo. If live userinfo verification is
24+
unavailable, privileged writes fail closed instead of falling back to cached connection metadata.
25+
2026
## Redirect URIs
2127

2228
Chromium redirect URIs use this shape:
@@ -60,7 +66,8 @@ required in the extension.
6066
2. Build/load each store variant and collect its exact redirect URI.
6167
3. Register all redirect URIs in Clerk.
6268
4. Set the public OAuth client ID in `.env`.
63-
5. Run `pnpm package` and test sign-in from all three packaged variants.
64-
6. Once the OAuth release is live, remove legacy extension URLs from Clerk `allowed_origins`.
69+
5. Commit the release source so the Git working tree is clean.
70+
6. Run `pnpm package` and test sign-in from all three packaged variants.
71+
7. Once the OAuth release is live, remove legacy extension URLs from Clerk `allowed_origins`.
6572

6673
Keep the normal usememos.com web origins configured for the web app itself.

scripts/package.mjs

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { loadEnv } from "vite";
1414
const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..");
1515
const DIST = join(ROOT, "dist");
1616
const ARTIFACTS = join(ROOT, "artifacts");
17+
const REVIEW_SOURCE_MARKER = ".memos-amo-source.json";
1718
const FIREFOX_ADDON_ID = "web-clipper@usememos.com";
1819
// Firefox desktop gained built-in data consent in 140; Android gained it in 142.
1920
// `gecko.strict_min_version` covers both unless a separate Android manifest is used.
@@ -26,6 +27,31 @@ if (!VALID_TARGETS.has(requestedTarget)) {
2627
}
2728

2829
const packageJson = JSON.parse(readFileSync(join(ROOT, "package.json"), "utf8"));
30+
31+
const requireTrustedSourceTree = () => {
32+
if (existsSync(join(ROOT, ".git"))) {
33+
const status = execFileSync("git", ["status", "--porcelain", "--untracked-files=normal"], {
34+
cwd: ROOT,
35+
encoding: "utf8",
36+
});
37+
if (status.trim()) throw new Error("Refusing to package a dirty working tree. Commit or stash all tracked and untracked files first.");
38+
return true;
39+
}
40+
41+
const markerPath = join(ROOT, REVIEW_SOURCE_MARKER);
42+
if (!existsSync(markerPath))
43+
throw new Error("Packaging requires either a clean Git checkout or an official AMO reviewer source archive.");
44+
const marker = JSON.parse(readFileSync(markerPath, "utf8"));
45+
if (marker.format !== 1 || marker.version !== packageJson.version || !/^(?:[0-9a-f]{40}|[0-9a-f]{64})$/.test(marker.commit)) {
46+
throw new Error("The AMO reviewer source marker is invalid or does not match package.json.");
47+
}
48+
return false;
49+
};
50+
51+
// Every store archive must correspond to one reproducible tracked source tree.
52+
// Official reviewer archives carry a generated marker because they intentionally omit .git.
53+
const isGitCheckout = requireTrustedSourceTree();
54+
2955
const baseManifestPath = join(DIST, "manifest.json");
3056
if (!existsSync(baseManifestPath)) throw new Error("dist/manifest.json is missing; run `pnpm build` first.");
3157

@@ -112,9 +138,9 @@ const packageFirefoxSource = () => {
112138
const sourcePath = join(ARTIFACTS, `memos-web-clipper-firefox-source-v${packageJson.version}.zip`);
113139

114140
try {
115-
// Include committed and non-ignored working-tree files so the archive matches the
116-
// exact source used for this local build, while naturally excluding .env and output.
117-
const files = execFileSync("git", ["ls-files", "-z", "--cached", "--others", "--exclude-standard"], {
141+
// Reviewer source is an exact tracked tree. Local and untracked files must never
142+
// enter a store upload, even when they are not covered by .gitignore.
143+
const files = execFileSync("git", ["ls-files", "-z", "--cached"], {
118144
cwd: ROOT,
119145
encoding: "utf8",
120146
})
@@ -133,6 +159,8 @@ const packageFirefoxSource = () => {
133159
// lets AMO reproduce the bundle without leaking CLERK_SECRET_KEY or other secrets.
134160
const viteEnv = publicViteEnvironment();
135161
if (viteEnv) writeFileSync(join(stage, ".env"), `# Public build-time values used for this submission.\n${viteEnv}\n`);
162+
const commit = execFileSync("git", ["rev-parse", "HEAD"], { cwd: ROOT, encoding: "utf8" }).trim();
163+
writeFileSync(join(stage, REVIEW_SOURCE_MARKER), `${JSON.stringify({ format: 1, version: packageJson.version, commit }, null, 2)}\n`);
136164

137165
zipDirectory(stage, sourcePath);
138166
} finally {
@@ -157,7 +185,7 @@ const packageFirefox = () => {
157185
rmSync(stage, { recursive: true, force: true });
158186
}
159187

160-
return { firefoxPath, sourcePath: packageFirefoxSource() };
188+
return { firefoxPath, sourcePath: isGitCheckout ? packageFirefoxSource() : null };
161189
};
162190

163191
const created = [];
@@ -166,7 +194,8 @@ if (requestedTarget === "all" || requestedTarget === "chrome" || requestedTarget
166194
}
167195
if (requestedTarget === "all" || requestedTarget === "firefox") {
168196
const { firefoxPath, sourcePath } = packageFirefox();
169-
created.push(firefoxPath, sourcePath);
197+
created.push(firefoxPath);
198+
if (sourcePath) created.push(sourcePath);
170199
}
171200

172201
console.log("\nCreated store artifacts:");

src/__tests__/background.dom.test.ts

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import { beforeEach, describe, expect, it, vi } from "vitest";
2+
import { SAVE_ATTEMPTS_KEY } from "@/background/memo-save";
23
import { VERSION_CACHE_KEY } from "@/lib/instance-version";
4+
import { POPUP_STATE_KEY } from "@/lib/popup-state";
35
import { CLIP_TEMPLATE_KEY } from "@/lib/template-settings";
46
import { browserMock, seedStorage } from "@/test/browser-mock";
57
import { jsonResponse, testCreds } from "@/test/fixtures";
@@ -22,11 +24,23 @@ const oauthMocks = vi.hoisted(() => ({
2224
beginOAuthSignIn: vi.fn(async () => undefined),
2325
clearOAuthSession: vi.fn(async () => undefined),
2426
getOAuthUser: vi.fn(),
27+
OAuthUnavailableError: class OAuthUnavailableError extends Error {
28+
constructor() {
29+
super("OAuth unavailable");
30+
this.name = "OAuthUnavailableError";
31+
}
32+
},
2533
}));
2634
vi.mock("@/auth/oauth-session", () => ({
2735
beginOAuthSignIn: oauthMocks.beginOAuthSignIn,
2836
clearOAuthSession: oauthMocks.clearOAuthSession,
2937
getOAuthUser: oauthMocks.getOAuthUser,
38+
OAuthUnavailableError: oauthMocks.OAuthUnavailableError,
39+
toOAuthIdentity: (user: { id: string; displayName: string; imageUrl?: string }) => ({
40+
id: user.id,
41+
displayName: user.displayName,
42+
...(user.imageUrl ? { imageUrl: user.imageUrl } : {}),
43+
}),
3044
}));
3145

3246
const memos = (extra: Record<string, unknown> = {}) => ({
@@ -38,6 +52,7 @@ const ready = () => {
3852
};
3953
const expected = { expectedUserId: "user_123", expectedInstanceUrl: testCreds.instanceUrl };
4054
const popupSender = { id: "test-id", url: "chrome-extension://test-id/src/popup/index.html" };
55+
const optionsSender = { id: "test-id", url: "chrome-extension://test-id/src/options/index.html" };
4156
const emitRuntime = (message: unknown, sender = popupSender) => browserMock.runtime.onMessage.emitFirst(message, sender);
4257

4358
// Import once: the module registers its listeners on the shared browser mock at load.
@@ -57,6 +72,12 @@ beforeEach(async () => {
5772
await import("@/background");
5873
});
5974

75+
describe("background — storage isolation", () => {
76+
it("restricts storage.local to trusted extension contexts", () => {
77+
expect(browserMock.storage.local.setAccessLevel).toHaveBeenCalledWith({ accessLevel: "TRUSTED_CONTEXTS" });
78+
});
79+
});
80+
6081
describe("background — SAVE_MEMO message", () => {
6182
beforeEach(ready);
6283

@@ -155,12 +176,98 @@ describe("background — SAVE_MEMO message", () => {
155176
vi.unstubAllGlobals();
156177
});
157178

179+
it("does not fetch an image from a private or loopback address", async () => {
180+
const privateImage = "https://127.0.0.1/admin.png";
181+
const mappedLoopbackImage = "https://[::ffff:127.0.0.1]/admin.png";
182+
const fetchMock = vi.fn((url: unknown) => {
183+
if (String(url).endsWith("/api/v1/memos")) return Promise.resolve(jsonResponse({ name: "memos/7", uid: "xy" }));
184+
return Promise.resolve(new Response(null, { status: 404 }));
185+
});
186+
vi.stubGlobal("fetch", fetchMock);
187+
188+
const result = await emitRuntime({
189+
type: "SAVE_MEMO",
190+
content: "hello",
191+
visibility: "PRIVATE",
192+
...expected,
193+
images: [privateImage, mappedLoopbackImage],
194+
});
195+
196+
expect(result).toEqual({ ok: true, webUrl: "https://memos.example.com/memos/xy", failedImages: 2 });
197+
expect(fetchMock.mock.calls.some(([url]) => [privateImage, mappedLoopbackImage].includes(String(url)))).toBe(false);
198+
vi.unstubAllGlobals();
199+
});
200+
201+
it("rejects an oversized image before buffering or uploading it", async () => {
202+
const fetchMock = vi.fn((url: unknown) => {
203+
const value = String(url);
204+
if (value === "https://cdn.example.com/huge.png") {
205+
return Promise.resolve(
206+
new Response(new Uint8Array([1]), {
207+
headers: { "content-type": "image/png", "content-length": String(10 * 1024 * 1024 + 1) },
208+
}),
209+
);
210+
}
211+
if (value.endsWith("/api/v1/memos")) return Promise.resolve(jsonResponse({ name: "memos/7", uid: "xy" }));
212+
return Promise.resolve(new Response(null, { status: 404 }));
213+
});
214+
vi.stubGlobal("fetch", fetchMock);
215+
216+
const result = await emitRuntime({
217+
type: "SAVE_MEMO",
218+
content: "hello",
219+
visibility: "PRIVATE",
220+
...expected,
221+
images: ["https://cdn.example.com/huge.png"],
222+
});
223+
224+
expect(result).toEqual({ ok: true, webUrl: "https://memos.example.com/memos/xy", failedImages: 1 });
225+
expect(fetchMock.mock.calls.some(([url]) => String(url).endsWith("/api/v1/attachments"))).toBe(false);
226+
vi.unstubAllGlobals();
227+
});
228+
229+
it("rejects active SVG content instead of uploading it as an attachment", async () => {
230+
const fetchMock = vi.fn((url: unknown) => {
231+
const value = String(url);
232+
if (value === "https://cdn.example.com/active.svg") {
233+
return Promise.resolve(new Response("<svg><script>alert(1)</script></svg>", { headers: { "content-type": "image/svg+xml" } }));
234+
}
235+
if (value.endsWith("/api/v1/memos")) return Promise.resolve(jsonResponse({ name: "memos/7", uid: "xy" }));
236+
return Promise.resolve(new Response(null, { status: 404 }));
237+
});
238+
vi.stubGlobal("fetch", fetchMock);
239+
240+
const result = await emitRuntime({
241+
type: "SAVE_MEMO",
242+
content: "hello",
243+
visibility: "PRIVATE",
244+
...expected,
245+
images: ["https://cdn.example.com/active.svg"],
246+
});
247+
248+
expect(result).toEqual({ ok: true, webUrl: "https://memos.example.com/memos/xy", failedImages: 1 });
249+
expect(fetchMock.mock.calls.some(([url]) => String(url).endsWith("/api/v1/attachments"))).toBe(false);
250+
vi.unstubAllGlobals();
251+
});
252+
158253
it("returns not-configured when there is no Memos connection", async () => {
159254
mockUser = { id: "user_123", unsafeMetadata: {} };
160255
const result = await emitRuntime({ type: "SAVE_MEMO", content: "hi", visibility: "PRIVATE", ...expected });
161256
expect(result).toEqual({ ok: false, errorKind: "not-configured" });
162257
});
163258

259+
it("fails closed when the OAuth identity service is temporarily unavailable", async () => {
260+
oauthMocks.getOAuthUser.mockRejectedValueOnce(new oauthMocks.OAuthUnavailableError());
261+
const fetchMock = vi.fn();
262+
vi.stubGlobal("fetch", fetchMock);
263+
264+
const result = await emitRuntime({ type: "SAVE_MEMO", content: "hi", visibility: "PRIVATE", ...expected });
265+
266+
expect(result).toEqual({ ok: false, errorKind: "auth-unavailable" });
267+
expect(fetchMock).not.toHaveBeenCalled();
268+
vi.unstubAllGlobals();
269+
});
270+
164271
it("maps an InstanceError to its kind (401 -> unauthorized)", async () => {
165272
vi.stubGlobal("fetch", vi.fn().mockResolvedValue(jsonResponse({}, 401)));
166273
const result = await emitRuntime({ type: "SAVE_MEMO", content: "hi", visibility: "PRIVATE", ...expected });
@@ -268,6 +375,49 @@ describe("background — popup state", () => {
268375
});
269376
});
270377

378+
describe("background — sanitized options protocol", () => {
379+
beforeEach(ready);
380+
381+
it("returns identity fields without unsafe metadata or either access token", async () => {
382+
const result = await emitRuntime({ type: "GET_AUTH_USER" }, optionsSender);
383+
384+
expect(result).toEqual({ id: "user_123", displayName: "Steven Li" });
385+
expect(JSON.stringify(result)).not.toContain("unsafeMetadata");
386+
expect(JSON.stringify(result)).not.toContain(testCreds.accessToken);
387+
});
388+
389+
it("returns only sanitized connection diagnostics", async () => {
390+
const result = await emitRuntime({ type: "GET_CONNECTION_STATE", refresh: false }, optionsSender);
391+
392+
expect(result).toEqual({
393+
instanceUrl: testCreds.instanceUrl,
394+
version: "0.29.1",
395+
status: "ready",
396+
verificationError: null,
397+
isUsingCachedVersion: true,
398+
});
399+
expect(JSON.stringify(result)).not.toContain(testCreds.accessToken);
400+
});
401+
});
402+
403+
describe("background — sign-out", () => {
404+
beforeEach(ready);
405+
406+
it("clears popup, version, and ambiguous-save caches before broadcasting", async () => {
407+
seedStorage({
408+
[POPUP_STATE_KEY]: { status: "ready" },
409+
[VERSION_CACHE_KEY]: { instanceUrl: testCreds.instanceUrl, version: "0.29.1" },
410+
[SAVE_ATTEMPTS_KEY]: { request_1: { fingerprint: "x", startedAt: Date.now() } },
411+
});
412+
413+
await emitRuntime({ type: "SIGN_OUT" }, optionsSender);
414+
415+
await expect(browserMock.storage.local.get([POPUP_STATE_KEY, VERSION_CACHE_KEY, SAVE_ATTEMPTS_KEY])).resolves.toEqual({});
416+
expect(oauthMocks.clearOAuthSession).toHaveBeenCalledOnce();
417+
expect(browserMock.runtime.sendMessage).toHaveBeenCalledWith({ type: "AUTH_CHANGED" });
418+
});
419+
});
420+
271421
describe("background — sign-in flow", () => {
272422
it("runs OAuth PKCE, refreshes state, and returns to options", async () => {
273423
ready();

0 commit comments

Comments
 (0)