--pre-js and --post-js options expose PII in full paths.

[erikh2000]

Please include the following in your bug report:

Version of emscripten/emsdk:

emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.51 (c0c2ca1314672a25699846b4663701bcb6f69cca)
clang version 18.0.0git (https://github.com/llvm/llvm-project f2464ca317bfeeedddb7cbdea3c2c8ec487890bb)
Target: wasm32-unknown-emscripten
Thread model: posix

Failing command line in full:
emcc --pre-js someFile.js --post-js someOtherFile.js helloWorld.c -o helloWorld.js

When JS files are included there is a helpful set of comments showing where the source came from. It looks like this:

// include: /Users/user-account-name/path/to/source/someFile.js
console.log('Doing something in my pre-JS.`);
// end include: /Users/user-account-name/path/to/source/someFile.js

This isn't a giant security flaw, but I really don't want my user name of information about the paths on my local filesystem in the source code. The code in question will be on an open source repository. I don't want it to be used as the basis of an attack. And also, even if I can do things easily to stay secure, my open source code might end up exposing somebody else's PII when they run a build.

My suggestion is that the path should by default output the exact path from the --pre-js or --post-js params. This might be an an absolute or relative path. It might even include the user's PII in the path, but in this case, the user would be explicitly causing it to happen.

Alternatively, a flag like "--output-no-paths" or something to suppress the output.

I'm new to emscripten, and half expect somebody to tell me I'm Doing It Wrong. No worries if that is the case. I'm trying to be constructive.

[kripken]

Thanks @erikh2000 , good points, I agree we should do something here. While this is in a comment, and comments are normally removed in optimized builds, sometimes people ship builds with some amount of comments for various reasons, and this might be surprising.

After a little reading I see that emitting the exact path the user provided may not be trivial, as it would need to be piped through. As this is really just a minor debug annotation, I think we can either just not emit it at all, or emit the filename but not the full path, that is, for the example above:

// include: someFile.js
console.log('Doing something in my pre-JS.`);
// end include: someFile.js

That would be either what the user provided, or shorter than that.

Just to check though, @sbc100 is there perhaps some reason I'm forgetting that we wanted the full path here?

[sbc100]

These paths/comments should only ever appear in debug builds. Are you seeing them in release builds too?

Given that debug builds already contain all kinds of path information (in the dwarf data) it seems reasonable to have these here too. If these are showing up in release builds that would indeed be a bug.

[kripken]

@sbc100 Fair point about the DWARF issues. However we don't emit DWARF in a simple emcc file.cpp build with no other flags: the user must explicitly provide -g to get DWARF in the final output. So I think the issue here is more surprising.

It also seems a simple thing to change with little downside, or do you disagree?

[sbc100]

Happy to use just the filename rather than the full pathname here, but also wanted to get confirmation from @erikh2000 that they understand this is a debug-build-only thing, and to make sure they were aware that they don't want to be shipping debug builds to end users?

[sbc100]

If we are worried about PII then I think we probably want to strip all comments when -g is not specified since comments often contain way more PII than just pathnames.

[sbc100]

Thinking about it, that makes more sense to me. Maybe we just strip all comments in -g0 mode?

[erikh2000]

but also wanted to get confirmation from @erikh2000 that they understand this is a debug-build-only thing,

Understood, thanks. That certainly makes the issue less of a problem.

[kripken]

@sbc100

Maybe we just strip all comments in -g0 mode?

Hmm, interesting idea in general I think, but I worry about a breaking change there. Some people surely happen to depend on the current form of the output in various ways. In fact my debug workflow is to use -g0 builds, so if I had to start doing -g to get JS comments that means I'd be including DWARF too, which has downsides (slower iteration, limitations in the binaryen optimizations, etc.). So I would prefer not to change that.

Perhaps we don't need to do anything here, but I'm still in favor of changing the paths here to just the filename.

[sbc100]

Given that IIUC comments in general contain more PII than filenames, I don't think its worth taking action here, unless we plan to strip all comments.

I won't object to the pathname->filename change but even the filename alone could contain private information remember. e.g. --pre-js=super_secret_project_name.js

[kripken]

Fair points. I don't feel strongly then.

[erikh2000]

Trying to understand your point, @sbc100, when you say "comments in general contain more PII than filenames".

To me, if I look at the built JS file, the comments seem to be boilerplate with the exception of the pre-js/post-js filepaths. Or if "boilerplate" is too simplistic, I mean that the generated comments I see in the JS file with exception of the filepath don't reveal anything about me or my development environment.

Maybe there are other use cases where the comments would include more information than what I see.

[sbc100]

Sorry, I should have been more specific. I was referring to comments that a user of emscripten might put in a --pre-js file, or a --js-library file. Comments in code tend to contain all kind of information about the code in question that could be proprietary to the author of the code.

My assertion is simply that leaking of project-specific code comments is likely much more serious than the leaking of a username on a machine. Therefore if we are going to strip full filenames to avoid leaking usernames we should probably also strip all other comments.

[sbc100]

I think we need to make it clear to folks that shipping non-optimized builds to production is not recommended, and does reveal more information about that project than an optimized build would.

[erikh2000]

Representing a new, clueless emscripten user, that makes solid sense to me.

Maybe a command-line output warning from the emcc build like "Warning: Debug builds are non-optimized and may contain information you do not want to distribute."

[kripken]

I opened #21147 now to add some docs about the unwanted information that may exist in unoptimized builds.

I would prefer not to add a commandline warning, as we'd need to show it in all unoptimized builds, which means all default builds, so it would constantly be shown to users. That could help new users but it would also quickly annoy existing ones, I worry.

[sgbeal]

Another voice for "please don't consider automatically stripping all comments from pre/post (or anything, for that matter)." In the sqlite project we ship API docs in there, as well as the license and library version info.