Skip to content

Commit 7937621

Browse files
committed
feat: security hardening
feat(jetstream-desktop): enhance external URL handling with safe scheme checks fix(jetstream-desktop): ensure file downloads use the correct basename for paths fix(jetstream-desktop): improve notification service to use safe URL opening fix(jetstream-desktop): update protocol service to use basename for download filenames feat(jetstream-desktop): add utility for safe external URL opening fix(web-extension): restrict message event handling to same-origin and window source refactor(web-extension): remove external message listener to enhance security fix(auth.db.service): improve logging and ensure correct session revocation feat(sync.types): add maximum nesting depth check for sync data payloads
1 parent ab68c3f commit 7937621

11 files changed

Lines changed: 573 additions & 68 deletions

File tree

SECURITY_REVIEW.md

Lines changed: 466 additions & 0 deletions
Large diffs are not rendered by default.

apps/geo-ip-api/src/main.ts

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,21 @@ import { ENV, getLogger, httpLogger, logger, requestContextMiddleware } from '@j
22
import { GeoIpLookupResult } from '@jetstream/types';
33
import { json, urlencoded } from 'body-parser';
44
import express from 'express';
5+
import { createHash, timingSafeEqual } from 'node:crypto';
56
import { z, ZodError } from 'zod';
67
import { downloadMaxMindDb, initMaxMind, lookupIpAddress, validateIpAddress } from './maxmind.service';
78
import { createRoute } from './route.utils';
89

10+
/**
11+
* Constant-time credential comparison. Both values are hashed to a fixed-length digest first so the
12+
* comparison does not leak length and does not throw on mismatched lengths.
13+
*/
14+
function timingSafeStringCompare(providedValue: string, expectedValue: string): boolean {
15+
const providedHash = createHash('sha256').update(String(providedValue)).digest();
16+
const expectedHash = createHash('sha256').update(String(expectedValue)).digest();
17+
return timingSafeEqual(providedHash, expectedHash);
18+
}
19+
920
const DISK_PATH = process.env.DISK_PATH ?? __dirname;
1021

