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 a bug that CORS headers are missing in some HTTP responses #41792

Merged
merged 2 commits into from
Oct 3, 2022

Conversation

FrankChen021
Copy link
Contributor

@FrankChen021 FrankChen021 commented Sep 26, 2022

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Fix a bug that CORS headers are missing in some HTTP responses

Description

In current implementation, for HTTP POST requests, CORS headers are added after user authentication. If the authentication fails, the returned HTTP response has no CORS headers. This leads some front-end libs, such as axios, fail to process the response - they can't even get the http status nor the error message because the missed headers block them to do so.

To make the front-end projects correctly handle such 403 exceptions or in case of any exception raised before CORS headers are added, we need to make some adjustment to current CORS handling, that is adding these headers before a query is processed.

BTW, there is a very old setting add_http_cors_header, this setting does not work if the authentication fails. The contradiction is we can only get the user setting after the authentication but the CORS processing requires us to add headers before any processing.

Link to #29155 for better understanding.

Signed-off-by: Frank Chen <frank.chen021@outlook.com>
Signed-off-by: Frank Chen <frank.chen021@outlook.com>
@robot-ch-test-poll robot-ch-test-poll added the pr-bugfix Pull request with bugfix, not backported by default label Sep 26, 2022
@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Sep 26, 2022
@FrankChen021
Copy link
Contributor Author

Looks like the failures in CI are not relevant to these changes.

@yakov-olkhovskiy yakov-olkhovskiy self-assigned this Sep 27, 2022
@FrankChen021
Copy link
Contributor Author

Hello @yakov-olkhovskiy Can this be merged?

@yakov-olkhovskiy
Copy link
Member

@FrankChen021 sure, but let me rerun integration test

@yakov-olkhovskiy yakov-olkhovskiy merged commit 0edae06 into ClickHouse:master Oct 3, 2022
@FrankChen021
Copy link
Contributor Author

Thanks you @yakov-olkhovskiy for the review.

@isublimity
Copy link
Contributor

After this fix, Tabix can`t send GET query to CH server, because it cannot pass the ORIGIN header.

If code like this, i think tabix work fine

if (settings.add_http_cors_header ||  ( !request.get("Origin", "").empty() && !config.has("http_options_response")))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can be tested Allows running workflows for external contributors pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants