alexa-js / alexa-app-server

An Alexa app server for alexa-app.
MIT License
403 stars 116 forks source link

alexa-verifier set the "verify" flag to true and calling from a non echo throws an error #50

Closed cpup22 closed 7 years ago

cpup22 commented 7 years ago

I used the example code here:

var AlexaAppServer = require("alexa-app-server");
var server = new AlexaAppServer( {
    port:3000
    ,verify: true
    ,public_html: "public"
    // Use preRequest to load user data on each request and add it to the request json.
    // In reality, this data would come from a db or files, etc.
    ,preRequest: function(json,req,res) {
        //console.log("preRequest fired");
        //json.userDetails = { "name":"Bob Smith" };
    }
    // Add a dummy attribute to the response
    ,postRequest: function(json,req,res) {
        //json.dummy = "text";
    }
} );
server.start();

and added the verify flag so that I can pass the skills certification. When I try to make a request through postman it does fail (which is good) but It's not throwing a 500, instead it throws the following error:

ReferenceError: er is not defined
    at Object.handle (/my/path/node_modules/alexa-app-server/index.js:97:57)
    at next_layer (/my/path/node_modules/alexa-app-server/node_modules/express/lib/router/route.js:103:13)
    at Route.dispatch (/my/path/node_modules/alexa-app-server/node_modules/express/lib/router/route.js:107:5)
    at /my/path/node_modules/alexa-app-server/node_modules/express/lib/router/index.js:195:24
    at Function.proto.process_params (/my/path/node_modules/alexa-app-server/node_modules/express/lib/router/index.js:251:12)
    at next (/my/path/node_modules/alexa-app-server/node_modules/express/lib/router/index.js:189:19)
    at Layer.handle (/my/path/node_modules/alexa-app-server/index.js:79:15)
    at trim_prefix (/my/path/node_modules/alexa-app-server/node_modules/express/lib/router/index.js:226:17)
    at /my/path/node_modules/alexa-app-server/node_modules/express/lib/router/index.js:198:9
    at Function.proto.process_params (/my/path/node_modules/alexa-app-server/node_modules/express/lib/router/index.js:251:12)
    at next (/my/path/node_modules/alexa-app-server/node_modules/express/lib/router/index.js:189:19)
    at next (/my/path/node_modules/alexa-app-server/node_modules/express/lib/router/index.js:166:38)
    at Layer.staticMiddleware [as handle] (/my/path/node_modules/alexa-app-server/node_modules/serve-static/index.js:55:61)
    at trim_prefix (/my/path/node_modules/alexa-app-server/node_modules/express/lib/router/index.js:226:17)
    at /my/path/node_modules/alexa-app-server/node_modules/express/lib/router/index.js:198:9
    at Function.proto.process_params (/my/path/node_modules/alexa-app-server/node_modules/express/lib/router/index.js:251:12)
    at next (/my/path/node_modules/alexa-app-server/node_modules/express/lib/router/index.js:189:19)
    at IncomingMessage.<anonymous> (/my/path/node_modules/alexa-app-server/index.js:184:5)
    at emitNone (events.js:86:13)
    at IncomingMessage.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:974:12)
    at _combinedTickCallback (internal/process/next_tick.js:74:11)
    at process._tickCallback (internal/process/next_tick.js:98:9)
dblock commented 7 years ago

Whcih version are you using? I believe this has been fixed, try the released 2.3.1 and if that doesn't work HEAD.

cpup22 commented 7 years ago

yea I'm on 2.3.1

Is there an easy way to use head within my node app? Or do I need to checkout the git repo and move the content into the npm_modules dir?

dblock commented 7 years ago

You should be able to do this in package.json:

"dependencies": {
    "alexa-app-server": "alexa-js/alexa-app-server"
}

See https://docs.npmjs.com/files/package.json#github-urls for details.

Can you please verify that this is actually fixed? We can close the issue then.

We'll release the next version shortly (see https://github.com/alexa-js/alexa-app-server/issues/47), after https://github.com/alexa-js/alexa-app-server/pull/48 is resolved.

cpup22 commented 7 years ago

Changed my dependencies per your suggestion in the package.json file and restarted my app. Getting the same error.

dblock commented 7 years ago

What's in alexa-app-server/index.js:97? You can see on master that https://github.com/alexa-js/alexa-app-server/blob/master/index.js#L97 is not at all able to throw that error.

cpup22 commented 7 years ago

you are right... I forgot to run an npm install before restarting. okay, after doing that it doesn't throw the er error I was seeing before. However, it also isn't throwing a 500 error when I try from postman and not an echo device. I get a valid response:

{
  "version": "1.0",
  "response": {
    "directives": [],
    "shouldEndSession": true,
    "card": {
      "type": "LinkAccount"
    },
    "outputSpeech": {
      "type": "SSML",
      "ssml": "<speak>Looks like you need to link your account. Please open the Alexa app for setup.</speak>"
    }
  },
  "sessionAttributes": {}
}

I thought with verify set to true that it would've thrown an error, correct?

dblock commented 7 years ago

I am not sure what this error is or what to do with it ...

cpup22 commented 7 years ago

It's not an error. That's the point. If a request comes in from a non-echo, my assumption is the alexa-verifier code should mark it as an error. and there is no error.

dblock commented 7 years ago

Possibly, I don't know much about this. I would read the code and docs wrt what it's supposed to do, maybe open an issue in alexa-verifier?

cpup22 commented 7 years ago

So, just to make sure I'm following you, are you saying that the flag "verify" in the alexa-app-server documentation:

// This will add verification for alexa requests as require by the alexa certification 
    // process. Provied by alexa-verifier 
    verify: false,

hasn't been tested? And I should use the alexa-verifier module directly?

dblock commented 7 years ago

I'm saying that I don't know much about it, I didn't write this code or actually used it myself ;) Others like @tejashah88 or @mreinstein will know a lot more!

mreinstein commented 7 years ago

I've never used alexa-app-server, so I can't claim to be any sort of expert on that. One thing I did notice in looking at the code:

https://github.com/alexa-js/alexa-app-server/blob/master/index.js#L85

this is not forcing the requests to pass the header check. It should instead probably be:

self.express.use(endpoint, alexaVerifierMiddleware({ strictHeaderCheck: true ));

^ this is assuming only Alexa requests will flow through this express instance.

mreinstein commented 7 years ago

@tejashah88 I need your input on something. The PR I just sent enforces strict header checking and all of the verification logic that amazon requires when submitting a skill to run on production. I've noticed these tests:

https://github.com/alexa-js/alexa-app-server/blob/master/test/test-examples-server-verification.js

2 of them expect a 200 code back. I think these should actually be 401 errors.

I'd appreciate your thoughts since it seems you actually wrote the tests.

tejashah88 commented 7 years ago

@mreinstein You're right! They should be expecting 401 errors.

I guess the only thing that seems to nag me is that we don't have a way to actually test if a 200 code is being sent back when using an actual Alexa device, since we are basically assuming that it should work anyways. Unless Amazon gave a way to test this with a virtual Amazon device or something, I guess one would have to have some sort of test skill that would verify that the verification is working as expected.

dblock commented 7 years ago

Closed via https://github.com/alexa-js/alexa-app-server/pull/51, thanks @mreinstein.