[Extensions] Match and dispatch webRequest listeners in JS [chromium/src : main]
woxxom (Gerrit)
woxxom added 6 comments![]( "Open in Gerrit")
FWIW the entire extensions js shim isn't immune to prototype hijacks which can be fixed using a few simple tweaks. I've listed several examples here. This can be also codified via eslint's `no-restricted-syntax` rule for future-proofing.
$Array.push(result, headers[i]);$Array.push will trigger a setter on Array.prototype for indexes. The only safe push method is, AFAIK, `Object.defineProperty(arr, arr.length, {__proto__: null, value: v, configurable: true, enumerable: true, writable: true})` (note that the options object is proto-less too to avoid hijacking of non-specified properties via Object.prototype)
var leafCopy = $Object.assign({}, certificates[0]);This will trigger setters on Object.prototype. Use `{...certificates[0]}` which clones the property descriptors.
if (copy.requestHeaders) {This will trigger Object.prototype getter. The solution is to either use `$Object.hasOwnProperty` or null the prototype `var copy={__proto__:null, ...details}` and optionally restore it at the end via `$Object.setPrototypeOf(copy, $Object.getPrototypeOf({}))`
$Function.apply(entry.callback, null, [listenerDetails, handledCallback]);Can be simplified using the standard method used by webpack and others: `(0,entry.callback)(listenerDetails, handledCallback)`
var entry = listenersById[matchingIds[i]];This will trigger a getter on Array.prototype on out-of-bounds. To avoid it, check matchingIds[i]<listenersById.length
Related details
- Andrea Orru
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrea Orru (Gerrit)
Andrea Orru added 1 comment![]( "Open in Gerrit")
FWIW the entire extensions js shim isn't immune to prototype hijacks which can be fixed using a few simple tweaks. I've listed several examples here. This can be also codified via eslint's `no-restricted-syntax` rule for future-proofing.
Thank you for your comments! They are valid, and the eslint rule is a good tip.
I'm aware of prototype hijacks. As a general rule, if one of my CLs is tagged as `WIP`, it means it's not yet meant for review. It can be a quick and dirty AI-generated experiment, and/or a way to test a hypothesis against CQ tests. It's basically a scratchpad until it's `Active` and sent for review.
You are welcome to comment on them (I don't mind) but I often abandon them, delete them, or otherwise rework them, so you should keep that in mind.
Related details
- woxxom
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrea Orru (Gerrit)
Andrea Orru added 1 comment![]( "Open in Gerrit")
This will trigger a getter on Array.prototype on out-of-bounds. To avoid it, check matchingIds[i]<listenersById.length
No out-of-bounds is possible here. Also, `listenersById` is not an array.
Related details
- woxxom
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrea Orru (Gerrit)
Andrea Orru voted Commit-Queue+1![]( "Open in Gerrit")
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrea Orru (Gerrit)
Andrea Orru added 1 comment![]( "Open in Gerrit")
// If the listener removed itself inside the callback,I will land a test for this edge case (we discussed it offline briefly).
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 6 comments![]( "Open in Gerrit")
Thanks, Andrea!
const matchingIds = webRequestNatives.GetMatchingListeners(do we need to set the prototype of the return value of GetMatchingListeners() to null in the C++ so that it can't be intercepted with getters to the index?
$Array.push(listener.blockedDispatches, dispatch);couldn't we do this above the function.apply() so that it always gets added / removed from the blockedDispatches set and we don't have to worry about whether the listener removed itself in the callback?
try {
let result =
$Function.apply(listener.callback, null, [filteredDetails]);
if (allowAsyncResponsesForAllEvents &&
result instanceof $Promise.self) {
// Blocking listeners can return a promise.
if (trackedListeners[listener.id] === listener) {
// Still registered. Track the dispatch so `removeListener()` can
// decrement the block count if removed while the promise is
// pending.
$Array.push(listener.blockedDispatches, dispatch);
} else {
// If the listener removed itself inside the callback,
// `removeListener()` already ran without seeing this dispatch.
// Decrement the block count now with no response.
decrementBlockCount(dispatch, undefined);
}
// Async rejections unblock the dispatch without recording an error
// (the synchronous dispatch loop has completed); rethrowing
// surfaces the rejection in the console.
$Promise.catch($Promise.then(result, function(asyncResult) {
onEventHandled(listener, dispatch, asyncResult);
}), function(e) {
onEventHandled(listener, dispatch, undefined);
throw e;
});
} else {
// Synchronous return value.
decrementBlockCount(dispatch, result, listener.extraInfoSpec);
}
} catch (e) {
// On synchronous failure, decrement the block count with no response
// and save the error.
recordError(dispatch, e);
decrementBlockCount(dispatch, undefined);
}this is a chunky block -- do you think we should extract to a helper function? (runSyncBlockingListener or similar?)
// Rethrows the first listener exception, if any.If I'm reading it correctly, we'd previously throw every listener error, not just the first. Is this a behavior change?
// Discards any pending asynchronous responses and decrements the block
// count with no response for each waiting dispatch.
const blockedDispatches = $Array.splice(listener.blockedDispatches, 0);
for (let i = 0; i < blockedDispatches.length; ++i) {
decrementBlockCount(blockedDispatches[i], undefined);
}hmm... is this the proper behavior? (Does this match the behavior today?)
The listener received the event, so removing itself as a listener isn't *that* unreasonable, even if it still wants to handle the event it got:
```
chrome.webRequest.onAuthRequired.addListener(
async function listener(event, reply) {
if (await isMyEvent(event)) {
// Found our event; no need to listen for more.
chrome.webRequest.onAuthRequired.removeListener(listener);
getEventResponse(event).then(reply);
return;
}
reply(undefined);
});
```
Related details
- Andrea Orru
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrea Orru (Gerrit)
Andrea Orru added 5 comments![]( "Open in Gerrit")
couldn't we do this above the function.apply() so that it always gets added / removed from the blockedDispatches set and we don't have to worry about whether the listener removed itself in the callback?
Ah, yes -- that's much, much cleaner. I made this change as an afterthought and didn't see there was a cleaner way.
We can also move `dispatch.blockCount++` inside the helpers (both `runBlockingListener` and `runAsyncBlockingListener`).
I will land a test for this edge case (we discussed it offline briefly).
Added a TODO.
try {this is a chunky block -- do you think we should extract to a helper function? (runSyncBlockingListener or similar?)
Done
If I'm reading it correctly, we'd previously throw every listener error, not just the first. Is this a behavior change?
You are right. Each sub event had its own `EventEmitter` doing try/catch, so this is a behavior change.
It seems like the correct way to do it is to use `bindingUtil.handleException` to mirror the way `EventEmitter::DispatchSync` does it in the legacy path: https://crsrc.org/c/extensions/renderer/bindings/event_emitter.cc;drc=11d511606e9d75ff9149f7b10fddd5e6af55af4a;l=301
This probably deserves a test to check that multiple throws are emitted in the same way across the two paths. So adding a TODO for it.
// Discards any pending asynchronous responses and decrements the block
// count with no response for each waiting dispatch.
const blockedDispatches = $Array.splice(listener.blockedDispatches, 0);
for (let i = 0; i < blockedDispatches.length; ++i) {
decrementBlockCount(blockedDispatches[i], undefined);
}hmm... is this the proper behavior? (Does this match the behavior today?)
The listener received the event, so removing itself as a listener isn't *that* unreasonable, even if it still wants to handle the event it got:
```
chrome.webRequest.onAuthRequired.addListener(
async function listener(event, reply) {
if (await isMyEvent(event)) {
// Found our event; no need to listen for more.
chrome.webRequest.onAuthRequired.removeListener(listener);
getEventResponse(event).then(reply);
return;
}
reply(undefined);
});
```
Yes, it matches today's behavior. I updated the comment to clarify it (+ reflect the other changes).
`CleanUpForListener` decrements with no response:
https://crsrc.org/c/extensions/browser/api/web_request/extension_web_request_event_router.cc;drc=2f59ca9d5a2454810abc62d5a235b4347c7e90c4;l=2671
Then when you get to `OnEventHandled` there's no blocked request for that listener and it returns:
https://crsrc.org/c/extensions/browser/api/web_request/extension_web_request_event_router.cc;drc=2f59ca9d5a2454810abc62d5a235b4347c7e90c4;l=1998
I don't love it. Maybe we should change it (in the future?).
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrea Orru (Gerrit)
Andrea Orru added 2 comments![]( "Open in Gerrit")
const matchingIds = webRequestNatives.GetMatchingListeners(do we need to set the prototype of the return value of GetMatchingListeners() to null in the C++ so that it can't be intercepted with getters to the index?
It seems like we don't.
https://crsrc.org/c/v8/src/api/api.cc;drc=2f59ca9d5a2454810abc62d5a235b4347c7e90c4;l=8231
`v8::Array::New` allocates and writes the content directly without calling setters.
It returns a dense array without holes (`PACKED_ELEMENTS`). In my understanding `(0, 1, ..., length - 1)` and the `.length` property itself are data properties.
So the prototype chain is not involved.
// Discards any pending asynchronous responses and decrements the block
// count with no response for each waiting dispatch.
const blockedDispatches = $Array.splice(listener.blockedDispatches, 0);
for (let i = 0; i < blockedDispatches.length; ++i) {
decrementBlockCount(blockedDispatches[i], undefined);
}Andrea Orruhmm... is this the proper behavior? (Does this match the behavior today?)
The listener received the event, so removing itself as a listener isn't *that* unreasonable, even if it still wants to handle the event it got:
```
chrome.webRequest.onAuthRequired.addListener(
async function listener(event, reply) {
if (await isMyEvent(event)) {
// Found our event; no need to listen for more.
chrome.webRequest.onAuthRequired.removeListener(listener);
getEventResponse(event).then(reply);
return;
}
reply(undefined);
});
```
Yes, it matches today's behavior. I updated the comment to clarify it (+ reflect the other changes).
`CleanUpForListener` decrements with no response:
https://crsrc.org/c/extensions/browser/api/web_request/extension_web_request_event_router.cc;drc=2f59ca9d5a2454810abc62d5a235b4347c7e90c4;l=2671Then when you get to `OnEventHandled` there's no blocked request for that listener and it returns:
https://crsrc.org/c/extensions/browser/api/web_request/extension_web_request_event_router.cc;drc=2f59ca9d5a2454810abc62d5a235b4347c7e90c4;l=1998I don't love it. Maybe we should change it (in the future?).
Sorry, the second link is: https://crsrc.org/c/extensions/browser/api/web_request/extension_web_request_event_router.cc;drc=2f59ca9d5a2454810abc62d5a235b4347c7e90c4;l=1989
Because the listener is gone. Either way the conclusion is the same (the response is ignored).
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |