Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[AUTH-1236] Prevent conditional requests from being made during tape recording, so that conditional responses are not returned during replay #473

Merged
merged 6 commits into from
Mar 11, 2022
Merged

[AUTH-1236] Prevent conditional requests from being made during tape recording, so that conditional responses are not returned during replay #473

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b788613
[AUTH-1236] Prevent conditional requests from being made during tape …
timdawborn Mar 9, 2022
3f2ad0d
Debugging.
timdawborn Mar 9, 2022
b36fe1b
Lowercase headers.
timdawborn Mar 9, 2022
60aee12
Remove debugging.
timdawborn Mar 9, 2022
a5fbaf1
Set default to be true.
timdawborn Mar 10, 2022
9243023
PR feedback.
timdawborn Mar 11, 2022
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
14 changes: 14 additions & 0 deletions src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ async function main(argv: string[]) {
"Request headers to redact",
commaSeparatedList
)
.option(
"--prevent-conditional-requests <flag>",
"When running in record mode, remove `If-*` headers from outgoing requests in an attempt to prevent the suite of conditional responses being returned (304).",
"true"
)
Copy link

Choose a reason for hiding this comment

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

nit: perhaps a negated boolean flag is a nicer CLI interface https://github.com/tj/commander.js/#other-option-types-negatable-boolean-and-booleanvalue?

--no-drop-conditional-request-headers ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sure, nice idea. I didn't bother looking up the docs for this command line arg parsing library to see if there was an option for that.

.parse(argv);

const initialMode: string = (program.mode || "").toLowerCase();
Expand All @@ -32,6 +37,8 @@ async function main(argv: string[]) {
const host: string = program.host;
const port = parseInt(program.port, 10);
const redactHeaders: string[] = program.redactHeaders;
const preventConditionalRequests: boolean =
program.preventConditionalRequests === "true";

switch (initialMode) {
case "record":
Expand All @@ -55,6 +62,12 @@ async function main(argv: string[]) {
);
}

if (initialMode === "record" && preventConditionalRequests) {
console.info(
"The prevent conditional requests flag is enabled in record mode. All received `If-*` headers will not be forwarded on upstream and will not be recorded in tapes."
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏼

);
}

// Expect a host unless we're in replay mode.
if (initialMode !== "replay") {
if (!host) {
Expand All @@ -72,6 +85,7 @@ async function main(argv: string[]) {
defaultTapeName,
enableLogging: true,
redactHeaders,
preventConditionalRequests,
});
await server.start(port);
console.log(chalk.green(`Proxying in ${initialMode} mode on port ${port}.`));
Expand Down
12 changes: 12 additions & 0 deletions src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export class RecordReplayServer {
private loggingEnabled: boolean;
private defaultTape: string;
private replayedTapes: Set<TapeRecord> = new Set();
private preventConditionalRequests?: boolean;

constructor(options: {
initialMode: Mode;
Expand All @@ -32,6 +33,7 @@ export class RecordReplayServer {
timeout?: number;
enableLogging?: boolean;
redactHeaders?: string[];
preventConditionalRequests?: boolean;
}) {
this.currentTapeRecords = [];
this.mode = options.initialMode;
Expand All @@ -41,6 +43,7 @@ export class RecordReplayServer {
const redactHeaders = options.redactHeaders || [];
this.persistence = new Persistence(options.tapeDir, redactHeaders);
this.defaultTape = options.defaultTapeName;
this.preventConditionalRequests = options.preventConditionalRequests;
this.loadTape(this.defaultTape);

this.server = http.createServer(async (req, res) => {
Expand All @@ -57,6 +60,15 @@ export class RecordReplayServer {
return;
}

if (
this.preventConditionalRequests &&
(req.method === "GET" || req.method === "HEAD")
) {
// Headers are always coming in as lowercase.
delete req.headers["if-modified-since"];
delete req.headers["if-none-match"];
Comment on lines +68 to +69
Copy link

Choose a reason for hiding this comment

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

Question: do we need to remove the headers from https://nodejs.org/api/http.html#messagerawheaders as well? I assume not if we're only forwarding req.headers elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I don't believe so as we're copying the headers out of req to pass upstream later on.

}

try {
const request: Request = {
method: req.method,
Expand Down