Closed jgawor closed 7 years ago
@jgawor Thank you for bringing that up. Unfortunately, I'm not very knowledgeable as to why this code is what it is. The original alexa-verifier module did have an example for using with express here, before it was moved to this module. @mreinstein might be able to explain more about this.
@jgawor Actually, looking back at the code, it does make sense that request should not go through if the signaturecertchainurl
header is not present and it is indeed an Alexa request. I guess the only caveat is that any non-Alexa requests to the server will fail. I'll run some tests later today.
One can easily bypass the Alexa verifier by simply not sending the signaturecertchainurl header
This was done originally because it wasn't clear to me if there was a way to run this middleware only for alexa related express routes. I've found that people tend to mess this up if it's just based on declaring middlewares/routes in the right order.
Keep in mind the original goal was not to have perfect security, but to at least handle the cases that Amazon checks for when reviewing Alexa skills. This module accomplishes that. (I think...I haven't published an Alexa skill in a while but last time I tried using this it passed.)
That said, I'm always open to improving the security where practical. Here's what I think we should do:
strictHeaderCheck
and have it be false
by default. Setting this to true
would reject any requests lacking the header
const app = express()
const alexaRouter = express.Router()
alexaRouter.use(alexaVerifierMiddleware({ strictHeaderCheck: true }))
// routes that handle alexa traffic are now attached here. since this is
// attached to a router mounted at /alexa, this endpoint will be accessible
// at /alexa/weather_info
alexaRouter.get('/weather_info', function(req, res) { ... })
app.use('/alexa', alexaRouter)
app.listen(3000)
NOTE: I have not tested this code sample
thoughts?
I've just merged the changes here. Let me know if this is sufficient enough before I publish a new version.
I would rather see the strictHeaderCheck
to default to true
to be on the safe side but this will work too. Thanks!
Just made the changes and will be pushing to a new version! Thanks!
so a few comments:
I would rather see the strictHeaderCheck to default to true
@jgawor I agree that this would be ideal, but this is a breaking change. It's going to change the current behavior of the module. (the current behavior is essentially the same as strictHeaderCheck: false
.
@tejashah88 this line:
module.exports = function alexaVerifierMiddleware(options)
you need to check that options
is actually defined. The current behavior of anyone using this middleware is to pass no arguments.
One can easily bypass the Alexa verifier by simply not sending the
signaturecertchainurl
header because of https://github.com/tejashah88/alexa-verifier-middleware/blob/master/container.js#L11. Meaning, somebody can still invoke the skill without any of the checks. I don't think that's desirable as one would want to fail any requests that are invalid. So, I think that check should be removed.