Refactorsept2025 by dhirmadi · Pull Request #56 · dhirmadi/mwapserver

dhirmadi · 2025-09-28T11:50:10Z

Pull Request

Description

Brief description of the changes in this PR.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactoring (no functional changes)
Performance improvement
Test coverage improvement

Related Issues

Closes #(issue number)

Changes Made

Change 1
Change 2
Change 3

Documentation Impact

No documentation changes needed
I have updated the Architecture Reference
I have updated the API Documentation
I have updated the Feature Documentation
I have updated the Integration Guides
I have updated the Frontend Documentation
I have updated the Testing Documentation
I have updated the README.md

Testing

I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
I have checked the Testing Documentation for guidance
I have run npm run docs:validate to check documentation links

Security Considerations

This change does not introduce security vulnerabilities
I have reviewed the security implications
Authentication/authorization is properly implemented
Input validation is in place where needed

Architecture Compliance

This change follows the patterns in Architecture Reference
This change aligns with the Domain Map
This change follows the Feature Patterns
API changes follow the API Documentation standards

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

Screenshots (if applicable)

Add screenshots to help explain your changes.

Additional Notes

Any additional information that reviewers should know.

- Add .cursorrules with repo summary, Node backend rules, security and workflow guidance - Add .cursor/agents for: node_backend, openhands_migrated_agents, auth0, codereview, security, github, schema_generator, feature_lifecycle - Preserve .openhands/ unchanged; mirror agents for Cursor discovery - Ensure Cursor understands stack, layout, and conventions for MWAP backend

…, .cursor/, .specstory/)

Copilot

Pull Request Overview

This refactoring pull request updates import/export statements across the codebase from .js to .ts extensions, adds comprehensive testing infrastructure, and implements security hardening improvements. The changes focus on modernizing the TypeScript module system while maintaining backward compatibility and adding production-ready safeguards.

Updated import statements from .js to .ts extensions across test files and OpenAPI endpoints
Implemented minimal Heroku-optimized testing strategy with critical test suites and release-phase validation
Added input sanitization, database indexing, and production error handling improvements

Reviewed Changes

Copilot reviewed 38 out of 42 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
tests/utils/tenants.service.test.ts	New unit tests for tenant service with mocking setup
tests/utils/projects.service.test.ts	New unit tests for projects service functionality
tests/setupTests.ts	Expanded test setup with comprehensive mocking and service stubs
tests/openapiendpoint/*.ts	Updated import paths from .js to .ts extensions
tests/oauth/oauthCallbackSecurity.test.ts	Fixed import paths and improved async test handling
tests/middleware/auth.test.ts	Updated imports and enhanced test reliability
tests/integration/test_cloud_providers.py	Simplified to basic endpoint protection validation
src/utils/validate.ts	Added string sanitization function
src/utils/logger.ts	Added correlation ID generation for better traceability
src/schemas/*.ts	Applied input sanitization to string fields
src/middleware/validateRequest.ts	New reusable validation middleware
src/middleware/errorHandler.ts	Added production-safe error logging
src/middleware/authorization.ts	Added unified permission checking system
src/features/projects/projects.service.ts	Fixed ObjectId query usage
src/config/env.ts	Removed debug logging for production safety
src/app.ts	Enhanced rate limiting with endpoint-specific rules
scripts/create-indexes.ts	New database index creation script
package.json	Added critical test runner and index creation to build
docs/*/.md	Updated documentation to reflect current architecture
.cursorrules	Added comprehensive development guidelines
Procfile	Added release-phase validation gate

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-28T11:53:08Z

tests/utils/tenants.service.test.ts

+    db.collection.mockReturnValueOnce({ findOne: vi.fn().mockResolvedValue(null) }) // owner check
+      .mockReturnValueOnce({ findOne: vi.fn().mockResolvedValue(null), insertOne: vi.fn().mockResolvedValue({ acknowledged: true }) }); // name check + insert


The chained mock setup is difficult to read and maintain. Consider extracting the mock configurations into named variables or helper functions to improve clarity and reusability.

Suggested change

db.collection.mockReturnValueOnce({ findOne: vi.fn().mockResolvedValue(null) }) // owner check

.mockReturnValueOnce({ findOne: vi.fn().mockResolvedValue(null), insertOne: vi.fn().mockResolvedValue({ acknowledged: true }) }); // name check + insert

// Extracted mock configurations for clarity

const ownerCheckMock = { findOne: vi.fn().mockResolvedValue(null) };

const nameCheckAndInsertMock = {

findOne: vi.fn().mockResolvedValue(null),

insertOne: vi.fn().mockResolvedValue({ acknowledged: true })

};

db.collection

.mockReturnValueOnce(ownerCheckMock) // owner check

.mockReturnValueOnce(nameCheckAndInsertMock); // name check + insert

Copilot · 2025-09-28T11:53:09Z

tests/setupTests.ts

