refactor(ses): getenv detects more errors#2690
Conversation
573a6d7 to
5a3139b
Compare
5a3139b to
3d29d44
Compare
3d29d44 to
5adc8ee
Compare
gibson042
left a comment
There was a problem hiding this comment.
Rather than manually typing the options, another possibility is updating getEnvironmentOption to do it, albeit at the cost of casting its third argument to @type {const}, as visible by hovering over bindings in the final region of this demonstration.
An advantage to the old ad-hoc manual style is that the enumeration of all possible alternatives was textually close to the dispatch on the alternative. This makes it easy to keep the two in sync. Thus, a comparative disadvantage of this PR is their separation, leading to maintenance hazards if one is updated but not the other.
I think typing can address this too, as also included in the above demonstration.
/** @typedef {(typeof overrideTamingOptions)[number]} OverrideTaming */
const overrideTamingOptions = /** @type {const} */ (['moderate', 'min', 'severe']);
…
const overrideTaming = getenv('LOCKDOWN_OVERRIDE_TAMING', 'moderate', overrideTamingOptions);
…
/**
* @param {OverrideTaming} overrideTaming
*/
function useOverrideTaming(overrideTaming) {
if (!['moderate', 'min', 'severe'].includes(overrideTaming)) throw Error();
}5adc8ee to
b26dc32
Compare
b26dc32 to
84234ce
Compare
Closes: #XXXX Refs: #961 #2690 #2723 ## Description #961 deviated from our general convention that lockdown option values be kebob-case, instead adding `evalTaming:` option values `safeEval`, `unsafeEval`, `noEval`. (I approved #961 at the time, apparently without noticing this discrepancy.) This PR fixes those to be `safe-eval`, `unsafe-eval`, and `no-eval`. But to avoid breaking any old usage, this PR ensure the only names continue to work for now, but always commented in the code as "deprecated". This PR does only that. Other changes to the relevant lockdown option or relevant lockdown options machinery are left to #2723 or #2690 respectively. I request that this PR go first, with those others adjusting to this one. ### Security Considerations none ### Scaling Considerations non ### Documentation Considerations This PR simply changes the documentation to use the new names without ever mentioning the deprecated old names. That seems like an appropriate simplification for the docs. ### Testing Considerations With a bit of duplication and renaming, we now test the new names and the old deprecated names. ### Compatibility Considerations To avoid breaking any old usage, this PR ensure the only names continue to work for now, but always commented in the code as "deprecated". It would be very nice to eventually be able to retire the deprecated names, but I find it hard to imagine how we'd test that it is safe to do so. ### Upgrade Considerations Nothing BREAKING, since the old deprecated names continue to work. - [x] Update `NEWS.md` for user-facing changes.
84234ce to
e6d8423
Compare
|
@kriskowal , reassigned to you. Thanks! |
e6d8423 to
d1f6daa
Compare
mhofman
left a comment
There was a problem hiding this comment.
I see that the validation is moving to getEnvironmentOption but are all places accepting a taming option plumbed to go through that? Aka all functions accepting a taming option are internal and the option would have been previously verified?
To make sure we don't regress on this, I'd like to see types constraining all params that accept such taming options.
d1f6daa to
d512db6
Compare
|
I’ve removed the |
d512db6 to
d4c1a93
Compare
d4c1a93 to
4f519e4
Compare
f98efc0 to
e7f81bd
Compare
e7f81bd to
fda2e75
Compare
Closes: #XXXX Refs: #1710 ## Description #1710 enhanced `getEnvironmentOption` with an optional third argument listing all the other allowed choices aside from the default choice. Previously, we had only manual code to detect and complain of unrecognized environment option values. #1710 provides a more declarative form, and reuses the checking-and-complaining logic. However, following #1710 we did not retire much of the previous ad-hoc manual code, nor did we make enough use of this declarative alternative. This PR fixes that. ### Security Considerations An advantage to the old ad-hoc manual style is that the enumeration of all possible alternatives was textually close to the dispatch on the alternative. This makes it easy to keep the two in sync. Thus, a comparative disadvantage of this PR is their separation, leading to maintenance hazards if one is updated but not the other. For example, if the declarative form adds another option that the dispatch code does not take into account, that other option might fall into the dispatch's fall-through case. This may or may not be good, depending on whether the fall through case is carefully chosen to also be appropriate for new not-previously-recognized options. ### Scaling Considerations none ### Documentation Considerations none ### Testing Considerations The text of the error messages for unrecognized options will likely be different. But aside from that there should be no observable difference. Since we do not consider a change to the text of an error message a correctness concern, we consider this PR to effectively be a pure refactor. ### Compatibility and Upgrade Considerations Both before and after this PR, we generally reject unrecognized environment options settings. This makes it difficult to grow existing options with new settings that are ignored if seen by previous code. This hinders plausible development patterns. This PR by itself does not change that. But by centralizing the detect-and-complaint logic and making it more declarative, perhaps we become better set up to address this issue later.
Closes: #XXXX
Refs: #1710
Description
#1710 enhanced
getEnvironmentOptionwith an optional third argument listing all the other allowed choices aside from the default choice. Previously, we had only manual code to detect and complain of unrecognized environment option values. #1710 provides a more declarative form, and reuses the checking-and-complaining logic. However, following #1710 we did not retire much of the previous ad-hoc manual code, nor did we make enough use of this declarative alternative. This PR fixes that.Security Considerations
An advantage to the old ad-hoc manual style is that the enumeration of all possible alternatives was textually close to the dispatch on the alternative. This makes it easy to keep the two in sync. Thus, a comparative disadvantage of this PR is their separation, leading to maintenance hazards if one is updated but not the other. For example, if the declarative form adds another option that the dispatch code does not take into account, that other option might fall into the dispatch's fall-through case. This may or may not be good, depending on whether the fall through case is carefully chosen to also be appropriate for new not-previously-recognized options.
Scaling Considerations
none
Documentation Considerations
none
Testing Considerations
The text of the error messages for unrecognized options will likely be different. But aside from that there should be no observable difference. Since we do not consider a change to the text of an error message a correctness concern, we consider this PR to effectively be a pure refactor.
Compatibility and Upgrade Considerations
Both before and after this PR, we generally reject unrecognized environment options settings. This makes it difficult to grow existing options with new settings that are ignored if seen by previous code. This hinders plausible development patterns. This PR by itself does not change that. But by centralizing the detect-and-complaint logic and making it more declarative, perhaps we become better set up to address this issue later.