Reconcile Git Proxy with most up-to-date improvements

[JamieSlome]

Closes #201 #48 #42 #38 #17 #9 #423 #28 #454

[netlify[bot]]

✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name	Link
🔨 Latest commit	248e5f8ac89efd4dc0b92d518a59e75a7f616b2f
🔍 Latest deploy log	https://app.netlify.com/sites/endearing-brigadeiros-63f9d0/deploys/66321ac3182a880008ac51b2

[maoo]

This is great, thanks @JamieSlome !

I looked at the diff, it seems that all changes apply to dashboard and testing, is that correct?

On top of the quickstart instructions, what are the steps to run the UI? I'd be happy to help testing/reviewing here.

[JamieSlome]

This is great, thanks @JamieSlome !

I looked at the diff, it seems that all changes apply to dashboard and testing, is that correct?

On top of the quickstart instructions, what are the steps to run the UI? I'd be happy to help testing/reviewing here.

Added you as a reviewer slightly too early @maoo! Still lots of changes to come 👍

[JamieSlome]

@maoo @coopernetes - getting extremely close with this now. It might be worth having a mull over the code. The service and proxy are broken in the current form, but I believe this is a result of too tightly coupling the setup to a MongoDB configuration rather than file-based storage.

Here if you have any questions 👍 ❤️

[coopernetes]

This change set is fantastic - much appreciate all the work that's going into this PR @JamieSlome ❤️

I started a cursory review of the proxy server & API changes and will provide feedback if needed. Lots of exciting stuff landing in this PR so want to give it the proper attention 🙌

[coopernetes]

Patch for Error: Cannot find module 'express-mongodb-session'

diff --git a/package.json b/package.json
index fb9e139..abb0686 100644
--- a/package.json
+++ b/package.json
@@ -38,6 +38,7 @@
     "email-validator": "^2.0.4",
     "express": "^4.18.2",
     "express-http-proxy": "^2.0.0",
+    "express-mongodb-session": "^3.2.1",
     "express-session": "^1.17.1",
     "generate-password": "^1.5.1",
     "history": "5.3.0",

[github-actions[bot]]

LCOV of commit 4cc2da0 during Node.js CI #1049

Summary coverage rate:
  lines......: 20.0% (16 of 80 lines)
  functions..: 20.0% (1 of 5 functions)
  branches...: 9.1% (2 of 22 branches)

Files changed coverage rate:
                      |Lines       |Functions  |Branches    
  Filename            |Rate     Num|Rate    Num|Rate     Num
  ==========================================================
  src/config/file.js  |80.0%      5|50.0%     2|50.0%      2
  src/db/index.js     | 9.3%     43| 0.0%     1| 5.0%     20
  src/service/index.js|25.0%     32| 0.0%     2|    -      0

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 31.07143% with 386 lines in your changes are missing coverage. Please review.

Project coverage is 55.46%. Comparing base (30271d1) to head (248e5f8).

Files	Patch %	Lines
src/service/routes/push.js	5.47%	69 Missing :warning:
src/proxy/processors/push-action/scanDiff.js	16.00%	42 Missing :warning:
src/proxy/processors/push-action/parsePush.js	0.00%	40 Missing :warning:
...roxy/processors/push-action/checkCommitMessages.js	13.95%	37 Missing :warning:
.../proxy/processors/push-action/checkAuthorEmails.js	18.18%	27 Missing :warning:
src/service/passport/activeDirectory.js	0.00%	23 Missing :warning:
src/service/routes/repo.js	42.10%	22 Missing :warning:
.../processors/push-action/checkUserPushPermission.js	19.23%	21 Missing :warning:
src/service/routes/auth.js	46.66%	16 Missing :warning:
src/db/file/pushes.js	30.00%	14 Missing :warning:
... and 14 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #335      +/-   ##
==========================================
- Coverage   64.69%   55.46%   -9.23%     
==========================================
  Files          40       45       +5     
  Lines        1198     1556     +358     
==========================================
+ Hits          775      863      +88     
- Misses        423      693     +270     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[JamieSlome]

@coopernetes @maoo - basically done with the full push now 👍 😌 Let's get this branch stable ❤️

Can you help figure out why the proxy and REST API aren't running properly on 8000 and 8080, respectively?

[coopernetes]

Can you help figure out why the proxy and REST API aren't running properly on 8000 and 8080, respectively?

@JamieSlome I'm not seeing this behaviour. I was getting an error with the file-based db.

0] Proxy Listening on 8000
[0] /Users/544820491/code/opensource/git-proxy/src/service/index.js:29
[0]       store: db.getSessionStore(session),
[0]                 ^
[0] 
[0] TypeError: db.getSessionStore is not a function
[0]     at Object.start (/Users/544820491/code/opensource/git-proxy/src/service/index.js:29:17)
[0]     at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
[0] 
[0] Node.js v20.10.0
[0] npm run server exited with code 1

After applying the below patch, both the proxy server & API is listening on 8000 & 8080 respectively. diff

diff --git a/src/db/file/index.js b/src/db/file/index.js
index 583be50..1402fda 100644
--- a/src/db/file/index.js
+++ b/src/db/file/index.js
@@ -23,3 +23,6 @@ module.exports.removeUserCanPush = repo.removeUserCanPush;
 module.exports.removeUserCanAuthorise = repo.removeUserCanAuthorise;
 
 module.exports.deleteRepo = repo.deleteRepo;
+module.exports.getSessionStore = (session) => {
+  console.log(`File does not store sessions, no-op. ${session}`);
+};

[0] Proxy Listening on 8000
[0] File does not store sessions, no-op. function session(options) {
[0]   var opts = options || {}
...
[0] Service Listening on 8080

[JamieSlome]

@coopernetes - once you apply the patch, try running the tests with npm run test and we get various breakages.

1. Apply getSessionStore patch
2. Run npm run build
3. Run npm run test - results in various breakages
4. Run npm run start - UI loads but having issues connecting and communicating with API

[JamieSlome]

@coopernetes @maoo - when I try and logout, I get a 404. Could this be because of the HTTP filter or the new port configuration via ENV variables? Shouldn't port 8000 be porting to GitHub.com?

[maoo]

@coopernetes @maoo - when I try and logout, I get a 404. Could this be because of the HTTP filter or the new port configuration via ENV variables? Shouldn't port 8000 be porting to GitHub.com?

If I run curl -X POST http://localhost:8080/api/auth/logout , the server logs the following error and dies.

/Users/m/w/git-proxy/node_modules/passport/lib/sessionmanager.js:83
    req.session.regenerate(function(err) {
                ^

TypeError: Cannot read properties of undefined (reading 'regenerate')
    at Immediate.<anonymous> (/Users/m/w/git-proxy/node_modules/passport/lib/sessionmanager.js:83:17)
    at process.processImmediate (node:internal/timers:478:21)

I'll check what's going on.

[maoo]

@JamieSlome - this worked for me - https://github.com/finos/git-proxy/compare/reconcile...fix-logout?expand=1

Can you please test on your end? It's 1-line fix, you don't need to switch branch.

[maoo]

@JamieSlome - kicked Netlify - here's the preview - https://deploy-preview-335--endearing-brigadeiros-63f9d0.netlify.app/

[JamieSlome]

@JamieSlome - this worked for me - https://github.com/finos/git-proxy/compare/reconcile...fix-logout?expand=1

Can you please test on your end? It's 1-line fix, you don't need to switch branch.

Hmm, doesn't seem to work for me. I am trying to invoke logout from the UI?

[JamieSlome]

  const logout = () => {
    axios.post('/api/auth/logout').then((res) => {
      if (res.status === 204) {
        navigate('/admin/profile', { replace: true });
      }
    });
  };

@maoo - see the above code in AdminNavbarLinks.jsx 👍

[socket-security[bot]]

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert	Package	Note	Source
Install scripts	npm/[email protected]	Install script: postinstall Source: node install.js	package-lock.json package.json
Install scripts	npm/[email protected]	Install script: install Source: node-pre-gyp install --fallback-to-build	package-lock.json package.json

View full report↗︎

Next steps

What is an install script?

Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts.

Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

• @SocketSecurity ignore npm/[email protected]
• @SocketSecurity ignore npm/[email protected]

[coopernetes]

@JamieSlome can you apply the below patch and see if that resolves the login issue? This is related to the session not being set as well as a raw Promise being passed into the file database (and have a null result when the login flow is initiated, causing the password hash comparison to fail):

diff --git a/src/db/index.js b/src/db/index.js
index aa5efb9..a1ff335 100644
--- a/src/db/index.js
+++ b/src/db/index.js
@@ -18,7 +18,7 @@ module.exports.createUser = async (username, password, email, gitAccount, admin
 
   const data = {
     username: username,
-    password: bcrypt.hash(password, 10),
+    password: await bcrypt.hash(password, 10),
     gitAccount: gitAccount,
     email: email,
     admin: admin,
diff --git a/src/service/index.js b/src/service/index.js
index ff96229..3eb9e41 100644
--- a/src/service/index.js
+++ b/src/service/index.js
@@ -31,7 +31,7 @@ const start = async () => {
       resave: false,
       saveUninitialized: false,
       cookie: {
-        secure: true,
+        secure: 'auto',
         maxAge: config.getSessionMaxAgeHours() * 60 * 60 * 1000,
       },
     }),

[JamieSlome]

@coopernetes - sorted 👍 Works a charm.

@maoo - I am now just working on fixing up logout and from a functionality perspective, we are done 🎉

[JamieSlome]

@JamieSlome - this worked for me - https://github.com/finos/git-proxy/compare/reconcile...fix-logout?expand=1

Can you please test on your end? It's 1-line fix, you don't need to switch branch.

Tried this fix, but doesn't actually log the user out, just prevents the error from bubbling 😢

[JamieSlome]

@maoo @coopernetes - login and logout fixed 👍

[JamieSlome]

@maoo @coopernetes - now only 2 unit tests away from passing all tests. Bit confused why these last two are failing. Any ideas?

  1) user creation
       should be able to create a new user:

      AssertionError: expected { Object (_events, _eventsCount, ...) } to have status code 200 but got 404
      + expected - actual

      -404

  2) user creation
       should access the profile:

      AssertionError: expected { Object (_events, _eventsCount, ...) } to have status code 200 but got 401
      + expected - actual

      -401
      +200

[JamieSlome]

@coopernetes - any ideas how we can turn on / off the UI via the npx command?

[coopernetes]

@coopernetes - any ideas how we can turn on / off the UI via the npx command?

Toggling the UI will likely require a runtime configuration flag. Alternatively, we can supply multiple "bin" values to support executing the git-proxy package with either the UI or without it. I don't think npx has any interaction with the running application unless we were to expose an API endpoint which shuts down the UI. That seems dubious.

diff --git a/package.json b/package.json
index 28966b2..a7f56b1 100644
--- a/package.json
+++ b/package.json
@@ -18,7 +18,10 @@
     "lint": "eslint --fix . --ext .js,.jsx",
     "gen-schema-doc": "node ./scripts/doc-schema.js"
   },
-  "bin": "./index.js",
+  "bin": {
+    "git-proxy": "./index.js",
+    "fullstack": "concurrently 'node ./index.js' 'vite --config vite.config.js'"
+  },
   "author": "Paul Groves",
   "license": "Apache-2.0",
   "dependencies": {

The above would support this use case:

# No arguments, start just the proxy & API server
$ npx @finos/git-proxy
# with the "fullstack" command, runs all three
$ npx @finos/git-proxy fullstack

Is that closer to what you're looking for?

[coopernetes]

Also, I would suggest that we complete this PR as feature-complete and prepare for merge & release. We can always release a newer version with additional enhancements & improvements.

[JamieSlome]

@coopernetes - any ideas how we can turn on / off the UI via the npx command?

Toggling the UI will likely require a runtime configuration flag. Alternatively, we can supply multiple "bin" values to support executing the git-proxy package with either the UI or without it. I don't think npx has any interaction with the running application unless we were to expose an API endpoint which shuts down the UI. That seems dubious.

diff --git a/package.json b/package.json
index 28966b2..a7f56b1 100644
--- a/package.json
+++ b/package.json
@@ -18,7 +18,10 @@
     "lint": "eslint --fix . --ext .js,.jsx",
     "gen-schema-doc": "node ./scripts/doc-schema.js"
   },
-  "bin": "./index.js",
+  "bin": {
+    "git-proxy": "./index.js",
+    "fullstack": "concurrently 'node ./index.js' 'vite --config vite.config.js'"
+  },
   "author": "Paul Groves",
   "license": "Apache-2.0",
   "dependencies": {

The above would support this use case:

# No arguments, start just the proxy & API server
$ npx @finos/git-proxy
# with the "fullstack" command, runs all three
$ npx @finos/git-proxy fullstack

Is that closer to what you're looking for?

Yes, definitely close to what I am looking for. Agree that having an endpoint to shutdown the UI is not particularly ideal. Multiple bin commands looks good 👍 Do you want to add to this branch or shall I update?

Also can we rename fullstack to with-ui? Something a little more descriptive?

[JamieSlome]

@maoo - can you give your green tick review here as well? 👍 ✅

[coopernetes]

This also closes #454 since the changes from related PR was integrated via https://github.com/finos/git-proxy/commit/6ff82d3a760d4f33ebc3673dcbd601ffa1cce9ed

[JamieSlome]

@maoo @coopernetes - I've discovered the correct pattern for applying CSRF and this now works 👍 The issue is that with CSRF protections, we'd need to make sure that any invocations against privileged endpoints are submitted with a CSRF token. You can see this demonstrated with the now failing tests.

I'd recommend that we make CSRF protections configurable. It is desirable to have CSRF protections with the UI. From my understanding CSRF is very much a browser-based vulnerability. I can add it as a flag to proxy.config.json.

Thoughts?