Skip to content

Properly Handle Certificate Expired Error #118

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

Closed
wants to merge 2 commits into from

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Jul 19, 2018

I run into this issue about twice a year where my server crashes with 500 error. It's stuck this way until a new cert is renewed.

If there is a Certificate Expired error on startup skip that push configuration.

There are more errors that could occur from the node-apn module but they rarely occur.

Let me know if there is a better way to do this.

@dplewis dplewis changed the title Skip Certificate Expired Error Properly Handle Certificate Expired Error Jul 19, 2018
@codecov
Copy link

codecov bot commented Jul 19, 2018

Codecov Report

Merging #118 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #118   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines         258    263    +5     
=====================================
+ Hits          258    263    +5
Impacted Files Coverage Δ
src/APNS.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7f9bc4...583fe6b. Read the comment docs.

@flovilmart
Copy link
Contributor

I’m pretty much against this change as it will silently fail after and users won’t know that their certificate is expired. The best technique is to keep a sticky note as for when to rotate your certificates.

If you have SSL on, you’ll have the same issue, if you forget when to rotate them, no one will be able to access your website/API.

So for me, crashing is the right thing.

@dplewis
Copy link
Member Author

dplewis commented Jul 19, 2018

@flovilmart I agree, Is there some way we can add an environment variable to make it optional?

@flovilmart
Copy link
Contributor

It’s a lot of trouble for not a lot I believe. I mean, if there was a way to check the expiration and warn big time that the certificate was about to expire, that would be nice :) better than silently ignoring the cert invalidity.

The exception makes sense for me, it’s not recoverable, and the developer need to act rapidely.

@dplewis
Copy link
Member Author

dplewis commented Jul 19, 2018

@flovilmart That would be great since I don't get emails from Apple for these kind of things. I think we can get the validity of the certs. I'll dig around some more.

@dplewis
Copy link
Member Author

dplewis commented Jul 19, 2018

@flovilmart I opened a PR. node-apn/node-apn#629 Let's see where this goes...

@dplewis
Copy link
Member Author

dplewis commented Sep 8, 2018

Closing as properly handled

@dplewis dplewis closed this Sep 8, 2018
@dplewis dplewis deleted the remove-cert-expire branch September 8, 2018 00:30
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.

2 participants