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

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

Closed
cpup22 opened this issue Jan 31, 2017 · 15 comments

Comments

Projects
None yet
4 participants
@cpup22
Copy link

commented Jan 31, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

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

@dblock dblock added the bug? label Jan 31, 2017

@cpup22

This comment has been minimized.

Copy link
Author

commented Jan 31, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

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 #47), after #48 is resolved.

@cpup22

This comment has been minimized.

Copy link
Author

commented Jan 31, 2017

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

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

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

This comment has been minimized.

Copy link
Author

commented Jan 31, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

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

@cpup22

This comment has been minimized.

Copy link
Author

commented Jan 31, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

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

This comment has been minimized.

Copy link
Author

commented Jan 31, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2017

@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

This comment has been minimized.

Copy link
Member

commented Jan 31, 2017

@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

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

Closed via #51, thanks @mreinstein.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.