Skip to content

Commit cbb9b10

Browse files
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
1 parent 9e8139e commit cbb9b10

File tree

7 files changed

+68
-26
lines changed

7 files changed

+68
-26
lines changed

lib/Parser/rterrors.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
//-------------------------------------------------------------------------------------------------------
22
// Copyright (C) Microsoft Corporation and contributors. All rights reserved.
3-
// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
3+
// Copyright (c) ChakraCore Project Contributors. All rights reserved.
44
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
55
//-------------------------------------------------------------------------------------------------------
66
#define IDS_COMPILATION_ERROR_SOURCE 4096
@@ -371,7 +371,7 @@ RT_ERROR_MSG(JSERR_InvalidIteratorObject, 5672, "%s : Invalid iterator object",
371371
RT_ERROR_MSG(JSERR_NoAccessors, 5673, "Invalid property descriptor: accessors not supported on this object", "", kjstTypeError, 0)
372372
RT_ERROR_MSG(JSERR_RegExpInvalidEscape, 5674, "", "Invalid regular expression: invalid escape in unicode pattern", kjstSyntaxError, 0)
373373
RT_ERROR_MSG(JSERR_RegExpTooManyCapturingGroups, 5675, "", "Regular expression cannot have more than 32,767 capturing groups", kjstRangeError, 0)
374-
RT_ERROR_MSG(JSERR_ProxyHandlerReturnedFalse, 5676, "Proxy %s handler returned false", "Proxy handler returned false", kjstTypeError, 0)
374+
RT_ERROR_MSG(JSERR_ProxyHandlerReturnedFalse, 5676, "Proxy '%s' handler returned falsish for property '%s'", "Proxy handler returned false", kjstTypeError, 0)
375375
RT_ERROR_MSG(JSERR_UnicodeRegExpRangeContainsCharClass, 5677, "%s", "Character classes not allowed in a RegExp class range.", kjstSyntaxError, 0)
376376
RT_ERROR_MSG(JSERR_DuplicateKeysFromOwnPropertyKeys, 5678, "%s", "Proxy's ownKeys trap returned duplicate keys", kjstTypeError, 0)
377377
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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9101,14 +9101,14 @@ using namespace Js;
91019101
// Return value:
91029102
// - TRUE = success.
91039103
// - FALSE (can throw depending on throwOnError parameter) = unsuccessful.
9104-
BOOL JavascriptOperators::DefineOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* scriptContext)
9104+
BOOL JavascriptOperators::DefineOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* scriptContext, PropertyOperationFlags flags /* = Js::PropertyOperation_None */)
91059105
{
91069106
Assert(obj);
91079107
Assert(scriptContext);
91089108

91099109
if (VarIs<JavascriptProxy>(obj))
91109110
{
9111-
return JavascriptProxy::DefineOwnPropertyDescriptor(obj, propId, descriptor, throwOnError, scriptContext);
9111+
return JavascriptProxy::DefineOwnPropertyDescriptor(obj, propId, descriptor, throwOnError, scriptContext, flags);
91129112
}
91139113
#ifdef _CHAKRACOREBUILD
91149114
else if (VarIs<CustomExternalWrapperObject>(obj))

lib/Runtime/Language/JavascriptOperators.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
//-------------------------------------------------------------------------------------------------------
22
// Copyright (C) Microsoft. All rights reserved.
3+
// Copyright (c) ChakraCore Project Contributors. All rights reserved.
34
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
45
//-------------------------------------------------------------------------------------------------------
56
#pragma once
@@ -612,7 +613,7 @@ namespace Js
612613
static Var FromPropertyDescriptor(const PropertyDescriptor& descriptor, ScriptContext* scriptContext);
613614
static void CompletePropertyDescriptor(PropertyDescriptor* resultDescriptor, PropertyDescriptor* likePropertyDescriptor, ScriptContext* requestContext);
614615
static BOOL SetPropertyDescriptor(RecyclableObject* object, PropertyId propId, const PropertyDescriptor& descriptor);
615-
static BOOL DefineOwnPropertyDescriptor(RecyclableObject* object, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* scriptContext);
616+
static BOOL DefineOwnPropertyDescriptor(RecyclableObject* object, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* scriptContext, PropertyOperationFlags flags = Js::PropertyOperation_None);
616617
static BOOL DefineOwnPropertyForArray(JavascriptArray* arr, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* scriptContext);
617618

618619
static BOOL DefineOwnPropertyForTypedArray(TypedArrayBase * typedArray, PropertyId propId, const PropertyDescriptor & descriptor, bool throwOnError, ScriptContext * scriptContext);

lib/Runtime/Library/JavascriptObject.cpp

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
//-------------------------------------------------------------------------------------------------------
22
// Copyright (C) Microsoft. All rights reserved.
3-
// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
3+
// Copyright (c) ChakraCore Project Contributors. All rights reserved.
44
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
55
//-------------------------------------------------------------------------------------------------------
66
#include "RuntimeLibraryPch.h"
@@ -1349,7 +1349,11 @@ Var JavascriptObject::EntryDefineProperty(RecyclableObject* function, CallInfo c
13491349
ModifyGetterSetterFuncName(propertyRecord, propertyDescriptor, scriptContext);
13501350
}
13511351

1352-
DefineOwnPropertyHelper(obj, propertyRecord->GetPropertyId(), propertyDescriptor, scriptContext);
1352+
BOOL success = DefineOwnPropertyHelper(obj, propertyRecord->GetPropertyId(), propertyDescriptor, scriptContext);
1353+
if (!success)
1354+
{
1355+
JavascriptError::ThrowTypeError(scriptContext, JSERR_DefineProperty_Default, scriptContext->GetPropertyName(propertyRecord->GetPropertyId())->GetBuffer());
1356+
}
13531357

13541358
return obj;
13551359
}
@@ -2174,7 +2178,7 @@ BOOL JavascriptObject::DefineOwnPropertyHelper(RecyclableObject* obj, PropertyId
21742178
// TODO: implement DefineOwnProperty for other object built-in exotic types.
21752179
else
21762180
{
2177-
returnValue = JavascriptOperators::DefineOwnPropertyDescriptor(obj, propId, descriptor, throwOnError, scriptContext);
2181+
returnValue = JavascriptOperators::DefineOwnPropertyDescriptor(obj, propId, descriptor, throwOnError, scriptContext, Js::PropertyOperation_StrictMode);
21782182
if (propId == PropertyIds::__proto__)
21792183
{
21802184
scriptContext->GetLibrary()->GetObjectPrototypeObject()->PostDefineOwnProperty__proto__(obj);

lib/Runtime/Library/JavascriptProxy.cpp

Lines changed: 31 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
//-------------------------------------------------------------------------------------------------------
22
// Copyright (C) Microsoft. All rights reserved.
3-
// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
3+
// Copyright (c) ChakraCore Project Contributors. All rights reserved.
44
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
55
//-------------------------------------------------------------------------------------------------------
66
#include "RuntimeLibraryPch.h"
@@ -677,7 +677,7 @@ namespace Js
677677
resultDescriptor.SetWritable(true);
678678
resultDescriptor.SetEnumerable(true);
679679
resultDescriptor.SetValue(value);
680-
return Js::JavascriptOperators::DefineOwnPropertyDescriptor(this, propertyId, resultDescriptor, true, requestContext);
680+
return Js::JavascriptOperators::DefineOwnPropertyDescriptor(this, propertyId, resultDescriptor, true, requestContext, flags);
681681
}
682682
else
683683
{
@@ -698,7 +698,7 @@ namespace Js
698698

699699
proxyPropertyDescriptor.SetValue(value);
700700
proxyPropertyDescriptor.SetOriginal(nullptr);
701-
return Js::JavascriptOperators::DefineOwnPropertyDescriptor(this, propertyId, proxyPropertyDescriptor, true, requestContext);
701+
return Js::JavascriptOperators::DefineOwnPropertyDescriptor(this, propertyId, proxyPropertyDescriptor, true, requestContext, flags);
702702
}
703703
}
704704

@@ -827,7 +827,12 @@ namespace Js
827827
{
828828
if (flags & PropertyOperation_StrictMode)
829829
{
830-
JavascriptError::ThrowTypeError(requestContext, JSERR_ProxyHandlerReturnedFalse, _u("deleteProperty"));
830+
JavascriptError::ThrowTypeErrorVar(
831+
requestContext,
832+
JSERR_ProxyHandlerReturnedFalse,
833+
_u("deleteProperty"),
834+
threadContext->GetPropertyName(propertyId)->GetBuffer()
835+
);
831836
}
832837
return trapResult;
833838
}
@@ -1695,7 +1700,7 @@ namespace Js
16951700
}
16961701

16971702

1698-
BOOL JavascriptProxy::DefineOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* requestContext)
1703+
BOOL JavascriptProxy::DefineOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* requestContext, PropertyOperationFlags flags)
16991704
{
17001705
// #sec-proxy-object-internal-methods-and-internal-slots-defineownproperty-p-desc
17011706
PROBE_STACK(requestContext, Js::Constants::MinStackDefault);
@@ -1735,7 +1740,7 @@ namespace Js
17351740
Assert(!requestContext->IsHeapEnumInProgress());
17361741
if (nullptr == defineOwnPropertyMethod)
17371742
{
1738-
return JavascriptOperators::DefineOwnPropertyDescriptor(targetObj, propId, descriptor, throwOnError, requestContext);
1743+
return JavascriptOperators::DefineOwnPropertyDescriptor(targetObj, propId, descriptor, throwOnError, requestContext, flags);
17391744
}
17401745

17411746
//8. Let descObj be FromPropertyDescriptor(Desc).
@@ -1754,6 +1759,15 @@ namespace Js
17541759
BOOL defineResult = JavascriptConversion::ToBoolean(definePropertyResult, requestContext);
17551760
if (!defineResult)
17561761
{
1762+
if (throwOnError && flags & PropertyOperation_StrictMode)
1763+
{
1764+
JavascriptError::ThrowTypeErrorVar(
1765+
requestContext,
1766+
JSERR_ProxyHandlerReturnedFalse,
1767+
_u("defineProperty"),
1768+
requestContext->GetPropertyName(propId)->GetBuffer()
1769+
);
1770+
}
17571771
return defineResult;
17581772
}
17591773

@@ -1847,23 +1861,23 @@ namespace Js
18471861
uint32 indexVal;
18481862
BOOL isNumericPropertyId = requestContext->IsNumericPropertyId(propertyId, &indexVal);
18491863
Assert(isNumericPropertyId);
1850-
return JavascriptOperators::SetItemOnTaggedNumber(receiver, targetObj, indexVal, newValue, requestContext, PropertyOperationFlags::PropertyOperation_None);
1864+
return JavascriptOperators::SetItemOnTaggedNumber(receiver, targetObj, indexVal, newValue, requestContext, propertyOperationFlags);
18511865
}
18521866
case SetPropertyTrapKind::SetPropertyOnTaggedNumberKind:
1853-
return JavascriptOperators::SetPropertyOnTaggedNumber(receiver, targetObj, propertyId, newValue, requestContext, PropertyOperation_None);
1867+
return JavascriptOperators::SetPropertyOnTaggedNumber(receiver, targetObj, propertyId, newValue, requestContext, propertyOperationFlags);
18541868
case SetPropertyTrapKind::SetPropertyKind:
1855-
return JavascriptOperators::SetProperty(receiver, targetObj, propertyId, newValue, requestContext);
1869+
return JavascriptOperators::SetProperty(receiver, targetObj, propertyId, newValue, requestContext, propertyOperationFlags);
18561870
case SetPropertyTrapKind::SetItemKind:
18571871
{
18581872
uint32 indexVal;
18591873
BOOL isNumericPropertyId = requestContext->IsNumericPropertyId(propertyId, &indexVal);
18601874
Assert(isNumericPropertyId);
1861-
return JavascriptOperators::SetItem(receiver, targetObj, indexVal, newValue, requestContext, PropertyOperationFlags::PropertyOperation_None, skipPrototypeCheck);
1875+
return JavascriptOperators::SetItem(receiver, targetObj, indexVal, newValue, requestContext, propertyOperationFlags, skipPrototypeCheck);
18621876
}
18631877
case SetPropertyTrapKind::SetPropertyWPCacheKind:
18641878
{
18651879
PropertyValueInfo propertyValueInfo;
1866-
return JavascriptOperators::SetPropertyWPCache(receiver, targetObj, propertyId, newValue, requestContext, PropertyOperationFlags::PropertyOperation_None, &propertyValueInfo);
1880+
return JavascriptOperators::SetPropertyWPCache(receiver, targetObj, propertyId, newValue, requestContext, propertyOperationFlags, &propertyValueInfo);
18671881
}
18681882
default:
18691883
AnalysisAssert(FALSE);
@@ -1886,9 +1900,13 @@ namespace Js
18861900
{
18871901
if (propertyOperationFlags & PropertyOperation_StrictMode)
18881902
{
1889-
JavascriptError::ThrowTypeError(requestContext, JSERR_ProxyHandlerReturnedFalse, _u("set"));
1903+
JavascriptError::ThrowTypeErrorVar(
1904+
requestContext,
1905+
JSERR_ProxyHandlerReturnedFalse,
1906+
_u("set"),
1907+
requestContext->GetPropertyName(propertyId)->GetBuffer()
1908+
);
18901909
}
1891-
18921910
return setResult;
18931911
}
18941912

