Refactor auth controller into modular structure

[Copilot]

Description

Decomposed monolithic /src/app/controllers/auth.ts (442 lines) into domain-organized modules under auth/ and user/ folders, using ES6 import statements throughout.

Changes

New controller modules:

• user/customers.ts - Customer management (customer, customerCreate, customerUpdate)
• user/account.ts - Stripe Connect accounts (account, accountCreate, accountBalance, accountUpdate, accountDelete, accountCountries)
• user/bank-account.ts - Bank account operations (createBankAccount, updateBankAccount, userBankAccount)
• auth/callbacks.ts - OAuth callbacks (callbackGithub, callbackBitbucket)

Relocated to user/user.ts:

• User lifecycle: register, activateUser, resendActivationEmail, deleteUserById
• Password management: forgotPasswordNotification, resetPassword, changePassword
• User data: preferences, organizations, userFetch
• Private task creation: createPrivateTask

Relocated to auth/auth.ts:

• OAuth flows: authorizeGithubPrivateIssue, connectGithub, disconnectGithub, authorizeLocal

Method renames (snake_case → camelCase):

• activate_user → activateUser
• resend_activation_email → resendActivationEmail

Route updates:

• Updated imports in routes/auth/auth.ts, routes/auth/providers.ts, routes/user.ts
• Maintained existing endpoint paths and behavior

Code modernization:

• Replaced all require() statements with ES6 import statements across all new controller modules
• Consistent use of modern import syntax for better type safety and tooling support
• Fixed TypeScript build errors:
  • Imported userDisconnectGithub directly from module file to resolve export issue
  • Applied proper TypeScript pattern for models: import Models from '../../../models' with const models = Models as any

Removed:

• /src/app/controllers/auth.ts (all methods migrated)

Thank you for your contribution!

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

In general, the fix is to add a rate-limiting middleware to routes that perform expensive operations or sensitive authorization flows, so that a single client cannot bombard them with requests. In an Express-based TypeScript application, a common approach is to use the well-known express-rate-limit package, configure a limiter (e.g., max N requests per time window), and then add that limiter to the relevant routes as an extra middleware argument.

For this file, the smallest targeted change that preserves existing functionality is:

• Import express-rate-limit.
• Define one or more limiter instances (e.g., for auth-related routes), using reasonable defaults (for example, 100 requests per 15 minutes per IP).
• Attach the limiter to the specific route that CodeQL flagged (/authorize/github/disconnect) and, optionally, to closely related authorization routes in this router. To keep the change minimal and focused, we will apply it to /authorize/github/disconnect only, since that is what CodeQL pointed out.
• This is done by adding the limiter as a middleware parameter in router.get('/authorize/github/disconnect', secure, disconnectGithub) so that the call becomes router.get('/authorize/github/disconnect', githubDisconnectRateLimiter, secure, disconnectGithub) (order: limiter first, then existing middlewares/handlers).

Concretely:

• At the top of src/app/routes/auth/providers.ts, add an import for express-rate-limit.
• Near the router definition (after creating router), define a githubDisconnectRateLimiter with windowMs and max settings.
• Update the /authorize/github/disconnect route line to include this limiter as an additional middleware.

In general, to fix missing rate limiting on an authorization endpoint, we add a rate-limiting middleware before the route handler. In an Express app, a common approach is to use a package like express-rate-limit to create a limiter (e.g., a small number of requests per IP per unit time for sensitive auth routes) and apply it specifically to the vulnerable route(s).

For this file, the least invasive fix is:

1. Import express-rate-limit.
2. Define a dedicated limiter for auth/provider routes (e.g., authProviderLimiter) near the router initialization.
3. Apply that limiter to /connect/github by inserting it into the middleware chain: router.get('/connect/github', authProviderLimiter, secure, connectGithub) so existing behavior is preserved except for the new limit.
4. Optionally, the same limiter could be reused for other /authorize/* or /callback/* routes, but to minimize change, we will only apply it to /connect/github as requested.

Concretely in src/app/routes/auth/providers.ts:

• Add import rateLimit from 'express-rate-limit' at the top.
• Add a constant const authProviderLimiter = rateLimit({ ... }) after creating router.
• Modify line 41 so that authProviderLimiter is inserted as the first middleware for that route, preserving secure and connectGithub order.

In general, the issue should be fixed by adding a rate-limiting middleware to sensitive routes that perform authorization or other expensive operations. In an Express-based app, this is commonly done with express-rate-limit, configuring limits that are appropriate for the route (e.g., low frequency for connect/link flows), and then inserting the limiter before the existing middleware/handler chain.

For this specific file, the minimal, non-breaking fix is to introduce a rate limiter and apply it to the /connect/github route (and, optionally, other sensitive auth routes if desired). We will import express-rate-limit, define a connectRateLimiter (for example, 10 requests per 15 minutes per IP, with standardHeaders and legacyHeaders configured), and add it as a middleware in the route definition, before secure and connectGithub. Concretely:

• At the top of src/app/routes/auth/providers.ts, add import rateLimit from 'express-rate-limit'.
• After initializing the router (right after const router = express.Router()), define a const connectRateLimiter = rateLimit({ ... }).
• Modify the /connect/github route so that it becomes router.get('/connect/github', connectRateLimiter, secure, connectGithub).

This keeps existing behavior intact while adding a clear in-code rate-limiting layer on the route that CodeQL has flagged.

To fix the problem, the controllers should avoid sending the raw error object (or any stack-trace-bearing details) back to the client. Instead, they should log the full error server-side and send a generic, non-sensitive error response body, such as a static message or a minimal structured JSON error, without including error.message, error.stack, or similar details.

Concretely, in src/app/controllers/user/user.ts, within the userFetch and deleteUserById handlers:

• Keep logging error to the console (or a real logger) so developers can still debug.
• Replace res.status(400).send(error) with a safe response, e.g. res.status(400).send({ success: false }) or a generic error message like "Unable to fetch user" / "Unable to delete user". This preserves the HTTP semantics (400 on failure) without exposing internal exception details.
• No new imports or helper methods are strictly needed; using simple string or object literals is sufficient and least invasive.

Other catch blocks in the snippet (preferences, organizations) already send only false without exposing the error, so they do not require changes.

In general, the fix is to avoid sending raw exception objects (or their stack traces) to the client. Instead, log the detailed error server-side, and return a generic, user‑friendly error message and code to the client that does not reveal implementation details. Where appropriate, you can include a simple error identifier or code that can be correlated with server logs.

For this specific case in src/app/controllers/user/user.ts, the problematic line is res.status(400).send(error) in the userFetch controller. To keep existing functionality as close as possible while fixing the leak, we should:

• Continue logging the error on the server using console.log(error) (as already done).
• Replace the response body error with a generic message object, such as { message: 'Unable to fetch user data.' } or simply false, depending on how the frontend expects errors. Because the success path sends data with a 200, the client is likely expecting JSON; sending a simple JSON error object with a generic message is a safe default.
• Do not introduce new behavior that changes status codes beyond the existing 400.

No new imports are needed: we can construct a simple literal object/message inline. Concretely:

• In userFetch, change line 253 from res.status(400).send(error) to something like res.status(400).send({ message: 'Unable to fetch user data.' }).

Other catch blocks (preferences, organizations, deleteUserById) already avoid sending the raw error and instead send false, so they do not need changes for this specific CodeQL alert.

In general, the fix is to avoid sending raw error/exception objects (including stack traces) to the client. Instead, log the detailed error on the server and return a generic, non-sensitive error message and appropriate HTTP status code (e.g., 500). This preserves debuggability while preventing information exposure.

For this file, the best low-impact fix is:

• Keep console.log(error) so developers still see the full error on the server.
• Change res.send(error) in all three catch blocks to a generic error response, e.g. res.status(500).send({ message: 'Internal server error' }) or similar.
• Optionally, you can use res.status(500).json({ ... }) if JSON is the standard, but since the existing code uses res.send, we can keep that and only adjust the content and status code.
• No new imports are required; we only modify the catch blocks.

Concretely:

• In createBankAccount (around line 10–14), replace res.send(error) with res.status(500).send({ message: 'Internal server error' }).
• In updateBankAccount (around line 21–25), perform the same replacement.
• In userBankAccount (around line 32–36), perform the same replacement.

This does not change the functional success path, only the error payload and status code.

In general, to fix this issue you should never send raw Error objects or stack traces to the client. Instead, log the full error on the server side and return a generic, non-sensitive error response to the user (for example, a boolean flag, a simple message, or a structured error code that does not include stack trace or internal implementation details).

For this specific file, the minimal and consistent fix is:

• Keep logging the full error object to the server console for debugging (as the rest of the file already does).
• Replace res.status(401).send(error) in accountDelete with a generic response, such as res.status(401).send(false) or a simple message like "Unable to delete account". To preserve existing behavior patterns in this file, it is best to respond with false, as done in account, accountCreate, accountCountries, and accountBalance.
• No new imports or helper methods are required; we only adjust what is returned to the client.

Concretely:

• In src/app/controllers/user/account.ts, lines 55–56 and 65–66 are currently sending the raw error. Update both accountUpdate and accountDelete to send a generic failure (false) instead:
  • Change res.status(401).send(error) to res.status(401).send(false) in both catch blocks.

In general, to fix information exposure through stack traces you should avoid sending raw exception objects or stack traces to clients. Instead, log the details on the server and return a generic, non-sensitive error response to the client (optionally with a simple error code/message that doesn’t reveal internal structure).

For this file, the minimal, non-breaking fix is to change accountUpdate (and, for consistency and security, accountDelete) so that they no longer send the raw error object. They should instead log the error (as they already do) and respond with a generic message or a simple boolean as done in the other handlers. To preserve existing semantics as much as possible, we can keep the same HTTP status code (401) but replace send(error) with a safe payload such as send(false) or a generic string like "Unauthorized" or "An error occurred while updating the account". Since the other endpoints respond with false on error, the most consistent choice is res.status(401).send(false).

Concretely:

• In src/app/controllers/user/account.ts, in accountUpdate, replace res.status(401).send(error) with res.status(401).send(false).
• In accountDelete, replace res.status(401).send(error) with res.status(401).send(false) to avoid the same pattern of exposing the raw error and to keep behavior consistent.
  No new imports or helper methods are required; we only change what is sent in the response.

In general, the problem should be fixed by ensuring that exception text or error objects that may contain user input are not sent back to clients in a form that can be interpreted as HTML/JS. Instead, return a controlled, neutral error structure (e.g., a generic message and optional error code), and avoid echoing raw exception messages. If you must include details, send them only in safe contexts (logging, monitoring) or after proper encoding/sanitization.

For this codebase, the best minimal fix is to change the accountUpdate and accountDelete controllers so they no longer send the raw error object. Instead, log the full error server-side (as already done), and return a generic, structured JSON error payload such as { success: false, error: '...' } with an appropriate content type and HTTP status code. This keeps existing semantics (401 status, indication of failure) but removes user-controlled content from the HTTP body. We will modify:

• src/app/controllers/user/account.ts, line 55: replace res.status(401).send(error) with res.status(401).json({ error: 'Could not update account' }) (or similar generic message).
• src/app/controllers/user/account.ts, line 66: replace res.status(401).send(error) with res.status(401).json({ error: 'Could not delete account' }).

No changes are needed in src/modules/users/userAccountUpdate.ts for this specific sink; the core issue is the unsafe exposure of error in the controller. No new imports are required, as res.json is part of Express’s response API and already available.

---

General approach: do not propagate raw exceptions or stack traces to user-facing responses. Instead, log detailed error information on the server and return a generic, safe payload to the caller (controller), which in turn should send only that generic payload to the client. That means userAccount should never return e directly; it should return either an empty object or a normalized error structure that does not include stack traces or internal implementation details. The controller logic can remain the same if the returned data is guaranteed safe.

Best concrete fix for this code:

1. In src/modules/users/userAccount.ts, change the catch block inside userAccount so that:
   • It still logs the full error to the server (console.log('could not find customer', e)).
   • It returns a safe, generic object instead of the raw error: for example, { error: 'ACCOUNT_RETRIEVAL_FAILED' } or {}. This prevents potential stack trace or internal Stripe information from being sent out.
2. In src/app/controllers/user/account.ts, the account handler currently returns whatever userAccount yields via res.send(data). Once userAccount is returning a sanitized object, this is safe and we do not need to modify the controller logic for this issue. The other handlers (accountCreate, accountCountries, accountBalance, accountUpdate, accountDelete) are not part of the taint path identified for this specific alert, and we leave them as is per the instructions not to change functionality unnecessarily.

No new imports or helper methods are needed; we only adjust the return value in the existing catch block.

[alexanmtz]

Many build errors

my-gitpay@0.0.0 build
tsc -p tsconfig.json

src/app/controllers/auth/auth.ts:55:29 - error TS2339: Property 'userDisconnectGithub' does not exist on type 'typeof import("/home/circleci/repo/src/modules/users/index")'.

55 const data = await user.userDisconnectGithub({ userId: req.user.id })
~~~~~~~~~~~~~~~~~~~~

src/app/controllers/user/user.ts:68:28 - error TS2339: Property 'User' does not exist on type 'typeof db'.

68 const token = models.User.generateToken()
~~~~

src/app/controllers/user/user.ts:69:20 - error TS2339: Property 'User' does not exist on type 'typeof db'.

69 await models.User.update({ recover_password_token: token }, { where: { email } })
~~~~

src/app/controllers/user/user.ts:92:36 - error TS2339: Property 'User' does not exist on type 'typeof db'.

92 const foundUser = await models.User.findOne({
~~~~

src/app/controllers/user/user.ts:96:33 - error TS2339: Property 'User' does not exist on type 'typeof db'.

96 const passwordHash = models.User.generateHash(req.body.password)
~~~~

src/app/controllers/user/user.ts:98:20 - error TS2339: Property 'User' does not exist on type 'typeof db'.

98 await models.User.update(
~~~~

src/app/controllers/user/user.ts:181:36 - error TS2339: Property 'User' does not exist on type 'typeof db'.

181 const foundUser = await models.User.findOne({ where: { activation_token: token, id: userId } })
~~~~

src/app/controllers/user/user.ts:185:39 - error TS2339: Property 'User' does not exist on type 'typeof db'.

185 const userUpdate = await models.User.update(
~~~~

src/app/controllers/user/user.ts:201:36 - error TS2339: Property 'User' does not exist on type 'typeof db'.

201 const foundUser = await models.User.findOne({ where: { id: userId } })
~~~~

src/app/controllers/user/user.ts:204:26 - error TS2339: Property 'User' does not exist on type 'typeof db'.

204 const token = models.User.generateToken()
~~~~

src/app/controllers/user/user.ts:207:21 - error TS2339: Property 'User' does not exist on type 'typeof db'.

207 (await models.User.update(
~~~~

Found 11 errors in 2 files.

Errors Files
1 src/app/controllers/auth/auth.ts:55
10 src/app/controllers/user/user.ts:68

Exited with code exit status 1

[Copilot]

Many build errors

my-gitpay@0.0.0 build...

Fixed in cc059d2. Resolved all 11 build errors:

userDisconnectGithub error: Import changed to direct import from the file:

import userDisconnectGithub from '../../../modules/users/userDisconectGithub'

models.User errors (10 instances): Fixed by using the proper TypeScript pattern:

import Models from '../../../models'
const models = Models as any

This matches the pattern used throughout the codebase (e.g., in src/queries/user/findUserById.ts).