Throw `TypeError` if `[[DefineOwnProperty]]` returns false (#6868)

According to es spec Object.defineProperty should throw if internal [[DefineOwnProperty]] returns false-ish.
This happens specifically if the defineProperty proxy trap returns false (See #6505).

Changes
Throw TypeError in JavascriptObject::EntryDefineProperty if DefineOwnPropertyHelper returns false-ish
Changed content of JSERR_ProxyHandlerReturnedFalse
Routed PropertyOperationFlags through the call stack

Fixes #6505

lib/Parser/rterrors.h

@@ -1,6 +1,6 @@
 //-------------------------------------------------------------------------------------------------------
 // Copyright (C) Microsoft Corporation and contributors. All rights reserved.
-// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
+// Copyright (c) ChakraCore Project Contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 #define IDS_COMPILATION_ERROR_SOURCE    4096
@@ -371,7 +371,7 @@ RT_ERROR_MSG(JSERR_InvalidIteratorObject, 5672, "%s : Invalid iterator object",
 RT_ERROR_MSG(JSERR_NoAccessors, 5673, "Invalid property descriptor: accessors not supported on this object", "", kjstTypeError, 0)
 RT_ERROR_MSG(JSERR_RegExpInvalidEscape, 5674, "", "Invalid regular expression: invalid escape in unicode pattern", kjstSyntaxError, 0)
 RT_ERROR_MSG(JSERR_RegExpTooManyCapturingGroups, 5675, "", "Regular expression cannot have more than 32,767 capturing groups", kjstRangeError, 0)
-RT_ERROR_MSG(JSERR_ProxyHandlerReturnedFalse, 5676, "Proxy %s handler returned false", "Proxy handler returned false", kjstTypeError, 0)
+RT_ERROR_MSG(JSERR_ProxyHandlerReturnedFalse, 5676, "Proxy '%s' handler returned falsish for property '%s'", "Proxy handler returned false", kjstTypeError, 0)
 RT_ERROR_MSG(JSERR_UnicodeRegExpRangeContainsCharClass, 5677, "%s", "Character classes not allowed in a RegExp class range.", kjstSyntaxError, 0)
 RT_ERROR_MSG(JSERR_DuplicateKeysFromOwnPropertyKeys, 5678, "%s", "Proxy's ownKeys trap returned duplicate keys", kjstTypeError, 0)
 RT_ERROR_MSG(JSERR_InvalidGloFuncDecl, 5679, "The global property %s is not configurable, writable, nor enumerable, therefore cannot be declared as a function", "", kjstTypeError, 0)

lib/Runtime/Language/JavascriptOperators.cpp

@@ -9101,14 +9101,14 @@ SetElementIHelper_INDEX_TYPE_IS_NUMBER:
     // Return value:
     // - TRUE = success.
     // - FALSE (can throw depending on throwOnError parameter) = unsuccessful.
-    BOOL JavascriptOperators::DefineOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* scriptContext)
+    BOOL JavascriptOperators::DefineOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* scriptContext, PropertyOperationFlags flags /*  = Js::PropertyOperation_None */)
     {
         Assert(obj);
         Assert(scriptContext);
 
         if (VarIs<JavascriptProxy>(obj))
         {
-            return JavascriptProxy::DefineOwnPropertyDescriptor(obj, propId, descriptor, throwOnError, scriptContext);
+            return JavascriptProxy::DefineOwnPropertyDescriptor(obj, propId, descriptor, throwOnError, scriptContext, flags);
         }
 #ifdef _CHAKRACOREBUILD
         else if (VarIs<CustomExternalWrapperObject>(obj))

lib/Runtime/Language/JavascriptOperators.h

@@ -1,5 +1,6 @@
 //-------------------------------------------------------------------------------------------------------
 // Copyright (C) Microsoft. All rights reserved.
+// Copyright (c) ChakraCore Project Contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 #pragma once
@@ -612,7 +613,7 @@ namespace Js
         static Var FromPropertyDescriptor(const PropertyDescriptor& descriptor, ScriptContext* scriptContext);
         static void CompletePropertyDescriptor(PropertyDescriptor* resultDescriptor, PropertyDescriptor* likePropertyDescriptor, ScriptContext* requestContext);
         static BOOL SetPropertyDescriptor(RecyclableObject* object, PropertyId propId, const PropertyDescriptor& descriptor);
-        static BOOL DefineOwnPropertyDescriptor(RecyclableObject* object, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* scriptContext);
+        static BOOL DefineOwnPropertyDescriptor(RecyclableObject* object, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* scriptContext, PropertyOperationFlags flags = Js::PropertyOperation_None);
         static BOOL DefineOwnPropertyForArray(JavascriptArray* arr, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* scriptContext);
 
         static BOOL DefineOwnPropertyForTypedArray(TypedArrayBase * typedArray, PropertyId propId, const PropertyDescriptor & descriptor, bool throwOnError, ScriptContext * scriptContext);

lib/Runtime/Library/JavascriptObject.cpp

@@ -1,6 +1,6 @@
 //-------------------------------------------------------------------------------------------------------
 // Copyright (C) Microsoft. All rights reserved.
-// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
+// Copyright (c) ChakraCore Project Contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 #include "RuntimeLibraryPch.h"
@@ -1349,7 +1349,11 @@ Var JavascriptObject::EntryDefineProperty(RecyclableObject* function, CallInfo c
         ModifyGetterSetterFuncName(propertyRecord, propertyDescriptor, scriptContext);
     }
 
-    DefineOwnPropertyHelper(obj, propertyRecord->GetPropertyId(), propertyDescriptor, scriptContext);
+    BOOL success = DefineOwnPropertyHelper(obj, propertyRecord->GetPropertyId(), propertyDescriptor, scriptContext);
+    if (!success)
+    {
+        JavascriptError::ThrowTypeError(scriptContext, JSERR_DefineProperty_Default, scriptContext->GetPropertyName(propertyRecord->GetPropertyId())->GetBuffer());
+    }
 
     return obj;
 }
@@ -2174,7 +2178,7 @@ BOOL JavascriptObject::DefineOwnPropertyHelper(RecyclableObject* obj, PropertyId
         // TODO: implement DefineOwnProperty for other object built-in exotic types.
         else
         {
-            returnValue = JavascriptOperators::DefineOwnPropertyDescriptor(obj, propId, descriptor, throwOnError, scriptContext);
+            returnValue = JavascriptOperators::DefineOwnPropertyDescriptor(obj, propId, descriptor, throwOnError, scriptContext, Js::PropertyOperation_StrictMode);
             if (propId == PropertyIds::__proto__)
             {
                 scriptContext->GetLibrary()->GetObjectPrototypeObject()->PostDefineOwnProperty__proto__(obj);

lib/Runtime/Library/JavascriptProxy.cpp

@@ -1,6 +1,6 @@
 //-------------------------------------------------------------------------------------------------------
 // Copyright (C) Microsoft. All rights reserved.
-// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
+// Copyright (c) ChakraCore Project Contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 #include "RuntimeLibraryPch.h"
@@ -677,7 +677,7 @@ namespace Js
             resultDescriptor.SetWritable(true);
             resultDescriptor.SetEnumerable(true);
             resultDescriptor.SetValue(value);
-            return Js::JavascriptOperators::DefineOwnPropertyDescriptor(this, propertyId, resultDescriptor, true, requestContext);
+            return Js::JavascriptOperators::DefineOwnPropertyDescriptor(this, propertyId, resultDescriptor, true, requestContext, flags);
         }
         else
         {
@@ -698,7 +698,7 @@ namespace Js
 
             proxyPropertyDescriptor.SetValue(value);
             proxyPropertyDescriptor.SetOriginal(nullptr);
-            return Js::JavascriptOperators::DefineOwnPropertyDescriptor(this, propertyId, proxyPropertyDescriptor, true, requestContext);
+            return Js::JavascriptOperators::DefineOwnPropertyDescriptor(this, propertyId, proxyPropertyDescriptor, true, requestContext, flags);
         }
     }
 
@@ -827,7 +827,12 @@ namespace Js
         {
             if (flags & PropertyOperation_StrictMode)
             {
-                JavascriptError::ThrowTypeError(requestContext, JSERR_ProxyHandlerReturnedFalse, _u("deleteProperty"));
+                JavascriptError::ThrowTypeErrorVar(
+                    requestContext, 
+                    JSERR_ProxyHandlerReturnedFalse, 
+                    _u("deleteProperty"),
+                    threadContext->GetPropertyName(propertyId)->GetBuffer()
+                );
             }
             return trapResult;
         }
@@ -1695,7 +1700,7 @@ namespace Js
     }
 
 
-    BOOL JavascriptProxy::DefineOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* requestContext)
+    BOOL JavascriptProxy::DefineOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* requestContext, PropertyOperationFlags flags)
     {
         // #sec-proxy-object-internal-methods-and-internal-slots-defineownproperty-p-desc
         PROBE_STACK(requestContext, Js::Constants::MinStackDefault);
@@ -1735,7 +1740,7 @@ namespace Js
         Assert(!requestContext->IsHeapEnumInProgress());
         if (nullptr == defineOwnPropertyMethod)
         {
-            return JavascriptOperators::DefineOwnPropertyDescriptor(targetObj, propId, descriptor, throwOnError, requestContext);
+            return JavascriptOperators::DefineOwnPropertyDescriptor(targetObj, propId, descriptor, throwOnError, requestContext, flags);
         }
 
         //8. Let descObj be FromPropertyDescriptor(Desc).
@@ -1754,6 +1759,15 @@ namespace Js
         BOOL defineResult = JavascriptConversion::ToBoolean(definePropertyResult, requestContext);
         if (!defineResult)
         {
+            if (throwOnError && flags & PropertyOperation_StrictMode)
+            {
+                JavascriptError::ThrowTypeErrorVar(
+                    requestContext,
+                    JSERR_ProxyHandlerReturnedFalse,
+                    _u("defineProperty"),
+                    requestContext->GetPropertyName(propId)->GetBuffer()
+                );
+            }
             return defineResult;
         }
 
@@ -1847,23 +1861,23 @@ namespace Js
                 uint32 indexVal;
                 BOOL isNumericPropertyId = requestContext->IsNumericPropertyId(propertyId, &indexVal);
                 Assert(isNumericPropertyId);
-                return JavascriptOperators::SetItemOnTaggedNumber(receiver, targetObj, indexVal, newValue, requestContext, PropertyOperationFlags::PropertyOperation_None);
+                return JavascriptOperators::SetItemOnTaggedNumber(receiver, targetObj, indexVal, newValue, requestContext, propertyOperationFlags);
             }
             case SetPropertyTrapKind::SetPropertyOnTaggedNumberKind:
-                return JavascriptOperators::SetPropertyOnTaggedNumber(receiver, targetObj, propertyId, newValue, requestContext, PropertyOperation_None);
+                return JavascriptOperators::SetPropertyOnTaggedNumber(receiver, targetObj, propertyId, newValue, requestContext, propertyOperationFlags);
             case SetPropertyTrapKind::SetPropertyKind:
-                return JavascriptOperators::SetProperty(receiver, targetObj, propertyId, newValue, requestContext);
+                return JavascriptOperators::SetProperty(receiver, targetObj, propertyId, newValue, requestContext, propertyOperationFlags);
             case SetPropertyTrapKind::SetItemKind:
             {
                 uint32 indexVal;
                 BOOL isNumericPropertyId = requestContext->IsNumericPropertyId(propertyId, &indexVal);
                 Assert(isNumericPropertyId);
-                return  JavascriptOperators::SetItem(receiver, targetObj, indexVal, newValue, requestContext, PropertyOperationFlags::PropertyOperation_None, skipPrototypeCheck);
+                return  JavascriptOperators::SetItem(receiver, targetObj, indexVal, newValue, requestContext, propertyOperationFlags, skipPrototypeCheck);
             }
             case SetPropertyTrapKind::SetPropertyWPCacheKind:
             {
                 PropertyValueInfo propertyValueInfo;
-                return JavascriptOperators::SetPropertyWPCache(receiver, targetObj, propertyId, newValue, requestContext, PropertyOperationFlags::PropertyOperation_None, &propertyValueInfo);
+                return JavascriptOperators::SetPropertyWPCache(receiver, targetObj, propertyId, newValue, requestContext, propertyOperationFlags, &propertyValueInfo);
             }
             default:
                 AnalysisAssert(FALSE);
