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

feat(ses): add SES version to lockdown shim #1854

Merged
merged 9 commits into from
Dec 5, 2023

Conversation

leotm
Copy link
Contributor

@leotm leotm commented Nov 8, 2023

closes: #1853

Description

Prepends the ses package.json > version as a comment to the bundled lockdown file

Security Considerations

Scaling Considerations

Documentation Considerations

Possibly worth mentioning the version number is now included in the SES lockdown shim

Testing Considerations

Possibly add a unit test to check the version number is present at the head of the file

Manually tested locally:

git clean -fdx
nvm use 16 # or 18 or 20
npm i --force # https://github.com/endojs/endo/assets/1881059/72a4079c-fe00-4de8-ac2d-fb13406edbad
cd packages/ses
npm run build
head dist/ses.cjs # ensure correct version present as comment on line 1
npm run test

on Node

Upgrade Considerations

packages/ses/scripts/bundle.js Outdated Show resolved Hide resolved
packages/ses/scripts/bundle.js Outdated Show resolved Hide resolved
packages/ses/scripts/bundle.js Outdated Show resolved Hide resolved
@leotm leotm changed the title feat(ses): add SES version feat(ses): add SES version to lockdown shim Nov 8, 2023
packages/ses/scripts/bundle.js Outdated Show resolved Hide resolved
@leotm leotm marked this pull request as ready for review November 9, 2023 14:33
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.

Feedback for your consideration. If you are unable to merge this, please reply when it’s ready to merge.

packages/ses/scripts/bundle.js Outdated Show resolved Hide resolved
packages/ses/scripts/bundle.js Outdated Show resolved Hide resolved
packages/ses/scripts/bundle.js Outdated Show resolved Hide resolved
Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Looks fine, but had a question or two

packages/ses/scripts/bundle.js Outdated Show resolved Hide resolved
packages/ses/scripts/bundle.js Outdated Show resolved Hide resolved
Comment on lines 20 to 23
const bytes = await fs.promises.readFile(
fileURLToPath(`${root}/package.json`),
);
const text = textDecoder.decode(bytes);
Copy link
Contributor

@boneskull boneskull Nov 13, 2023

Choose a reason for hiding this comment

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

Can we not do this:

Suggested change
const bytes = await fs.promises.readFile(
fileURLToPath(`${root}/package.json`),
);
const text = textDecoder.decode(bytes);
const text = await fs.promises.readFile(
fileURLToPath(`${root}/package.json`),
'utf8'
);

and drop the TextDecoder? Is this an Endo convention?

Copy link
Contributor Author

@leotm leotm Nov 20, 2023

Choose a reason for hiding this comment

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

nice the 2nd arg w utf8 cleans up the need for TextDecoder ^
spotted utf-8 is also used, but don't think it matters? (same result)
if not, then RE convention i favour our suggestion (the default)

Screenshot 2023-11-20 at 4 39 04 pm

edit: i guess the same change could be made as follow-up in packages/check-bundle/index.js
and possibly deeper in various other parts the codebase


edit: just realised could do this aswell 🤷‍♂️

  const bytes = await fs.promises.readFile(
    fileURLToPath(`${root}/package.json`),
  );
  const packageJson = JSON.parse(bytes);

leotm added a commit to leotm/endo that referenced this pull request Nov 20, 2023
@leotm
Copy link
Contributor Author

leotm commented Nov 20, 2023

Feedback for your consideration. If you are unable to merge this, please reply when it’s ready to merge.

Changes made, re-ready 2 merge @kriskowal (i'm not authorized to merge)

packages/ses/scripts/bundle.js Outdated Show resolved Hide resolved
kriskowal pushed a commit to leotm/endo that referenced this pull request Dec 5, 2023
@kriskowal kriskowal force-pushed the leotm-versioned-lockdown-bundle branch from 273db98 to 02e8896 Compare December 5, 2023 23:32
@kriskowal kriskowal enabled auto-merge December 5, 2023 23:33
@kriskowal kriskowal force-pushed the leotm-versioned-lockdown-bundle branch from 02e8896 to a3c3cc8 Compare December 5, 2023 23:46
@kriskowal kriskowal merged commit 8fd4295 into endojs:master Dec 5, 2023
14 checks passed
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.

Add version number to SES lockdown shim
3 participants