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.

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

[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

Conversation

timdawborn
Copy link
Contributor

Prevent conditional requests from being made during tape recording, so that conditional responses are not returned during replay.

…recording, so that conditional responses are not returned during replay.
@timdawborn timdawborn force-pushed the AUTH-1236-prevent-conditional-requests branch from c3cecd9 to b788613 Compare March 9, 2022 02:39
@timdawborn timdawborn force-pushed the AUTH-1236-prevent-conditional-requests branch from dd2b63f to 3f2ad0d Compare March 9, 2022 03:23
@timdawborn timdawborn marked this pull request as ready for review March 10, 2022 00:49
Comment on lines +68 to +69
delete req.headers["if-modified-since"];
delete req.headers["if-none-match"];

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.

src/cli.ts Outdated
Comment on lines 27 to 31
.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"
)

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.

@@ -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.

👍🏼

Copy link
Contributor

@mcinbell mcinbell left a comment

Choose a reason for hiding this comment

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

I like @rickyrattlesnake's suggestion of boolean flag, but other than that it looks good to me.

@timdawborn timdawborn merged commit 60f46aa into master Mar 11, 2022
@timdawborn timdawborn deleted the AUTH-1236-prevent-conditional-requests branch March 11, 2022 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants