aws-lambda-nodejs: banner and footer values are not escaped

[moltar]

Source:

aws-cdk/packages/@aws-cdk/aws-lambda-nodejs/lib/bundling.ts

Lines 156 to 157 in 8b00de0

	...this.props.banner ? [`--banner='${this.props.banner}'`] : [],
	...this.props.footer ? [`--footer='${this.props.footer}'`] : [],

The values for banner and footer are not properly escaped for shell execution, resulting in errors, and potentially a command injection!

❯ cdk synth
Bundling asset ACPipelineStack/Staging/AtlanticCoreApp/.../Code/Stage...
 > error: Invalid build flag: "--banner=// Source: ./src/modules/foo/index.ts\n\n"

Reproduction Steps

    const defaultProperties: NodejsFunctionProps = {
      bundling: {
        banner: `// Source: ./src/modules/foo/index.ts`,
      },
    }

What did you expect to happen?

Values to be escaped.

What actually happened?

Ran as is, without escaping.

Environment

• CDK CLI Version: 1.93.0
• Framework Version: 1.93.0
• Node.js Version: v14.15.5
• OS : macOS
• Language (Version): TypeScript (3.8.3)

Other

    "@aws-cdk/aws-lambda-nodejs": "1.93.0",

---

This is 🐛 Bug Report

[jogold]

+ --banner is now --banner:js and --banner:css (https://github.com/evanw/esbuild/blob/master/CHANGELOG.md#090)

[eladb]

Can this be closed?

[moltar]

@eladb Has it been fixed?

[eladb]

No, apologies I misunderstood @jogold's comment.

Contributions are welcome.

Keeping open

[jogold]

@eladb it's an easy fix here but what do you suggest for the breaking change? how should we handle esbuild breaking changes since we don't depend on it?

[eladb]

Aren't we checking that we use a compatible major version?

[jogold]

Aren't we checking that we use a compatible major version?

Yes but esbuild is currently on 0.x... meaning that we need to check the minor, should I do that? This is very restrictive.

[eladb]

I don't see any other reliable way. Perhaps we can add a switch to opt-out of version check if people insist they want to use a newer version

[gurshafriri]

👋 @eladb, do you see this as a valid potential security issue?
If it is, we (at snyk) would like to add it to our vulnerability DB, better with a fixed version at hand.

[eladb]

@gurshafriri Are you on CDK.dev slack? I'd like to discuss this with you directly

[nihalgonsalves]

@eladb would you accept a pull request to switch to the esbuild JavaScript API? This would eliminate bugs such as this and #13842 / #14065, and would enable the construct to allow custom config overrides for the esbuild settings.

[eladb]

@nihalgonsalves we have considered this in the past and decided that this is not the best approach. @jogold can shed some light.

[eladb]

@jogold let's first escape the shell command. I think JSON.stringify(x) should do the trick, correct?

As for breaking changes in esbuild, I believe in this case, we don't have to break the construct API, correct? Just use the new API from esbuild.

[jogold]

@jogold let's first escape the shell command. I think JSON.stringify(x) should do the trick, correct?

Will have a look.

As for breaking changes in esbuild, I believe in this case, we don't have to break the construct API, correct? Just use the new API from esbuild.

Correct because we don't care about --banner:css here.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.