Skip to content

Commit 87b510b

Browse files
K. Kaushik ReddyK. Kaushik Reddy
authored andcommitted
fix bugs for IDE: port forwarding backend sync + disable devtools
1 parent 445d536 commit 87b510b

14 files changed

Lines changed: 773 additions & 35 deletions

File tree

CLAUDE.md

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,11 +130,48 @@ project** — `cd desktop && npm install`, not part of the root `package.json`.
130130
```bash
131131
cd desktop
132132
npm start # run npm run dev # run with DevTools
133+
npm test # drift guard for the DevTools kill switch
133134
npm run dist:mac # dmg + zip (arm64 + x64), also :win / :linux
134135
npm run smoke -- --url https://deepsql.example.com # headless connection check
135136
npm run selftest:tunnel # end-to-end SSH tunnel test (in-process SSH server)
137+
npm run selftest:settings # proves an edited setting reaches the live connection
136138
```
137139

140+
**A saved profile edit rebuilds the live connection; saving alone was never the
141+
bug.** The launcher persists the form before every Connect and Test, so
142+
`profiles.json` was always correct — but `transport.connect()` reused any live
143+
connection unconditionally, so changing a tunnel's remote port and pressing
144+
Connect did nothing, and Test reported a confident pass for settings the user had
145+
just replaced. `profiles.transportFingerprint()` now decides whether a live
146+
connection still *is* the connection being asked for; `transport.reconcile()`
147+
rebuilds it on save (`ipc.saveAndReconcile`), and `Workspace.updateProfile()`
148+
re-points the window, since a rebuilt tunnel binds a different local port and so
149+
changes the origin. The fingerprint deliberately excludes `name` (a rename must
150+
not drop a tunnel) and `stickyLocalPort` (chosen by us and rewritten every
151+
connect — including it would make a connection differ from itself). A failed
152+
rebuild does **not** restore the old connection: it was built from settings that
153+
no longer exist, so it stays closed and the failure is reported. Entries also
154+
store a profile re-read *after* the connect path's trust-on-first-use writes, or
155+
the next connect would see a mismatch it caused itself.
156+
157+
**DevTools are disabled in packaged builds, and `IS_DEV` is the wrong switch for
158+
it.** Every window passes `webPreferences.devTools: DEVTOOLS_ENABLED`, defined in
159+
`config.js` as `!app.isPackaged` and nothing else. Do not "simplify" it to
160+
`IS_DEV`: `IS_DEV` is also true when `DEEPSQL_DESKTOP_DEV=1`, which any user can
161+
export against the shipped app — that is precisely the hole this closes, and it
162+
used to open DevTools automatically on both windows with no menu item involved.
163+
`devTools: false` is the load-bearing part (Chromium then refuses to attach at
164+
all, making `openDevTools()` a no-op); removing the menu item only hides the
165+
door, though it also drops the `Alt+Cmd+I`/`Ctrl+Shift+I` binding, since a custom
166+
`Menu.setApplicationMenu` means Electron contributes no `toggleDevTools` role.
167+
Separately, `index.js` exits on `--remote-debugging-port` and friends: those open
168+
a DevTools *protocol* endpoint that `devTools: false` does not cover. Verified
169+
behaviourally on Electron 43 (`devTools:false``isDevToolsOpened()` stays false
170+
after `openDevTools()`; a `devTools:true` control opens, so the check is not
171+
vacuous). `desktop/src/main/devtools.test.js` fails the build if a new
172+
`webPreferences` block omits `devTools` — the regression is otherwise silent,
173+
since Chromium's default is *enabled*.
174+
138175
**It is a thin client and deliberately does not bundle the React frontend.** It
139176
navigates a `WebContentsView` at the real DeepSQL origin, so the UI is always the
140177
version the VM is running — no bundle/backend skew, and no second copy of 40+

desktop/README.md

