cnnlabs / cnn-hapi

CNN Hapi
10 stars 4 forks source link

Routes/State Configuration is not picked up by CNN-HAPI Abstraction #33

Closed rbp15 closed 8 years ago

rbp15 commented 8 years ago

JIRA Ticket http://tickets.turner.com/browse/CNNGAMP-193

Error To reproduce the error run the cnn amp app and load the following example url.

https://localhost:5007/cnn/2016/08/18/sport/us-swimmers-olympics-robbery-questions/index.html

Once loaded create a custom cookie with the name value pair as follows and then refresh the page:

cookie 
name:  EPi:NumberOfVisits  value: 1,2016-08-17T22:51:48

After refreshing the page you should get this error:

{"statusCode":400,"error":"Bad Request","message":"Invalid cookie value"}

Why this relates to/depends on CNN-HAPI HAPI parses cookies and stores them into the request state. Here are similar logged issues. https://github.com/hapijs/hapi/issues/2513 https://github.com/hapijs/statehood/issues/11

Discovered thus far: Pseudo code representation of main.js from cnn-hapi module

var connectionOptions = {};

connectionOptions.tls = options.tls;

Assume the below options.connections.routes.state. is there - just to shorten this example

connectionOptions.state = options.connections.routes.state;

server.connection(connectionOptions);

From schema.js in cnn-hapi module - JOI(module) schema enforcement object notice there is nothing for routes or state allowed

internals.connection = internals.connectionBase.keys({
    autoListen: Joi.boolean(),
    host: Joi.string().hostname(),
    address: Joi.string().hostname(),
    labels: Joi.array().items(Joi.string()).single(),
    listener: Joi.any(),
    port: Joi.alternatives([
        Joi.number().integer().min(0),          // TCP port
        Joi.string().regex(/\//),               // Unix domain socket
        Joi.string().regex(/^\\\\\.\\pipe\\/)   // Windows named pipe
    ])
        .allow(null),
    tls: Joi.alternatives([
        Joi.object().allow(null),
        Joi.boolean()
    ]),
    uri: Joi.string().regex(/[^/]$/)
});

So if I do the above in the "real" code the following error is thrown:

ValidationError: Invalid state definition defaults {
         "strictHeader": true,
         "ignoreErrors": false,
         "isSecure": false,
         "isHttpOnly": false,
         "path": null,
         "domain": null,
         "ttl": null,
         "encoding": "none",
         "failAction" [1]: "log",  ***
         "parse" [2]: false  ***
     }

     [1] "parse" is not allowed
     [2] "failAction" is not allowed

    at Object.exports.Err.exports.process
    (/Users/rpotter/Documents/SourceControl/cnn-google-amp-service/node_modules/joi/lib/errors.js:154:19)

Now if this is the case you could assume that some object nesting is needed based on the above.

From the schema.js file it is eventually deduced(fact check please) that state is located in the object like this:

 server
        host
        port
        tls
        connections
            routes
                state
                    parse
                    failAction

However constructing the connectionOptions object to handle this nesting state results in an error as well. There's a double dose of state parse failAction because the routes object from the AMP app contains the state information into it. Right now my main.js(custom tweaked) is set to also build the second state config you see below.

Looking at the HAPI schema JOI is enforcing it looks like having state under "routes" is the way to go.

Here's that error mentioned:

  "connections" [1]: {
         "routes": {
             "files": {
                "relativeTo": "/Users/rpotter/Documents/SourceControl/cnn-google-amp-service/public"
             },
             "state": {
                 "parse": false,
                 "failAction": "log"
             }
         },
         "state": {
             "parse": false,
             "failAction": "log"
         }
     }

 [1] "connections" is not allowed
 at Object.exports.unique.exports.contain.exports.reachTemplate.exports.assert.condition [as assert] (/Users/rpotter/Documents/SourceControl/cnn-google-amp-service/node_modules/hoek/lib/index.js:736:11)
 at Object.exports.apply (/Users/rpotter/Documents/SourceControl/cnn-google-amp-service/node_modules/hapi/lib/schema.js:17:10)

Vague references from schema.js on how state is nested under routes below: schema.js line: 60

 internals.routeBase = Joi.object({
        ...
         state: Joi.object({
            parse: Joi.boolean(),
            failAction: Joi.string().valid('error', 'log', 'ignore')
         }),
        ...

Schema.js line: 173 Where routeBase is used?

 internals.connectionBase = Joi.object({
                     app: Joi.object().allow(null),
                     compression: Joi.boolean(),
                     load: Joi.object(),
                     plugins: Joi.object(),
                     router: Joi.object({
                        isCaseSensitive: Joi.boolean(),
                        stripTrailingSlash: Joi.boolean()
                     }),
                routes: internals.routeBase,
                state: Joi.object() // Cookie defaults
             });

Where connectionBase is used? schema.js line 187

  internals.server = Joi.object({
                app: Joi.object().allow(null),
                cache: Joi.allow(null),                                 // Validated elsewhere
                connections: internals.connectionBase,
                debug: Joi.object({
                    request: Joi.array().allow(false),
                    log: Joi.array().allow(false)
                }).allow(false),
                load: Joi.object(),
                mime: Joi.object(),
                plugins: Joi.object(),
                useDomains: Joi.boolean()
            });
adslaton commented 8 years ago

Doing this route config allows it to work:

config: {
            state: {
                failAction: 'log'
            }
 }
adslaton commented 8 years ago

Resolved in https://github.com/cnnlabs/cnn-hapi/pull/34