Skip to content

Conversation

narengogi
Copy link
Collaborator

image

narengogi and others added 25 commits September 30, 2025 19:13
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
@narengogi narengogi marked this pull request as draft October 10, 2025 12:34
Copy link
Contributor

matter-code-review bot commented Oct 10, 2025

Code Quality New Feature Security Vulnerability

Description

Motivation

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)

How Has This Been Tested?

  • Unit Tests
  • Integration Tests
  • Manual Testing

Screenshots (if applicable)

N/A

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
  • I have made corresponding changes to the documentation
  • 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

Related Issues

N/A

Summary By MatterAI MatterAI logo

🔄 What Changed

This pull request introduces a new admin API endpoint and corresponding UI functionality to reset integration rate limits. In src/handlers/adminRoutesHandler.ts, a PUT endpoint /admin/integrations/ratelimit/:integrationId/reset was added, which retrieves rate limit configurations for a given integration and iteratively deletes associated entries from the cache. src/middlewares/portkey/handlers/albus.ts includes a minor change to initialize isLocalConfigEnabled from environment variables. src/public/index.html received significant updates, adding a JSON editor for integration configuration, new action buttons (Save, Cancel, Delete, Reset All Rate Limits), and improved error messages. New integrations now default to a crypto.randomUUID() generated slug and allow_all_models: true.

🔍 Impact of the Change

These changes empower administrators to directly manage and reset rate limits for specific integrations via the local admin UI, enhancing control over resource consumption. The JSON editor streamlines local configuration management for integrations, including budgets and rate limits. The isLocalConfigEnabled modification provides greater flexibility in configuration loading. However, the previously identified critical security vulnerability related to adminApiKey storage and transmission persists and is now utilized by the new rate limit reset functionality, increasing its exposure.

📁 Total Files Changed

  • src/handlers/adminRoutesHandler.ts: Modified to add a new endpoint for resetting integration rate limits and associated cache deletion logic.
  • src/middlewares/portkey/handlers/albus.ts: Modified to initialize isLocalConfigEnabled from environment variables.
  • src/public/index.html: Heavily modified to introduce a JSON editor for integration configuration, new action buttons (save, cancel, delete, reset rate limits), and improved error handling messages.

🧪 Test Added

Manual testing is implied for the new admin UI to verify the functionality of the JSON editor, saving integrations, and resetting rate limits. No explicit unit or integration tests were provided for the new backend endpoint or UI logic.

🔒Security Vulnerabilities

The admin UI in src/public/index.html continues to prompt for an adminApiKey and stores it in localStorage. This is a critical security vulnerability as localStorage is susceptible to Cross-Site Scripting (XSS) attacks, allowing an attacker to steal the API key. The new "Reset All Rate Limits" functionality also uses this insecurely stored key for authentication, further exposing it.

Caution

Package Vulnerabilities

Package Version Severity CVE Fix Version Vulnerability
hono ^4.6.10 MODERATE CVE-2025-59139 4.9.7 Hono has Body
Limit Middleware
Bypass
@hono/node-server ^1.3.3 HIGH CVE-2024-32652 1.10.1 @hono/node-server has Denial
of Service
risk when
receiving Host
header that
cannot be
parsed
@hono/node-server ^1.3.3 MODERATE CVE-2024-23340 1.4.1 @hono/node-server cannot handle
"double dots"
in URL
rollup ^4.9.1 HIGH CVE-2024-47068 4.22.4 DOM Clobbering Gadget
found in
rollup bundled
scripts that
leads to
XSS

Tip

Quality Recommendations

  1. Implement a more secure authentication mechanism for the admin UI, avoiding localStorage for API key storage. Consider session-based authentication or a more robust token management system.

  2. Add input validation for the integrationId parameter in resetIntegrationRateLimitHandler to ensure it's a valid slug and prevent potential injection or unexpected behavior.

  3. Correct the error message in resetAllRateLimits in index.html when response.ok is false. The current message Failed to reset rate limit: ${rateLimit.type} uses an undefined rateLimit variable in that scope.

  4. Add unit and integration tests for the resetIntegrationRateLimitHandler endpoint to ensure correct cache invalidation logic and API behavior.

  5. Consider abstracting the client-side API calls and UI logic in index.html into separate JavaScript files for better organization, maintainability, and testability as the admin UI grows.

Tanka Poem ♫

Limits reset now,
Cache cleared, fresh start for all.
Admin's touch, swift,
Data flows, a new clean slate,
Efficiency's gentle hum. 🚀

Sequence Diagram

