Skip to content
New issue

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

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

Already on GitHub? # to your account

Account for botManagement proxy in structuredClone #566

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 24, 2023

This is ugly and horrible but hopefully temporary. While we are logging the cf.botManagement uses, since we are using a Proxy to catch accesses and Proxy is not supported via structuredClone, we end up having to intercept and handle the case where request.cf is passed off to structuredClone.

It's possible we might have to do this elsewhere if folks are passing request.cf off to other things that using the v8 serializer? I certainly hope not.

@jasnell jasnell requested review from jclee and kentonv April 24, 2023 20:03
@KianNH
Copy link
Contributor

KianNH commented Apr 24, 2023

I believe the DO Storage & Queues bindings both use the V8 serializer directly, rather than going through the structuredClone() codepath. i.e:

// Use a specific serialization version to avoid sending messages using a new version before all
// runtimes at the edge know how to read it.
jsg::Serializer serializer(isolate, jsg::Serializer::Options {
.version = 15,
.omitHeader = false,
});
serializer.write(body);
kj::Array<kj::byte> serialized = serializer.release().data;

kj::Array<kj::byte> serializeV8Value(v8::Local<v8::Value> value, v8::Isolate* isolate) {
jsg::Serializer serializer(isolate, jsg::Serializer::Options {
.version = 15,
.omitHeader = false,
});
serializer.write(value);
auto released = serializer.release();
return kj::mv(released.data);
}

Would those be covered by this change? My bad if they already are!

@jasnell jasnell force-pushed the jsnell/james-and-his-very-bad-no-good-hack branch from 314110b to 337b5a0 Compare April 25, 2023 14:24
@jasnell
Copy link
Member Author

jasnell commented Apr 25, 2023

@KianNH ... yeah, good point. Ok, I've updated the PR to account for the additional places. Hopefully this will be temporary and we can pull this back out soon.

Copy link
Contributor

@jclee jclee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me (although I'm not sure I would have necessarily caught the additional serializer uses).

This is ugly and horrible but hopefully temporary. While we are logging
the cf.botManagement uses, since we are using a Proxy to catch accesses
and Proxy is not supported via structuredClone, we end up having to
intercept and handle the case where `request.cf` is passed off to
structuredClone.

It's *possible* we might have to do this elsewhere if folks are
passing request.cf off to other things that using the v8 serializer?
I certainly hope not.
@jasnell jasnell force-pushed the jsnell/james-and-his-very-bad-no-good-hack branch from 337b5a0 to 57ca2bc Compare April 26, 2023 14:07
@jasnell jasnell merged commit ada2dd0 into main Apr 26, 2023
@kentonv
Copy link
Member

kentonv commented Apr 26, 2023

TBH I would rather just remove the botManagement logging at this point. It seems like it's just not workable. I don't like that this PR adds expense to every serialization operation, and meanwhile it seems very possible that more obscure stuff is broken.

@kentonv kentonv deleted the jsnell/james-and-his-very-bad-no-good-hack branch April 26, 2023 16:34
@kentonv
Copy link
Member

kentonv commented Apr 26, 2023

I suppose this hack only even covers the case where the request.cf object is being serialized. If you place request.cf inside a larger object and serialize that larger object, there is still a problem, I think?

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

Successfully merging this pull request may close these issues.

4 participants