Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure internal slots are used when available #231

Closed
littledan opened this issue Oct 30, 2019 · 33 comments
Closed

Ensure internal slots are used when available #231

littledan opened this issue Oct 30, 2019 · 33 comments

Comments

@littledan
Copy link
Member

I see these being used effectively in some cases, but in other cases, there are ordinary Gets being applied to objects, in a way which I think deviates from our typical conventions in ECMAScript. For example, in the ToPartialDate, ToPartialTime, and ToDurationLike algorithms: If these are passed a Date, Time or Duration, the internal slots should be directly used.

@Ms2ger
Copy link
Collaborator

Ms2ger commented Oct 30, 2019

Would this affect anything besides date.with(otherDate) and similar? That seems like a very not-useful thing to do, so I don't know if we should add extra overhead to support it.

@littledan
Copy link
Member Author

Sorry, I probably made the original post too strong. If people think it's better as is, I'm fine with dropping this point. But I'm just suggesting to follow the convention @jswalden established in Intl.Locale. I'm not sure if it would be more overhead in practice...

@gibson042
Copy link
Collaborator

One area where direct use of internal slots should be avoided is time zone lookup, so that e.g. Temporal.Absolute.from("2020-01-01T00:00Z").inTimeZone("America/Sao_Paulo") and Temporal.Absolute.from("2019-12-31T22:00-02:00[America/Sao_Paulo]") cannot bypass security- or testing-motivated attenuation by detecting whether host tzdata includes Daylight Saving Time for Brazil (cf. https://www.timeanddate.com/news/time/brazil-scraps-dst.html ).

@ljharb
Copy link
Member

ljharb commented Nov 15, 2019

@gibson042 can you elaborate on that? it seems like use of internal slots is unrelated to whether it's possible to observe inconsistencies in timezone data.

@gibson042
Copy link
Collaborator

If mapping IANA names to time zone data always goes through a code-exposed interface, then attenuation is possible by simply replacing the interface. Otherwise, it requires replacing every function that can accept a time zone name anywhere in its input.

@ljharb
Copy link
Member

ljharb commented Nov 15, 2019

I think it's totally reasonable to need to replace all of Temporal in order to control the specific data that's exposed.

@gibson042
Copy link
Collaborator

Aside from being a forward compatibility hazard (even if you did replace every function, what happens when a host starts shipping Temporal v2 with new functions?), that burden is so great that it basically incentivizes suppressing Temporal altogether, as SES currently does with Intl—it's like requiring full replacement of Error to hide stack traces or full replacement of Math to prevent use of Math.random().

@ljharb
Copy link
Member

ljharb commented Nov 16, 2019

Can you elaborate on why you'd need to attenuate the timezone data in this fashion?

@gibson042
Copy link
Collaborator

Two reasons, both mentioned in the original comment.

  1. Testing: Validate that Temporal-dependent code behaves correctly when running in a specific time zone and/or before/after a tzdata update.
  2. Security: Deny untrusted code access to nondeterminism (which is potentially exploitable in unforeseen ways).

@ljharb
Copy link
Member

ljharb commented Nov 16, 2019

For the former, it seems like it’d be better to have an explicit way for users to inject replacement timezone data - and for the latter, how would it be nondeterminism if it’s always right or always wrong for a given api in a given timezone? Can you help me understand the attack vector here?

@gibson042
Copy link
Collaborator

"an explicit way for users to inject replacement timezone data" is precisely what I'm asking for. And the nondeterminism in question is unpredictable output from code like Temporal.Absolute.from("2020-01-01T00:00Z").inTimeZone("America/Sao_Paulo"), which AFAICT isn't guaranteed to be the same from one call to the next in the same active agent evaluation (e.g., a tzdata update may be visible from inside running code). But even if it were guaranteed, that's still cross-agent nondeterminism and still potentially exploitable—attacks can be very clever, such as choosing to hide theirselves when conditions suggest that the host is under developer control rather than running on a real-world target. SES hint at this in their documentation of intlMode (which was actually removed this past February):

intlMode: 'allow': The platform normally supplies a default locale, for use in Intl.DateTimeFormat and Intl.NumberFormat calls that don't supply a specific locale to use. This platform locale introduces nondeterminism, so these must be disabled… We may be able to bring back most of Intl by default, but platforms currently appear to supply the platform-default locale even to calls that supply a specific one, if the requested locale is not available (e.g. Intl.NumberFormat('es') will return the default locale's formatter function if it doesn't have a Spanish one available), which will take more work to tame.

@littledan
Copy link
Member Author

@gibson042 I'm confused by your comment. How are you imagining that this would work? It sounds like you're making a much bigger feature request here than what I was getting at when I opened this issue. I'm not convinced that we should go in this direction, but I'm not even sure what you're proposing. I want to suggest that we pursue this in a separate issue where you lay out out from the beginning.

@ptomato
Copy link
Collaborator

ptomato commented Jan 23, 2020

I'm starting to take a look at this issue. In many of the cases where we apply Get to an object where we could get its internal slots instead, it's in operations like ToPartialDate, ToPartialDateTime, etc. which are only used in Date.with(), DateTime.with(), etc. While we certainly could change the spec to use internal slots if available, why would any client code be calling dateObj.with(otherDateObj) when they just could use otherDateObj anyway? Maybe we should check this case earlier in the with() method, and just return otherDateObj in that case?

However, the problem as originally described does occur for ToDurationLike and ToTimeDurationLike, which are used in all the plus() and minus() methods, where it would make sense to pass a real Duration object a lot of the time.

@ptomato
Copy link
Collaborator

ptomato commented Jan 23, 2020

(And I'm also wondering... if you call dateTimeObj.with(someDateObj), shouldn't it be consistent with otherDateObj.with(someDateObj)? i.e. either in both cases the argument's internal slots are fetched, or in both cases Get is called? Otherwise you could get weird code like this...

const dateTimeObj = Temporal.DateTime.from('2020-01-22T14:30:00');
const someDateObj = Temporal.Date.from('1999-12-31');
const otherDateObj = Temporal.Date.from('2020-01-22');
Object.defineProperty(someDateObj, 'year', {
   get() { return 2000; }
});  // spec doesn't say the property is non-configurable...
log(dateTimeObj.with(someDateObj));  // does Get => '2000-12-31T14:30:00'
log(otherDateObj.with(someDateObj));  // reads internal slot => '1999-12-31'

@ljharb
Copy link
Member

ljharb commented Jan 23, 2020

It seems like I’d want to do that often, to clone/convert one type of another type, creating a distinct object?

@littledan
Copy link
Member Author

littledan commented Jan 23, 2020

I thought the API for such cloning was Temporal.Date.from(otherDate), and that with was more about changing just certain properties. But still, I might expect that with would "work" anyway.

@ptomato
Copy link
Collaborator

ptomato commented Jan 23, 2020

It seems like I’d want to do that often, to clone/convert one type of another type, creating a distinct object?

True, I was thinking that it wouldn't be useful to clone an immutable Date object, but I guess you can set expando properties on it, and it's not immutable in that regard.

I thought the API for such cloning was Temporal.Date.from(otherDate), and that with was more about changing just certain properties. But still, I might expect that with would "work" anyway.

According to the current spec, Temporal.Date.from(otherDate) doesn't clone, it returns the original object. So I guess with is currently the only way to clone using only a method. It certainly isn't an obvious way though...

Any thoughts about the other question above, whether dateTime.with(otherDateTime) should be consistent with dateTime.with(date) in whether it accesses internal slots?

@ljharb
Copy link
Member

ljharb commented Jan 23, 2020

@ptomato even if immutable and frozen, it still has identity (===, Object.is, Map/WeakMap keys, Set/WeakSet membership) and it's important to be able to easily create a new identity.

@ptomato
Copy link
Collaborator

ptomato commented Feb 5, 2020

Anyone have an opinion on the pathological code I wrote in #231 (comment) ? I think that question is the only thing blocking this pull request.

@Ms2ger
Copy link
Collaborator

Ms2ger commented Feb 6, 2020

For with, I still don't see the point in using internal slots at all, given how little sense it makes to call them with an existing Temporal object.

For plus/minus, I'm finding it hard to come to a conclusion.

@ptomato
Copy link
Collaborator

ptomato commented Feb 6, 2020

I think I agree with #341 since with is such an un-obvious way to clone objects. If we go with that, then it would be best to use Get in with.

@littledan
Copy link
Member Author

OK, now that the scope of where we do coercion is reduced, and also that we're allowing so much dynamicism with calendars and timezones, I agree that it makes sense to use Get in these cases.

@ptomato
Copy link
Collaborator

ptomato commented Feb 27, 2020

Discussed in Feb. 27 meeting, closing this accordingly.

@ptomato ptomato closed this as completed Feb 27, 2020
@ljharb
Copy link
Member

ljharb commented Feb 27, 2020

Please elaborate? I very much want intrinsic data for a Temporal instance to be stored with internal slots, and only exposed via accessors/methods on the prototype.

@ptomato
Copy link
Collaborator

ptomato commented Feb 27, 2020

Sure. The reasoning was that 1) if we do #341 then it wouldn't be useful to call someDate.with(otherDate) anyway, and internal slots are already used in methods like withTime(); and 2) internal slots are already used for plus() and minus() since #362 (spec) and #385 (implementation) landed.

@ljharb
Copy link
Member

ljharb commented Feb 27, 2020

Why wouldn't it be useful to call someDate.with(otherDate), whether #341 is done or not?

@ptomato
Copy link
Collaborator

ptomato commented Feb 27, 2020

someDate.with(otherDate) would replace all of the fields on someDate, so you might as well use otherDate or do Date.from(otherDate) if you wanted to clone it.

@ljharb
Copy link
Member

ljharb commented Feb 27, 2020

someDate.with(someYearMonth)?

@ptomato
Copy link
Collaborator

ptomato commented Feb 27, 2020

That's similar to the question I asked in #231 (comment) about using internal slots of other Temporal types than the type whose with() method you're calling. The closed PR didn't cover that case anyway.

I can put this on the agenda for next week's call, although I guess there's also someYearMonth.withDay(someDate.day).

@Ms2ger
Copy link
Collaborator

Ms2ger commented Feb 28, 2020

Please elaborate? I very much want intrinsic data for a Temporal instance to be stored with internal slots, and only exposed via accessors/methods on the prototype.

Could you explain how this is not already the case?

@ljharb
Copy link
Member

ljharb commented Feb 28, 2020

The proposal is far too large and changes far too quickly for me to have any idea what things are like at a given moment; that an issue with this title is closed as “we don’t want this” leads me to inquire.

@pipobscure
Copy link
Collaborator

To use with() with a temporal object of the same type is a non-sensical operation, since that would only overwrite all fields.

The sensible options in terms of temporal objects would be:

  • date.with(yearMonth)
  • date.with(monthDay)
  • dateTime.with(date)
  • dateTime.with(time)
  • dateTime.with(yearMonth)
  • dateTime.with(monthDay)

while those are all possible, the more general supported case is the one with a plain javascript object. date.with({ year: 2020 }) for example. In general, we see the temporal objects very much akin to this kind of struct, albeit immutable and having a bit more functionality.

The question therefore becomes: How do these cases behave?

  • date.with({ year: 2020, month: 3 })
  • date.with(new Temporal.YearMonth(2020, 3))
  • date.with(Object.assign({}, new Temporal.YearMonth(2020, 3)))

We're looking to treat these identically and to do that we want to get the properties rather than looking at the internal slots.

We also think that the core case for using the internal slots would be the case of date.with(date) where the argument object is of the same type as the method receiver. But that's also the one case that makes no sense at all.

So while the data is actually stored in intrinsic slots that are accessed via getter properties, and where we have a "typed" interaction we do use these slots generally. The case for with does not lend itself to that philosophy.

@ljharb
Copy link
Member

ljharb commented Mar 6, 2020

Gotcha - i agree we want to Get the properties inside with, but for Temporal objects, that should result in triggering prototype accessors that read from internal slots. Thanks for clarifying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants