Skip to content

Add Clerk authentication with Angular login component and Electron IPC token storage #635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Copilot wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Add Clerk authentication with Angular login component and Electron IPC token storage #635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
13a1d2d
Initial plan
Copilot Mar 31, 2026
6e0e17e
Add Clerk authentication integration with Angular login component and…
Copilot Mar 31, 2026
8a03f24
Merge branch 'master' into copilot/create-angular-login-component
highperformancecoder Mar 31, 2026
3ebc404
Added a "login to clerk" menu item, and added a clerk publishable key.
highperformancecoder Mar 31, 2026
9d1d261
Fix login window not rendering by using createPopupWindowWithRouting
Copilot Mar 31, 2026
62a9ead
Fix login window showing app shell (header/tabs) by excluding login r…
Copilot Mar 31, 2026
f9b417d
Move login route to headless/login for consistency with other popup w…
Copilot Mar 31, 2026
6f7259d
Add openLoginWindow() promise that resolves when auth token is stored
Copilot Mar 31, 2026
60f50ea
Rearrange promise from events to WindowManager
highperformancecoder Mar 31, 2026
8da2877
Close login dialog window once submitted.
highperformancecoder Mar 31, 2026
59e8663
Got the upgradeUsingClerk process working.
highperformancecoder Apr 1, 2026
6539e6e
Dead code removal
highperformancecoder Apr 1, 2026
8663d10
More dead code removal.
highperformancecoder Apr 1, 2026
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
13 changes: 13 additions & 0 deletions gui-js/apps/minsky-electron/src/app/events/electron.events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,3 +288,16 @@ ipcMain.handle(events.OPEN_URL, (event,options)=> {
let window=WindowManager.createWindow(options);
window.loadURL(options.url);
});