1122
if (ENV.ENVIRONMENT !== 'development' && (!ENV.GEO_IP_API_USERNAME || !ENV.GEO_IP_API_PASSWORD)) {
@@ -39,7 +50,11 @@ app.use((req, res, next) => {
3950
throw new Error('Unauthorized');
4051
}
4152
const [username, password] = Buffer.from(token, 'base64').toString().split(':');
42-
if (username !== ENV.GEO_IP_API_USERNAME || password !== ENV.GEO_IP_API_PASSWORD) {
53+
// Evaluate both comparisons (no short-circuit) with a constant-time compare so response time
54+
// does not reveal whether the username alone was correct.
55+
const usernameMatches = timingSafeStringCompare(username ?? '', ENV.GEO_IP_API_USERNAME ?? '');
56+
const passwordMatches = timingSafeStringCompare(password ?? '', ENV.GEO_IP_API_PASSWORD ?? '');
57+
if (!usernameMatches || !passwordMatches) {
4358
throw new Error('Unauthorized');
4459
}
4560
next();
@@ -101,7 +116,7 @@ app.get(
101116
*/
102117
app.post(
103118
'/api/lookup',
104-
createRoute({ body: z.object({ ips: z.string().array() }) }, async ({ body }, _, res, next) => {
119+
createRoute({ body: z.object({ ips: z.string().array().max(1000) }) }, async ({ body }, _, res, next) => {
105120
try {
106121
const ipAddresses = body.ips;
107122
await initMaxMind(DISK_PATH);

apps/jetstream-desktop/src/browser/browser.ts

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
import { HTTP } from '@jetstream/shared/constants';
2-
import { app, BrowserWindow, clipboard, Menu, nativeImage, shell } from 'electron';
2+
import { app, BrowserWindow, clipboard, Menu, nativeImage } from 'electron';
33
import log from 'electron-log/main';
44
import * as path from 'path';
55
import { ENV } from '../config/environment';
66
import { initApiConnection } from '../utils/route.utils';
7+
import { openExternalSafe } from '../utils/url.utils';
78
import { isMac } from '../utils/utils';
89
import { getWindowConfig } from './config';
910

@@ -48,16 +49,15 @@ export class Browser {
4849
if (connection) {
4950
connection.jetstreamConn.org.identity().then(() => {
5051
const sfdcUrl = connection.jetstreamConn.org.getFrontdoorLoginUrl(returnUrl);
51-
shell.openExternal(sfdcUrl);
52+
openExternalSafe(sfdcUrl);
5253
});
5354
return { action: 'deny' };
5455
}
5556
}
5657

57-
// Open all external links in the browser
58-
// FIXME: this should be more protective over what is externally opened
58+
// Open all external links in the browser (only safe schemes are actually opened)
5959
if (url.host !== clientUrl.host) {
60-
shell.openExternal(details.url);
60+
openExternalSafe(details.url);
6161
return { action: 'deny' };
6262
}
6363

@@ -68,6 +68,26 @@ export class Browser {
6868
};
6969
});
7070

71+
// Prevent the renderer from navigating the privileged window away from the app.
72+
// Anything cross-origin is blocked and, if it uses a safe scheme, handed to the default browser
73+
// instead. In-app (SPA) routing uses the history API and does not trigger these events.
74+
const handleWindowNavigation = (event: Electron.Event, targetUrl: string) => {
75+
let parsedUrl: URL;
76+
try {
77+
parsedUrl = new URL(targetUrl);
78+
} catch {
79+
event.preventDefault();
80+
return;
81+
}
82+
const isSameAppOrigin = parsedUrl.protocol === clientUrl.protocol && parsedUrl.host === clientUrl.host;
83+
if (!isSameAppOrigin) {
84+
event.preventDefault();
85+
openExternalSafe(targetUrl);
86+
}
87+
};
88+
browserWindow.webContents.on('will-navigate', handleWindowNavigation);
89+
browserWindow.webContents.on('will-redirect', handleWindowNavigation);
90+
7191
this.createDock();
7292

7393
browserWindow.once('ready-to-show', () => {

apps/jetstream-desktop/src/services/file-download.service.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { Maybe } from '@jetstream/types';
55
import { app, dialog, WebContents } from 'electron';
66
import logger from 'electron-log';
77
import { createWriteStream, existsSync } from 'node:fs';
8-
import { join } from 'node:path';
8+
import { basename, join } from 'node:path';
99
import { Readable } from 'stream';
1010
import { pipeline } from 'stream/promises';
1111
import { setRecentDocument } from '../utils/utils';
@@ -33,7 +33,7 @@ export async function downloadAndZipFilesToDisk(
3333
let downloadPath: string;
3434

3535
if (downloadPreferences?.omitPrompt && downloadPreferences?.downloadPath && existsSync(downloadPreferences.downloadPath)) {
36-
downloadPath = join(downloadPreferences.downloadPath, zipFileName);
36+
downloadPath = join(downloadPreferences.downloadPath, basename(zipFileName));
3737
} else {
3838
const defaultPath = app.getPath('downloads');
3939
const result = await dialog.showSaveDialog({
@@ -125,7 +125,7 @@ export async function downloadBulkApiFileAndSaveToDisk({
125125
let downloadPath: string;
126126

127127
if (downloadPreferences?.omitPrompt && downloadPreferences?.downloadPath && existsSync(downloadPreferences.downloadPath)) {
128-
downloadPath = join(downloadPreferences.downloadPath, fileName);
128+
downloadPath = join(downloadPreferences.downloadPath, basename(fileName));
129129
} else {
130130
const defaultPath = app.getPath('downloads');
131131
const result = await dialog.showSaveDialog({

apps/jetstream-desktop/src/services/notification.service.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { Maybe } from '@jetstream/types';
2-
import { BrowserWindow, dialog, Notification, shell } from 'electron';
2+
import { BrowserWindow, dialog, Notification } from 'electron';
33
import logger from 'electron-log';
4+
import { openExternalSafe } from '../utils/url.utils';
45
import { checkNotifications } from './api.service';
56
import * as dataService from './persistence.service';
67

@@ -40,7 +41,7 @@ export function showCriticalNotification(title: string, message: string, actionU
4041

4142
notification.on('click', () => {
4243
if (actionUrl) {
43-
shell.openExternal(actionUrl);
44+
openExternalSafe(actionUrl);
4445
}
4546
});
4647

@@ -61,6 +62,6 @@ export function showCriticalConfirmationDialog(title: string, message: string, a
6162
});
6263

6364
if (response === 0 && actionUrl) {
64-
shell.openExternal(actionUrl);
65+
openExternalSafe(actionUrl);
6566
}
6667
}

apps/jetstream-desktop/src/services/protocol.service.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ export function registerDownloadHandler() {
141141
) {
142142
return;
143143
}
144-
const downloadFilename = join(downloadPreferences.downloadPath, item.getFilename());
144+
const downloadFilename = join(downloadPreferences.downloadPath, path.basename(item.getFilename()));
145145
item.setSavePath(downloadFilename);
146146
item.once('done', (_, state) => {
147147
if (state === 'completed') {
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import { shell } from 'electron';
2+
import log from 'electron-log/main';
3+
4+
/**
5+
* Schemes that are safe to hand to the OS via shell.openExternal.
6+
* Everything else (file:, smb:/UNC, and custom-protocol handlers) can be coerced by the OS into
7+
* launching a local file or another application, so those are blocked.
8+
*/
9+
const EXTERNAL_URL_ALLOWED_PROTOCOLS = new Set(['https:', 'http:', 'mailto:']);
10+
11+
/**
12+
* Open a URL in the user's default browser, but only if it uses a safe scheme.
13+
* Use this everywhere instead of calling shell.openExternal directly so a renderer-supplied or
14+
* server-supplied URL cannot trigger a dangerous scheme.
15+
*/
16+
export function openExternalSafe(rawUrl: string): void {
17+
try {
18+
const { protocol } = new URL(rawUrl);
19+
if (EXTERNAL_URL_ALLOWED_PROTOCOLS.has(protocol)) {
20+
shell.openExternal(rawUrl);
21+
} else {
22+
log.warn(`Blocked shell.openExternal for disallowed scheme: ${protocol}`);
23+
}
24+
} catch {
25+
log.warn('Blocked shell.openExternal for an invalid URL');
26+
}
27+
}

apps/jetstream-web-extension/src/extension-scripts/content-auth-script.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ const EVENT_MAP = {
1313
} as const;
1414

1515
window.addEventListener('message', async (event) => {
16-
if (event.origin !== targetOrigin) {
16+
// Only accept messages posted by this same page (not an embedding/opener frame) and from our origin
17+
if (event.source !== window || event.origin !== targetOrigin) {
1718
return;
1819
}
1920
if (event.data?.message === EVENT_MAP.ACKNOWLEDGE) {

apps/jetstream-web-extension/src/extension-scripts/service-worker.ts

Lines changed: 8 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import { initApiClientAndOrg } from '../utils/api-client';
1616
import {
1717
AUTH_CHECK_INTERVAL_MIN,
1818
AuthTokensStorage,
19-
eventPayload,
2019
ExternalIdentifier,
2120
ExtIdentifierStorage,
2221
GetPageUrl,
@@ -154,51 +153,15 @@ browser.commands.onCommand.addListener(async (command, tab) => {
154153
});
155154

156155
/**
157-
* Handle authentication events from Jetstream server
158-
* User is redirected and authenticated on the Jetstream server
159-
* and tokens are sent back to the extension and stored in chrome storage
156+
* NOTE: There is intentionally no `onMessageExternal` listener.
157+
*
158+
* By default `onMessageExternal` is reachable by any other installed extension (and the extension
159+
* id is public), which previously allowed a co-installed extension to read this device's identifier
160+
* and inject an attacker-owned, device-bound auth token (session fixation). The legitimate login
161+
* flow runs entirely through the origin-validated `contentAuthScript` content script (injected only
162+
* on getjetstream.app/web-extension/*) and the internal `onMessage` handler below, so no external
163+
* messaging surface is required.
160164
*/
161-
browser.runtime.onMessageExternal.addListener(async (message: unknown) => {
162-
try {
163-
logger.log('Received message from external extension', message);
164-
const event = eventPayload.parse(message);
165-
switch (event.type) {
166-
case 'EXT_IDENTIFIER': {
167-
let result = (await browser.storage.sync.get(storageTypes.extIdentifier.key)) as Partial<ExtIdentifierStorage>;
168-
if (!result.extIdentifier) {
169-
result = { extIdentifier: { id: crypto.randomUUID() } };
170-
await browser.storage.sync.set(result);
171-
storageSyncCache.extIdentifier = result.extIdentifier;
172-
}
173-
if (!result.extIdentifier?.id) {
174-
throw new Error('Could not get or initialize extension identifier');
175-
}
176-
logger.info('Extension identifier', result.extIdentifier.id);
177-
return { success: true, data: result.extIdentifier.id };
178-
}
179-
case 'TOKENS': {
180-
const { exp, userProfile } = jwtDecode<JwtPayload>(event.data.accessToken);
181-
const expiresAt = exp ? fromUnixTime(exp) : new Date();
182-
const authState = {
183-
accessToken: event.data.accessToken,
184-
userProfile,
185-
expiresAt: expiresAt.getTime(),
186-
lastChecked: null,
187-
loggedIn: true,
188-
};
189-
await browser.storage.sync.set({ [storageTypes.authTokens.key]: authState });
190-
storageSyncCache.authTokens = authState;
191-
return { success: true };
192-
}
193-
default: {
194-
return { success: false, error: 'Unknown message type' };
195-
}
196-
}
197-
} catch (ex) {
198-
logger.error('Error handling message', ex);
199-
return { success: false, error: 'Error handling message' };
200-
}
201-
});
202165

203166
// connections seem to continually get reset
204167
// and we cannot make async calls in the fetch event listener to get from storage

libs/auth/server/src/lib/auth.db.service.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,7 @@ export async function hasRememberDeviceRecord({
315315
if (rememberMe.userAgent && userAgent) {
316316
const isSimilar = checkUserAgentSimilarity(rememberMe.userAgent, userAgent);
317317
if (!isSimilar) {
318-
logger.warn({ deviceId, userId }, `User agent mismatch for remembered device: ${rememberMe.userAgent} !== ${userAgent}`);
318+
logger.warn({ userId }, `User agent mismatch for remembered device: ${rememberMe.userAgent} !== ${userAgent}`);
319319
return false;
320320
}
321321
}
@@ -834,8 +834,8 @@ export async function revokeExternalSession(userId: string, sessionId: string) {
834834
if (!userId || !sessionId) {
835835
throw new Error('Invalid parameters');
836836
}
837-
await prisma.webExtensionToken.delete({
838-
where: { id: sessionId },
837+
await prisma.webExtensionToken.deleteMany({
838+
where: { id: sessionId, userId },
839839
});
840840
}
841841

0 commit comments

Comments
 (0)