@@ -1886,9 +1900,13 @@ namespace Js
         {
             if (propertyOperationFlags & PropertyOperation_StrictMode)
             {
-                JavascriptError::ThrowTypeError(requestContext, JSERR_ProxyHandlerReturnedFalse, _u("set"));
+                JavascriptError::ThrowTypeErrorVar(
+                    requestContext,
+                    JSERR_ProxyHandlerReturnedFalse,
+                    _u("set"),
+                    requestContext->GetPropertyName(propertyId)->GetBuffer()
+                );
             }
-
             return setResult;
         }
 

lib/Runtime/Library/JavascriptProxy.h

@@ -1,5 +1,6 @@
 //-------------------------------------------------------------------------------------------------------
 // Copyright (C) Microsoft. All rights reserved.
+// Copyright (c) ChakraCore Project Contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 //  Implements JavascriptProxy.
@@ -70,7 +71,7 @@ namespace Js
         static JavascriptProxy* Create(ScriptContext* scriptContext, Arguments args);
 
         static BOOL GetOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propertyId, ScriptContext* requestContext, PropertyDescriptor* propertyDescriptor);
-        static BOOL DefineOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* requestContext);
+        static BOOL DefineOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* requestContext, PropertyOperationFlags flags);
 
         static DWORD GetOffsetOfTarget() { return offsetof(JavascriptProxy, target); }
 

test/Bugs/misc_bugs.js

@@ -189,10 +189,28 @@ var tests = [
     {
         name: "Strict Mode : throw type error when the handler returns falsy value",
         body: function () {
-            assert.throws(() => { "use strict"; let p1 = new Proxy({}, { set() { } }); p1.foo = 1; }, TypeError, "returning undefined on set handler is return false which will throw type error", "Proxy set handler returned false");
-            assert.throws(() => { "use strict"; let p1 = new Proxy({}, { deleteProperty() { } }); delete p1.foo; }, TypeError, "returning undefined on deleteProperty handler is return false which will throw type error", "Proxy deleteProperty handler returned false");
-            assert.throws(() => { "use strict"; let p1 = new Proxy({}, { set() { return false; } }); p1.foo = 1; }, TypeError, "set handler is returning false which will throw type error", "Proxy set handler returned false");
-            assert.throws(() => { "use strict"; let p1 = new Proxy({}, { deleteProperty() { return false; } }); delete p1.foo; }, TypeError, "deleteProperty handler is returning false which will throw type error", "Proxy deleteProperty handler returned false");
+            assert.throws(() => { "use strict"; let p1 = new Proxy({}, { set() { } }); p1.foo = 1; }, TypeError, "returning undefined on set handler is return false which will throw type error", "Proxy 'set' handler returned falsish for property 'foo'");
+            assert.throws(() => { "use strict"; let p1 = new Proxy({}, { deleteProperty() { } }); delete p1.foo; }, TypeError, "returning undefined on deleteProperty handler is return false which will throw type error", "Proxy 'deleteProperty' handler returned falsish for property 'foo'");
+            assert.throws(() => { "use strict"; let p1 = new Proxy({}, { set() { return false; } }); p1.foo = 1; }, TypeError, "set handler is returning false which will throw type error", "Proxy 'set' handler returned falsish for property 'foo'");
+            assert.throws(() => { "use strict"; let p1 = new Proxy({}, { deleteProperty() { return false; } }); delete p1.foo; }, TypeError, "deleteProperty handler is returning false which will throw type error", "Proxy 'deleteProperty' handler returned falsish for property 'foo'");
+
+            const proxy = new Proxy({}, {
+                defineProperty() {
+                    return false;
+                }
+            });
+            assert.doesNotThrow(() => {
+                proxy.a = {};
+            }, "Set property in NON-strict mode does NOT throw if trap returns falsy");
+            assert.throws(() => {
+                "use strict";
+                proxy.b = {};
+            }, TypeError, "Set property in strict mode does DOES throw if trap returns falsy");
+            assert.throws(() => {
+                Object.defineProperty(proxy, "c", {
+                    value: {}
+                });
+            }, TypeError, "Calling 'Object.defineProperty' throws if trap returns falsy");
         }
     },
     {