sequenceDiagram
    participant AdminUI as Admin UI (index.html)
    participant AdminAPI as Admin API (adminRoutesHandler.ts)
    participant Cache as Cache Service

    AdminUI->>AdminAPI: PUT /admin/integrations/ratelimit/:integrationId/reset (adminApiKey)
    Note over AdminAPI: resetIntegrationRateLimitHandler(c)
    AdminAPI->>AdminAPI: Get integrationId from c.req.param('integrationId')
    AdminAPI->>AdminAPI: Get settings, organisationId, workspaceId
    AdminAPI->>AdminAPI: Find rate_limits for integrationId
    opt If rate_limits exist
        loop For each rateLimit in rate_limits
            AdminAPI->>AdminAPI: Generate rateLimitKey(organisationId, rateLimit.type, RateLimiterKeyTypes.INTEGRATION_WORKSPACE, workspaceKey, rateLimit.unit)
            AdminAPI->>AdminAPI: Construct finalKey (e.g., {rate:key}:type)
            AdminAPI->>Cache: delete(finalKey)
            Cache-->>AdminAPI: Deletion confirmation
        end
    end
    AdminAPI-->>AdminUI: 200 OK / Error Response
    AdminUI-->>AdminUI: Display success/error alert
Loading

Copy link
Contributor

@matter-code-review matter-code-review bot left a comment

Choose a reason for hiding this comment

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

Potential security and performance issues identified in the new code additions.

Skipped files
  • conf.example.json: Skipped file pattern
  • conf_sample.json: File hunk diff too large
  • package-lock.json: Skipped file pattern

Comment on lines +54 to +56
console.warn(
'you need to set the REDIS_CONNECTION_STRING environment variable for rate limits to wrok'
);
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 Security Issue

Issue: Insecure Redis connection warning message
Fix: Improve the warning message to be more descriptive and actionable
Impact: Better developer experience and security awareness

Suggested change
console.warn(
'you need to set the REDIS_CONNECTION_STRING environment variable for rate limits to wrok'
);
console.warn(
'Redis connection string is missing. Rate limits will not work without Redis.'
);

Comment on lines +591 to +596
if ([MODES.PROXY && MODES.PROXY_V2].includes(store.proxyMode)) {
splitPattern = getStreamModeSplitPattern(
provider,
winkyBaseLog.requestURL
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🟡 Performance Issue

Issue: Inefficient conditional check for streaming mode split pattern
Fix: Simplify the condition to avoid unnecessary array creation
Impact: Minor performance improvement

Suggested change
if ([MODES.PROXY && MODES.PROXY_V2].includes(store.proxyMode)) {
splitPattern = getStreamModeSplitPattern(
provider,
winkyBaseLog.requestURL
);
}
let splitPattern = '\
\
';
if (store.proxyMode === MODES.PROXY || store.proxyMode === MODES.PROXY_V2) {
splitPattern = getStreamModeSplitPattern(
provider,
winkyBaseLog.requestURL
);
}

Copy link
Contributor

@matter-code-review matter-code-review bot left a comment

Choose a reason for hiding this comment

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

Reviewing the new budget and rate limit implementation for potential issues.

Comment on lines +86 to +88
const rateLimits = settings.integrations.find(
(integration) => integration.slug === integrationId
)?.integration_details?.rate_limits;
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 Security Issue

Issue: Potential null dereference when accessing rate_limits
Fix: Add null check for integration_details before accessing rate_limits
Impact: Prevent runtime errors when integration details are missing

Suggested change
const rateLimits = settings.integrations.find(
(integration) => integration.slug === integrationId
)?.integration_details?.rate_limits;
const rateLimits = settings.integrations.find(
(integration) => integration.slug === integrationId
)?.integration_details?.rate_limits || [];

);
const finalKey = `{rate:${rateLimitKey}}:${rateLimit.type}`;
const cache = getDefaultCache();
await cache.delete(finalKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 Error Handling Issue

Issue: Missing error handling for cache operations
Fix: Add try-catch around cache operations to handle potential failures
Impact: Prevent unhandled exceptions that could crash the application

Suggested change
await cache.delete(finalKey);
try {
await cache.delete(finalKey);
} catch (cacheError) {
console.error(`Failed to delete cache key ${finalKey}:`, cacheError);
}

Comment on lines +2870 to +2871
alert('Integration must have provider and slug fields');
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

🟡 Input Validation Issue

Issue: Missing validation for integration slug uniqueness
Fix: Add check to ensure slug is unique among existing integrations
Impact: Prevent accidental overwrites of existing integrations

                  // Basic validation
                  if (!integration.provider || !integration.slug) {
                    alert('Integration must have provider and slug fields');
                    return;
                  }
                  
                  // Check for duplicate slug
                  const isDuplicate = integrationsData.some((existing, i) => 
                    existing.slug === integration.slug && i !== index
                  );
                  if (isDuplicate) {
                    alert('Integration slug must be unique');
                    return;
                  }

adminApiKey = '';
throw new Error('Invalid Admin API Key. Please refresh and try again.');
}
throw new Error(`Failed to reset rate limit: ${rateLimit.type}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 Error Handling Issue

Issue: Incorrect error message in rate limit reset function
Fix: Reference the correct rate limit variable in error message
Impact: Provide accurate error information to users

                      throw new Error(`Failed to reset rate limits for integration: ${integration.slug}`);

@narengogi narengogi changed the title [Beta]: Budgets and rate limits [Beta]: Budgets and rate limits alongwith UI and everything Oct 11, 2025
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.

1 participant