From 27f5dd69116d022bbb647ec2883bfbf965076c74 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 23a3f6e8cd7c1c..6d857a74ddfdbd 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;
@@ -29,27 +30,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);
+}