Skip to content

feat(express): migrate handleCreateSignerMacaroon to typed routes #6967

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

Jump to bottom
Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

feat(express): migrate handleCreateSignerMacaroon to typed routes #6967

wants to merge 9 commits into from

Conversation

danielzhao122
Copy link
Contributor

Ticket: WP-5445

@danielzhao122 danielzhao122 requested review from a team as code owners September 11, 2025 21:13
Taseen08
Taseen08 previously approved these changes Sep 11, 2025
View reviewed changes
OttoAllmendinger
OttoAllmendinger requested changes Sep 16, 2025
View reviewed changes
zahin-mohammad
zahin-mohammad previously approved these changes Sep 19, 2025
View reviewed changes
alextse-bg
alextse-bg previously approved these changes Sep 19, 2025
View reviewed changes
zahin-mohammad
zahin-mohammad previously approved these changes Sep 25, 2025
View reviewed changes
zahin-mohammad
zahin-mohammad requested changes Sep 29, 2025
View reviewed changes
Copy link
Contributor

@zahin-mohammad zahin-mohammad left a comment

Choose a reason for hiding this comment

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

Changes to express.lightning.signerMacaroon seem fine, but looks like a bad merge conflict resolution.

Taseen08
Taseen08 previously approved these changes Oct 7, 2025
View reviewed changes
Taseen08
Taseen08 reviewed Oct 7, 2025
View reviewed changes
Copy link
Contributor

@Taseen08 Taseen08 left a comment

Choose a reason for hiding this comment

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

lgtm, but merge conflicts

@danielzhao122 danielzhao122 dismissed stale reviews from Taseen08 and zahin-mohammad via 3ff273a October 9, 2025 02:01
@danielzhao122 danielzhao122 force-pushed the WP-5445-express-migrate-api-v2-coin-wallet-id-signer-macaroon-to-typed-routes branch from 4d24100 to 3ff273a Compare October 9, 2025 02:01
Taseen08
Taseen08 reviewed Oct 9, 2025
View reviewed changes
Copy link
Contributor

@Taseen08 Taseen08 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

another merge conflict yet again unfortunately

zahin-mohammad
zahin-mohammad reviewed Oct 10, 2025
View reviewed changes
Copy link
Contributor

@zahin-mohammad zahin-mohammad left a comment

Choose a reason for hiding this comment

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

lgtm, but conflicts

Taseen08
Taseen08 previously approved these changes Oct 17, 2025
View reviewed changes
Copy link
Contributor

@Taseen08 Taseen08 left a comment

Choose a reason for hiding this comment

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

lgtm

@danielzhao122 danielzhao122 force-pushed the WP-5445-express-migrate-api-v2-coin-wallet-id-signer-macaroon-to-typed-routes branch from 329cb8d to ee326a2 Compare October 17, 2025 20:29
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 17, 2025
View reviewed changes
modules/express/test/unit/typedRoutes/signerMacaroon.ts

