Oauthchange by dhirmadi · Pull Request #63 · dhirmadi/mwapserver

dhirmadi · 2025-10-07T09:32:25Z

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.

…nd backend development, including PWA features, security best practices, and OAuth integration. Consolidate legacy integration documentation and remove outdated files to improve maintainability and clarity.

…endpoint for resetting OAuth context and enhancements to state parameter handling. Archive legacy integrations documentation for clarity and maintainability.

…dd flow-lock to prevent double-initiation of OAuth processes, and include code challenge parameters in authorization URL generation. Tighten rate limiting on the callback endpoint for added protection. Update integration tests to validate PKCE implementation.

…ameters and adjusting rate limiting for test environments. Update integration and performance tests to validate new state parameter handling and ensure consistent behavior across various scenarios.

…the addition of HMAC-signed state parameters, a new reset endpoint for OAuth context, and adjustments to endpoint statistics. Enhance clarity on backend-driven OAuth implementation and security features.

openhands-ai · 2025-10-07T09:32:50Z

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 #63 at branch `oauthchange`

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

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

Copilot

Pull Request Overview

This PR implements a comprehensive backend-driven OAuth security enhancement for cloud provider integrations. The changes migrate from client-side OAuth handling to a server-controlled flow with HMAC-signed state parameters, server-side PKCE, encrypted token storage, and enhanced security monitoring.

Backend-driven OAuth flow with server-side PKCE generation and token exchange
HMAC-signed state parameters with cryptographic verification for anti-tampering
Encrypted storage of OAuth tokens and PKCE verifiers with comprehensive test coverage updates

Reviewed Changes

Copilot reviewed 28 out of 38 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/staging-api-test.py	Comprehensive Python-based staging API test suite with OAuth flow validation
tests/requirements.txt	Python dependencies for staging test suite
tests/performance/oauth-performance.test.ts	Updated performance tests with server-side PKCE and HMAC state validation
tests/oauth/pkce.test.ts	Enhanced PKCE tests with mocked OAuth service to prevent unhandled rejections
tests/oauth/oauthCallbackSecurity.test.ts	Updated security tests with HMAC envelope validation and encrypted state handling
tests/integration/oauth-flow-end-to-end.test.ts	Comprehensive end-to-end OAuth integration tests with server-side flow
tests/integration/oauth-callback.integration.test.ts	Updated callback integration tests with HMAC-signed state parameters
tests/STAGING-TEST-README.md	Comprehensive documentation for staging API test suite usage
src/schemas/cloudProviderIntegration.schema.ts	Updated schema with encrypted token storage and ephemeral OAuth context
src/features/oauth/oauthCallbackSecurity.service.ts	Enhanced security service with HMAC state validation and server-side PKCE
src/features/oauth/oauth.service.ts	Updated OAuth service with PKCE parameter support in authorization URLs
src/features/oauth/oauth.routes.ts	Added OAuth flow reset endpoint for clearing ephemeral context
src/features/oauth/oauth.controller.ts	Updated controller with server-side PKCE and encrypted token storage
src/features/cloud-integrations/cloudIntegrations.service.ts	Added OAuth flow context management and encrypted token storage
src/config/env.ts	Added OAuth state secret and encryption key environment variables
src/app.ts	Increased rate limits for test environment
docs/	Updated documentation with backend-driven OAuth architecture and staging test suite

Comments suppressed due to low confidence (1)

src/config/env.ts:1

Critical security environment variables should be required rather than optional to prevent production deployments without proper security configuration.

import { z } from 'zod';

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

Copilot · 2025-10-07T09:34:06Z

tests/staging-api-test.py

+init(autoreset=True)
+
+# Configuration
+API_BASE_URL = "https://mwapss.shibari.photo/api/v1"


The hardcoded staging URL should be configurable via environment variable to support different deployment environments.

Suggested change

API_BASE_URL = "https://mwapss.shibari.photo/api/v1"

API_BASE_URL = os.environ.get("MWAP_API_BASE_URL", "https://mwapss.shibari.photo/api/v1")

Copilot · 2025-10-07T09:34:07Z

tests/performance/oauth-performance.test.ts

+        userPreferences: Array.from({ length: 200 }, (_, i) => ({
          key: `preference_${i}`,
-          value: `value_${i}`.repeat(10)
+          value: `value_${i}`.repeat(5)
        })),
-        sessionData: 'x'.repeat(10000),
-        temporaryData: Array.from({ length: 100 }, (_, i) => `temp_data_${i}`)
+        sessionData: 'x'.repeat(2000),
+        temporaryData: Array.from({ length: 50 }, (_, i) => `temp_data_${i}`)


[nitpick] The reduced test data sizes (from 1000 to 200, 10000 to 2000, 100 to 50) may not adequately test large payload scenarios that could occur in production.

Copilot · 2025-10-07T09:34:07Z

src/features/oauth/oauthCallbackSecurity.service.ts

        return result;
      }
-      stateData = decodedStateObj as StateParameter;
+      const secret = process.env.OAUTH_STATE_SECRET || 'dev-state-secret';


Using a fallback development secret in production could be a security risk. Consider throwing an error if OAUTH_STATE_SECRET is not set in production environments.

Suggested change

const secret = process.env.OAUTH_STATE_SECRET || 'dev-state-secret';

let secret = process.env.OAUTH_STATE_SECRET;

if (!secret) {

if (process.env.NODE_ENV === 'production') {

throw new Error('OAUTH_STATE_SECRET must be set in production environments');

}

secret = 'dev-state-secret';

}

Copilot · 2025-10-07T09:34:07Z

src/features/oauth/oauth.controller.ts

+    const status = (integration as any)?.oauth?.status as string | undefined;
+    if (status && ['initiated','exchanging'].includes(status) && expiresAt && expiresAt > nowMs) {
+      logInfo('OAuth initiate blocked due to active flow lock', { integrationId, tenantId, status, expiresAt });
+      throw new ApiError('OAuth flow already initiated', 409);


[nitpick] The error message 'OAuth flow already initiated' could be more user-friendly. Consider providing guidance on how to resolve the issue (e.g., 'OAuth flow in progress. Please wait or reset the flow.').

Suggested change

throw new ApiError('OAuth flow already initiated', 409);

throw new ApiError('OAuth flow in progress. Please wait for completion or reset the flow before trying again.', 409);

dhirmadi added 5 commits October 7, 2025 09:59

Enhance documentation by creating comprehensive guides for frontend a…

bb6248f

…nd backend development, including PWA features, security best practices, and OAuth integration. Consolidate legacy integration documentation and remove outdated files to improve maintainability and clarity.

Update documentation to reflect changes in OAuth flow, including new …

90eaa28

…endpoint for resetting OAuth context and enhancements to state parameter handling. Archive legacy integrations documentation for clarity and maintainability.

Enhance OAuth callback security by implementing HMAC-signed state par…

ae609fa

…ameters and adjusting rate limiting for test environments. Update integration and performance tests to validate new state parameter handling and ensure consistent behavior across various scenarios.

Update API documentation to reflect changes in OAuth flow, including …

445b7d6

…the addition of HMAC-signed state parameters, a new reset endpoint for OAuth context, and adjustments to endpoint statistics. Enhance clarity on backend-driven OAuth implementation and security features.

dhirmadi requested a review from Copilot October 7, 2025 09:32

dhirmadi self-assigned this Oct 7, 2025

dhirmadi merged commit 7ad97d3 into main Oct 7, 2025
2 of 3 checks passed

Copilot AI reviewed Oct 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oauthchange#63

Oauthchange#63
dhirmadi merged 5 commits intomainfrom
oauthchange

dhirmadi commented Oct 7, 2025

Uh oh!

openhands-ai bot commented Oct 7, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Oct 7, 2025

Uh oh!

Copilot AI Oct 7, 2025

Uh oh!

Copilot AI Oct 7, 2025

Uh oh!

Copilot AI Oct 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	API_BASE_URL = "https://mwapss.shibari.photo/api/v1"
	API_BASE_URL = os.environ.get("MWAP_API_BASE_URL", "https://mwapss.shibari.photo/api/v1")

-      const secret = process.env.OAUTH_STATE_SECRET || 'dev-state-secret';
+      let secret = process.env.OAUTH_STATE_SECRET;
+      if (!secret) {
+        if (process.env.NODE_ENV === 'production') {
+          throw new Error('OAUTH_STATE_SECRET must be set in production environments');
+        }
+        secret = 'dev-state-secret';
+      }

	throw new ApiError('OAuth flow already initiated', 409);
	throw new ApiError('OAuth flow in progress. Please wait for completion or reset the flow before trying again.', 409);

Conversation

dhirmadi commented Oct 7, 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!

openhands-ai bot commented Oct 7, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant