From 5df3dc1169a22093f7c6a13111a938ac01a9c39d Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Tue, 7 Nov 2017 19:28:06 +0800
Subject: [PATCH] src: make StreamBase prototype accessors robust

This PR makes the prototype accessors added by StreamBase::AddMethods
nonenumerable and checks the signatures in the accessors so they
throw instead of raising assertions when called with incompatible
receivers. They could be enumerated when inspecting the prototype
with util.inspect or the inspector protocol.

PR-URL: https://github.com/nodejs/node/pull/16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 src/stream_base-inl.h                         | 15 ++++++++---
 ...-base-prototype-accessors-enumerability.js | 19 +++++++++++++
 .../test-stream-base-prototype-accessors.js   | 27 +++++++++++++++++++
 3 files changed, 57 insertions(+), 4 deletions(-)
 create mode 100644 test/parallel/test-stream-base-prototype-accessors-enumerability.js
 create mode 100644 test/parallel/test-stream-base-prototype-accessors.js

diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h
index e1e50802f2a06a..807e138ef7b6df 100644
--- a/src/stream_base-inl.h
+++ b/src/stream_base-inl.h
@@ -11,6 +11,7 @@
 
 namespace node {
 
+using v8::AccessorSignature;
 using v8::External;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
@@ -31,27 +32,33 @@ void StreamBase::AddMethods(Environment* env,
   HandleScope scope(env->isolate());
 
   enum PropertyAttribute attributes =
-      static_cast<PropertyAttribute>(v8::ReadOnly | v8::DontDelete);
+      static_cast<PropertyAttribute>(
+          v8::ReadOnly | v8::DontDelete | v8::DontEnum);
+  Local<AccessorSignature> signature =
+      AccessorSignature::New(env->isolate(), t);
   t->PrototypeTemplate()->SetAccessor(env->fd_string(),
                                       GetFD<Base>,
                                       nullptr,
                                       env->as_external(),
                                       v8::DEFAULT,
-                                      attributes);
+                                      attributes,
+                                      signature);
 
   t->PrototypeTemplate()->SetAccessor(env->external_stream_string(),
                                       GetExternal<Base>,
                                       nullptr,
                                       env->as_external(),
                                       v8::DEFAULT,
-                                      attributes);
+                                      attributes,
+                                      signature);
 
   t->PrototypeTemplate()->SetAccessor(env->bytes_read_string(),
                                       GetBytesRead<Base>,
                                       nullptr,
                                       env->as_external(),
                                       v8::DEFAULT,
-                                      attributes);
+                                      attributes,
+                                      signature);
 
   env->SetProtoMethod(t, "readStart", JSMethod<Base, &StreamBase::ReadStart>);
   env->SetProtoMethod(t, "readStop", JSMethod<Base, &StreamBase::ReadStop>);
diff --git a/test/parallel/test-stream-base-prototype-accessors-enumerability.js b/test/parallel/test-stream-base-prototype-accessors-enumerability.js
new file mode 100644
index 00000000000000..f59aced197c515
--- /dev/null
+++ b/test/parallel/test-stream-base-prototype-accessors-enumerability.js
@@ -0,0 +1,19 @@
+'use strict';
+
+require('../common');
+
+// This tests that the prototype accessors added by StreamBase::AddMethods
+// are not enumerable. They could be enumerated when inspecting the prototype
+// with util.inspect or the inspector protocol.
+
+const assert = require('assert');
+
+// Or anything that calls StreamBase::AddMethods when setting up its prototype
+const TTY = process.binding('tty_wrap').TTY;
+
+{
+  assert.strictEqual(TTY.prototype.propertyIsEnumerable('bytesRead'), false);
+  assert.strictEqual(TTY.prototype.propertyIsEnumerable('fd'), false);
+  assert.strictEqual(
+    TTY.prototype.propertyIsEnumerable('_externalStream'), false);
+}
diff --git a/test/parallel/test-stream-base-prototype-accessors.js b/test/parallel/test-stream-base-prototype-accessors.js
new file mode 100644
index 00000000000000..f9e12582a098d8
--- /dev/null
+++ b/test/parallel/test-stream-base-prototype-accessors.js
@@ -0,0 +1,27 @@
+'use strict';
+
+require('../common');
+
+// This tests that the prototype accessors added by StreamBase::AddMethods
+// do not raise assersions when called with incompatible receivers.
+
+const assert = require('assert');
+
+// Or anything that calls StreamBase::AddMethods when setting up its prototype
+const TTY = process.binding('tty_wrap').TTY;
+
+// Should throw instead of raise assertions
+{
+  const msg = /TypeError: Method \w+ called on incompatible receiver/;
+  assert.throws(() => {
+    TTY.prototype.bytesRead;
+  }, msg);
+
+  assert.throws(() => {
+    TTY.prototype.fd;
+  }, msg);
+
+  assert.throws(() => {
+    TTY.prototype._externalStream;
+  }, msg);
+}