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

feature: less verbose CORS error logging #1581

Merged
merged 4 commits into from
Sep 9, 2023
Merged

Conversation

JacoKoster
Copy link
Contributor

Fixes #1576

Copy link
Member

@tkurki tkurki left a comment

Choose a reason for hiding this comment

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

With this change there's no logging of failed requests, right? I have found that useful, as CORS failures are so opaque otherwise.

Setting up CORS is kinda painful. I was thinking the server could have a feature where the admin UI shows CORS failures with a button to add the origin.

src/cors.ts Show resolved Hide resolved
@tkurki tkurki added the fix label Jul 18, 2023
@tkurki
Copy link
Member

tkurki commented Aug 27, 2023

Do you plan to continue with this?

@JacoKoster
Copy link
Contributor Author

Yes, i will probably pick it up again when i'm back from sailing, unless you want to pick it up, then we can close it ;-)

@JacoKoster
Copy link
Contributor Author

Instead of using the dynamic load of cors, i added an additional middleware for the specific purpose of logging potential cors-issues. Be aware that debugging cors should be done in the browser.
image
image

@tkurki
Copy link
Member

tkurki commented Sep 9, 2023

😄 the amount and structure of the code is almost exactly the same as the original.

@tkurki tkurki merged commit 6b2d0a2 into SignalK:master Sep 9, 2023
2 checks passed
@tkurki tkurki added the feature label Sep 9, 2023
@tkurki tkurki changed the title Fix: CORS logging issue #1576 feature: less verbose CORS error logging Sep 9, 2023
@tkurki
Copy link
Member

tkurki commented Sep 9, 2023

I reworded the PR title a little as release notes are created from PRs, not issues, so they should be self documenting when possible.

Fix or feature - or is it just a change or improvement? In the end it is in the eye of the reader 🤷 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cors failure logs too much
2 participants