From 48d512f567cc2e5305c0200fe0ad14dc15504229 Mon Sep 17 00:00:00 2001 From: viruscamp Date: Fri, 13 Oct 2023 15:06:40 +0800 Subject: [PATCH] Improve Proxy conformance and support CLR interop better (#1645) provide detailed message for exceptions in JsProxy do not use ReferenceEquals on data property value comparing improve `JsValue.SameValue` to compare `ObjectWrapper` add a new test `ProxyHandlerGetDataPropertyShouldNotCheckClrType` for `JsString` and `ConcatenatedString` --- Jint.Tests/Runtime/ProxyTests.cs | 241 ++++++++++++++++++++++++++++++- Jint/Native/JsValue.cs | 15 +- Jint/Native/Proxy/JsProxy.cs | 36 +++-- 3 files changed, 277 insertions(+), 15 deletions(-) diff --git a/Jint.Tests/Runtime/ProxyTests.cs b/Jint.Tests/Runtime/ProxyTests.cs index 25d71f3bbb..3685956996 100644 --- a/Jint.Tests/Runtime/ProxyTests.cs +++ b/Jint.Tests/Runtime/ProxyTests.cs @@ -1,4 +1,7 @@ -namespace Jint.Tests.Runtime; +using Jint.Native.Error; +using Jint.Runtime; + +namespace Jint.Tests.Runtime; public class ProxyTests { @@ -183,4 +186,240 @@ public void ConstructHandlerInvariant() Assert.True(_engine.Evaluate(Script).AsBoolean()); } + + [Fact] + public void ProxyHandlerGetDataPropertyShouldNotUseReferenceEquals() + { + // There are two JsString which should be treat as same value, + // but they are not ReferenceEquals. + _engine.Execute(""" + let o = Object.defineProperty({}, 'value', { + configurable: false, + value: 'in', + }); + const handler = { + get(target, property, receiver) { + return 'Jint'.substring(1,3); + } + }; + let p = new Proxy(o, handler); + let pv = p.value; + """); + } + + [Fact] + public void ProxyHandlerGetDataPropertyShouldNotCheckClrType() + { + // There are a JsString and a ConcatenatedString which should be treat as same value, + // but they are different CLR Type. + _engine.Execute(""" + let o = Object.defineProperty({}, 'value', { + configurable: false, + value: 'Jint', + }); + const handler = { + get(target, property, receiver) { + return 'Ji'.concat('nt'); + } + }; + let p = new Proxy(o, handler); + let pv = p.value; + """); + } + + class TestClass + { + public static readonly TestClass Instance = new TestClass(); + public string StringValue => "StringValue"; + public int IntValue => 42424242; // avoid small numbers cache + public TestClass ObjectWrapper => Instance; + } + + [Fact] + public void ProxyClrPropertyPrimitiveString() + { + _engine.SetValue("testClass", TestClass.Instance); + var result = _engine.Evaluate(""" + const handler = { + get(target, property, receiver) { + return Reflect.get(target, property, receiver); + } + }; + const p = new Proxy(testClass, handler); + return p.StringValue; + """); + Assert.Equal(TestClass.Instance.StringValue, result.AsString()); + } + + [Fact] + public void ProxyClrPropertyPrimitiveInt() + { + _engine.SetValue("testClass", TestClass.Instance); + var result = _engine.Evaluate(""" + const handler = { + get(target, property, receiver) { + return Reflect.get(target, property, receiver); + } + }; + const p = new Proxy(testClass, handler); + return p.IntValue; + """); + Assert.Equal(TestClass.Instance.IntValue, result.AsInteger()); + } + + [Fact] + public void ProxyClrPropertyObjectWrapper() + { + _engine.SetValue("testClass", TestClass.Instance); + var result = _engine.Evaluate(""" + const handler = { + get(target, property, receiver) { + return Reflect.get(target, property, receiver); + } + }; + const p = new Proxy(testClass, handler); + return p.ObjectWrapper; + """); + } + + private static ErrorPrototype TypeErrorPrototype(Engine engine) + => engine.Realm.Intrinsics.TypeError.PrototypeObject; + + private static void AssertJsTypeError(Engine engine, JavaScriptException ex, string msg) + { + Assert.Same(TypeErrorPrototype(engine), ex.Error.AsObject().Prototype); + Assert.Equal(msg, ex.Message); + } + + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/Proxy/get#invariants + // The value reported for a property must be the same as + // the value ofthe corresponding target object property, + // if the target object property is + // a non-writable, non-configurable own data property. + [Fact] + public void ProxyHandlerGetInvariantsDataPropertyReturnsDifferentValue() + { + _engine.Execute(""" + let o = Object.defineProperty({}, 'value', { + writable: false, + configurable: false, + value: 42, + }); + const handler = { + get(target, property, receiver) { + return 32; + } + }; + let p = new Proxy(o, handler); + """); + var ex = Assert.Throws(() => _engine.Evaluate("p.value")); + AssertJsTypeError(_engine, ex, "'get' on proxy: property 'value' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value (expected '42' but got '32')"); + } + + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/Proxy/get#invariants + // The value reported for a property must be undefined, + // if the corresponding target object property is + // a non-configurable own accessor property + // that has undefined as its [[Get]] attribute. + [Fact] + public void ProxyHandlerGetInvariantsAccessorPropertyWithoutGetButReturnsValue() + { + _engine.Execute(""" + let o = Object.defineProperty({}, 'value', { + configurable: false, + set() {}, + }); + const handler = { + get(target, property, receiver) { + return 32; + } + }; + let p = new Proxy(o, handler); + """); + var ex = Assert.Throws(() => _engine.Evaluate("p.value")); + AssertJsTypeError(_engine, ex, "'get' on proxy: property 'value' is a non-configurable accessor property on the proxy target and does not have a getter function, but the trap did not return 'undefined' (got '32')"); + } + + private const string ScriptProxyHandlerSetInvariantsDataPropertyImmutable = """ + let o = Object.defineProperty({}, 'value', { + writable: false, + configurable: false, + value: 42, + }); + const handler = { + set(target, property, value, receiver) { + return true; + } + }; + let p = new Proxy(o, handler); + """; + + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/Proxy/set#invariants + // Cannot change the value of a property to be different from + // the value of the corresponding target object property, + // if the corresponding target object property is + // a non-writable, non-configurable data property. + [Fact] + public void ProxyHandlerSetInvariantsDataPropertyImmutableChangeValue() + { + _engine.Execute(ScriptProxyHandlerSetInvariantsDataPropertyImmutable); + var ex = Assert.Throws(() => _engine.Evaluate("p.value = 32")); + AssertJsTypeError(_engine, ex, "'set' on proxy: trap returned truish for property 'value' which exists in the proxy target as a non-configurable and non-writable data property with a different value"); + } + + [Fact] + public void ProxyHandlerSetInvariantsDataPropertyImmutableSetSameValue() + { + _engine.Execute(ScriptProxyHandlerSetInvariantsDataPropertyImmutable); + _engine.Evaluate("p.value = 42"); + } + + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/Proxy/set#invariants + // Cannot set the value of a property, + // if the corresponding target object property is + // a non-configurable accessor property + // that has undefined as its [[Set]] attribute. + [Fact] + public void ProxyHandlerSetInvariantsAccessorPropertyWithoutSetChange() + { + _engine.Execute(""" + let o = Object.defineProperty({}, 'value', { + configurable: false, + get() { return 42; }, + }); + const handler = { + set(target, property, value, receiver) { + return true; + } + }; + let p = new Proxy(o, handler); + """); + var ex = Assert.Throws(() => _engine.Evaluate("p.value = 42")); + AssertJsTypeError(_engine, ex, "'set' on proxy: trap returned truish for property 'value' which exists in the proxy target as a non-configurable and non-writable accessor property without a setter"); + } + + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/Proxy/set#invariants + // In strict mode, a false return value from the set() handler + // will throw a TypeError exception. + [Fact] + public void ProxyHandlerSetInvariantsReturnsFalseInStrictMode() + { + var ex = Assert.Throws(() => _engine.Evaluate(""" + 'use strict'; + let p = new Proxy({}, { set: () => false }); + p.value = 42; + """)); + // V8: "'set' on proxy: trap returned falsish for property 'value'", + AssertJsTypeError(_engine, ex, "Cannot assign to read only property 'value' of [object Object]"); + } + + [Fact] + public void ProxyHandlerSetInvariantsReturnsFalseInNonStrictMode() + { + _engine.Evaluate(""" + // 'use strict'; + let p = new Proxy({}, { set: () => false }); + p.value = 42; + """); + } } diff --git a/Jint/Native/JsValue.cs b/Jint/Native/JsValue.cs index 6499e0d58d..23cb199e8a 100644 --- a/Jint/Native/JsValue.cs +++ b/Jint/Native/JsValue.cs @@ -9,6 +9,7 @@ using Jint.Native.Object; using Jint.Native.Symbol; using Jint.Runtime; +using Jint.Runtime.Interop; namespace Jint.Native { @@ -465,6 +466,11 @@ internal virtual bool OrdinaryHasInstance(JsValue v) internal static bool SameValue(JsValue x, JsValue y) { + if (ReferenceEquals(x, y)) + { + return true; + } + var typea = x.Type; var typeb = y.Type; @@ -510,8 +516,15 @@ internal static bool SameValue(JsValue x, JsValue y) return true; case Types.Symbol: return x == y; + case Types.Object: + if (x is ObjectWrapper xo && y is ObjectWrapper yo) + { + return ReferenceEquals(xo.Target, yo.Target) + && xo.ClrType == yo.ClrType; + } + return false; default: - return ReferenceEquals(x, y); + return false; } } diff --git a/Jint/Native/Proxy/JsProxy.cs b/Jint/Native/Proxy/JsProxy.cs index 463cf05529..1607632b32 100644 --- a/Jint/Native/Proxy/JsProxy.cs +++ b/Jint/Native/Proxy/JsProxy.cs @@ -2,6 +2,7 @@ using Jint.Native.Object; using Jint.Runtime; using Jint.Runtime.Descriptors; +using Jint.Runtime.Interop; namespace Jint.Native.Proxy { @@ -134,13 +135,21 @@ public override JsValue Get(JsValue property, JsValue receiver) var targetDesc = target.GetOwnProperty(property); if (targetDesc != PropertyDescriptor.Undefined) { - if (targetDesc.IsDataDescriptor() && !targetDesc.Configurable && !targetDesc.Writable && !ReferenceEquals(result, targetDesc._value)) + if (targetDesc.IsDataDescriptor()) { - ExceptionHelper.ThrowTypeError(_engine.Realm); + var targetValue = targetDesc.Value; + if (!targetDesc.Configurable && !targetDesc.Writable && !SameValue(result, targetValue)) + { + ExceptionHelper.ThrowTypeError(_engine.Realm, $"'get' on proxy: property '{property}' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value (expected '{targetValue}' but got '{result}')"); + } } - if (targetDesc.IsAccessorDescriptor() && !targetDesc.Configurable && (targetDesc.Get ?? Undefined).IsUndefined() && !result.IsUndefined()) + + if (targetDesc.IsAccessorDescriptor()) { - ExceptionHelper.ThrowTypeError(_engine.Realm, $"'get' on proxy: property '{property}' is a non-configurable accessor property on the proxy target and does not have a getter function, but the trap did not return 'undefined' (got '{result}')"); + if (!targetDesc.Configurable && (targetDesc.Get ?? Undefined).IsUndefined() && !result.IsUndefined()) + { + ExceptionHelper.ThrowTypeError(_engine.Realm, $"'get' on proxy: property '{property}' is a non-configurable accessor property on the proxy target and does not have a getter function, but the trap did not return 'undefined' (got '{result}')"); + } } } @@ -152,7 +161,7 @@ public override JsValue Get(JsValue property, JsValue receiver) /// public override List GetOwnPropertyKeys(Types types = Types.None | Types.String | Types.Symbol) { - if (!TryCallHandler(TrapOwnKeys, new JsValue[] { _target }, out var result)) + if (!TryCallHandler(TrapOwnKeys, new[] { _target }, out var result)) { return _target.GetOwnPropertyKeys(types); } @@ -321,14 +330,15 @@ public override bool Set(JsValue property, JsValue value, JsValue receiver) return false; } - var targetDesc = _target.GetOwnProperty(property); + var targetDesc = _target.GetOwnProperty(property); if (targetDesc != PropertyDescriptor.Undefined) { if (targetDesc.IsDataDescriptor() && !targetDesc.Configurable && !targetDesc.Writable) { - if (targetDesc.Value != value) + var targetValue = targetDesc.Value; + if (!SameValue(targetValue, value)) { - ExceptionHelper.ThrowTypeError(_engine.Realm); + ExceptionHelper.ThrowTypeError(_engine.Realm, $"'set' on proxy: trap returned truish for property '{property}' which exists in the proxy target as a non-configurable and non-writable data property with a different value"); } } @@ -336,7 +346,7 @@ public override bool Set(JsValue property, JsValue value, JsValue receiver) { if ((targetDesc.Set ?? Undefined).IsUndefined()) { - ExceptionHelper.ThrowTypeError(_engine.Realm); + ExceptionHelper.ThrowTypeError(_engine.Realm, $"'set' on proxy: trap returned truish for property '{property}' which exists in the proxy target as a non-configurable and non-writable accessor property without a setter"); } } } @@ -405,7 +415,7 @@ private static bool IsCompatiblePropertyDescriptor(bool extensible, PropertyDesc /// public override bool HasProperty(JsValue property) { - if (!TryCallHandler(TrapHas, new [] { _target, TypeConverter.ToPropertyKey(property) }, out var jsValue)) + if (!TryCallHandler(TrapHas, new[] { _target, TypeConverter.ToPropertyKey(property) }, out var jsValue)) { return _target.HasProperty(property); } @@ -474,7 +484,7 @@ public override bool Delete(JsValue property) /// public override bool PreventExtensions() { - if (!TryCallHandler(TrapPreventExtensions, new JsValue[] { _target }, out var result)) + if (!TryCallHandler(TrapPreventExtensions, new[] { _target }, out var result)) { return _target.PreventExtensions(); } @@ -496,7 +506,7 @@ public override bool Extensible { get { - if (!TryCallHandler(TrapIsExtensible, new JsValue[] { _target }, out var result)) + if (!TryCallHandler(TrapIsExtensible, new[] { _target }, out var result)) { return _target.Extensible; } @@ -516,7 +526,7 @@ public override bool Extensible /// protected internal override ObjectInstance? GetPrototypeOf() { - if (!TryCallHandler(TrapGetProtoTypeOf, new JsValue[] { _target }, out var handlerProto )) + if (!TryCallHandler(TrapGetProtoTypeOf, new[] { _target }, out var handlerProto)) { return _target.Prototype; }