Runbot skip test jobs #51
Conversation
|
Please rebase to let only relevant commits |
|
Anyways seeing the 2 commits, 👍 |
|
👍 |
|
@lepistone We triued this development, but pass as parameters this information, how ensure you are not opening all the server to the user interface? I do not see any virtualenv (may be I miss something) or ny container.... For example if it allows me pass paramenters I can pass Did you verify that, may be my code per review was not deep enought... regards. |
|
It's a known security issue and it exists even in odoo runbot. Anyone who changes the executable in a pull request can run whatever command they want. |
|
That's true but this open a second security hole didn't it? Your comment is regarding to "what a PR can make" I understand and I agree, but this is (my question) what a command setted can do. They are security hole because the same reason but different ones, and we are introducing such error. |
|
@nhomar You are right, it is a risk if anyone can access to the configuration of the builds, still this is not a problem in some use cases (if only you have access to the configuration / runbot instance is not shared). |
lepistone
force-pushed
the
runbot-skip-test-jobs
branch
from
6117b37 to
25aefc2
Compare
|
@nhomar anyone who can add harmful code to the build instructions needs to be either admin or part of the runbot group. On top of that, adding bash expressions That's not to say the executable couldn't be changed to something harmful, but that is the same issue as with the vanilla runbot. The vanilla runbot security issue is more serious because anyone who can do a pull request can change the code that is run, while build_instructions one would be limited with those who have been granted the rights. I see them as the same security issue since they're both affected by the more serious issue or the pullrequester being able to change the code that is run. Therefore there needs to be sandboxing such as dockerization. |
| def job_10_test_base(self, cr, uid, build, lock_path, log_path): | ||
| if build.branch_id.repo_id.skip_test_jobs: | ||
| _logger.info('skipping job_10_test_base') | ||
| return -2 |
There was a problem hiding this comment.
There was a problem hiding this comment.
Please put a note explaining that this is the expected number in runbot for directly calling next job
There was a problem hiding this comment.
Even better: factor it as a global variable, that way it's self documenting and if it changes in the future, it can be fixed at one place.
|
As discussed, I don't think we're making the vulnerability worse. While anyone can make a public pull request, only an administrator can change the runbot settings. I think this PR can go forward independently of #38. Thanks! |
|
@lepistone Agreed |
|
A disclaimer in the module is enough at least until we find and or develop On Thursday, August 20, 2015, Guewen Baconnier notifications@github.com
Saludos Cordiales CEO at Vauxoo https://www.vauxoo.com Odoo's Gold Partner. [image: --] |
|
That could be added to the module's description, but I would respectfully ask you to do this in another pull request. This one is on the affected module, but the changeset not related and we shouldn't be tagging on feature requests until we end up patchbombing. |
|
2015-08-20 10:46 GMT-05:00 Sandy notifications@github.com:
Agreed also. Saludos Cordiales CEO at Vauxoo https://www.vauxoo.com Odoo's Gold Partner. [image: --] |
|
👍 |
|
The fail is due to odoo/odoo-extra#60 |
|
👍 |
…escobarvx [FIX] runbot_send_email: Changed 'if' blocks to fix wrong save template
To avoid conflicts I included #49 here. The diff will look correct here once #49 is merged. Thanks!