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

fix(core): bundling directory access permission is too restrictive #8767

Merged
merged 4 commits into from Jun 29, 2020

Conversation

misterjoshua
Copy link
Contributor

@misterjoshua misterjoshua commented Jun 27, 2020

The new bundler uses mkdtempSync to pre-create uniquely named directories for asset staging. But, mkdtempSync creates the staging directories with a restrictive 0700 & ~umask mode, rather than mkdir's usual 0777 & ~umask mode.

In Bitbucket Pipelines, these restrictive permissions prevent the bundler from accessing its /asset-output volume. And, if the bundler can't access /asset-output, bundling fails.

This fix chmods the asset staging directory to 0777. This change fixes my Bitbucket Pipelines issue.

Closes #8757


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@misterjoshua misterjoshua changed the title fix(core): chmod the bundle dir to full access while respecting process umask #8757 fix(core): chmod the bundle dir to full access while respecting process umask fixes #8757 Jun 27, 2020
@misterjoshua misterjoshua changed the title fix(core): chmod the bundle dir to full access while respecting process umask fixes #8757 fix(core): chmod the bundle dir to full access while respecting process umask (#8757) Jun 27, 2020
@eladb
Copy link
Contributor

eladb commented Jun 28, 2020

@jogold can you take a look at this please?

@eladb eladb self-assigned this Jun 28, 2020
@@ -144,6 +144,9 @@ export class AssetStaging extends Construct {

// Create temp directory for bundling inside the temp staging directory
const bundleDir = path.resolve(fs.mkdtempSync(path.join(stagingTmp, 'asset-bundle-')));
// Chmod the bundleDir to full access after applying the process the umask.
// tslint:disable-next-line:no-bitwise
fs.chmodSync(bundleDir, 0o777 & (~process.umask()));
Copy link
Contributor

@jogold jogold Jun 28, 2020

Choose a reason for hiding this comment

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

Looks like calling process.umask() with no argument is now deprecated in Node.js v14: https://nodejs.org/docs/latest-v14.x/api/process.html#process_process_umask

What would be the alternative? What about using only 0o777, would it be OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another alternative is to set a new umask (to get the return value), then restore the original umask. I've seen a getumask manpage that describes this approach. But, mode 0o777 alone would certainly satisfy my use case. I can't think of any major downsides. What's your preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

But, mode 0o777 alone would certainly satisfy my use case. I can't think of any major downsides.

OK, let's go for this.

@misterjoshua misterjoshua changed the title fix(core): chmod the bundle dir to full access while respecting process umask (#8757) fix(core): chmod the staged asset bundling dir to full access (#8757) Jun 29, 2020
@jogold
Copy link
Contributor

jogold commented Jun 29, 2020

@eladb LGTM, maybe change the PR title to something like fix(core): bundling directory access permission is too restrictive

@eladb eladb changed the title fix(core): chmod the staged asset bundling dir to full access (#8757) fix(core): fix(core): bundling directory access permission is too restrictive Jun 29, 2020
@eladb eladb changed the title fix(core): fix(core): bundling directory access permission is too restrictive fix(core): bundling directory access permission is too restrictive Jun 29, 2020
eladb
eladb previously approved these changes Jun 29, 2020
@mergify mergify bot dismissed eladb’s stale review June 29, 2020 19:45

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 7c74631
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Jun 29, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 1842168 into aws:master Jun 29, 2020
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.

[aws-lambda-nodejs] Broken in BitBucket Pipelines
4 participants