From 597adef6ae4b87cf59da0ba0a470edc8966b3e69 Mon Sep 17 00:00:00 2001 From: Timo van Veenendaal Date: Thu, 14 Apr 2022 11:02:01 -0700 Subject: [PATCH] [Recorder] Ensure redirected requests are passed to the recorder in Node (#21355) --- sdk/test-utils/recorder/src/recorder.ts | 26 ++++++++---- .../recorder/test/testProxyClient.spec.ts | 28 ++++++++----- .../recorder/test/testProxyTests.spec.ts | 41 +++++++++++++++++++ sdk/test-utils/recorder/test/utils/server.ts | 34 ++++++++++++++- 4 files changed, 109 insertions(+), 20 deletions(-) diff --git a/sdk/test-utils/recorder/src/recorder.ts b/sdk/test-utils/recorder/src/recorder.ts index 396693fbcc2c..bb521313b45c 100644 --- a/sdk/test-utils/recorder/src/recorder.ts +++ b/sdk/test-utils/recorder/src/recorder.ts @@ -79,7 +79,21 @@ export class Recorder { * - PipelineRequest -> core-v2 (recorderHttpPolicy calls this method on the request to modify and hit the proxy-tool with appropriate headers.) */ private redirectRequest(request: WebResource | PipelineRequest): void { - if (!isLiveMode() && !request.headers.get("x-recording-id")) { + const upstreamUrl = new URL(request.url); + const redirectedUrl = new URL(request.url); + const testProxyUrl = new URL(this.url); + + // Sometimes, due to the service returning a redirect or due to the retry policy, redirectRequest + // may be called multiple times. We only want to update the request the second time if the request's + // URL has been changed between calls (this may happen in the case of a redirect, but generally + // not in the case of a retry). Otherwise, we might accidentally update the X-Recording-Upstream-Base-Uri + // header to point to the test proxy instead of the true upstream. + const requestAlreadyRedirected = + upstreamUrl.host === testProxyUrl.host && + upstreamUrl.port === testProxyUrl.port && + upstreamUrl.protocol === testProxyUrl.protocol; + + if (!isLiveMode() && !requestAlreadyRedirected) { if (this.recordingId === undefined) { throw new RecorderError("Recording ID must be defined to redirect a request"); } @@ -87,13 +101,9 @@ export class Recorder { request.headers.set("x-recording-id", this.recordingId); request.headers.set("x-recording-mode", getTestMode()); - const upstreamUrl = new URL(request.url); - const redirectedUrl = new URL(request.url); - const providedUrl = new URL(this.url); - - redirectedUrl.host = providedUrl.host; - redirectedUrl.port = providedUrl.port; - redirectedUrl.protocol = providedUrl.protocol; + redirectedUrl.host = testProxyUrl.host; + redirectedUrl.port = testProxyUrl.port; + redirectedUrl.protocol = testProxyUrl.protocol; request.headers.set("x-recording-upstream-base-uri", upstreamUrl.toString()); request.url = redirectedUrl.toString(); diff --git a/sdk/test-utils/recorder/test/testProxyClient.spec.ts b/sdk/test-utils/recorder/test/testProxyClient.spec.ts index 7d8294e099fa..395cc7e774c1 100644 --- a/sdk/test-utils/recorder/test/testProxyClient.spec.ts +++ b/sdk/test-utils/recorder/test/testProxyClient.spec.ts @@ -57,17 +57,23 @@ describe("TestProxyClient functions", () => { }); ["record", "playback"].forEach((testMode) => { - it(`${testMode} mode: ` + "request unchanged if `x-recording-id` in headers", function () { - env.TEST_MODE = testMode; - testRedirectedRequest( - client, - () => ({ - ...initialRequest, - headers: createHttpHeaders({ "x-recording-id": "dummy-recording-id" }), - }), - (req) => req - ); - }); + it( + `${testMode} mode: ` + "request unchanged if request URL already points to test proxy", + function () { + env.TEST_MODE = testMode; + testRedirectedRequest( + client, + () => ({ + ...initialRequest, + url: "http://localhost:5000/dummy_path?sas=sas", + headers: createHttpHeaders({ + "x-recording-upstream-uri": "https://dummy_url.windows.net/dummy_path?sas=sas", + }), + }), + (req) => req + ); + } + ); it( `${testMode} mode: ` + "url and headers get updated if no `x-recording-id` in headers", diff --git a/sdk/test-utils/recorder/test/testProxyTests.spec.ts b/sdk/test-utils/recorder/test/testProxyTests.spec.ts index a9c2ab422cb1..775a734d6029 100644 --- a/sdk/test-utils/recorder/test/testProxyTests.spec.ts +++ b/sdk/test-utils/recorder/test/testProxyTests.spec.ts @@ -2,6 +2,7 @@ // Licensed under the MIT license. import { ServiceClient } from "@azure/core-client"; +import { isNode } from "@azure/core-util"; import { CustomMatcherOptions, isPlaybackMode, Recorder } from "../src"; import { isLiveMode, TestMode } from "../src/utils/utils"; import { getTestServerUrl, makeRequestAndVerifyResponse, setTestMode } from "./utils/utils"; @@ -36,6 +37,46 @@ import { getTestServerUrl, makeRequestAndVerifyResponse, setTestMode } from "./u ); }); + it("redirect (redirect location has host)", async function (this: Mocha.Context) { + await recorder.start({ envSetupForPlayback: {} }); + + if (!isNode) { + // In the browser, redirects get handled by fetch/XHR and we can't guarantee redirect behavior. + this.skip(); + } + + await makeRequestAndVerifyResponse( + client, + { path: `/redirectWithHost`, method: "GET" }, + { val: "abc" } + ); + }); + + it("redirect (redirect location is relative)", async function (this: Mocha.Context) { + await recorder.start({ envSetupForPlayback: {} }); + + if (!isNode) { + // In the browser, redirects get handled by fetch/XHR and we can't guarantee redirect behavior. + this.skip(); + } + + await makeRequestAndVerifyResponse( + client, + { path: `/redirectWithoutHost`, method: "GET" }, + { val: "abc" } + ); + }); + + it("retry", async () => { + await recorder.start({ envSetupForPlayback: {} }); + await makeRequestAndVerifyResponse( + client, + { path: "/reset_retry", method: "GET" }, + undefined + ); + await makeRequestAndVerifyResponse(client, { path: "/retry", method: "GET" }, { val: "abc" }); + }); + it("sample_response with random string in path", async () => { await recorder.start({ envSetupForPlayback: {} }); diff --git a/sdk/test-utils/recorder/test/utils/server.ts b/sdk/test-utils/recorder/test/utils/server.ts index ebf7bc433aca..c5a79ff15ce8 100644 --- a/sdk/test-utils/recorder/test/utils/server.ts +++ b/sdk/test-utils/recorder/test/utils/server.ts @@ -15,7 +15,39 @@ app.get("/", (_, res) => { res.send("Hello world!"); }); -app.get("/sample_response", (_, res) => { +app.get("/redirectWithHost", (req, res) => { + res.redirect(307, `http://${req.hostname}:${port}/sample_response`); +}); + +app.get("/redirectWithoutHost", (_, res) => { + res.redirect(307, `/sample_response`); +}); + +let sendRetryResponse = true; + +app.get("/reset_retry", (_, res) => { + sendRetryResponse = true; + res.send("The retry flag was reset. The next call to /retry will return a 429 status."); +}); + +app.get("/retry", (_, res) => { + if (sendRetryResponse) { + res + .status(429) + .header("Retry-After", new Date().toUTCString()) + .send({ error: "429 Too Many Requests" }); + sendRetryResponse = false; + } else { + res.send({ val: "abc" }); + } +}); + +app.get("/sample_response", (req, res) => { + if (req.header("x-recording-id") !== undefined) { + res.status(400).send({ error: "This request bypassed the proxy tool!" }); + return; + } + res.send({ val: "abc" }); });