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

Ignore Headers / Send Proxy Port / Dump Matcher Fails #604

Merged
merged 10 commits into from
May 14, 2024

Conversation

tft7000
Copy link
Contributor

@tft7000 tft7000 commented May 13, 2024

I added three flags that are needed in my situation and might be helpful for others:

--ignore-headers: Allows users to specify a list of headers that should be ignored by Proxay. This is especially useful together with the --exact-request-matching flag.

--send-proxy-port: Sends the proxays port to the proxied host, similar to nginx' proxy_set_header Host $host:$server_port;. This helps against redirect issues.

--dump-matcher-fails: Dumps the headers from the current and recorded request, if they are not matching. This helps to find headers that should be ignored in the specific situation.

For all three parts there are simple tests. I also added a paragraph in the Readme that documents those flags.

will ignore additional comma separated headers in replay and mimic mode while comparing.
will dump the two header or query object, if they are different.
will send the proxays port to the proxied host to improve redirects.
@tft7000 tft7000 requested a review from a team as a code owner May 13, 2024 07:59
@tft7000 tft7000 requested a review from njday May 13, 2024 07:59
@tft7000 tft7000 changed the title Ignore Headers / Send Proxy Port / Dump Matcher FailsIgnore headers Ignore Headers / Send Proxy Port / Dump Matcher Fails May 13, 2024
@tft7000
Copy link
Contributor Author

tft7000 commented May 13, 2024

Note: OT: locally (on a mac) the test Persistence › persists binary encoded with gzip still compressed fails with "data": "H4sIAAAAAAAAA2MyUMqYzfiqhwmNBgCNSozuGAAAAA==", being slightly different by one character. However in the CI here the test seems to pass. I don't know the details, but it looks like there is a possibility that there are different (valid?) gzip implementations.

@CLAassistant
Copy link

CLAassistant commented May 13, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@timdawborn timdawborn left a comment

Choose a reason for hiding this comment

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

This all makes sense to me. Thanks for the contribution!

A couple of suggestions and then I think we're good.

src/similarity.ts Show resolved Hide resolved
README.md Outdated
`-r, --redact-headers <headers>`: This option enables the redaction of specific HTTP header values, which are replaced by `XXXX` to maintain privacy or confidentiality during the recording of network interactions. The headers should be provided as a comma-separated list.


`--dump-matcher-fails`: When exact request matching is enabled, this flag provides some debug information on why a request did not match any recorded tape. It is useful for troubleshooting and refining the conditions under which requests are considered equivalent, focusing on differences in headers and query parameters.
Copy link
Contributor

@timdawborn timdawborn May 14, 2024

Choose a reason for hiding this comment

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

Suggestion: Perhaps instead of the word dump, we could use the word debug instead throughout this PR? Its intent is a bit more obvious IMO.

.option(
"-r, --redact-headers <headers>",
"Request headers to redact",
"Request headers to redact (values will be replaced by XXXX)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks 👍

@timdawborn
Copy link
Contributor

Note: OT: locally (on a mac) the test Persistence › persists binary encoded with gzip still compressed fails with "data": "H4sIAAAAAAAAA2MyUMqYzfiqhwmNBgCNSozuGAAAAA==", being slightly different by one character. However in the CI here the test seems to pass. I don't know the details, but it looks like there is a possibility that there are different (valid?) gzip implementations.

Yup, I get the same, and I have the same conclusion that you had — it's another valid but different encoding.

@tft7000
Copy link
Contributor Author

tft7000 commented May 14, 2024

@timdawborn : sorry, I had to revert the suggestion, because otherwise there will be a type problem (I think, because of safeHeaders: HttpHeaders).

Copy link
Contributor

@timdawborn timdawborn left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@timdawborn
Copy link
Contributor

@tft7000 Thanks! Do you have permission to merge this or do we need to merge it for you? (I can't easily check)

@tft7000
Copy link
Contributor Author

tft7000 commented May 14, 2024

@timdawborn : thanks for the review!
no, I do not have permissions. I would be happy, if you could merge / release it.

@timdawborn timdawborn merged commit 1202a41 into airtasker:master May 14, 2024
7 checks passed
@timdawborn
Copy link
Contributor

I would be happy if you could merge / release it.

I'll aim to create a release tomorrow my time.

@timdawborn
Copy link
Contributor

@tft7000 This contribution is now in the 1.9.0 release: https://github.com/airtasker/proxay/releases/tag/v1.9.0

@tft7000
Copy link
Contributor Author

tft7000 commented May 15, 2024

@timdawborn : Thanks for the quick review and merging / releasing. I appreciate your promptness.

@tft7000 tft7000 deleted the ignore-headers branch May 15, 2024 04:42
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