Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,10 @@ interface EnvVarsSectionProps {
onRemove: (index: number) => void;
onChange: (index: number, field: 'key' | 'value', value: string) => void;
submitAttempted: boolean;
onPendingInputChange?: (hasPendingInput: boolean) => void;
onPendingInputChange: (
hasPendingInput: boolean,
pendingEnvVar: { key: string; value: string } | null
) => void;
}

export default function EnvVarsSection({
Expand All @@ -65,7 +68,9 @@ export default function EnvVarsSection({
// Notify parent when pending input changes
React.useEffect(() => {
const hasPendingInput = newKey.trim() !== '' || newValue.trim() !== '';
onPendingInputChange?.(hasPendingInput);
const pendingEnvVar =
newKey.trim() && newValue.trim() ? { key: newKey, value: newValue } : null;
onPendingInputChange(hasPendingInput, pendingEnvVar);
}, [newKey, newValue, onPendingInputChange]);

const handleAdd = () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,20 @@
import { describe, it, expect, vi } from 'vitest';
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { render, type RenderOptions, screen, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import ExtensionModal from './ExtensionModal';
import { ExtensionFormData } from '../utils';
import { IntlTestWrapper } from '../../../../i18n/test-utils';
import { upsertConfig } from '../../../../api';

vi.mock('../../../../api', async () => {
const actual = await vi.importActual<typeof import('../../../../api')>('../../../../api');
return {
...actual,
upsertConfig: vi.fn().mockResolvedValue({ data: 'ok' }),
};
});

const mockedUpsertConfig = vi.mocked(upsertConfig);

const renderWithIntl = (ui: React.ReactElement, options?: RenderOptions) =>
render(ui, { wrapper: IntlTestWrapper, ...options });
Expand Down Expand Up @@ -246,4 +257,140 @@ describe('ExtensionModal', () => {
{ key: 'Authorization', value: 'Bearer abc123', isEdited: true },
]);
});

describe('pending env var capture (fix for #8969)', () => {
beforeEach(() => {
mockedUpsertConfig.mockClear();
mockedUpsertConfig.mockResolvedValue({
data: 'ok',
error: undefined,
request: new globalThis.Request('http://localhost/test'),
response: new globalThis.Response(),
});
});

const emptyInitialData: ExtensionFormData = {
name: '',
description: '',
type: 'stdio',
cmd: '',
endpoint: '',
enabled: true,
timeout: 300,
envVars: [],
headers: [],
};

// Returns the env-var key+value inputs (scoped to the "Environment Variables" section,
// disambiguated from the header inputs which share the "Value" placeholder).
function getEnvVarInputs() {
const envVarKeyInput = screen.getByPlaceholderText('Variable name');
const envVarValueInput = screen
.getAllByPlaceholderText('Value')
.find((input) =>
input.parentElement?.parentElement?.parentElement?.textContent?.includes(
'Environment Variables'
)
);
return { envVarKeyInput, envVarValueInput };
}

it('captures a pending env var typed but not "+ Added" when Submit is clicked', async () => {
const user = userEvent.setup();
const mockOnSubmit = vi.fn();
const mockOnClose = vi.fn();

renderWithIntl(
<ExtensionModal
title="Add custom extension"
initialData={emptyInitialData}
onClose={mockOnClose}
onSubmit={mockOnSubmit}
submitLabel="Add Extension"
modalType="add"
/>
);

await user.type(screen.getByPlaceholderText('Enter extension name...'), 'WooMCP');
await user.type(
screen.getByPlaceholderText(/^e\.g\. npx/),
'npx -y @automattic/mcp-wordpress-remote@latest'
);

const { envVarKeyInput, envVarValueInput } = getEnvVarInputs();
await user.type(envVarKeyInput, 'JWT_TOKEN');
if (envVarValueInput) {
await user.type(envVarValueInput, 'my_very_long_token');
}

// Note: intentionally NOT clicking the "+ Add" button — this is the #8969 repro.
await user.click(screen.getByTestId('extension-submit-btn'));

await waitFor(() => {
expect(mockOnSubmit).toHaveBeenCalled();
});

expect(mockedUpsertConfig).toHaveBeenCalledWith(
expect.objectContaining({
body: expect.objectContaining({
is_secret: true,
key: 'JWT_TOKEN',
value: 'my_very_long_token',
}),
})
);

const submittedData = mockOnSubmit.mock.calls[0][0];
expect(submittedData.envVars).toEqual(
expect.arrayContaining([
expect.objectContaining({
key: 'JWT_TOKEN',
value: 'my_very_long_token',
isEdited: true,
}),
])
);
});

it('does not capture a pending env var when only the key is filled', async () => {
const user = userEvent.setup();
const mockOnSubmit = vi.fn();
const mockOnClose = vi.fn();

renderWithIntl(
<ExtensionModal
title="Add custom extension"
initialData={emptyInitialData}
onClose={mockOnClose}
onSubmit={mockOnSubmit}
submitLabel="Add Extension"
modalType="add"
/>
);

await user.type(screen.getByPlaceholderText('Enter extension name...'), 'WooMCP');
await user.type(screen.getByPlaceholderText(/^e\.g\. npx/), 'npx -y something');

const { envVarKeyInput } = getEnvVarInputs();
await user.type(envVarKeyInput, 'LONELY_KEY');
// Intentionally leaving the value field empty.

await user.click(screen.getByTestId('extension-submit-btn'));

await waitFor(() => {
expect(mockOnSubmit).toHaveBeenCalled();
});

expect(mockedUpsertConfig).not.toHaveBeenCalledWith(
expect.objectContaining({
body: expect.objectContaining({ key: 'LONELY_KEY' }),
})
);

const submittedData = mockOnSubmit.mock.calls[0][0];
expect(submittedData.envVars).not.toEqual(
expect.arrayContaining([expect.objectContaining({ key: 'LONELY_KEY' })])
);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ export default function ExtensionModal({
const [submitAttempted, setSubmitAttempted] = useState(false);
const [showCloseConfirmation, setShowCloseConfirmation] = useState(false);
const [hasPendingEnvVars, setHasPendingEnvVars] = useState(false);
const [pendingEnvVar, setPendingEnvVar] = useState<{ key: string; value: string } | null>(null);
const [hasPendingHeaders, setHasPendingHeaders] = useState(false);
const [pendingHeader, setPendingHeader] = useState<{ key: string; value: string } | null>(null);

Expand Down Expand Up @@ -232,6 +233,14 @@ export default function ExtensionModal({
[]
);

const handlePendingEnvVarChange = useCallback(
(hasPending: boolean, envVar: { key: string; value: string } | null) => {
setHasPendingEnvVars(hasPending);
setPendingEnvVar(envVar);
},
[]
);

// Function to store a secret value
const storeSecret = async (key: string, value: string) => {
try {
Expand Down Expand Up @@ -289,6 +298,19 @@ export default function ExtensionModal({
return finalHeaders;
};

const getFinalEnvVars = () => {
const finalEnvVars = [...formData.envVars];
if (
pendingEnvVar &&
pendingEnvVar.key.trim() !== '' &&
pendingEnvVar.value.trim() !== '' &&
!pendingEnvVar.key.includes(' ')
) {
finalEnvVars.push({ ...pendingEnvVar, isEdited: true });
Comment on lines +304 to +309

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Skip pending env var merge for non-env extension types

getFinalEnvVars() always appends pendingEnvVar when it is non-empty, even if the env var section has been unmounted after switching formData.type away from stdio/streamable_http. In that flow, a user can type a pending env var, change type to sse, and submit; the hidden pending value is still sent to storeSecret even though createExtensionConfig for sse drops env keys, so the UI silently persists an orphaned secret that is not part of the saved extension config.

Useful? React with 👍 / 👎.

}
return finalEnvVars;
};

const isHeadersValid = () => {
return getFinalHeaders().every(
({ key, value }) => (key === '' && value === '') || (key !== '' && value !== '')
Expand Down Expand Up @@ -323,6 +345,7 @@ export default function ExtensionModal({
if (isFormValid()) {
const finalFormData = {
...formData,
envVars: getFinalEnvVars(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Skip pending env var merge for non-env extension types

Guard the pending-env merge by formData.type before submit. As written, handleSubmit always calls getFinalEnvVars(), so a pending row typed under stdio/streamable_http can still be submitted after switching to sse (the env var UI unmounts but pendingEnvVar state is retained). This causes storeSecret to persist a secret that is not represented in the final extension config path for non-env types, creating orphaned config entries.

Useful? React with 👍 / 👎.

headers: getFinalHeaders(),
};

Expand Down Expand Up @@ -437,7 +460,7 @@ export default function ExtensionModal({
onRemove={handleRemoveEnvVar}
onChange={handleEnvVarChange}
submitAttempted={submitAttempted}
onPendingInputChange={setHasPendingEnvVars}
onPendingInputChange={handlePendingEnvVarChange}
/>
</div>
</>
Expand Down