-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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(templates): disable use of jsonnet with /api/v2/templates/apply
#23030
Conversation
/api/v2/templates/apply
cf9d14c
to
edd5fdc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking question about error information (current code may be correct for security reasons), but if not a security issue, error handing could be more informative.
|
||
for _, rem := range remotes { | ||
// While things like '.%6Aonnet' evaluate to the default encoding (yaml), let's unescape and catch those too | ||
decoded, err := url.QueryUnescape(rem) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to return the error here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or does that let too much security information escape?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be some security concern, although there shouldn't be much of an issue since our api.Err(...)
methods return sanitized errors to clients. Mostly I am keeping these consistent with the IDPE error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see @jdstrand has already merged the code in IDPE, so probably we shouldn't become incompatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're referring to the specific error from url.QueryUnescape()
instead of a generic error, I erred on the side of caution since I felt legitimate use of the API should not be causing this error and so giving the attacker a generic error was fine. If we find this to be problematic in practice, we can return a more specific error.
Disables server-side jsonnet processing to eliminate potential issues with the
go-jsonnet
implementation.