Skip to content

refactor(ses): getenv detects more errors#2690

Merged
erights merged 2 commits into
masterfrom
markm-getenv-detects-more-errors
May 13, 2025
Merged

refactor(ses): getenv detects more errors#2690
erights merged 2 commits into
masterfrom
markm-getenv-detects-more-errors

Conversation

@erights

@erights erights commented Jan 13, 2025

Copy link
Copy Markdown
Contributor

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.

@erights erights self-assigned this Jan 13, 2025
@erights erights force-pushed the markm-getenv-detects-more-errors branch from 573a6d7 to 5a3139b Compare January 18, 2025 02:15
@erights erights force-pushed the markm-getenv-detects-more-errors branch from 5a3139b to 3d29d44 Compare January 27, 2025 21:16
@erights erights marked this pull request as ready for review January 27, 2025 21:37
@erights erights requested review from kriskowal and mhofman January 27, 2025 21:37
@erights erights force-pushed the markm-getenv-detects-more-errors branch from 3d29d44 to 5adc8ee Compare February 17, 2025 03:04

@gibson042 gibson042 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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();
}

Comment thread packages/ses/src/lockdown.js Outdated
Comment thread packages/ses/src/lockdown.js Outdated
Comment thread packages/ses/src/lockdown.js Outdated
Comment thread packages/ses/src/lockdown.js Outdated
Comment thread packages/ses/src/lockdown.js Outdated
@erights erights force-pushed the markm-getenv-detects-more-errors branch from 5adc8ee to b26dc32 Compare March 12, 2025 20:15
@erights erights force-pushed the markm-getenv-detects-more-errors branch from b26dc32 to 84234ce Compare March 12, 2025 21:37
@erights erights changed the base branch from master to markm-lockdown-options-kebob-case March 12, 2025 21:39
Base automatically changed from markm-lockdown-options-kebob-case to master March 12, 2025 23:16
erights added a commit that referenced this pull request Mar 12, 2025
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.
@erights erights force-pushed the markm-getenv-detects-more-errors branch from 84234ce to e6d8423 Compare March 12, 2025 23:26
@erights

erights commented Mar 18, 2025

Copy link
Copy Markdown
Contributor Author

@kriskowal , reassigned to you. Thanks!

@kriskowal kriskowal force-pushed the markm-getenv-detects-more-errors branch from e6d8423 to d1f6daa Compare March 20, 2025 00:33

@mhofman mhofman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread packages/ses/src/lockdown.js Outdated
@kriskowal kriskowal force-pushed the markm-getenv-detects-more-errors branch from d1f6daa to d512db6 Compare March 20, 2025 00:58
@kriskowal

Copy link
Copy Markdown
Member

I’ve removed the next-release label. We can continue iterating and I’m continuing to release now.

@kriskowal kriskowal removed their assignment Mar 20, 2025
Comment thread packages/ses/src/lockdown.js Outdated
@erights erights force-pushed the markm-getenv-detects-more-errors branch from d512db6 to d4c1a93 Compare March 22, 2025 20:19
@erights erights force-pushed the markm-getenv-detects-more-errors branch from d4c1a93 to 4f519e4 Compare March 24, 2025 23:44
@erights erights force-pushed the markm-getenv-detects-more-errors branch 2 times, most recently from f98efc0 to e7f81bd Compare April 8, 2025 00:11
@erights erights force-pushed the markm-getenv-detects-more-errors branch from e7f81bd to fda2e75 Compare May 12, 2025 19:45
@erights erights merged commit 86d983a into master May 13, 2025
16 checks passed
@erights erights deleted the markm-getenv-detects-more-errors branch May 13, 2025 01:13
boneskull pushed a commit that referenced this pull request Jun 4, 2025
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.
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.

4 participants