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

fix: now WAF bypass token header is forwarded #178

Merged
merged 10 commits into from
Aug 3, 2023

Conversation

nickytonline
Copy link

@nickytonline nickytonline commented Jul 31, 2023

Fixes an issue when the web application firewall (WAF) is active.

Now if the source image being loaded is a local image, the X-Nf-Waf-Bypass-Token header is passed so that the image on the origin server can be loaded for transformation.

There is nothing to test in this PR aside from ensuring the tests are all green.

Closes https://github.com/netlify/pod-ecosystem-frameworks/issues/592

@github-actions github-actions bot added the type: bug code to address defects in shipped code label Jul 31, 2023
@nickytonline
Copy link
Author

nickytonline commented Jul 31, 2023

It looks like the ci step times out for tests. Ran it locally as well and it times out there too. Looks like my change is causing the timeout. 🤔

Merging #179 seems to have fixed it.

@nickytonline
Copy link
Author

I'm going to take it for a test drive by linking it to the next-runtime and then link the next-runtime in a test project. 😅

@nickytonline nickytonline marked this pull request as ready for review August 2, 2023 20:27
@nickytonline nickytonline requested a review from a team as a code owner August 2, 2023 20:27
@nickytonline nickytonline self-assigned this Aug 2, 2023
@@ -51,7 +55,7 @@ export function createIPXHandler ({
responseHeaders,
localPrefix,
...opts
}: IPXHandlerOptions = {}) {
}: IPXHandlerOptions = {}, loadSourceImage = defaultLoadSourceImage) {
Copy link

Choose a reason for hiding this comment

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

Is this just to allow testing?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that was the main reason, but we were already importing loadSourceImage, so why not compose instead.

ascorbic
ascorbic previously approved these changes Aug 3, 2023
Copy link

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Looks good! Presumably it's not a risk to leak the existence of the token

@nickytonline
Copy link
Author

Looks good! Presumably it's not a risk to leak the existence of the token

I don't think so. Thoughts on this @merlyn-at-netlify?

test/index.test.ts Outdated Show resolved Hide resolved
Co-authored-by: Michal Piechowiak <[email protected]>
Copy link

@merlyn-at-netlify merlyn-at-netlify left a comment

Choose a reason for hiding this comment

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

Looks harmless to me. No added security concern from forwarding this header.

@nickytonline nickytonline merged commit 95645b8 into main Aug 3, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants