Make coprdirs have their repo available in the buildroot #3369
Conversation
|
This would bring security issue into the We need to solve the s.f.o integration first, somehow. |
Pull Request validationFailed🔴 Review - Missing review from a member (2 required) Success🟢 CI - All checks have passed |
FrostyX
force-pushed
the
coprdir-buildroot-repo
branch
from
ff65588 to
df12563
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
FrostyX
force-pushed
the
coprdir-buildroot-repo
branch
2 times, most recently
from
764cf2d to
a14af3f
Compare
|
Took me only half a year but updated, PTAL. |
| @@ -692,8 +694,7 @@ def validate(cls, copr, dirname): | |||
| f"Please use directory format {copr.name}:custom:<SUFFIX_OF_CHOICE> " | |||
| f"or {copr.name}:pr:<ID> (for automatically removed directories)" | |||
| ) | |||
|
|
|||
| if not all(x.isalnum() for x in dirname.split(":")[1:]): | |||
| if not all(x.isalnum() for x in re.split(r"[:-]+", dirname)[1:]): | |||
There was a problem hiding this comment.
this deserves an in-line note :-) I'm not sure this is correct
We basically only want to allow one more :-separated field, right? I'd wish we had a test-case for the validation.
There was a problem hiding this comment.
Then, I'm a bit scare of creating custom dirnames, because we don't have a method for deleting them.
Then, note #820 -> will that still work & remove PR dirs after certain period of time?
|
Mtg: Test for validating dir names and make sure the Cron for cleaning up old dirs still work. |
Alternatively, we could separate then through `event_info.user` which would produce CoprDir names like `foocopr:pr:jdoe-1`. This wouldn't separate the different packages but it would be good enough to fix the security issue. And it would produce shorter names than the project URL paths.
This is useful in general but especially for pull requests that build multiple packages which depend on each other (e.g. `python-copr` and `copr-cli`). Up until now, it had to be workarounded by building the dependency into the main copr repository.
FrostyX
force-pushed
the
coprdir-buildroot-repo
branch
from
a14af3f to
34e4ca4
Compare
I didn't change any code but I added a test for the validation method. All patterns that I would expect to fail, fail. @praiskup were you worried about some different ones?
I submitted a build: and tried: which returned nothing. Then I did: and tried again: Also looking at the deletion code, there doesn't seem to be any logic related to the dir name. So I think the cleanups should still work as expected. |
|
Only a nit; there are no "Fixes: " labels in git commit messages; but this certainly has some issue counterparts. |
|
This means we can no longer submit all builds to |
|
Hello @hroncok, Sorry, I implemented two unrelated things in this PR, to make it more confusing. The "isolation" part of this PR fixes #767, and it is only relevant for |
This is useful in general but especially for pull requests that build multiple packages which depend on each other (e.g.
python-coprandcopr-cli). Up until now, it had to be workarounded by building the dependency into the main copr repository.