before(function () {
// Create a temporary JSON file for lightning signer config
fs.writeFileSync(tempFilePath, JSON.stringify({}));

Check failure

Code scanning / CodeQL

Insecure temporary file High test

Insecure creation of file in
the os temp dir
Loading
.

Copilot Autofix

AI 2 days ago

To securely create a temporary file, we should use a well-tested library that prevents predictable file names and ensures atomic creation with correct permissions. In NodeJS, the tmp package provides such functionality. In this case:

  • Replace the hardcoded path with a securely generated temporary file using tmp.fileSync.
  • You must add the package import at the top of the file.
  • Instantiate the temp file in before and clean it up in after using the cleanup function provided by tmp.
  • Ensure all references to tempFilePath are updated to use the secure path.
  • Do not change unrelated test logic or usage.
Suggested changeset 2
modules/express/test/unit/typedRoutes/signerMacaroon.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/modules/express/test/unit/typedRoutes/signerMacaroon.ts b/modules/express/test/unit/typedRoutes/signerMacaroon.ts
--- a/modules/express/test/unit/typedRoutes/signerMacaroon.ts
+++ b/modules/express/test/unit/typedRoutes/signerMacaroon.ts
@@ -1,6 +1,7 @@
 import * as assert from 'assert';
 import * as sinon from 'sinon';
 import * as fs from 'fs';
+import * as tmp from 'tmp';
 import { agent as supertest } from 'supertest';
 import 'should';
 import 'should-http';
@@ -12,10 +13,14 @@
 
 describe('Signer Macaroon Typed Routes Tests', function () {
   let agent: ReturnType<typeof supertest>;
-  const tempFilePath = '/tmp/test-lightning-signer.json';
+  let tempFilePath: string;
+  let tempFileCleanup: (() => void) | undefined;
 
   before(function () {
-    // Create a temporary JSON file for lightning signer config
+    // Create a securely random temporary JSON file for lightning signer config
+    const tmpFile = tmp.fileSync({ postfix: '.json' });
+    tempFilePath = tmpFile.name;
+    tempFileCleanup = tmpFile.removeCallback;
     fs.writeFileSync(tempFilePath, JSON.stringify({}));
 
     const { app } = require('../../../src/expressApp');
@@ -30,8 +33,8 @@
 
   after(function () {
     // Clean up the temporary file
-    if (fs.existsSync(tempFilePath)) {
-      fs.unlinkSync(tempFilePath);
+    if (tempFileCleanup) {
+      tempFileCleanup();
     }
   });
 
EOF
@@ -1,6 +1,7 @@
import * as assert from 'assert';
import * as sinon from 'sinon';
import * as fs from 'fs';
import * as tmp from 'tmp';
import { agent as supertest } from 'supertest';
import 'should';
import 'should-http';
@@ -12,10 +13,14 @@

describe('Signer Macaroon Typed Routes Tests', function () {
let agent: ReturnType<typeof supertest>;
const tempFilePath = '/tmp/test-lightning-signer.json';
let tempFilePath: string;
let tempFileCleanup: (() => void) | undefined;

before(function () {
// Create a temporary JSON file for lightning signer config
// Create a securely random temporary JSON file for lightning signer config
const tmpFile = tmp.fileSync({ postfix: '.json' });
tempFilePath = tmpFile.name;
tempFileCleanup = tmpFile.removeCallback;
fs.writeFileSync(tempFilePath, JSON.stringify({}));

const { app } = require('../../../src/expressApp');
@@ -30,8 +33,8 @@

after(function () {
// Clean up the temporary file
if (fs.existsSync(tempFilePath)) {
fs.unlinkSync(tempFilePath);
if (tempFileCleanup) {
tempFileCleanup();
}
});

modules/express/package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/modules/express/package.json b/modules/express/package.json
--- a/modules/express/package.json
+++ b/modules/express/package.json
@@ -55,7 +55,8 @@
     "morgan": "^1.9.1",
     "proxy-agent": "6.4.0",
     "proxyquire": "^2.1.3",
-    "superagent": "^9.0.1"
+    "superagent": "^9.0.1",
+    "tmp": "^0.2.5"
   },
   "devDependencies": {
     "@bitgo/public-types": "5.31.0",
EOF
@@ -55,7 +55,8 @@
"morgan": "^1.9.1",
"proxy-agent": "6.4.0",
"proxyquire": "^2.1.3",
"superagent": "^9.0.1"
"superagent": "^9.0.1",
"tmp": "^0.2.5"
},
"devDependencies": {
"@bitgo/public-types": "5.31.0",
This fix introduces these dependencies
Package Version Security advisories
tmp (npm) 0.2.5 None
Copilot is powered by AI and may make mistakes. Always verify output.
Taseen08
Taseen08 reviewed Oct 17, 2025
View reviewed changes
Copy link
Contributor

@Taseen08 Taseen08 left a comment

Choose a reason for hiding this comment

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

CI failures + copilot suggestion (not sure if that's relevant)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Taseen08 Taseen08 Taseen08 left review comments

@OttoAllmendinger OttoAllmendinger Awaiting requested review from OttoAllmendinger

@alextse-bg alextse-bg Awaiting requested review from alextse-bg

@mohammadalfaiyazbitgo mohammadalfaiyazbitgo Awaiting requested review from mohammadalfaiyazbitgo

@zahin-mohammad zahin-mohammad Awaiting requested review from zahin-mohammad

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@danielzhao122 @OttoAllmendinger @zahin-mohammad @Taseen08 @mohammadalfaiyazbitgo @alextse-bg