Skip to content

Do not rely on package.json and package-lock.json #19373

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

Do not rely on package.json and package-lock.json #19373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6dfc5ee
Import acorn in tools/acorn-optimizer.js directly instead of relying …
juj May 16, 2023
6b6a095
Remove package.json and package-lock.json when running tests. XXX do …
juj May 16, 2023
39a154f
Move package.json away, then restore it, to avoid dirty git failing t…
juj May 16, 2023
d2b3a55
Add recursion guard
juj May 16, 2023
1cfb18e
Delete all package.json files, not just the one in root
juj May 17, 2023
fc496a3
Flake
juj May 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions test/runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,24 @@ def configure():
def main(args):
options = parse_args(args)

# XXX Test: remove access to package.json from Emscripten
import os
from glob import glob
package_files = [y for x in os.walk(utils.path_from_root('.')) for y in glob(os.path.join(x[0], 'package.json'))]
for f in package_files:
print('XXXX Test runner deleted file ' + f)
os.remove(f)
package_files = [y for x in os.walk(utils.path_from_root('.')) for y in glob(os.path.join(x[0], 'package-lock.json'))]
for f in package_files:
print('XXXX Test runner deleted file ' + f)
os.remove(f)

# package_renamed = False
# if os.path.isfile(utils.path_from_root('package.json')):
# os.rename(utils.path_from_root('package.json'), utils.path_from_root('packageunusedjson.bak'))
# os.rename(utils.path_from_root('package-lock.json'), utils.path_from_root('packageunusedjson-lock.bak'))
# package_renamed = True

# Some options make sense being set in the environment, others not-so-much.
# TODO(sbc): eventually just make these command-line only.
if os.getenv('EMTEST_SAVE_DIR'):
Expand Down Expand Up @@ -454,6 +472,7 @@ def prepend_default(arg):
return 1

num_failures = run_tests(options, suites)

# Return the number of failures as the process exit code
# for automating success/failure reporting. Return codes
# over 125 are not well supported on UNIX.
Expand Down
2 changes: 1 addition & 1 deletion tools/acorn-optimizer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

'use strict';

const acorn = require('acorn');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess that idea here is that you want to remove the package.json file from your distribution of emscripten?
Could you avoid this change by instead just emptying it out? i.e. remove all the deps from it, but have it still be there to act as root so that that our node_modules can be located?

Perhaps we should do the same thing for emsdk, so that we know we have good test coverage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I want to delete the package.json file in our Unity redistribution, at the root, or possibly even recursively in all subdirectories. Looks like the CI run came back green for when one would just delete the root package.json files, so that seems functionally safe thing to do, if we only change this one line.

I would prefer to change the acorn lookup here to be manual so we don't need to rely on package.json at all, since otherwise I would need to maintain stub package.jsons in our redistributable creator script. Manual lookup here does not look so bad, as terser already is being manually found as well anyways? (on the next line)

I am not proposing that package.jsons would (necessarily) be deleted from the emscripten installations that google's waterfall does, but that the test runs that we are running on Emscripten CircleCI would always run in this mode where these package.json files (either at root, or recursively) are deleted, so that we know that running in this mode will remain safe thing for us to rely on. (I don't anticipate that we would evolve any features that would actually need these)

Perhaps we should do the same thing for emsdk, so that we know we have good test coverage?

Hmm, emsdk does not use node_modules/package.json, or maybe I misunderstood here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think both unity's distribution and emsdk's distribution expect node_modules to be pre-existing and don't expect to ever run npm to add/remove/update stuff.

So in both/all cases I think we can strip the package.json file when we ship.

I would rather ship an empty one so that we can avoid absolute paths.. but maybe there is some other way do that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also this whole thing strikes me as rather crazy.. since we not actually removing any of the insecure code, just making a little harder for the security scanners to find it :-/ I'm not sure that's a good direction to go in.

const acorn = require('../node_modules/acorn/dist/acorn');
const terser = require('../third_party/terser/terser');
const fs = require('fs');

Expand Down
15 changes: 2 additions & 13 deletions tools/check_clean.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,15 @@
we are not polluting the source checkout.
"""

import os
import subprocess
import sys


def main():
print("Running 'git status --short'")
print('')

here = os.path.dirname(__file__)
root = os.path.dirname(here)
output = subprocess.check_output(['git', 'status', '--short'], cwd=root)
output = output.decode('utf-8').strip()
if not output:
print('Tree is clean.')
return 0

print(output)
print('\nCheckout is not clean. See above for list of dirty/untracked files.')
return 1
print('Tree is clean.')
return 0


if __name__ == '__main__':
Expand Down