lib/Runtime/Library/JavascriptProxy.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
//-------------------------------------------------------------------------------------------------------
22
// Copyright (C) Microsoft. All rights reserved.
3+
// Copyright (c) ChakraCore Project Contributors. All rights reserved.
34
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
45
//-------------------------------------------------------------------------------------------------------
56
// Implements JavascriptProxy.
@@ -70,7 +71,7 @@ namespace Js
7071
static JavascriptProxy* Create(ScriptContext* scriptContext, Arguments args);
7172

7273
static BOOL GetOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propertyId, ScriptContext* requestContext, PropertyDescriptor* propertyDescriptor);
73-
static BOOL DefineOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* requestContext);
74+
static BOOL DefineOwnPropertyDescriptor(RecyclableObject* obj, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* requestContext, PropertyOperationFlags flags);
7475

7576
static DWORD GetOffsetOfTarget() { return offsetof(JavascriptProxy, target); }
7677

test/Bugs/misc_bugs.js

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -189,10 +189,28 @@ var tests = [
189189
{
190190
name: "Strict Mode : throw type error when the handler returns falsy value",
191191
body: function () {
192-
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");
193-
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");
194-
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");
195-
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");
192+
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'");
193+
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'");
194+
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'");
195+
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'");
196+
197+
const proxy = new Proxy({}, {
198+
defineProperty() {
199+
return false;
200+
}
201+
});
202+
assert.doesNotThrow(() => {
203+
proxy.a = {};
204+
}, "Set property in NON-strict mode does NOT throw if trap returns falsy");
205+
assert.throws(() => {
206+
"use strict";
207+
proxy.b = {};
208+
}, TypeError, "Set property in strict mode does DOES throw if trap returns falsy");
209+
assert.throws(() => {
210+
Object.defineProperty(proxy, "c", {
211+
value: {}
212+
});
213+
}, TypeError, "Calling 'Object.defineProperty' throws if trap returns falsy");
196214
}
197215
},
198216
{

0 commit comments

Comments
 (0)