hapijs / crumb

CSRF crumb generation and validation for hapi
Other
171 stars 50 forks source link

6.0.1 update breaks route-restful option #80

Closed gdibble closed 8 years ago

gdibble commented 8 years ago

An || got change to an &&:

image

stongo commented 8 years ago

isn't the && correct? the route setting should precede the plugin setting in the check and both conditions should pass.

gdibble commented 8 years ago

I thought so too, but in application my routes which specify...

    config: {
        plugins: {
            crumb: {
                restful: false
            }
        }
    }

...stopped working (whereas these send a form-field-value instead of req. header) when I upgraded to v6.0.1

Sorry if I've misintrepreted anything, I just couldn't get it to work w/o changing that && => ||


btw sorry I haven't done more; trying to prep for nodebots classes for 100 students so i have a lot of code & curriculum to write at the moment; after i hope to do more investigation & check the tests/etc

aef- commented 8 years ago

We need to add a condition that explicitly checks for false as the value and to short-circuit the plugin/settings config condition.

I think this should do the trick (test ln 659 should expect 200 instead of 403, this causes it to fail appropriately)

const routeIsRestful = request.route.settings.plugins._crumb ? 
                       request.route.settings.plugins._crumb.restful : undefined;
 if (routeIsRestful === false ||
     (routeIsRestful !== true && settings.restful === false))
stongo commented 8 years ago

The pull request of branch route-config should fix this. Can anyone confirm?

stongo commented 8 years ago

Fixed by release 6.0.2

gdibble commented 8 years ago

Many thanks

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.