Skip to content

Commit

Permalink
Throw TypeError if [[DefineOwnProperty]] returns false (chakra-co…
Browse files Browse the repository at this point in the history 
…re#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 chakra-core#6505).

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

Fixes chakra-core#6505
  • Loading branch information
@ShortDevelopment
ShortDevelopment authored Jan 25, 2023
1 parent 9e8139e commit cbb9b10
Show file tree
Hide file tree
Showing 7 changed files with 68 additions and 26 deletions.
4 changes: 2 additions & 2 deletions lib/Parser/rterrors.h
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions lib/Runtime/Language/JavascriptOperators.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9101,14 +9101,14 @@ using namespace Js;
// 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))
Expand Down
3 changes: 2 additions & 1 deletion lib/Runtime/Language/JavascriptOperators.h
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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);
Expand Down
10 changes: 7 additions & 3 deletions lib/Runtime/Library/JavascriptObject.cpp
View file
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down
44 changes: 31 additions & 13 deletions lib/Runtime/Library/JavascriptProxy.cpp
View file
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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
{
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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).
Expand All @@ -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;
}

Expand Down Expand Up @@ -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);
Expand All @@ -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;
}

Expand Down
3 changes: 2 additions & 1 deletion lib/Runtime/Library/JavascriptProxy.h
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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); }

Expand Down
26 changes: 22 additions & 4 deletions test/Bugs/misc_bugs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
},
{
Expand Down

0 comments on commit cbb9b10

Please # to comment.