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

Cannot read property 'encrypted' of undefined #2994

Closed
cressie176 opened this issue May 12, 2016 · 9 comments
Closed

Cannot read property 'encrypted' of undefined #2994

cressie176 opened this issue May 12, 2016 · 9 comments

Comments

@cressie176
Copy link

cressie176 commented May 12, 2016

Simply requiring express then attempting to safe stringify process causes an exception.

var stringify = require('json-stringify-safe')
var express = require('express')
stringify(process)

Result

TypeError: Cannot read property 'encrypted' of undefined
    at IncomingMessage.protocol (/Users/steve/Development/guidesmiths/express-bug/node_modules/express/lib/request.js:288:30)
    at Object.stringify (native)
    at stringify (/Users/steve/Development/guidesmiths/express-bug/node_modules/json-stringify-safe/stringify.js:5:15)
    at Object.<anonymous> (/Users/steve/Development/guidesmiths/express-bug/index.js:3:1)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:441:10)
    at startup (node.js:139:18)
"express": "^4.13.4",
"json-stringify-safe": "^5.0.1"
@tunniclm
Copy link
Contributor

tunniclm commented May 12, 2016

Hi @cressie176 -- you are correct, during the stringify of process it attempts to stringify process.mainModule which contains information about the whole tree of loaded modules which will include express since you require()d it. This includes stringifying the exports property of the express module, and it exports request. Now, request has a number of properties that are dynamically determined via a function (they were defined using defineGetter()).

Some of these properties rely on certain values being non-null -- evaluating them outside the context of an application/active connection means some of these values are null and generate the TypeErrors you see.

I just hacked my local express to include null-checks around the problematic property getters and it is possible to make it work. Does the @expressjs/express-tc have an opinion on whether we should add null-checks for these property getters?

@LinusU
Copy link
Member

LinusU commented May 12, 2016

What is the use case for stringifying process?

@hacksparrow
Copy link
Member

Also, wondering if this can be considered a valid issue in Express, at all.

@tunniclm
Copy link
Contributor

tunniclm commented May 12, 2016

It seems like express.request and express.response are exposed so that they can be modified (there are some tests that check that the functionality works). I don't know whether that is documented anywhere -- if it is, then that would be the place to document whether it is valid to access these problematic properties (eg: protocol, query, fresh, ip, ips, path, host), or similar methods (eg: header) without being inside an app / having a live connection.

@dougwilson
Copy link
Contributor

dougwilson commented May 12, 2016

I don't think this is a big in Express, and I don't think we should add a bunch of null check on everything (what would we do if it was null? Return null? How would we document under what conditions those will be null? What is the user expected to do when they return null? How will we know or users know about bugs if everything just silently returns null when fundamental object assumptions are not true?).

The issue here is that the user is trying to read properties on our export of a prototype. If those exports are not documented, that is a separate issue and they should be, as they are useful to add new global getters/setters, and also make Express behave better with the htttp2 module.

As for when it is valid to access the properties, the answer is that object represents a prototype, so the only time is though a concrete object that has that prototype in it's chain.

Maybe there is a better solution somewhere to this, but I don't think null checks is the right solution.

@dougwilson
Copy link
Contributor

I would ask why happens if someone does

JSON.stringify(process)

with Express loaded? If it does not die, then maybe there is a bug in "json-stringify-safe"? It would sound less safe than standard JSON stringify if this is the case.

@tunniclm
Copy link
Contributor

tunniclm commented May 12, 2016

@dougwilson I gave JSON.stringify(process) a quick test, but it fails with TypeError: Converting circular structure to JSON both with and without the null-checks.

@dougwilson
Copy link
Contributor

Gotcha. This issue does not seem specific to Express, so I'm not sure changing anything here makes sense. For example, you can try various modules and you'll run into various errors all over the place, because no module expects that it's exports is going to get walked by a stringification.

For example, here is npm:

$ cat test.js
var stringify = require('json-stringify-safe')
var npm = require('npm')
stringify(process)

$ node test.js

Error: Call npm.load(config, cb) before using this command.
See the README.md or cli.js for example usage.
    at Object.defineProperty.get [as access] (node_modules\npm\lib\npm.js:77:15)
    at Object.stringify (native)
    at stringify (node_modules\json-stringify-safe\stringify.js:5:15)
    at Object.<anonymous> (test.js:3:1)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)

@cressie176
Copy link
Author

For some reason I haven't been getting notifications for this otherwise I would have responded. I'd incorrectly assumed express intentionally was modifying the process for some reason, and hadn't realised the recursion into process.mainModule was the cause. Thanks for investigating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants