Add a option to force override module name

[raincz]

The module name is implied from the git repository name. However, both r10k and puppet are very particular about module naming, and the implicit rewrites rules can get in the way. Puppet will understand any amount of dashes in the project name as <vendor>-<module>, rewriting - to /. For git repositories, there are often arbitrary naming rules with copious amounts of these control characters in repository names, making it nearly impossible to serve both worlds at the same time.

This URL parameter (module_name) allows to override the module name directly in the URL, completely ignoring the git repository name.

[raincz]

@dhollinger Sorry, another rather similar one. :D Description in the commit message. I did not find any documentation which needed to be updated.

[dhollinger]

I don't see any issues with this. Could you add some documentation in the readme. I have an issue to clean up the docs and having that there will be useful when I finally get to it.

[raincz]

Sure thing. Should I do it as a separate PR?

[raincz]

I added something to the README in a second commit. There really wasn't anything to start with so I just added a new section.

[raincz]

Hmm, I see you approved it @dhollinger but maybe it's the automated checks that are stopping the merge? The thing is, the failing check seems to be new and it's failing outside of the code I touched.

[smortex]

The thing is, the failing check seems to be new and it's failing outside of the code I touched.

I feel like this is caused by the module_name not being sanitized, e.g. one can pass module_name=%24%28rm%20-rf%20%2A%29foo, and this would have unexpected consequences? Maye can be tested a safe way with module_name=%24%28touch%20%2Ftmp%2Fflag%29foo (module_name=$(touch /tmp/flag)foo)?

Not sure how to fix that properly in go. Generally it is safer to run a command by passing an array of arguments rather that a string fed to a shell.

[raincz]

That is a fair point, I did not consider this. Silly that the check puts the blame on a completely detached line.

[raincz]

Mmm, so I looked at the docs. For one it does not seem like any shell quoting is necessary to begin with, as https://pkg.go.dev/os/exec#Command does not really use sh -c to run commands. But it's true that handling of bad invocation of r10k isn't great in the webhook in general, so it makes sense to explicitly validate this based on the documented Puppet rules. I added that. The CodeQL check is overly dogmatic though and still fails. I'll try to think of a way to shut it up.

[raincz]

Well. I'm at a loss. This monstrosity of a security check is simply forbidding variables touched by user input to be used for insertion into commands. With gosec there's a way to disable the linter checks with a comment, but I don't see a way how to do it with this CodeQL. I understand the overall rationale, and I fully support having to sanitize anything that goes into a system command. But this alert offers no way to really fix this, and it seems like it will not accept anything less than a full concession and not implementing any functionality like this. The "suggestion" to use a set of predefined consts and choose from them based on the user input is really entirely senseless here.

[dhollinger]

@raincz The issue here is that by allowing a user to pass in the value from the url directly, it allows anyone with credentials, or some other mechanism, to execute code maliciously on the system. We probably need to look at a value that can be passed in as a Header or some other mechanism supported by the Git provider as a way to override the module name.

Ideally, I'd prefer to have an API I can call to run r10k-like functionality, but that currently doesn't exist and I haven't had time to look at/work on g10k to make it useable with this in place of r10k

[raincz]

Yeah. This is true in general, but sanitizing the input should be sufficient. The Puppet module name has a pretty strict regex that I put in (lowercase leters, digits, underscore), which pretty much precludes any injection. This should be sufficient for this particular use case, but regardless of that, there is simply no way to tell this to CodeQL.

[dhollinger]

@raincz I will override and merge this PR. Though I will continue to look for a better way to do this