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

Very dangerous log.Fatal called in several places #762

Closed
paologaleotti opened this issue Jan 10, 2025 · 5 comments · Fixed by #763
Closed

Very dangerous log.Fatal called in several places #762

paologaleotti opened this issue Jan 10, 2025 · 5 comments · Fixed by #763
Milestone

Comments

@paologaleotti
Copy link

paologaleotti commented Jan 10, 2025

(using v6.0.2)
We are about to use this in production and we noticed that when passing an invalid certificate it panics. This can cause unexpected failures at runtimes, and i saw that #616 was marked as closed but it does not seem to be.

There are three log.Fatal in the codebase:

  • config.go line 325: "Failed to parse certificate"
  • server.go line 112 and line 151

I wonder why the error cannot bubble up to the NewClient because that now returns an error.

Are we missing something? Thanks in advance

@danomagnum
Copy link
Contributor

I put together a fix for this that does basically the same thing as #616: #763

@paologaleotti
Copy link
Author

Thank you for the very quick response! Hope it will get merged soon and included in the next release :)

@magiconair
Copy link
Member

That's probably an oversight from NewClient() returning an error. Good catch.

@magiconair magiconair added this to the v0.6.3 milestone Jan 11, 2025
magiconair added a commit that referenced this issue Jan 11, 2025
Issue #762: Remove log.Fatal's.
@magiconair
Copy link
Member

Fixed with #763

@magiconair
Copy link
Member

Great work everyone! Thank you!

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 a pull request may close this issue.

3 participants