ipcMain.handle(events.SET_AUTH_TOKEN, async (event, token: string | null) => {
if (token) {
StoreManager.store.set('authToken', token);
} else {
StoreManager.store.delete('authToken');
}
Comment on lines +292 to +297
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Avoid persisting the raw bearer token in the app store.

This writes a reusable auth credential to disk via StoreManager.store. Any user or process that can read the app-data directory can replay it until expiry. Please keep the token in memory only, or move persistence to OS-backed secret storage.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@gui-js/apps/minsky-electron/src/app/events/electron.events.ts` around lines
292 - 297, The handler for ipcMain.handle(events.SET_AUTH_TOKEN) is persisting
the raw bearer token to disk via StoreManager.store.set/delete
(StoreManager.store), which is insecure; update the handler to stop writing the
token to persistent store and instead keep it in memory (e.g., use or create an
in-memory AuthTokenManager singleton or a module-scoped variable accessed by the
renderer via IPC) and remove calls to StoreManager.store.set/delete; if
persistent storage is absolutely required, replace direct disk persistence with
an OS-backed secret store integration (e.g., keytar or platform-specific secure
storage) and ensure any persisted value is encrypted and has a strict
expiry/rotation policy rather than storing the raw bearer token.

if (WindowManager._resolveAuthToken) {
WindowManager._resolveAuthToken(token);
WindowManager._resolveAuthToken = null;
}
return { success: true };
});
9 changes: 9 additions & 0 deletions gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,15 @@ export class ApplicationMenuManager {
window.loadURL('https://www.patreon.com/logout');
},
},
{
label: 'Upgrade via Clerk',
click() {CommandsManager.upgradeUsingClerk();},
},
Comment on lines +131 to +133
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Upgrade via Clerk currently exposes a cancel-loop failure mode.

From the linked flow (CommandsManager.upgradeUsingClerk + WindowManager.openLoginWindow), closing the login window resolves null, but upgrade logic keeps reopening login in a while (!authToken) loop. This makes this menu action non-cancelable for users.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@gui-js/apps/minsky-electron/src/app/managers/ApplicationMenuManager.ts`
around lines 131 - 133, The "Upgrade via Clerk" menu action currently spins in a
non-cancelable loop; modify CommandsManager.upgradeUsingClerk to stop reopening
the login when WindowManager.openLoginWindow returns null (user
closed/cancelled) by handling a null/undefined authToken result and aborting the
upgrade flow (or returning an explicit cancellation) instead of looping forever;
optionally replace the infinite while (!authToken) with a finite retry policy or
a user confirmation prompt before retrying so the operation can be cancelled by
the user.

{
label: 'Logout Clerk',
click() {WindowManager.openLoginWindow();},
},

{
label: 'New System',
accelerator: 'CmdOrCtrl + Shift + N',
Expand Down
195 changes: 158 additions & 37 deletions gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,67 @@
import {exec,spawn} from 'child_process';
import decompress from 'decompress';
import {promisify} from 'util';
import {net} from 'electron';

function semVer(version: string) {
const pattern=/(\d+)\.(\d+)\.(\d+)/;
let [,major,minor,patch]=pattern.exec(version);
return {major: +major,minor: +minor, patch: +patch};
}
function semVerLess(x: string, y: string): boolean {
let xver=semVer(x), yver=semVer(y);
return xver.major<yver.major ||
xver.major===yver.major && (
xver.minor<yver.minor ||
xver.minor===yver.minor && xver.patch<yver.patch
);
}

const backendAPI='https://minskybe-x7dj1.sevalla.app/api';
// perform a call on the backend API, returning the JSON encoded result
// options is passed to the constructor of a ClienRequest object https://www.electronjs.org/docs/latest/api/client-request#requestendchunk-encoding-callback
async function callBackendAPI(options: string|Object, token: string) {
return new Promise<string>((resolve, reject)=> {
let request=net.request(options);
request.setHeader('Authorization',`Bearer ${token}`);
request.on('response', (response)=>{
let chunks=[];
response.on('data', (chunk)=>{chunks.push(chunk);});
response.on('end', ()=>resolve(Buffer.concat(chunks).toString()));
response.on('error',()=>reject(response.statusMessage));
});
request.on('error',(err)=>reject(err.toString()));
request.end();
});
}

// to handle redirects
async function getFinalUrl(initialUrl, token) {
try {
const response = await fetch(initialUrl, {
method: 'GET',
headers: {
'Authorization': `Bearer ${token}`
},
redirect: 'manual' // This tells fetch NOT to follow the link automatically
});
Comment on lines +66 to +72

Check failure

Code scanning / CodeQL

Server-side request forgery Critical

The
URL
Loading
of this request depends on a
user-provided value
Loading
.

Copilot Autofix

AI 2 days ago

In general, to fix this problem you should ensure that any URL used in fetch or downloadURL is restricted to a safe subset, typically by validating it against an allow‑list of hostnames and schemes and by rejecting or normalizing any unexpected value. Instead of blindly trusting the download_url field from the backend, parse it with URL, check that it uses https: and points to an expected host (or set of hosts), and only then pass it to fetch/downloadURL. If validation fails, abort the operation or show an error.

The best targeted fix here is to add a small helper, e.g. sanitizeAssetUrl, that takes the tainted URL (like minskyAsset or ravelAsset), parses it, enforces https: and a specific host or host suffix, and returns either a safe normalized string or null. We then: (1) call this helper on minskyAsset and ravelAsset immediately after retrieving them in upgradeUsingClerk, and (2) modify getFinalUrl so it receives only already-sanitized URLs, or at least re-validates initialUrl before using it in fetch. Concretely:

  • In CommandsManager.ts, define const allowedDownloadHost = '...'; (or a small allow‑list) and a sanitizeAssetUrl(assetUrl: string | undefined): string | null function near the other helpers (e.g., after backendAPI or before getFinalUrl).
  • In upgradeUsingClerk, after getRelease(...), pass minskyAsset and ravelAsset through sanitizeAssetUrl. If both sanitize to null, show the “Everything’s up to date” message as before. If one sanitizes to null but not the other, proceed only with the safe one, or show an error dialog explaining that the download URL is invalid.
  • Update calls to getFinalUrl to pass the sanitized URLs and the token correctly. Also fix the inconsistent call getFinalUrl(ravelAsset) (missing token).
  • Within getFinalUrl, perform a quick re-check of initialUrl (again parse with URL and ensure allowed protocol/hostname) before calling fetch, to ensure it is never used with an unsafe target even if another caller reuses it in the future.

This preserves existing functionality (still uses backend-provided URLs) but adds a strict guardrail so that only URLs to the intended domain and scheme are ever used for SSRF‑capable operations.

Suggested changeset 1
gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts b/gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts
--- a/gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts
+++ b/gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts
@@ -43,6 +43,22 @@
 }
 
 const backendAPI='https://minskybe-x7dj1.sevalla.app/api';
+const allowedDownloadHost='minskybe-x7dj1.sevalla.app';
+
+// Ensure that asset download URLs are restricted to expected schemes/hosts.
+function sanitizeAssetUrl(assetUrl: string | undefined | null): string | null {
+  if (!assetUrl) return null;
+  try {
+    const parsed = new URL(assetUrl);
+    // Only allow HTTPS downloads from the expected host (or subdomains, if desired).
+    if (parsed.protocol !== 'https:') return null;
+    if (parsed.hostname !== allowedDownloadHost) return null;
+    return parsed.toString();
+  } catch {
+    return null;
+  }
+}
+
 // perform a call on the backend API, returning the JSON encoded result
 // options is passed to the constructor of a ClienRequest object https://www.electronjs.org/docs/latest/api/client-request#requestendchunk-encoding-callback
 async function callBackendAPI(options: string|Object, token: string) {
@@ -61,9 +77,14 @@
 }
 
 // to handle redirects
-async function getFinalUrl(initialUrl, token) {
-try {
-    const response = await fetch(initialUrl, {
+async function getFinalUrl(initialUrl: string, token: string) {
+  // Re-validate the URL before using it in a network request.
+  const safeUrl = sanitizeAssetUrl(initialUrl);
+  if (!safeUrl) {
+    throw new Error('Invalid or unsafe download URL');
+  }
+  try {
+    const response = await fetch(safeUrl, {
       method: 'GET',
       headers: {
         'Authorization': `Bearer ${token}`
@@ -74,10 +95,16 @@
     // In 'manual' mode, a redirect returns an 'opaqueredirect' type or status 302
     if (response.status >= 300 && response.status < 400) {
       const redirectUrl = response.headers.get('location');
-      if (redirectUrl) return redirectUrl;
+      if (redirectUrl) {
+        const safeRedirect = sanitizeAssetUrl(redirectUrl);
+        if (!safeRedirect) {
+          throw new Error('Invalid or unsafe redirect URL');
+        }
+        return safeRedirect;
+      }
     }
 
-    if (response.ok) return initialUrl;
+    if (response.ok) return safeUrl;
     
     throw new Error(`Server responded with ${response.status}`);
   } catch (error) {
@@ -1494,21 +1519,24 @@
     let token=StoreManager.store.get('authToken');
 
     const window=WindowManager.getMainWindow();
-    let minskyAsset;
+    let minskyAssetRaw;
     if (installCase===InstallCase.theLot)
-      minskyAsset=await CommandsManager.getRelease('minsky', false, token);
-    let ravelAsset=await CommandsManager.getRelease('ravel', installCase===InstallCase.previousRavel, token);
+      minskyAssetRaw=await CommandsManager.getRelease('minsky', false, token);
+    let ravelAssetRaw=await CommandsManager.getRelease('ravel', installCase===InstallCase.previousRavel, token);
 
+    const minskyAsset = sanitizeAssetUrl(minskyAssetRaw);
+    const ravelAsset = sanitizeAssetUrl(ravelAssetRaw);
+
     if (minskyAsset) {
       if (ravelAsset) { // stash ravel upgrade to be installed on next startup
-        StoreManager.store.set('ravelPlugin',await getFinalUrl(ravelAsset));
+        StoreManager.store.set('ravelPlugin', await getFinalUrl(ravelAsset, token));
       }
       window.webContents.session.on('will-download',this.downloadMinsky);
-      window.webContents.downloadURL(await getFinalUrl(minskyAsset,token));
+      window.webContents.downloadURL(await getFinalUrl(minskyAsset, token));
       return;
     } else if (ravelAsset) {
       window.webContents.session.on('will-download',this.downloadRavel);
-      window.webContents.downloadURL(await getFinalUrl(ravelAsset,token));
+      window.webContents.downloadURL(await getFinalUrl(ravelAsset, token));
       return;
     }
     dialog.showMessageBoxSync(WindowManager.getMainWindow(),{
EOF
Copilot is powered by AI and may make mistakes. Always verify output.
Copy link
Copy Markdown
Owner

@highperformancecoder highperformancecoder Apr 2, 2026

Choose a reason for hiding this comment

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

OK - the security issue is less severe than made out, as the backend server is trusted. But we can sanitise on delivered names - need to figure out what they are - and prefer to do this in the upgrade* methods, rather than the generic finalURL method.


// In 'manual' mode, a redirect returns an 'opaqueredirect' type or status 302
if (response.status >= 300 && response.status < 400) {
const redirectUrl = response.headers.get('location');
if (redirectUrl) return redirectUrl;
}

if (response.ok) return initialUrl;

throw new Error(`Server responded with ${response.status}`);
} catch (error) {
// If redirect: 'manual' is used, fetch might throw a 'TypeError'
// when it hits the redirect—this is actually what we want to catch.
console.error("Fetch encountered the redirect/error:", error);
throw error;
}
}

export class CommandsManager {
static activeGodleyWindowItems = new Map<string, CanvasItem>();
Expand Down Expand Up @@ -1137,6 +1198,7 @@

// handler for downloading Ravel and installing it
static downloadRavel(event,item,webContents) {

switch (process.platform) {
case 'win32':
const savePath=dirname(process.execPath)+'/libravel.dll';
Expand All @@ -1158,7 +1220,7 @@
// handler for when download completed
item.once('done', (event,state)=>{
progress.close();

if (state==='completed') {
dialog.showMessageBoxSync(WindowManager.getMainWindow(),{
message: 'Ravel plugin updated successfully - restart Ravel to use',
Expand Down Expand Up @@ -1308,6 +1370,45 @@
modal: false,
});
}

// return information about the current system
static async buildState(previous: boolean) {
// need to pass what platform we are
let state;
switch (process.platform) {
case 'win32':
state={system: 'windows', distro: '', version: '', arch:'', previous: ''};
break;
case 'darwin':
state={system: 'macos', distro: '', version: '', arch: `${process.arch}`, previous: ''};
break;
case 'linux':
state={system: 'linux', distro: '', version: '',arch:'', previous: ''};
// figure out distro and version from /etc/os-release
let aexec=promisify(exec);
let osRelease='/etc/os-release';
if (existsSync(process.resourcesPath+'/os-release'))
osRelease=process.resourcesPath+'/os-release';
let distroInfo=await aexec(`grep ^ID= ${osRelease}`);
// value may or may not be quoted
let extractor=/.*=['"]?([^'"\n]*)['"]?/;
state.distro=extractor.exec(distroInfo.stdout)[1];
distroInfo=await aexec(`grep ^VERSION_ID= ${osRelease}`);
state.version=extractor.exec(distroInfo.stdout)[1];
break;
Comment on lines +1385 to +1398
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Locate the CommandsManager.ts file
find . -name "CommandsManager.ts" -type f

Repository: highperformancecoder/minsky

Length of output: 136

🏁 Script executed:

#!/bin/bash
# Once we find the file, read the specific lines mentioned
# First, let's find the file
FILE=$(find . -name "CommandsManager.ts" -type f)
if [ -n "$FILE" ]; then
  echo "File found at: $FILE"
  # Get total line count to understand file size
  wc -l "$FILE"
  # Read lines 1380-1410 to capture context around the target lines
  sed -n '1380,1410p' "$FILE"
fi

Repository: highperformancecoder/minsky

Length of output: 1540

🌐 Web query:

Biome noSwitchDeclarations rule TypeScript JavaScript

💡 Result:

The noSwitchDeclarations rule in Biome is a correctness linter rule that disallows lexical declarations (const, let, function, class) directly in switch clauses. This prevents declarations from being accessible across the entire switch statement, where they might be used uninitialized in other cases. It applies to JavaScript and super languages like TypeScript. Configuration in biome.json: { "linter": { "rules": { "correctness": { "noSwitchDeclarations": "error" } } } } Invalid examples: switch (foo) { case 0: const x = 1; // Error: accessible in other cases break; } Valid fix: Wrap in block switch (foo) { case 0: { const x = 1; break; } } The rule is fixable (suggests wrapping in block) and sources from ESLint's no-case-declarations.

Citations:

Wrap the Linux case in braces.

The let declarations in this clause violate Biome's noSwitchDeclarations rule because they are scoped across the entire switch statement instead of being local to the case. Adding braces keeps the bindings local and resolves the lint error.

🧩 Suggested fix 
-    case 'linux':
-      state={system: 'linux', distro: '', version: '',arch:'', previous: ''};
-      // figure out distro and version from /etc/os-release
-      let aexec=promisify(exec);
-      let osRelease='/etc/os-release';
-      if (existsSync(process.resourcesPath+'/os-release'))
-        osRelease=process.resourcesPath+'/os-release';
-      let distroInfo=await aexec(`grep ^ID= ${osRelease}`);
-      // value may or may not be quoted
-      let extractor=/.*=['"]?([^'"\n]*)['"]?/;
-      state.distro=extractor.exec(distroInfo.stdout)[1];
-      distroInfo=await aexec(`grep ^VERSION_ID= ${osRelease}`);
-      state.version=extractor.exec(distroInfo.stdout)[1];
-      break;
+    case 'linux': {
+      state={system: 'linux', distro: '', version: '',arch:'', previous: ''};
+      // figure out distro and version from /etc/os-release
+      const aexec=promisify(exec);
+      let osRelease='/etc/os-release';
+      if (existsSync(process.resourcesPath+'/os-release'))
+        osRelease=process.resourcesPath+'/os-release';
+      let distroInfo=await aexec(`grep ^ID= ${osRelease}`);
+      // value may or may not be quoted
+      const extractor=/.*=['"]?([^'"\n]*)['"]?/;
+      state.distro=extractor.exec(distroInfo.stdout)[1];
+      distroInfo=await aexec(`grep ^VERSION_ID= ${osRelease}`);
+      state.version=extractor.exec(distroInfo.stdout)[1];
+      break;
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
case 'linux':
state={system: 'linux', distro: '', version: '',arch:'', previous: ''};
// figure out distro and version from /etc/os-release
let aexec=promisify(exec);
let osRelease='/etc/os-release';
if (existsSync(process.resourcesPath+'/os-release'))
osRelease=process.resourcesPath+'/os-release';
let distroInfo=await aexec(`grep ^ID= ${osRelease}`);
// value may or may not be quoted
let extractor=/.*=['"]?([^'"\n]*)['"]?/;
state.distro=extractor.exec(distroInfo.stdout)[1];
distroInfo=await aexec(`grep ^VERSION_ID= ${osRelease}`);
state.version=extractor.exec(distroInfo.stdout)[1];
break;
case 'linux': {
state={system: 'linux', distro: '', version: '',arch:'', previous: ''};
// figure out distro and version from /etc/os-release
const aexec=promisify(exec);
let osRelease='/etc/os-release';
if (existsSync(process.resourcesPath+'/os-release'))
osRelease=process.resourcesPath+'/os-release';
let distroInfo=await aexec(`grep ^ID= ${osRelease}`);
// value may or may not be quoted
const extractor=/.*=['"]?([^'"\n]*)['"]?/;
state.distro=extractor.exec(distroInfo.stdout)[1];
distroInfo=await aexec(`grep ^VERSION_ID= ${osRelease}`);
state.version=extractor.exec(distroInfo.stdout)[1];
break;
}
🧰 Tools
🪛 Biome (2.4.9)

[error] 1388-1388: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

(lint/correctness/noSwitchDeclarations)

[error] 1389-1389: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

(lint/correctness/noSwitchDeclarations)

[error] 1392-1392: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

(lint/correctness/noSwitchDeclarations)

[error] 1394-1394: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

(lint/correctness/noSwitchDeclarations)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts` around lines
1385 - 1398, The `case 'linux'` block in CommandsManager.ts declares lets
(aexec, osRelease, distroInfo, extractor, state assignment) that leak across the
switch and trigger the noSwitchDeclarations lint rule; fix it by wrapping the
entire case 'linux' body in { ... } so those let/const bindings are block-scoped
and local to that case (ensure you move the existing code for case 'linux'
including state={...}, promisify(exec) assignment, os-release selection, grep
calls and extractor usage inside the new braces and keep the final break).

default:
dialog.showMessageBoxSync(WindowManager.getMainWindow(),{
message: `In app update is not available for your operating system yet, please check back later`,
type: 'error',
});
window.close();
return;
Comment on lines +1399 to +1405
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Remove the out-of-scope window.close() call.

There is no local window binding here, so the unsupported-platform branch throws instead of exiting cleanly after showing the dialog.

🧹 Suggested fix 
     default:
       dialog.showMessageBoxSync(WindowManager.getMainWindow(),{
             message: `In app update is not available for your operating system yet, please check back later`,
             type: 'error',
       });
-      window.close();
       return;
-      break;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
default:
dialog.showMessageBoxSync(WindowManager.getMainWindow(),{
message: `In app update is not available for your operating system yet, please check back later`,
type: 'error',
});
window.close();
return;
default:
dialog.showMessageBoxSync(WindowManager.getMainWindow(),{
message: `In app update is not available for your operating system yet, please check back later`,
type: 'error',
});
return;
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts` around lines
1399 - 1405, The default (unsupported-platform) branch in CommandsManager.ts
calls an out-of-scope window.close(), which throws; remove that call and ensure
the flow exits cleanly after
dialog.showMessageBoxSync(WindowManager.getMainWindow(), ...) — either delete
the window.close() line or replace it with a valid call on the WindowManager
(e.g., WindowManager.getMainWindow().close() or app.quit()) so the dialog is
shown and the process exits without referencing an undefined window identifier.

break;
}
if (await minsky.ravelAvailable() && previous)
state.previous=/[^:]*/.exec(await minsky.ravelVersion())[0];
return state;
}

static async upgrade(installCase: InstallCase=InstallCase.theLot) {
const window=this.createDownloadWindow();
Expand Down Expand Up @@ -1344,7 +1445,7 @@
}
if (ravelFile) {
// currently on latest, so reinstall ravel
window.webContents.session.on('will-download',this.downloadRavel);
window.webContents.session.on('will-download',this.downloadRavel);
window.webContents.downloadURL(ravelFile);
return;
}
Expand All @@ -1358,43 +1459,63 @@
});

let clientId='-PiL7snNmZL_BlLJTPm62SHBcFTMG5d46m2336r118mfrp6sz4ty0g-thbKAs76c';
// need to pass what platform we are
let state;
switch (process.platform) {
case 'win32':
state={system: 'windows', distro: '', version: '', arch:'', previous: ''};
break;
case 'darwin':
state={system: 'macos', distro: '', version: '', arch: `${process.arch}`, previous: ''};
break;
case 'linux':
state={system: 'linux', distro: '', version: '',arch:'', previous: ''};
// figure out distro and version from /etc/os-release
let aexec=promisify(exec);
let osRelease='/etc/os-release';
if (existsSync(process.resourcesPath+'/os-release'))
osRelease=process.resourcesPath+'/os-release';
let distroInfo=await aexec(`grep ^ID= ${osRelease}`);
// value may or may not be quoted
let extractor=/.*=['"]?([^'"\n]*)['"]?/;
state.distro=extractor.exec(distroInfo.stdout)[1];
distroInfo=await aexec(`grep ^VERSION_ID= ${osRelease}`);
state.version=extractor.exec(distroInfo.stdout)[1];
break;
default:
dialog.showMessageBoxSync(WindowManager.getMainWindow(),{
message: `In app update is not available for your operating system yet, please check back later`,
type: 'error',
});
window.close();
return;
break;
}
if (await minsky.ravelAvailable() && installCase===InstallCase.previousRavel)
state.previous=/[^:]*/.exec(await minsky.ravelVersion())[0];
let encodedState=encodeURI(JSON.stringify(state));
let encodedState=encodeURI(JSON.stringify(await CommandsManager.buildState(installCase==InstallCase.previousRavel)));
// load patreon's login page
window.loadURL(`https://www.patreon.com/oauth2/authorize?response_type=code&client_id=${clientId}&redirect_uri=https://ravelation.net/ravel-downloader.cgi&scope=identity%20identity%5Bemail%5D&state=${encodedState}`);
}


// gets release URL for current system from Ravelation.net backend
static async getRelease(product: string, previous: boolean, token: string) {
let state=await CommandsManager.buildState(previous);
let query=`product=${product}&os=${state.system}&arch=${state.arch}&distro=${state.distro}&distro_version=${state.version}`;
try {
if (previous) {
let releases=JSON.parse(await callBackendAPI(`${backendAPI}/releases?${query}`, token));
let prevRelease;
for (let release of releases)
if (semVerLess(release.version, state.previous))
prevRelease=release;
if (prevRelease) return prevRelease.download_url;
// if not, then treat the request as latest
}
let release=JSON.parse(await callBackendAPI(`${backendAPI}/releases/latest?${query}`, token));
return release?.release?.download_url;
}
catch (error) {
console.error(error);

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
Loading
.
Log entry depends on a
user-provided value
Loading
.

Copilot Autofix

AI 2 days ago

In general, to fix log injection issues, any value that can contain untrusted content (especially from network or user-controlled sources) should be sanitized before being written to logs. For plain-text logs, this typically means removing or replacing newline (\n) and carriage return (\r) characters so that a single log statement cannot be split into multiple log lines. It also helps to add a clear prefix or context indicating where the untrusted data is.

For this specific case, the best fix without changing existing functionality is to sanitize the error value before passing it to console.error in CommandsManager.getRelease. We should convert error to a string in a controlled way, strip \r and \n, and then log a message that includes that sanitized string. This ensures that regardless of whether the caught value is an Error, a string containing newlines, or something else, the logged line will not be able to inject extra lines. Since we already use standard JavaScript/TypeScript features, we can implement sanitization inline with String.prototype.replace, without introducing new dependencies.

Concretely, in gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts, around lines 1485–1487, replace console.error(error); with code that builds a safe string: e.g., const msg = String(error).replace(/[\r\n]/g, ' '); console.error('getRelease error:', msg);. No new imports or helper functions are strictly necessary.

Suggested changeset 1
gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts b/gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts
--- a/gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts
+++ b/gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts
@@ -1483,7 +1483,8 @@
       return release?.release?.download_url;
     }
     catch (error) {
-      console.error(error);
+      const safeErrorMessage = String(error).replace(/[\r\n]/g, ' ');
+      console.error('getRelease error:', safeErrorMessage);
       return "";
     }
   }
EOF
@@ -1483,7 +1483,8 @@
return release?.release?.download_url;
}
catch (error) {
console.error(error);
const safeErrorMessage = String(error).replace(/[\r\n]/g, ' ');
console.error('getRelease error:', safeErrorMessage);
return "";
}
}
Copilot is powered by AI and may make mistakes. Always verify output.
return "";
}
Comment on lines +1469 to +1488
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Don't collapse backend/auth failures into a falsy URL.

release?.release?.download_url and the catch path both turn failures into falsy values. upgradeUsingClerk() then shows the "Everything's up to date" dialog on bad tokens and backend outages instead of prompting re-auth or retry.

🧰 Tools
🪛 GitHub Check: CodeQL

[warning] 1486-1486: Log injection
Log entry depends on a user-provided value.
Log entry depends on a user-provided value.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts` around lines
1469 - 1488, getRelease currently swallows backend/auth failures by returning a
falsy URL (via optional chaining and a catch that returns ""), so
upgradeUsingClerk sees "no update" instead of surfacing errors; update
getRelease (the static method) to stop collapsing errors: remove or change the
catch to rethrow the caught error (or throw a new Error with context) so
auth/backend failures propagate, and explicitly validate the parsed response
(check that the parsed object from callBackendAPI and
release.release.download_url exist) before returning the URL — only return an
empty string when you deterministically know there is no release (e.g., no
prevRelease found when previous==true), but do not suppress exceptions from
callBackendAPI or malformed responses.

}

static async upgradeUsingClerk(installCase: InstallCase=InstallCase.theLot) {
while (!StoreManager.store.get('authToken'))
await WindowManager.openLoginWindow();
let token=StoreManager.store.get('authToken');
Comment on lines +1491 to +1494
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Let the user cancel login.

openLoginWindow() resolves null when the popup closes, but this loop ignores that value and immediately opens another window. As written, closing the dialog never cancels the upgrade.

↩️ Suggested fix 
-    while (!StoreManager.store.get('authToken'))
-      await WindowManager.openLoginWindow();
-    let token=StoreManager.store.get('authToken');
+    let token=StoreManager.store.get('authToken');
+    while (!token) {
+      token=await WindowManager.openLoginWindow();
+      if (!token) return;
+    }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts` around lines
1491 - 1494, The loop in CommandsManager.upgradeUsingClerk continuously calls
WindowManager.openLoginWindow() until StoreManager.store.get('authToken')
exists, but openLoginWindow() can resolve null when the user closes the popup;
update upgradeUsingClerk to capture the result of
WindowManager.openLoginWindow(), check for null, and break/return/throw to allow
cancelation instead of immediately reopening the dialog—use the returned value
from openLoginWindow() (or a boolean) before re-checking
StoreManager.store.get('authToken') so a closed popup stops the loop and cancels
the upgrade flow.


const window=WindowManager.getMainWindow();
let minskyAsset;
if (installCase===InstallCase.theLot)
minskyAsset=await CommandsManager.getRelease('minsky', false, token);
let ravelAsset=await CommandsManager.getRelease('ravel', installCase===InstallCase.previousRavel, token);

if (minskyAsset) {
if (ravelAsset) { // stash ravel upgrade to be installed on next startup
StoreManager.store.set('ravelPlugin',await getFinalUrl(ravelAsset));
}
Comment on lines +1503 to +1505
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Pass the token when stashing the deferred Ravel download URL.

getFinalUrl() expects the auth token, but this call omits it. In the combined Minsky+Ravel path the stored plugin URL is resolved with Bearer undefined, so the next-startup Ravel install can fail even after a successful login.

🔐 Suggested fix 
-        StoreManager.store.set('ravelPlugin',await getFinalUrl(ravelAsset));
+        StoreManager.store.set('ravelPlugin', await getFinalUrl(ravelAsset, token));
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@gui-js/apps/minsky-electron/src/app/managers/CommandsManager.ts` around lines
1503 - 1505, The stored Ravel URL is resolved without the auth token, causing
future installs to use "Bearer undefined"; update the stash logic so
StoreManager.store.set('ravelPlugin', await getFinalUrl(ravelAsset, token))
passes the same auth token that other getFinalUrl calls in this file use (e.g.
the authToken/AuthManager.getToken() value present in CommandsManager). Ensure
you retrieve the current token the same way as other code in this module and
pass it as the second argument to getFinalUrl when ravelAsset is truthy.

window.webContents.session.on('will-download',this.downloadMinsky);
window.webContents.downloadURL(await getFinalUrl(minskyAsset,token));
return;
} else if (ravelAsset) {
window.webContents.session.on('will-download',this.downloadRavel);
window.webContents.downloadURL(await getFinalUrl(ravelAsset,token));
return;
}
dialog.showMessageBoxSync(WindowManager.getMainWindow(),{
message: "Everything's up to date, nothing to do.\n"+
"If you're trying to download the Ravel plugin, please ensure you are logged into an account subscribed to Ravel Fan or Explorer tiers.",
type: 'info',
});
}

}
1 change: 1 addition & 0 deletions gui-js/apps/minsky-electron/src/app/managers/StoreManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ interface MinskyStore {
defaultModelDirectory: string;
defaultDataDirectory: string;
ravelPlugin: string; // used for post installation installation of Ravel
authToken?: string;
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Avoid persisting JWT bearer tokens in plain electron-store.

Line 22 adds long-lived token storage in the regular app store, which is readable from disk and weakens account security. Store auth tokens in OS credential storage (e.g., keychain/credential vault) and keep only non-sensitive session state in electron-store.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@gui-js/apps/minsky-electron/src/app/managers/StoreManager.ts` at line 22, The
StoreManager currently persists a sensitive authToken property in
electron-store; instead remove authToken from the persisted store schema and
move storage to the OS credential vault (e.g., use keytar). Update StoreManager
to stop writing/reading authToken from electron-store: add async methods like
getAuthToken/setAuthToken/deleteAuthToken that call keytar (or another OS
credential API), migrate any existing token on startup by reading the old
electron-store value, saving it to keytar, and deleting it from electron-store,
and keep only non-sensitive session state in electron-store.

}

class StoreManager {
Expand Down
27 changes: 27 additions & 0 deletions gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export class WindowManager {
static canvasWidth: number;
static scaleFactor: number;
static currentTab=minsky.canvas as RenderNativeWindow;
// Pending resolver for the auth-token promise created by openLoginWindow()
static _resolveAuthToken: ((token: string | null) => void) | null = null;
Comment on lines +31 to +32
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Reuse the in-flight login promise/window.

A second openLoginWindow() call replaces _resolveAuthToken before the first promise settles. The first caller then waits forever, and the earlier popup becomes orphaned. This is reachable because both the upgrade flow and the menu can open the login window. Return the existing pending promise instead of overwriting the resolver.

Also applies to: 387-398

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts` around lines
31 - 32, openLoginWindow currently overwrites the shared resolver
_resolveAuthToken when called a second time, orphaning the first caller; fix by
detecting an in-flight auth promise/resolver and returning that existing promise
instead of creating a new one, and only create/assign a new _resolveAuthToken
and _authTokenPromise when none exists; ensure you also reuse any existing
_loginWindow instead of creating a second popup (see references to
openLoginWindow, _resolveAuthToken, _authTokenPromise and _loginWindow and the
similar logic around lines 387-398) so multiple callers share the same
promise/window rather than replacing the resolver.


static activeWindows = new Map<number, ActiveWindow>();
private static uidToWindowMap = new Map<string, ActiveWindow>();
Expand Down Expand Up @@ -381,4 +383,29 @@ export class WindowManager {
catch (err) {} // absorb any exceptions due to windows disappearing
}
}

static async openLoginWindow() {
const promise = new Promise<string | null>((resolve) => {
WindowManager._resolveAuthToken = resolve;
});

const loginWindow = WindowManager.createPopupWindowWithRouting({
width: 420,
height: 500,
title: 'Login',
modal: false,
url: '#/headless/login',
});

// Resolve with null if the user closes the window before authenticating
loginWindow.once('closed', () => {
if (WindowManager._resolveAuthToken) {
WindowManager._resolveAuthToken(null);
WindowManager._resolveAuthToken = null;
}
});

return promise;
}

}
7 changes: 6 additions & 1 deletion gui-js/apps/minsky-web/src/app/app-routing.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ import {
EditHandleDescriptionComponent,
EditHandleDimensionComponent,
PickSlicesComponent,
LockHandlesComponent
LockHandlesComponent,
LoginComponent,
} from '@minsky/ui-components';

const routes: Routes = [
Expand Down Expand Up @@ -149,6 +150,10 @@ const routes: Routes = [
path: 'headless/variable-pane',
component: VariablePaneComponent,
},
{
path: 'headless/login',
component: LoginComponent,
},
{
path: '**',
component: PageNotFoundComponent,
Expand Down
2 changes: 1 addition & 1 deletion gui-js/apps/minsky-web/src/app/app.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
}

@if (!loading) {
@if (router.url.includes('headless');) {
@if (router.url.includes('headless')) {
<router-outlet></router-outlet>
}
@else {
Expand Down
4 changes: 4 additions & 0 deletions gui-js/apps/minsky-web/src/environments/environment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@ export const AppConfig = {
production: false,
environment: 'LOCAL',
};

export const environment = {
clerkPublishableKey: 'pk_test_cG9zaXRpdmUtcGhvZW5peC04NS5jbGVyay5hY2NvdW50cy5kZXYk',
};
Comment on lines +6 to +8
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Consolidate Clerk key into the existing app config surface.

Line 6 introduces a second config object (environment) instead of extending AppConfig, which risks this value never being consumed consistently. With ClerkService still hardcoding the key, this new value is currently ineffective and can drift.

Proposed fix 
 export const AppConfig = {
   production: false,
   environment: 'LOCAL',
+  clerkPublishableKey: 'pk_test_cG9zaXRpdmUtcGhvZW5peC04NS5jbGVyay5hY2NvdW50cy5kZXYk',
 };
-
-export const environment = {
-  clerkPublishableKey: 'pk_test_cG9zaXRpdmUtcGhvZW5peC04NS5jbGVyay5hY2NvdW50cy5kZXYk',
-};
🧰 Tools
🪛 Betterleaks (1.1.1)

[high] 7-7: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@gui-js/apps/minsky-web/src/environments/environment.ts` around lines 6 - 8,
You added a standalone environment object with clerkPublishableKey instead of
adding it to the existing AppConfig, so the value can be unused; modify the code
to move clerkPublishableKey into the existing AppConfig interface/object (extend
the AppConfig declaration to include clerkPublishableKey and set its value
there) and remove the separate environment export, then update ClerkService to
read the key from the injected AppConfig (use the AppConfig property rather than
a hardcoded string) so the publishable key is consistently sourced from the app
config.

1 change: 1 addition & 0 deletions gui-js/apps/minsky-web/src/environments/environment.web.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@
export const AppConfig = {
production: false,
environment: 'DEV',
clerkPublishableKey: '',
};
3 changes: 2 additions & 1 deletion gui-js/libs/core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ export * from './lib/component/dialog/dialog.component';
export * from './lib/services/communication/communication.service';
export * from './lib/services/electron/electron.service';
export * from './lib/services/WindowUtility/window-utility.service';
export * from './lib/services/TextInputUtilities';
export * from './lib/services/TextInputUtilities';
export * from './lib/services/clerk/clerk.service';
Loading
Loading