-
Notifications
You must be signed in to change notification settings - Fork 80
fix(ses): to workaround #2908 use current console #2934
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
Conversation
|
#945 (comment) may be as simple and much less fragile. |
db4bb38 to
9f4838e
Compare
|
See also nodejs/node#59456 |
gibson042
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check in general whether
consoleremains mutable after lockdown, and as endowed to child compartments. If not, that is already a security problem independent of this PR.
I don't know to what extent it's a security problem, but the console property of the global object, like all other properties of the global object, keeps its configurability and writability:
$ node --input-type=module --import @endo/init -e '
import { inspect } from "node:util";
const globalDescs = Object.getOwnPropertyDescriptors(globalThis);
console.log("console", inspect(globalDescs.console, { depth: 0 }));
console.log(
"non-configurable and/or non-writable",
Object.entries(globalDescs).flatMap(([key, desc]) =>
desc.configurable || desc.writable ? [] : [key],
),
);
'
console {
value: [Object],
writable: true,
enumerable: false,
configurable: true
}
non-configurable and/or non-writable [ 'Infinity', 'NaN', 'undefined' ]And at any rate, I think this also calls for updating lockdown.md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be better if the output from chooseReporter included a method facet for accepting a replacement printer, for use after the SES console is instantiated.
We discussed this and the conclusion was: perhaps, but not in this PR |
Done |
973bc72 to
d0cb1bc
Compare
Closes: #2908
Refs: #636 #944 #945 nodejs/node#59456
Description
Due to timing, reporter.js runs before lockdown replaces the system console with our causal console. Thus, when
makeReportermakes a reporter that uses some global console, it uses the platform's original global console rather than the causal console. I don't know if this was purposeful or good enough at the time, so I don't know whether this PR is a workaround or a proper fix. In any case, it fixes the case of immediate interest by using the current global console to report rather than the original one.Security Considerations
@gibson042 checked below and found that the
consoleproperty of the global of the start compartments remains mutable after lockdown. This is a security hazard, but is consistent with our stance on the privileges available in the start compartment. In any case, this hazard is not affected by this PR.Scaling Considerations
none
Documentation Considerations
none
Testing Considerations
Hard to write a regression test to ensure that a stack is output since there is no portable agreement on what stacks look like. But not impossible.
Manually running the test case from #2908 , we get the correct outputs at #2908 (comment) .
Running that test case from #2908 on Node 22 and 24, we find that the bug does not manifest there either. The problem is only on Node 20. I don't understand how that can be. In any case, this PR does fix the behavior across all these versions.
Compatibility Considerations
If someone suppresses the
consolereplacement withconsoleTaming: 'unsafe', then this PR in its current state fails to workaround #2908If some code depends on reporters using the original
consolerather than the current one, that code might break or misbehave.Upgrade Considerations
none