Lines changed: 86 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,8 @@ launch needs **right-click → Open** (or `xattr -dr com.apple.quarantine
6161
cd desktop
6262
npm install
6363
npm start # run the app
64-
npm run dev # run with DevTools open
64+
npm run dev # run with DevTools open (unpackaged only — see "DevTools")
65+
npm test # drift guard for the DevTools kill switch
6566
```
6667

6768
## Building installers
@@ -195,6 +196,81 @@ connect time and says so, instead of letting it surface as a failed login.
195196
everything else goes to the OS browser.
196197
- **No credential proxying**: the client never sees database credentials. It
197198
speaks to DeepSQL's own API surface exactly as a browser does.
199+
- **DevTools are off in packaged builds** — see below.
200+
201+
## DevTools
202+
203+
A packaged build cannot open DevTools. Every window sets
204+
`webPreferences.devTools: DEVTOOLS_ENABLED`, and `config.js` defines that as
205+
`!app.isPackaged` — nothing else. Chromium then refuses to attach DevTools at
206+
all, so `openDevTools()` is a no-op and the shortcuts do nothing; the View menu
207+
omits **Toggle Developer Tools** (and with it the `Alt+Cmd+I` / `Ctrl+Shift+I`
208+
binding, which that item owned, since the app installs its own menu and so gets
209+
no `toggleDevTools` role from Electron).
210+
211+
`DEVTOOLS_ENABLED` is deliberately **not** `IS_DEV`. `IS_DEV` is true whenever
212+
`DEEPSQL_DESKTOP_DEV=1`, and any user can set that on the shipped app —
213+
`DEEPSQL_DESKTOP_DEV=1 open -a DeepSQL` used to open DevTools automatically on
214+
both windows, no menu involved. Gating on `app.isPackaged` alone is what makes
215+
the switch unreachable from outside the build.
216+
217+
The app also refuses to start when passed `--remote-debugging-port`,
218+
`--remote-debugging-pipe`, `--remote-allow-origins`, or `--inspect*`. Those open
219+
a DevTools *protocol* endpoint, a separate door that `devTools: false` does not
220+
close, and Chromium parses them before any app code runs — so the only remedy is
221+
to exit immediately, before a window opens or a tunnel comes up.
222+
223+
Development is unaffected: `npm start` and `npm run dev` are unpackaged, so
224+
DevTools work as before.
225+
226+
**Verifying the block** (source assertions cannot prove runtime behaviour):
227+
228+
```bash
229+
npm test # drift guard: every window sets devTools, gated correctly
230+
npm run dist:mac # then, in the installed app:
231+
# View menu has no "Toggle Developer Tools"; Alt+Cmd+I does nothing
232+
DEEPSQL_DESKTOP_DEV=1 open -a DeepSQL # no DevTools — the closed hole
233+
/Applications/DeepSQL.app/Contents/MacOS/DeepSQL --remote-debugging-port=9222
234+
# exits 1, logging "refusing to start with remote debugging enabled"
235+
```
236+
237+
Known limits, stated plainly: `ELECTRON_RUN_AS_NODE=1` turns the binary into a
238+
plain Node process that never loads the app, and anyone able to modify the app
239+
bundle can undo any of this. These controls stop a curious user poking at the
240+
shipped client; they are not a defence against someone who controls the machine.
241+
Treat the backend's authorization as the real boundary.
242+
243+
## Editing a connected profile
244+
245+
Saving a profile applies it. If the connection is live and the edit changes what
246+
the transport does, the connection is rebuilt onto the new settings and the open
247+
window follows it to the new origin — a rebuilt tunnel binds a different local
248+
port, so the origin changes with it.
249+
250+
The comparison is `profiles.transportFingerprint()`, over the fields that decide
251+
what the connection *is*: transport, URL, every TLS field, and the SSH host,
252+
port, username, auth method, key path, remote host/port/scheme, pinned local
253+
port, and pinned host key. Renaming a connection is not in it, so a rename never
254+
costs you a working tunnel. `stickyLocalPort` is not in it either — we choose
255+
that, not the user, and including it would make every connection differ from
256+
itself on the next connect.
257+
258+
This is all-or-nothing per save. If the rebuild fails, the old connection is
259+
**not** kept: it was built from settings that no longer exist, so it is closed
260+
and the failure is reported, naming the fact that the previous session used the
261+
settings you replaced. A window that looks connected while serving settings you
262+
have changed is the state this design removes.
263+
264+
Before this, `connect()` reused any live connection unconditionally. The launcher
265+
persists the form before every Connect, so the stored profile was always correct
266+
and the *store* was never the problem — the reused connection simply kept running
267+
the old settings, and Test reported a confident pass for settings that had been
268+
replaced. Changing a tunnel's remote port and pressing Connect did nothing at
269+
all.
270+
271+
```bash
272+
npm run selftest:settings # two fake servers; proves an edit moves the connection
273+
```
198274

199275
## Deep links
200276

@@ -212,8 +288,17 @@ npm run smoke -- --ssh-host 20.29.48.144 --ssh-user ubuntu --key ~/keys/vm.pem
212288

213289
# Exercise the tunnel end to end against a throwaway in-process SSH server
214290
npm run selftest:tunnel
291+
292+
# Prove an edited setting reaches the live connection (two fake DeepSQL servers)
293+
npm run selftest:settings
294+
295+
# Drift guard for the DevTools kill switch
296+
npm test
215297
```
216298

299+
Both self-tests redirect `userData` to a temp directory, so they never touch your
300+
real `profiles.json`.
301+
217302
Logs are at `<userData>/logs/desktop.log`; the launcher footer has an **Open log
218303
file** link. Connection profiles live in `<userData>/profiles.json` (mode 0600,
219304
secrets stored only as `safeStorage` ciphertext).

desktop/package.json

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
"scripts": {
1515
"start": "electron .",
1616
"dev": "DEEPSQL_DESKTOP_DEV=1 electron .",
17+
"test": "node --test src/main/*.test.js",
1718
"icons": "electron scripts/generate-icons.js",
1819
"pack": "electron-builder --dir",
1920
"dist": "electron-builder",
@@ -22,7 +23,8 @@
2223
"dist:linux": "electron-builder --linux",
2324
"dist:all": "electron-builder --mac --win --linux",
2425
"smoke": "electron scripts/smoke.js",
25-
"selftest:tunnel": "electron scripts/tunnel-selftest.js"
26+
"selftest:tunnel": "electron scripts/tunnel-selftest.js",
27+
"selftest:settings": "electron scripts/settings-selftest.js"
2628
},
2729
"dependencies": {
2830
"electron-updater": "^6.8.9",
Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
'use strict';
2+
3+
/**
4+
* End-to-end self-test for "an edited setting takes effect".
5+
*
6+
* Stands up two fake DeepSQL servers on different loopback ports and drives the
7+
* real profiles + transport code against them. Editing a profile to point at the
8+
* second server must move the live connection to it; that is the whole bug this
9+
* covers, and it is invisible to any check that only inspects what was saved,
10+
* because saving was never the broken half.
11+
*
12+
* npm run selftest:settings
13+
*/
14+
15+
const fs = require('node:fs');
16+
const http = require('node:http');
17+
const os = require('node:os');
18+
const path = require('node:path');
19+
const { app } = require('electron');
20+
21+
// Before anything reads it: the real store holds the user's live connections.
22+
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'deepsql-settings-test-'));
23+
app.setPath('userData', tmpDir);
24+
25+
const results = [];
26+
function check(name, condition, detail = '') {
27+
results.push({ name, ok: Boolean(condition), detail });
28+
process.stdout.write(` ${condition ? 'PASS' : 'FAIL'} ${name}${detail ? ` — ${detail}` : ''}\n`);
29+
}
30+
31+
/** A stand-in for the DeepSQL nginx, tagged so we can tell the two apart. */
32+
function fakeDeepSql(label) {
33+
return http.createServer((req, res) => {
34+
if (req.url === '/api/actuator/health') {
35+
res.writeHead(200, { 'Content-Type': 'application/json' });
36+
res.end(JSON.stringify({ status: 'UP', server: label }));
37+
return;
38+
}
39+
res.writeHead(404);
40+
res.end();
41+
});
42+
}
43+
44+
function listen(server) {
45+
return new Promise((resolve) => server.listen(0, '127.0.0.1', () => resolve(server.address().port)));
46+
}
47+
48+
app.whenReady().then(async () => {
49+
const profiles = require('../src/main/profiles');
50+
const transport = require('../src/main/transport');
51+
52+
const serverA = fakeDeepSql('A');
53+
const serverB = fakeDeepSql('B');
54+
const portA = await listen(serverA);
55+
const portB = await listen(serverB);
56+
const originA = `http://127.0.0.1:${portA}`;
57+
const originB = `http://127.0.0.1:${portB}`;
58+
59+
process.stdout.write(`\nFake DeepSQL A=${originA} B=${originB}\n\n`);
60+
61+
try {
62+
// ── The reported bug: change a setting, and the behaviour must follow ──
63+
process.stdout.write('Edited settings reach the live connection\n');
64+
65+
const saved = profiles.upsert({ name: 'Test', transport: 'direct', url: originA });
66+
const id = saved.id;
67+
68+
const first = await transport.connect(id);
69+
check('connects to the configured server', first.origin === originA, first.origin);
70+
71+
const second = await transport.connect(id);
72+
check(
73+
'an unchanged reconnect reuses the live connection',
74+
second.reused === true && second.origin === originA,
75+
`reused=${second.reused} origin=${second.origin}`,
76+
);
77+
78+
// Exactly what the launcher does on Connect: persist the form, then connect.
79+
profiles.upsert({ id, name: 'Test', transport: 'direct', url: originB });
80+
const third = await transport.connect(id);
81+
check(
82+
'connecting after an edit moves to the new server',
83+
third.origin === originB,
84+
`origin=${third.origin}${third.reused ? ' (REUSED STALE CONNECTION)' : ''}`,
85+
);
86+
check(
87+
'the stale connection was not reused',
88+
third.reused !== true,
89+
third.reused ? 'connect() returned the pre-edit origin' : '',
90+
);
91+
92+
// ── reconcile(): what makes a save take effect immediately ────────────
93+
process.stdout.write('\nreconcile() applies a save without waiting for Connect\n');
94+
95+
profiles.upsert({ id, name: 'Test', transport: 'direct', url: originA });
96+
const reconciled = await transport.reconcile(id);
97+
check(
98+
'reconcile rebuilds onto the saved settings',
99+
reconciled.changed === true && reconciled.ok === true && reconciled.origin === originA,
100+
`changed=${reconciled.changed} ok=${reconciled.ok} origin=${reconciled.origin}`,
101+
);
102+
check(
103+
'the live origin now matches the saved profile',
104+
transport.originFor(id) === originA,
105+
transport.originFor(id),
106+
);
107+
108+
// A rename must not cost the user their session.
109+
const before = transport.get(id);
110+
profiles.upsert({ id, name: 'Renamed', transport: 'direct', url: originA });
111+
const renamed = await transport.reconcile(id);
112+
check(
113+
'renaming does not rebuild the connection',
114+
renamed.changed === false && transport.get(id) === before,
115+
`changed=${renamed.changed} sameEntry=${transport.get(id) === before}`,
116+
);
117+
118+
// ── A failed rebuild must not leave the old connection running ────────
119+
process.stdout.write('\nA rebuild that fails leaves no half-applied state\n');
120+
121+
await new Promise((resolve) => serverB.close(resolve));
122+
profiles.upsert({ id, name: 'Renamed', transport: 'direct', url: originB });
123+
const failed = await transport.reconcile(id);
124+
check(
125+
'reconcile reports the failure',
126+
failed.changed === true && failed.ok === false,
127+
`ok=${failed.ok} detail=${failed.detail}`,
128+
);
129+
check(
130+
'no connection is left serving the replaced settings',
131+
transport.isConnected(id) === false,
132+
transport.isConnected(id) ? `still connected to ${transport.originFor(id)}` : '',
133+
);
134+
135+
// ── test() must describe the settings on screen ───────────────────────
136+
process.stdout.write('\nTest reports on current settings, not the live ones\n');
137+
138+
profiles.upsert({ id, name: 'Renamed', transport: 'direct', url: originA });
139+
await transport.connect(id);
140+
const serverC = fakeDeepSql('C');
141+
const portC = await listen(serverC);
142+
const originC = `http://127.0.0.1:${portC}`;
143+
profiles.upsert({ id, name: 'Renamed', transport: 'direct', url: originC });
144+
const tested = await transport.test(id);
145+
check(
146+
'testing after an edit exercises the edited settings',
147+
tested.ok === true && tested.rebuilt === true && transport.originFor(id) === originC,
148+
`ok=${tested.ok} rebuilt=${tested.rebuilt} live=${transport.originFor(id)}`,
149+
);
150+
await new Promise((resolve) => serverC.close(resolve));
151+
152+
// ── The fingerprint itself ────────────────────────────────────────────
153+
process.stdout.write('\nTransport fingerprint\n');
154+
155+
const base = profiles.get(id);
156+
const sameName = { ...base, name: 'Something else' };
157+
check(
158+
'name is not part of the transport identity',
159+
profiles.transportFingerprint(base) === profiles.transportFingerprint(sameName),
160+
);
161+
const otherPort = { ...base, ssh: { ...base.ssh, remotePort: base.ssh.remotePort + 1 } };
162+
check(
163+
'the SSH remote port is part of it',
164+
profiles.transportFingerprint(base) !== profiles.transportFingerprint(otherPort),
165+
);
166+
const otherLocal = { ...base, ssh: { ...base.ssh, localPort: 44444 } };
167+
check(
168+
'the pinned local port is part of it',
169+
profiles.transportFingerprint(base) !== profiles.transportFingerprint(otherLocal),
170+
);
171+
const otherSticky = { ...base, ssh: { ...base.ssh, stickyLocalPort: 44444 } };
172+
check(
173+
'the sticky local port is NOT (we choose it, not the user)',
174+
profiles.transportFingerprint(base) === profiles.transportFingerprint(otherSticky),
175+
);
176+
177+
await transport.disconnectAll();
178+
} catch (err) {
179+
check('self-test ran to completion', false, err.stack || err.message);
180+
}
181+
182+
await new Promise((resolve) => serverA.close(resolve));
183+
184+
const failed = results.filter((r) => !r.ok);
185+
process.stdout.write(
186+
`\n${results.length - failed.length}/${results.length} checks passed\n`,
187+
);
188+
fs.rmSync(tmpDir, { recursive: true, force: true });
189+
app.exit(failed.length === 0 ? 0 : 1);
190+
});

0 commit comments

Comments
 (0)