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

Partially corrected CORS checks on the client side #237

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bioluks
Copy link
Contributor

@bioluks bioluks commented Jul 4, 2023

Hi, like explained here since version 3.2 the enable_cors option is under [chttpd] and not [httpd]. I only moved the enable_cors option. Result on my testing rig:

cors-check-fix


The cors.origins check still needs an addition, it works just fine when there are no spaces between the origins, on my setup I have them so that's the reason why I have this error; since both are valid in the local.ini configuration both can be accepted?

@bioluks
Copy link
Contributor Author

bioluks commented Jul 4, 2023

Also a couchdb version check can be added; if it's lower than 3.2 [httpd].enable_cors would be the right path

@vrtmrz
Copy link
Owner

vrtmrz commented Jul 18, 2023

Sorry for my absence! and thank you for making this PR! I did not realise it yet!
This steady fixing would help us! However, as you pointed out, there might be users of v3.2 before.
Would you mind if I ask you to leave the previous check logic as a warning with a comment like (v3.1 or before)?
(i.e., ⚠ chttpd.enable_cors is wrong (v3.1 or before) ).

It would be harmless even if chttpd.enable_cors is configured after v3.2.

@bioluks
Copy link
Contributor Author

bioluks commented Jul 18, 2023

No thing, thanks for this great project!

Would you mind if I ask you to leave the previous check logic as a warning with a comment like (v3.1 or before)?
(i.e., ⚠ chttpd.enable_cors is wrong (v3.1 or before) ).

We can do that, but wouldn't it be better if we do a server version check first? Visiting the server gives a JSON response like the following already:

responsejson

So what I'm saying is if it's lower than 3.2 it should say httpd.enable_cors is ok and shouldn't print information about chttpd.enable_cors; but if it's 3.2 and higher it should say chttpd.enable_cors is ok without the unneccesary httpd.enable_cors part in this case. The https.enable_cors is wrong warning made me lose hope first time setting up couchdb. Short comments next to important errors are not working so well for the majority, my personal thought. What do you think?

@vrtmrz
Copy link
Owner

vrtmrz commented Jul 20, 2023

Yes, checking the remote version should be the correct way. As your mentioned, we should make an easy-usable fix than an easy fix.
Thank you for bringing me back to the attitude we should be.

Would you mind if I ask you to add that logic as well, please?

@bioluks
Copy link
Contributor Author

bioluks commented Jul 20, 2023

Of course I can and I wanted to add it; before that I wanted to hear your opinion. Next week I'll have the free time to add this check and test it out thoroughly.

@cwsqer006
Copy link

DFGTYHKL;

@cwsqer006
Copy link

DFGHJKL/

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.

3 participants