Skip to content

Commit a67b728

Browse files
eleqtriziteleqtrizit
committed
fix(msteams): harden service url validation
1 parent 47baffc commit a67b728

4 files changed

Lines changed: 152 additions & 13 deletions

File tree

‎extensions/msteams/src/attachments/bot-framework.test.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,45 @@ describe("downloadMSTeamsBotFrameworkAttachment", () => {
226226
expect(runtime.saveCalls).toHaveLength(0);
227227
});
228228

229+
it("sends Bot Framework service tokens to auth-allowlisted service hosts", async () => {
230+
const seenAuth: Array<string | null> = [];
231+
const fileBytes = Buffer.from("BFBYTES", "utf-8");
232+
const fetchFn: typeof fetch = (async (input: RequestInfo | URL, init?: RequestInit) => {
233+
const url =
234+
typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url;
235+
seenAuth.push(new Headers(init?.headers).get("authorization"));
236+
if (url.endsWith("/v3/attachments/att-1")) {
237+
return new Response(
238+
JSON.stringify({
239+
name: "doc.pdf",
240+
type: "application/pdf",
241+
views: [{ viewId: "original", size: fileBytes.byteLength }],
242+
}),
243+
{ status: 200, headers: { "content-type": "application/json" } },
244+
);
245+
}
246+
if (url.endsWith("/v3/attachments/att-1/views/original")) {
247+
return new Response(fileBytes, {
248+
status: 200,
249+
headers: { "content-length": String(fileBytes.byteLength) },
250+
});
251+
}
252+
return new Response("not found", { status: 404 });
253+
}) as typeof fetch;
254+
255+
const media = await downloadMSTeamsBotFrameworkAttachment({
256+
serviceUrl: "https://smba.trafficmanager.net/amer",
257+
attachmentId: "att-1",
258+
tokenProvider: buildTokenProvider(),
259+
maxBytes: 10_000_000,
260+
fetchFn,
261+
resolveFn: resolvePublicHost,
262+
});
263+
264+
expect(media?.path).toBe(runtime.savePath);
265+
expect(seenAuth).toEqual(["Bearer bf-token", "Bearer bf-token"]);
266+
});
267+
229268
it("skips when attachment view size exceeds maxBytes", async () => {
230269
const info = {
231270
name: "huge.bin",

‎extensions/msteams/src/monitor.lifecycle.test.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,6 +322,9 @@ describe("monitorMSTeamsProvider lifecycle", () => {
322322
if (!app) {
323323
throw new Error("expected Express app to be created");
324324
}
325+
// This test intentionally locks auth middleware ordering: the cheap Bearer
326+
// gate must run before bounded JSON parsing, and JWT validation must run
327+
// after parsing so it can bind the token to Activity.serviceUrl.
325328
expect(app.use).toHaveBeenCalledTimes(4);
326329

327330
const jsonMiddleware = vi.mocked((await import("express")).json).mock.results[0]?.value;
@@ -369,6 +372,27 @@ describe("monitorMSTeamsProvider lifecycle", () => {
369372
expect(next).toHaveBeenCalledTimes(1);
370373
});
371374

375+
jwtValidate.mockReset().mockResolvedValueOnce(false);
376+
const missingServiceUrlNext = vi.fn();
377+
const missingServiceUrlResponse = {
378+
status: vi.fn().mockReturnThis(),
379+
json: vi.fn(),
380+
} as unknown as Response;
381+
jwtMiddleware(
382+
{
383+
headers: { authorization: "Bearer token-no-service-url" },
384+
body: { type: "message" },
385+
} as Request,
386+
missingServiceUrlResponse,
387+
missingServiceUrlNext,
388+
);
389+
390+
await vi.waitFor(() => {
391+
expect(jwtValidate).toHaveBeenCalledWith("Bearer token-no-service-url", undefined);
392+
expect(missingServiceUrlResponse.status).toHaveBeenCalledWith(401);
393+
expect(missingServiceUrlNext).not.toHaveBeenCalled();
394+
});
395+
372396
abort.abort();
373397
await task;
374398
});

‎extensions/msteams/src/sdk.test.ts‎

Lines changed: 77 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,16 @@ describe("createBotFrameworkJwtValidator", () => {
365365
expect(opts.audience).toContain("https://api.botframework.com");
366366
});
367367

368+
it("accepts tokens with documented serviceUrl claim casing", async () => {
369+
jwtState.decodedPayload = { iss: "https://api.botframework.com" };
370+
jwtState.verifyResult = {
371+
serviceUrl: activityServiceUrl,
372+
};
373+
374+
const validator = await createBotFrameworkJwtValidator(creds);
375+
await expect(validator.validate("Bearer botfw-token", activityServiceUrl)).resolves.toBe(true);
376+
});
377+
368378
it("accepts global audience tokens when azp matches the configured app id", async () => {
369379
jwtState.decodedPayload = { iss: "https://api.botframework.com" };
370380
jwtState.verifyResult = {
@@ -403,6 +413,18 @@ describe("createBotFrameworkJwtValidator", () => {
403413
await expect(validator.validate("Bearer botfw-token", activityServiceUrl)).resolves.toBe(false);
404414
});
405415

416+
it("rejects schemeless activity serviceUrls even when the host matches the token claim", async () => {
417+
jwtState.decodedPayload = { iss: "https://api.botframework.com" };
418+
jwtState.verifyResult = {
419+
serviceurl: activityServiceUrl,
420+
};
421+
422+
const validator = await createBotFrameworkJwtValidator(creds);
423+
await expect(
424+
validator.validate("Bearer botfw-token", "smba.trafficmanager.net/amer/"),
425+
).resolves.toBe(false);
426+
});
427+
406428
it("rejects tokens when the serviceurl claim is missing", async () => {
407429
jwtState.decodedPayload = { iss: "https://api.botframework.com" };
408430
jwtState.verifyResult = {
@@ -415,18 +437,69 @@ describe("createBotFrameworkJwtValidator", () => {
415437

416438
it("rejects tokens when the activity serviceUrl is missing", async () => {
417439
jwtState.decodedPayload = { iss: "https://api.botframework.com" };
440+
jwtState.verifyResult = {
441+
serviceurl: activityServiceUrl,
442+
};
418443

419444
const validator = await createBotFrameworkJwtValidator(creds);
420445
await expect(validator.validate("Bearer botfw-token", undefined)).resolves.toBe(false);
421446
});
422447

423-
it("rejects serviceUrl values that are not Bot Framework base URLs", async () => {
448+
it("rejects tokens when the activity serviceUrl is malformed", async () => {
424449
jwtState.decodedPayload = { iss: "https://api.botframework.com" };
450+
jwtState.verifyResult = {
451+
serviceurl: activityServiceUrl,
452+
};
425453

426454
const validator = await createBotFrameworkJwtValidator(creds);
427-
await expect(
428-
validator.validate("Bearer botfw-token", `${activityServiceUrl}?target=attacker`),
429-
).resolves.toBe(false);
455+
await expect(validator.validate("Bearer botfw-token", "not a url")).resolves.toBe(false);
456+
});
457+
458+
it.each([
459+
"http://smba.trafficmanager.net/amer",
460+
"HTTP://smba.trafficmanager.net/amer",
461+
"wss://smba.trafficmanager.net/amer",
462+
"ftp://smba.trafficmanager.net/amer",
463+
])("rejects non-HTTPS activity serviceUrl %s", async (serviceUrl) => {
464+
jwtState.decodedPayload = { iss: "https://api.botframework.com" };
465+
jwtState.verifyResult = {
466+
serviceurl: serviceUrl,
467+
};
468+
469+
const validator = await createBotFrameworkJwtValidator(creds);
470+
await expect(validator.validate("Bearer botfw-token", serviceUrl)).resolves.toBe(false);
471+
});
472+
473+
it("rejects serviceUrl values with query strings", async () => {
474+
const queriedServiceUrl = `${activityServiceUrl}?target=attacker`;
475+
jwtState.decodedPayload = { iss: "https://api.botframework.com" };
476+
jwtState.verifyResult = {
477+
serviceurl: queriedServiceUrl,
478+
};
479+
480+
const validator = await createBotFrameworkJwtValidator(creds);
481+
await expect(validator.validate("Bearer botfw-token", queriedServiceUrl)).resolves.toBe(false);
482+
});
483+
484+
it("rejects serviceUrl values with fragments", async () => {
485+
const fragmentServiceUrl = `${activityServiceUrl}#fragment`;
486+
jwtState.decodedPayload = { iss: "https://api.botframework.com" };
487+
jwtState.verifyResult = {
488+
serviceurl: fragmentServiceUrl,
489+
};
490+
491+
const validator = await createBotFrameworkJwtValidator(creds);
492+
await expect(validator.validate("Bearer botfw-token", fragmentServiceUrl)).resolves.toBe(false);
493+
});
494+
495+
it("rejects tokens when the serviceurl claim is not a string", async () => {
496+
jwtState.decodedPayload = { iss: "https://api.botframework.com" };
497+
jwtState.verifyResult = {
498+
serviceurl: 123,
499+
};
500+
501+
const validator = await createBotFrameworkJwtValidator(creds);
502+
await expect(validator.validate("Bearer botfw-token", activityServiceUrl)).resolves.toBe(false);
430503
});
431504

432505
it("rejects non-object verified payloads", async () => {

‎extensions/msteams/src/sdk.ts‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -735,7 +735,7 @@ function hasExpectedBotIdentity(payload: unknown, expectedAppId: string): boolea
735735
);
736736
}
737737

738-
function normalizeBotFrameworkServiceUrl(value: unknown): string | null {
738+
function validateAndNormalizeBotFrameworkServiceUrl(value: unknown): string | null {
739739
if (typeof value !== "string") {
740740
return null;
741741
}
@@ -745,10 +745,10 @@ function normalizeBotFrameworkServiceUrl(value: unknown): string | null {
745745
}
746746
try {
747747
const url = new URL(trimmed);
748-
if (url.protocol !== "https:") {
749-
return null;
750-
}
751-
if (url.search || url.hash) {
748+
// Match the signed endpoint, not a loosely equivalent URL: the URL parser
749+
// normalizes host/default port, while path casing and encoding stay intact.
750+
// Query/fragment values are not valid Bot Framework service endpoints.
751+
if (url.protocol !== "https:" || url.search || url.hash) {
752752
return null;
753753
}
754754
return url.toString().replace(/\/+$/, "");
@@ -761,11 +761,14 @@ function hasMatchingServiceUrlClaim(
761761
payload: BotFrameworkJwtPayload,
762762
activityServiceUrl: string | undefined,
763763
): boolean {
764-
const expectedServiceUrl = normalizeBotFrameworkServiceUrl(activityServiceUrl);
764+
const expectedServiceUrl = validateAndNormalizeBotFrameworkServiceUrl(activityServiceUrl);
765765
if (!expectedServiceUrl) {
766766
return false;
767767
}
768-
const claimServiceUrl = normalizeBotFrameworkServiceUrl(payload.serviceurl ?? payload.serviceUrl);
768+
// Bot Framework tokens commonly use lowercase `serviceurl`; keep the
769+
// documented camelCase spelling as a narrow fallback for SDK/source variants.
770+
const claimValue = payload.serviceurl ?? payload.serviceUrl;
771+
const claimServiceUrl = validateAndNormalizeBotFrameworkServiceUrl(claimValue);
769772
return claimServiceUrl === expectedServiceUrl;
770773
}
771774

@@ -836,11 +839,11 @@ async function loadBotFrameworkJwtDeps(): Promise<BotFrameworkJwtDeps> {
836839
* - signature verification via issuer-specific JWKS endpoints
837840
* - audience validation: appId, api://appId, and https://api.botframework.com
838841
* - issuer validation: strict allowlist (Bot Framework + tenant-scoped Entra)
839-
* - service URL binding: JWT serviceurl claim must match Activity.serviceUrl
842+
* - service URL binding: JWT serviceurl claim must match a usable Activity.serviceUrl
840843
* - expiration validation with 5-minute clock tolerance
841844
*/
842845
export async function createBotFrameworkJwtValidator(creds: MSTeamsCredentials): Promise<{
843-
validate: (authHeader: string, activityServiceUrl: string | undefined) => Promise<boolean>;
846+
validate: (authHeader: string, activityServiceUrl?: string) => Promise<boolean>;
844847
}> {
845848
const { jwt, JwksClient } = await loadBotFrameworkJwtDeps();
846849

0 commit comments

Comments
 (0)