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

Deduplicate failure text in CORS preflight response #1199

Merged
merged 2 commits into from
Jun 13, 2021

Conversation

laggardkernel
Copy link
Contributor

@laggardkernel laggardkernel commented Jun 13, 2021

Break the loop to t avoid appending string headers into failures multiple times. Otherwise it causes duplication in CORS response text (not response header).

        elif requested_headers is not None:
            for header in [h.lower() for h in requested_headers.split(",")]:
                if header.strip() not in self.allow_headers:
                    failures.append("headers")
+                    break
        ...
        if failures:
            failure_text = "Disallowed CORS " + ", ".join(failures)
            return PlainTextResponse(failure_text, status_code=400, headers=headers)

@Kludex Kludex added the cors Cross-Origin Resource Sharing label Jun 13, 2021
@JayH5
Copy link
Member

JayH5 commented Jun 13, 2021

Thanks. Any way we can add a test for this?

@laggardkernel
Copy link
Contributor Author

laggardkernel commented Jun 13, 2021

It's such a trivial change, just break the loop to avoid appending a value into the same list multiple times. Do we really need a test for it? @JayH5

@laggardkernel
Copy link
Contributor Author

Anyway, I added a test and force pushed the commit.

Copy link
Member

@JayH5 JayH5 left a comment

Choose a reason for hiding this comment

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

It's such a trivial change, just break the loop to avoid appending a value into the same list multiple times. Do we really need a test for it?

I didn't completely understand this at first so just thought bug == test please 🙂. Probably wasn't super necessary but can't hurt, thanks.

@JayH5 JayH5 merged commit 15761fb into encode:master Jun 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cors Cross-Origin Resource Sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants