okta / samples-nodejs-express-4

Express 4 samples. Will publish an artifact that can be consumed by end-to-end sample repos
Other
119 stars 119 forks source link

Error handling for non-https #15

Open alexdahl-okta opened 7 years ago

alexdahl-okta commented 7 years ago

tl;dr In the config for oktaUrl, I accidentally used http instead of https. The resulting error is obscure, and I had a hard time troubleshooting.

Steps to reproduce

  1. Follow the OIDC setup to create a new organization and app
  2. Follow the readme quick start, but use"oktaUrl": "http://{{yourOktaOrg}}.oktapreview.com" instead of https.
  3. Run npm start and open localhost:3000.
  4. Go to "Log in by redirecting to Okta" and click "Sign in".
  5. Submit the Okta sign in form with your credentials

Result

"id_token could not be decoded from the response":

result

I tried to debug by printing the token. I searched my local source for the error message, found route-handlers.js:174, and added + json to the status message. This revealed a redirect message instead of a token: screen shot 2017-03-27 at 12 39 12 pm

Following the redirect link revealed a server error response: screen shot 2017-03-27 at 12 39 15 pm

I did not mentally connect this back to my config URL. It wasn't until I was comparing my config file with someone else's that I noticed the http/https difference.

Thanks to @nbarbettini for helping me debug!

Potential improvements

nbarbettini commented 7 years ago

Thanks for the great issue reporting @alexdahl-okta 😄

Another potential fix is inspecting the config at startup, and throwing a warning or error if the oktaUrl is http but NOT localhost.

nbarbettini commented 6 years ago

Since we rebuilt all these samples from scratch, I tested this again. If I use http instead of https in webServer.oidc.issuer in .samples.config.json, the app refuses to start with a cryptic error message:

C:\Users\Nate\Documents\code\samples-nodejs-express-4 [master ≡ +0 ~2 -0 !]> npm run okta-hosted-login-server
(node:18300) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): SyntaxError: Unexpected token < in JSON at position 0
(node:18300) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Since the config file now contains https by default, I don't think many people will run into this. Still, it'd be nice to output a warning if the issuer does not begin with https. And, if the discovery document can't be parsed, throw a meaningful error message.

cc @robertdamphousse-okta