+});
+
+// Ensure mocks also match other specifiers used by tests/services
+vi.mock('../../src/config/db', () => {


There are three identical mock implementations for the same module with different specifiers ('../../src/config/db', '../../src/config/db.js', '../../src/config/db'). This code duplication should be consolidated into a single mock definition or extracted into a shared helper function.

Copilot · 2025-09-28T11:53:09Z

tests/setupTests.ts

+  return { getDB: () => db, db };
+});
+
+vi.mock('../../src/config/db.js', () => {


There are three identical mock implementations for the same module with different specifiers ('../../src/config/db', '../../src/config/db.js', '../../src/config/db'). This code duplication should be consolidated into a single mock definition or extracted into a shared helper function.

Copilot · 2025-09-28T11:53:09Z

tests/oauth/oauthCallbackSecurity.test.ts

+vi.mock('../../src/utils/logger', () => ({
  logInfo: vi.fn(),
  logError: vi.fn(),
  logAudit: vi.fn()
 }));

-vi.mock('../../src/features/cloud-integrations/cloudIntegrations.service.js', () => ({
+vi.mock('../../src/features/cloud-integrations/cloudIntegrations.service', () => ({
  CloudIntegrationsService: vi.fn().mockImplementation(() => ({
    findById: vi.fn()
  }))
 }));

-vi.mock('../../src/features/cloud-providers/cloudProviders.service.js', () => ({
+vi.mock('../../src/features/cloud-providers/cloudProviders.service', () => ({


Mock paths are inconsistent - some use full paths with extensions, others don't. For consistency and maintainability, standardize the mock path format throughout the test files.

Copilot · 2025-09-28T11:53:09Z

tests/oauth/oauthCallbackSecurity.test.ts

+vi.mock('../../src/utils/logger', () => ({
  logInfo: vi.fn(),
  logError: vi.fn(),
  logAudit: vi.fn()
 }));

-vi.mock('../../src/features/cloud-integrations/cloudIntegrations.service.js', () => ({
+vi.mock('../../src/features/cloud-integrations/cloudIntegrations.service', () => ({
  CloudIntegrationsService: vi.fn().mockImplementation(() => ({
    findById: vi.fn()
  }))
 }));

-vi.mock('../../src/features/cloud-providers/cloudProviders.service.js', () => ({
+vi.mock('../../src/features/cloud-providers/cloudProviders.service', () => ({


Mock paths are inconsistent - some use full paths with extensions, others don't. For consistency and maintainability, standardize the mock path format throughout the test files.

Copilot · 2025-09-28T11:53:10Z

tests/middleware/auth.test.ts

      expect(logAudit).toHaveBeenCalledWith(
        'auth.failed',
-        undefined,
+        'unknown',


The test expects 'unknown' as the actor parameter, but this should likely be 'undefined' or the actual user ID when available. The hardcoded 'unknown' string may not match the actual behavior in production.

Suggested change

'unknown',

undefined,

Copilot · 2025-09-28T11:53:10Z

src/utils/validate.ts


+export function sanitizeString(input: string): string {
+  return input.trim().replace(/[<>]/g, '');


The sanitization function only removes angle brackets but may not be sufficient for comprehensive XSS prevention. Consider using a more robust sanitization library or expanding the character filtering to include other potentially dangerous characters like quotes, scripts, and HTML entities.

Suggested change

export function sanitizeString(input: string): string {

return input.trim().replace(/[<>]/g, '');

import sanitizeHtml from 'sanitize-html';

export function sanitizeString(input: string): string {

return sanitizeHtml(input.trim(), { allowedTags: [], allowedAttributes: {} });

Copilot · 2025-09-28T11:53:10Z

src/utils/logger.ts

+function getCorrelationId(): string {
+  // Simple correlation ID generator; can be extended to read from headers later
+  return `req_${Date.now().toString(36)}_${Math.random().toString(36).slice(2, 8)}`;
+}


Generating a new correlation ID for every log entry can be inefficient and may not provide proper request tracing. Consider generating the correlation ID once per request (e.g., in middleware) and passing it through the request context instead of creating a new one for each log call.

Copilot · 2025-09-28T11:53:10Z

src/middleware/authorization.ts

+      if (permission.startsWith('tenant:')) {
+        // For tenant-scoped permissions, we reuse existing ownership checks
+        // and superadmin bypass where applicable.
+        const param = req.params.tenantId || req.params.id;


[nitpick] Using fallback parameter selection (tenantId || id) makes the permission logic implicit and harder to debug. Consider making the parameter name explicit or documenting which routes use which parameter pattern.

Copilot · 2025-09-28T11:53:10Z

scripts/create-indexes.ts

+  tasks.push(db.collection('tenants').createIndex({ ownerId: 1 }, { name: 'tenants_ownerId_idx', unique: true }).catch(() => {}));
+
+  // Projects: tenant and members lookups
+  tasks.push(db.collection('projects').createIndex({ tenantId: 1 }, { name: 'projects_tenantId_idx' }).catch(() => {}));
+  tasks.push(db.collection('projects').createIndex({ 'members.userId': 1 }, { name: 'projects_members_userId_idx' }).catch(() => {}));
+
+  // Superadmins: presence check
+  tasks.push(db.collection('superadmins').createIndex({ userId: 1 }, { name: 'superadmins_userId_idx', unique: true }).catch(() => {}));


Silently catching and ignoring all errors with .catch(() => {}) can hide important indexing failures. Consider logging the errors or at least distinguishing between 'already exists' errors and actual failures.

Suggested change

tasks.push(db.collection('tenants').createIndex({ ownerId: 1 }, { name: 'tenants_ownerId_idx', unique: true }).catch(() => {}));

// Projects: tenant and members lookups

tasks.push(db.collection('projects').createIndex({ tenantId: 1 }, { name: 'projects_tenantId_idx' }).catch(() => {}));

tasks.push(db.collection('projects').createIndex({ 'members.userId': 1 }, { name: 'projects_members_userId_idx' }).catch(() => {}));

// Superadmins: presence check

tasks.push(db.collection('superadmins').createIndex({ userId: 1 }, { name: 'superadmins_userId_idx', unique: true }).catch(() => {}));

tasks.push(

db.collection('tenants')

.createIndex({ ownerId: 1 }, { name: 'tenants_ownerId_idx', unique: true })

.catch((err) => {

// Ignore "already exists" errors, log others

if (!(err && (err.code === 85 || (typeof err.message === 'string' && err.message.includes('already exists'))))) {

// eslint-disable-next-line no-console

console.error('Error creating tenants_ownerId_idx:', err);

}

})

);

// Projects: tenant and members lookups

tasks.push(

db.collection('projects')

.createIndex({ tenantId: 1 }, { name: 'projects_tenantId_idx' })

.catch((err) => {

if (!(err && (err.code === 85 || (typeof err.message === 'string' && err.message.includes('already exists'))))) {

// eslint-disable-next-line no-console

console.error('Error creating projects_tenantId_idx:', err);

}

})

);

tasks.push(

db.collection('projects')

.createIndex({ 'members.userId': 1 }, { name: 'projects_members_userId_idx' })

.catch((err) => {

if (!(err && (err.code === 85 || (typeof err.message === 'string' && err.message.includes('already exists'))))) {

// eslint-disable-next-line no-console

console.error('Error creating projects_members_userId_idx:', err);

}

})

);

// Superadmins: presence check

tasks.push(

db.collection('superadmins')

.createIndex({ userId: 1 }, { name: 'superadmins_userId_idx', unique: true })

.catch((err) => {

if (!(err && (err.code === 85 || (typeof err.message === 'string' && err.message.includes('already exists'))))) {

// eslint-disable-next-line no-console

console.error('Error creating superadmins_userId_idx:', err);

}

})

);

openhands-ai · 2025-09-28T11:58:46Z

Looks like there are a few issues preventing this PR from being merged!

GitHub Actions are failing:
- Documentation Validation

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #56 at branch `refactorsept2025`

Feel free to include any additional details that might help me get this PR into a better state.

_{^{You can manage your notification settings}}

dhirmadi added 5 commits September 28, 2025 09:42

chore(gitignore): ignore Cursor config and chat history (.cursorrules…

1f31d54

…, .cursor/, .specstory/)

cursor config

88e263b

review actions

99b1ca1

code review and optimozation

ee8e8be

dhirmadi requested a review from Copilot September 28, 2025 11:50

dhirmadi self-assigned this Sep 28, 2025

Copilot AI reviewed Sep 28, 2025

View reviewed changes

update git review

5b3ea28

dhirmadi closed this Sep 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactorsept2025#56

Refactorsept2025#56
dhirmadi wants to merge 6 commits intomainfrom
refactorsept2025

dhirmadi commented Sep 28, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 28, 2025

Uh oh!

Copilot AI Sep 28, 2025

Uh oh!

Copilot AI Sep 28, 2025

Uh oh!

Copilot AI Sep 28, 2025

Uh oh!

Copilot AI Sep 28, 2025

Uh oh!

Copilot AI Sep 28, 2025

Uh oh!

Copilot AI Sep 28, 2025

Uh oh!

Copilot AI Sep 28, 2025

Uh oh!

Copilot AI Sep 28, 2025

Uh oh!

Copilot AI Sep 28, 2025

Uh oh!

openhands-ai bot commented Sep 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		db.collection.mockReturnValueOnce({ findOne: vi.fn().mockResolvedValue(null) }) // owner check
		.mockReturnValueOnce({ findOne: vi.fn().mockResolvedValue(null), insertOne: vi.fn().mockResolvedValue({ acknowledged: true }) }); // name check + insert


		export function sanitizeString(input: string): string {
		return input.trim().replace(/[<>]/g, '');

Conversation

dhirmadi commented Sep 28, 2025

Pull Request

Description

Type of Change

Related Issues

Changes Made

Documentation Impact

Testing

Security Considerations

Architecture Compliance

Checklist

Screenshots (if applicable)

Additional Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

openhands-ai bot commented Sep 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant