Getter property redefinition bug: "req.session._csrf" #879

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@pilwon

The line delete req.session._csrf; in lib/middleware/csrf.js does not actually delete pre-defined property as _csrf is a non-configurable property defined by Object.defineProperty. As a result, property redefinition error occurs when the inner function createToken() is called the second time.

This PR fixes the bug by setting configurable attribute to descriptor during property creation, as advised in Mozilla Doc - Object.defineProperty: Modifying a property:

When the property already exists, Object.defineProperty() attempts to modify the property according to the values in the descriptor and the object's current configuration. If the old descriptor had its configurable attribute set to false (the property is said to be "non-configurable"), then no attribute besides writable can be changed. In that case, it is also not possible to switch back and forth between the data and accessor property types. If a property is non-configurable, its writable attribute can only be changed to false. A TypeError is thrown when attempts are made to change non-configurable property attributes (besides the writable attribute) unless the current and new values are the same.
@tj
Sencha Labs member
tj commented Aug 29, 2013

not sure why we have that delete anyway, removed

@tj tj closed this Aug 29, 2013
@pilwon

@visionmedia Here's the reason we need that delete. The following code...

if ('GET' == req.method || 'HEAD' == req.method || 'OPTIONS' == req.method) return next();

...simply protects entire routes (except those three verbs described above) from csrf attacks if we attach the middleware to the server. By reverting 2b2e19c and applying my PR, we now have freedom to do csrf validation only for certain routes we need.

@pilwon

@visionmedia If you could re-enable the use case above, please also expose createToken() so token that would satisfy checkToken() can be generated from outside. It no longer uses simple token-value comparison so it seems I cannot generate a random token when it's not present.

@tj
Sencha Labs member
tj commented Aug 29, 2013

why not just "wrap" the middleware? var csrf = express.csrf(); <-- and invoke csrf(req, res, next) in other middleware once you've applied some ad-hoc logic. That's what I typically do (and just did for our app the other day for csrf)

@pilwon

@visionmedia Aha I see what you mean. That does seem like the intended way of using it. Thanks TJ-

@tj
Sencha Labs member
tj commented Aug 29, 2013

np! It's an under-utilized technique for weird edge-cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment