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
4 changes: 2 additions & 2 deletions client/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,12 @@ const App = () => {
return migrateFromLegacyAuth(legacyToken, legacyHeaderName);
}

// Default to Authorization: Bearer as the most common case
// Default to empty array
return [
{
name: "Authorization",
value: "Bearer ",
enabled: true,
Copy link
Member

@cliffhall cliffhall Sep 26, 2025

Choose a reason for hiding this comment

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

Would an better way to do this be to leave it, but disable it? So for the common use case of adding an authorization header they just enable it and add their token, but otherwise it's just ignored when headers are sent? Makes it handy, but removes the footgun. With the robust checking you've added above in useConnection, we can be confident in the proper behavior should they switch it on but fail to add a token.

Copy link
Contributor Author

@kentcdodds kentcdodds Sep 26, 2025

Choose a reason for hiding this comment

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

I wonder if there's a situation where someone would want to test the lack of an auth header without actually removing the auth token from their client 🤔

Disabling the Auth header would be how I would want to do that.

Copy link
Member

Choose a reason for hiding this comment

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

So instead of removing this chunk, should we just set the default to enabled: false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little confused why this chunk was added in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess for myself, I always just use the auth flow that's built into the inspector. But I suppose some people have something more complicated and they have to get an auth token through some other means. But to me, that seems like more of an edge case. And a person in that situation would know how to add a new header called authorization with a bearer prefix. I don't think that they need our help to do that.

Copy link
Member

Choose a reason for hiding this comment

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

Just speaking for myself and a few others I've seen chime in on the original feature request - I've worked with quite a few servers that don't support OAuth and where I need to pass something like a Personal Access Token through the authorization header, so this UI option seems more intuitive for that scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to go whatever direction you all want with this. Just seems odd to me to have the headers include an invalid auth header (even if it's disabled) when it's quite easy to add and then it even saves to localstorage once you have 🤷‍♂️

Additionally, I do think this is a much less common case so we are increasing confusion for more folks to make things a tiny bit easier for fewer folks 🤔

Copy link
Member

@cliffhall cliffhall Sep 30, 2025

Choose a reason for hiding this comment

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

@kentcdodds can we just leave it and turn it off by default? The point is that it saves the person who needs to add a bearer token some typing and possible fat-finger errors. I don't see how it's really going to sow confusion. Anyone monkeying with headers will know the bearer token header pattern, and if they don't, they'll probably just leave it turned off. If they do just turn it on and mash connect, they'll get a toast about it being incomplete and see the error of their ways. It's not a footgun, it's just a convenience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I changed it though so it does not automatically replace an empty auth header to reduce "magic." People will see the toast and go disable the header. I'm happy with that :)

enabled: false,
},
];
});
Expand Down
39 changes: 38 additions & 1 deletion client/src/lib/hooks/__tests__/useConnection.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,10 @@ jest.mock("@modelcontextprotocol/sdk/client/auth.js", () => ({
}));

// Mock the toast hook
const mockToast = jest.fn();
jest.mock("@/lib/hooks/useToast", () => ({
useToast: () => ({
toast: jest.fn(),
toast: mockToast,
}),
}));

Expand Down Expand Up @@ -913,6 +914,42 @@ describe("useConnection", () => {
expect(headers).toHaveProperty("Authorization", "Bearer mock-token");
});

test("warns of enabled empty Bearer token", async () => {
// This test prevents regression of the bug where default "Bearer " header
// prevented OAuth token injection, causing infinite auth loops
const customHeaders: CustomHeaders = [
{
name: "Authorization",
value: "Bearer ", // Empty Bearer token placeholder
enabled: true, // enabled
},
];

const propsWithEmptyBearer = {
...defaultProps,
customHeaders,
};

const { result } = renderHook(() => useConnection(propsWithEmptyBearer));

await act(async () => {
await result.current.connect();
});

const headers = mockSSETransport.options?.requestInit?.headers;

expect(headers).toHaveProperty("Authorization", "Bearer");
// Should not have the x-custom-auth-headers since Authorization is standard
expect(headers).not.toHaveProperty("x-custom-auth-headers");

// Should show toast notification for empty Authorization header
expect(mockToast).toHaveBeenCalledWith({
title: "Invalid Authorization Header",
description: expect.any(String),
variant: "destructive",
});
});

test("prioritizes custom headers over legacy auth", async () => {
const customHeaders: CustomHeaders = [
{ name: "Authorization", value: "Bearer custom-token", enabled: true },
Expand Down
30 changes: 28 additions & 2 deletions client/src/lib/hooks/useConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -400,11 +400,37 @@ export function useConnection({
// Use custom headers (migration is handled in App.tsx)
let finalHeaders: CustomHeaders = customHeaders || [];

// Add OAuth token if available and no custom headers are set
if (finalHeaders.length === 0) {
const isEmptyAuthHeader = (header: CustomHeaders[number]) =>
header.name.trim().toLowerCase() === "authorization" &&
header.value.trim().toLowerCase() === "bearer";

// Check for empty Authorization headers and show validation error
const hasEmptyAuthHeader = finalHeaders.some(
(header) => header.enabled && isEmptyAuthHeader(header),
);

if (hasEmptyAuthHeader) {
toast({
title: "Invalid Authorization Header",
description:
"Authorization header is enabled but empty. Please add a token or disable the header.",
variant: "destructive",
});
}

const needsOAuthToken = !finalHeaders.some(
(header) =>
header.enabled &&
header.name.trim().toLowerCase() === "authorization",
);

if (needsOAuthToken) {
const oauthToken = (await serverAuthProvider.tokens())?.access_token;
if (oauthToken) {
// Add the OAuth token
finalHeaders = [
// Remove any existing Authorization headers with empty tokens
...finalHeaders.filter((header) => !isEmptyAuthHeader(header)),
{
name: "Authorization",
value: `Bearer ${oauthToken}`,
Expand Down
Loading