Skip to content
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.

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

Split middleware into two different components #1174

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Jomaguy
Copy link

@Jomaguy Jomaguy commented Mar 13, 2025

I split the middleware into two distinct parts, so that the frontend can now determine captach requirements. PuterHomePageService can set GUI parameters for captcha requirements. The /whoarewe endpoint provides captcha requirement information and the extensuo system integration is maintained

Jomaguy and others added 9 commits March 10, 2025 12:24
…em so that we prevent our system from replay attacks
…ed to our previous version that uses in memory storage
… captcha verification system now works on both login and sign int
…s to control wether a captcha should be required,

I fixed the code in CaptchaModule to use config
and got rid of the lines that made captcha middleware available since it wasn't used anywhre
…can now determine captach requirements. PuterHomePageService can set GUI parameters for captcha requirements. The /whoarewe endpoint provides captcha requirement information and the extensuo system integration is maintained
.gitignore Outdated
# Language Server Protocol cache
.lsp/
.lsp/.cache/
.lsp/.cache/db.transit.json
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines belong in the global .gitignore on your local machine, not in our repo

.gitignore Outdated
# clj-kondo cache
.clj-kondo/
.clj-kondo/.cache/
.clj-kondo/.cache/v1/lock
Copy link
Contributor

Choose a reason for hiding this comment

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

(same as previous comment)

* This allows the frontend to know in advance whether to display captcha
* elements without needing to make a request and get a rejection first.
*/
const WHOAREWE_GET = eggspress('/whoarewe', {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have /whoarewe implemented in PuterHomepageService, so adding another endpoint with the same name will break detection of whether or not user signup is disabled. This behavior should be moved to the endpoint in PuterHomepageService.

@KernelDeimos
Copy link
Contributor

KernelDeimos commented Mar 13, 2025

I tried to manually test this and I ran into a few issues. The first one seems to be some missing changes; if you did manual testing than I assume you made these changes and didn't commit them or accidentally stashed them. Here's the error message I got:

TypeError: requireCaptcha is not a function
    at Object.<anonymous> (/home/edube/rgroups/puter/puter/src/backend/src/routers/login.js:59:66)
    at Module._compile (node:internal/modules/cjs/loader:1434:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1518:10)
    at Module.load (node:internal/modules/cjs/loader:1249:32)
    at Module._load (node:internal/modules/cjs/loader:1065:12)
    at Module.require (node:internal/modules/cjs/loader:1271:19)
    at require (node:internal/modules/helpers:123:16)
    at __on_install.routes (/home/edube/rgroups/puter/puter/src/backend/src/services/PuterAPIService.js:71:17)
    at PuterAPIService.__on (/home/edube/rgroups/puter/puter/src/backend/src/services/BaseService.js:106:22)
    at /home/edube/rgroups/puter/puter/src/backend/src/services/Container.js:219:69
    at /home/edube/rgroups/puter/puter/src/backend/src/util/context.js:178:26
    at AsyncLocalStorage.run (node:async_hooks:346:14)
    at Context.arun (/home/edube/rgroups/puter/puter/src/backend/src/util/context.js:176:26)
    at Context.arun (/home/edube/rgroups/puter/puter/src/backend/src/util/context.js:69:27)
    at Container.emit (/home/edube/rgroups/puter/puter/src/backend/src/services/Container.js:219:39)
    at __on_boot.consolidation (/home/edube/rgroups/puter/puter/src/backend/src/modules/web/WebServerService.js:66:24)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async WebServerService.__on (/home/edube/rgroups/puter/puter/src/backend/src/services/BaseService.js:106:16)
    at async /home/edube/rgroups/puter/puter/src/backend/src/util/context.js:178:20
    at async Context.arun (/home/edube/rgroups/puter/puter/src/backend/src/util/context.js:176:16)
    at async Promise.all (index 89)
    at async Container.emit (/home/edube/rgroups/puter/puter/src/backend/src/services/Container.js:222:9)
    at async Kernel._boot_services (/home/edube/rgroups/puter/puter/src/backend/src/Kernel.js:187:9)
    at async /home/edube/rgroups/puter/puter/src/backend/src/Kernel.js:121:13
    at async /home/edube/rgroups/puter/puter/src/backend/src/util/context.js:178:20
    at async Context.arun (/home/edube/rgroups/puter/puter/src/backend/src/util/context.js:176:16) 

These were the changes that fixed it:

diff --git a/src/backend/src/routers/login.js b/src/backend/src/routers/login.js
index 85c51551..60722eb9 100644
--- a/src/backend/src/routers/login.js
+++ b/src/backend/src/routers/login.js
@@ -22,7 +22,7 @@ const router = new express.Router();
 const { get_user, body_parser_error_handler } = require('../helpers');
 const config = require('../config');
 const { DB_WRITE } = require('../services/database/consts');
-const requireCaptcha = require('../modules/captcha/middleware/captcha-middleware');
+const { requireCaptcha } = require('../modules/captcha/middleware/captcha-middleware');
 
 
 const complete_ = async ({ req, res, user }) => {
diff --git a/src/backend/src/routers/signup.js b/src/backend/src/routers/signup.js
index 49050389..d94f5f16 100644
--- a/src/backend/src/routers/signup.js
+++ b/src/backend/src/routers/signup.js
@@ -25,7 +25,7 @@ const { DB_WRITE } = require('../services/database/consts');
 const { generate_identifier } = require('../util/identifier');
 const { is_temp_users_disabled: lazy_temp_users, 
         is_user_signup_disabled: lazy_user_signup } = require("../helpers")
-const requireCaptcha = require('../modules/captcha/middleware/captcha-middleware');
+const { requireCaptcha } = require('../modules/captcha/middleware/captcha-middleware');
 
 async function generate_random_username () {
     let username;

The next is a security issue. With these changes, logging in now puts the users password in the querystring portion of the URL. That must be fixed before this is merged.

@Jomaguy
Copy link
Author

Jomaguy commented Mar 14, 2025

I went ahead and fixed the security issue where the logging flow was putting the users password in the URL, I updated the imports you told me about, the captcha verification system now uses the correct whoarewe and I removed the other one.

@KernelDeimos
Copy link
Contributor

KernelDeimos commented Mar 14, 2025

Thanks! I just tested it out and it's working great. There are two things that I didn't realize before that we need to update as well, and then it should be ready to merge.

  1. We need to be able to disable captcha with config. I think it should be disabled by default for now because this is the sort of thing we really need to play around with for a while before we push it to prod.
  2. "Enter" for me was refreshing the captcha instead of logging in, so I got very confused while testing it thinking that it wasn't working. We should make sure pressing Enter always does a login attempt because I think that's what most users will expect.

I recorded manual testing on video in case that's a helpful reference.

pr1174_1.5x.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants