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

Validate env var names before writing env var files #365

Merged
merged 4 commits into from
Aug 2, 2022
Merged

Conversation

fg-j
Copy link

@fg-j fg-j commented Jul 29, 2022

Summary

In the process of using Netflix/go-env to log build configuration, I uncovered this bug. I'm hoping to fix it upstream, but have to go through some bureaucracy to do it. For now, I've added validation to the FormattedMap constructor that should help out with this problem.

This has also made me wonder whether we should have some validation in the packit.Environment() functions, since right now a user can easily add invalid env var values to the environment. I'm not sure how the lifecycle handles that situation today.

❓ @paketo-buildpacks/tooling-maintainers , do you think we should add validation to the packit.Environment methods?

Use Cases

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@fg-j fg-j marked this pull request as ready for review July 29, 2022 14:42
@fg-j fg-j requested a review from a team as a code owner July 29, 2022 14:42
@ryanmoran
Copy link
Member

Are we worried about it being less clear what is happening here since this code now just silently ignores bad inputs?

@fg-j
Copy link
Author

fg-j commented Aug 1, 2022

@ryanmoran You're right. I think it's confusing. I'll get around the bug in go-env a different way. But I do think some validation of environment variables could be useful.

I just played around with using packit.Environment to set environment variables with names that don't comply with the regex validation (e.g. setting DOTNET ROOT instead of DOTNET_ROOT or RUNTIME=VERSION instead of RUNTIME_VERSION. These get loaded into the environment without error, but evaluating them results in unexpected behaviour. For example, in a launch-time container,

cnb@5f362f11abd6:/workspace$ env
...
...
DOTNET_=ROOT=/workspace/.dotnet_root
...
...
cnb@5f362f11abd6:/workspace$ echo $DOTNET_=ROOT
ROOT=/workspace/.dotnet_root=ROOT

For that reason, I think the validation would make more sense as part of the EnvironmentWriter(). If that sounds reasonable, I'll shift the validation in this PR.

@fg-j fg-j changed the title Validate env var names before adding to FormattedMap Validate env var names before writing env var files Aug 1, 2022
@ForestEckhardt ForestEckhardt added the semver:patch A change requiring a patch version bump label Aug 2, 2022
@fg-j fg-j merged commit 548b1e3 into v2 Aug 2, 2022
@fg-j fg-j deleted the validate-env-vars branch August 2, 2022 16:29
@fg-j fg-j added this to the v2.4.0 milestone Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants