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

refactor(ses): refactor console init and test cleanup #1941

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented Jan 9, 2024

closes: #XXXX
refs: #1345 #1942

Description

A flaw in tame-console.js made it hard to use throws-and-logs.js to test the console output that happens during lockdown. The flaw is that tame-console.js read the value of the global console variable at initialization, rather than when tameConsole() is called. This PR moves that initialization code into tameConsole, which is highly unlikely to break any working code.

Using that fix, this PR also tests the current behavior that #1345 complains about. A later PR #1942 will fix that problem, building on this PR so that the difference is itself tested.

Security Considerations

Prior to this PR, tame-console.js used console and print as ambient globals without a /* global ... */ annotation. For console this is not surprising, as we generally assume this is ambiently available. But for print it is surprising. Do we have a bug in our lint diagnostic framework? Attn @kriskowal

In any case, this PR changes those specifically to globalThis.console and globalThis.print, where that globalThis is itself obtained from commons.js.

Moving the sampling of these globals into tameConsole() enables them to be changed before tameConsole() samples them, as we now intend. This should have zero security implications as these will still be sampled before lockdown() returns.

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

Moving the sampling of console this way should enable other uses of throws-and-logs.js for testing console output that would not work before this PR.

Upgrade Considerations

none.

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

I do not know whether the base lint environment implies print, but I would not be surprised if it did. I also do not know where to look.

https://github.com/eslint/eslint/blob/main/conf/globals.js provides neither console nor print, so is clearly not our source of truth

I did learn there are some interesting environment names, like shared-node-browser, that we might try out. https://eslint.org/docs/latest/use/configure/language-options#specifying-environments

@erights erights force-pushed the markm-test-cleanup-diagnostics branch from f10bccf to fd0c811 Compare January 10, 2024 00:04
@erights erights enabled auto-merge January 10, 2024 00:05
@erights erights merged commit ce62087 into master Jan 10, 2024
14 checks passed
@erights erights deleted the markm-test-cleanup-diagnostics branch January 10, 2024 00:10
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 this pull request may close these issues.

2 participants