-
Notifications
You must be signed in to change notification settings - Fork 232
Additional checks for security concerns during Import YAML and Template process #1321
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
Conversation
@@ -47,7 +47,7 @@ | |||
"angular-utf8-base64": "0.0.5", | |||
"file-saver": "1.3.3", | |||
"bootstrap-switch": "3.3.3", | |||
"origin-web-common": "0.0.3" | |||
"origin-web-common": "0.0.5" |
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.
Did you mean to bump this?
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.
Yes, won't be able to create cluster resources without this.
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.
Right of course :)
var clusterStrs = []; | ||
_.each(clusterScopedResources, function(resource){ | ||
clusterStrs.push(humanizeKind(resource.kind)); | ||
}); |
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.
Probably want to make sure there are no duplicates in case the same kind appears in the list twice. Also suggest _.map
here.
var clusterStrs = _.uniq(_.map(clusterScopedResources, function(resource) {
return humanizeKind(resource.kind);
}));
var roleBindingStrs = []; | ||
_.each(roleBindingResources, function(resource){ | ||
_.each(resource.subjects, function(subject) { | ||
var str = resource.roleRef.name + " to " + humanizeKind(subject.kind) + " "; |
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.
The role name in this message I'm more worried about than the other. It might be a role created in the same YAML/template and sound harmless, but isn't. I'd just say it will add roles to subject-names.
563fb79
to
2b12a0f
Compare
2b12a0f
to
768e3e8
Compare
@spadgett ok think this is ready for final review, added a uniq call for roleBindingStrs as well since its only showing the subjects now |
Here's what I think the whitelist should be:
If you're not in this list, you get warned |
@spadgett added a second commit with the feedback from @smarterclayton can you review those changes, and then i'll squash and rebuild the dist |
I put the whitelist in Constants just in case someone yells, but I don't think I want to document it exists |
Second commit looks good to me |
Thank you for your tolerance of my opinions :) |
…te process Also enables the ability to create cluster scoped resources from Import YAML
7ec1e50
to
fb82c39
Compare
[merge] |
Evaluated for origin web console merge up to fb82c39 |
Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1145/) (Base Commit: d733d79) |
Also enables the ability to create cluster scoped resources from Import YAML
Implements https://trello.com/c/RCU304BD