Restore patchset function and add patch upload functionality - #1563
Open
nuclearcat wants to merge 15 commits into
Open
Restore patchset function and add patch upload functionality#1563nuclearcat wants to merge 15 commits into
nuclearcat wants to merge 15 commits into
Conversation
This reverts commit 1bdde98. Bring back the patchset service and its docker-compose entry as a starting point for restoring the patch testing feature. The service is restored verbatim; the bugs that made it fail in staging and production are addressed in the following commits. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
tempfile.NamedTemporaryFile() defaults to binary mode ('w+b') and
passing an encoding along with a binary mode raises:
ValueError: binary mode doesn't take an encoding argument
so the service crashed on every single patch it tried to apply.
The whole charset-detection dance is unnecessary: the file is only
used as a download target for the patch (written by a separate binary
open()) and then read back as bytes for hashing, so no text encoding
is involved at any point. It also performed an extra HTTP request per
patch just to peek at the headers, and used Logger.warn() which does
not exist on the pipeline Logger class (only warning()), so even the
fallback path would have raised AttributeError.
Drop the charset detection entirely and open the temp file with
default binary mode.
Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
The hashable patch lines were joined with a literal b"/n" typo instead of a newline. Fix the separator; note this changes the patchset hash values compared to what the old code would have produced. Also drop the debug log dumping the whole hashable content: it decoded arbitrary patch bytes as UTF-8 and would raise UnicodeDecodeError on patches with a different encoding. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
Patch application order matters for a series. The artifacts are keyed patch0, patch1, ... by the pipeline patchset endpoint, but the code iterated the artifacts dict in whatever order the API returned it, and a plain lexicographic sort would also be wrong (patch10 < patch2). Sort the artifacts by their numeric suffix before applying. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
The name of the directory extracted from the checkout tarball was reconstructed from the kernel revision fields (tree, branch, describe). This breaks whenever the tarball service applies its own transformations to the name, e.g. replacing "/" with "_" for branch names containing slashes. The extracted directory always matches the tarball file name, so derive it directly from the tarball artifact URL basename instead. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
Several failure cases went unnoticed or produced broken errors: - pull_tarball() returning False (download failed after retries) was ignored, so processing continued against a missing source tree. Raise instead so the node is marked as failed. Also place the downloaded tarball in the output directory rather than whatever the current working directory happens to be. - A checkout node without a tarball artifact would fail with a bare KeyError; check for it explicitly with a clear message. - RuntimeError/ValueError were raised with logging-style lazy formatting arguments, producing tuples instead of readable messages. Use f-strings. - Fix garbled log message when a patchset node has no parent. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
The service subscribed to pub/sub events for patchset nodes but never consumed them, driving all its work from the polling loop instead. With durable per-subscriber queues, an unconsumed subscription just accumulates events on the API side, so drop it and rely on polling only. Polling also matches the workflow here: the service has to wait for the parent checkout node to complete, which an op=created event cannot signal. The old _setup() signature was also wrong (*args), hiding the args parameter passed by the Service base class. Move the loop body into a helper and catch request exceptions around it so a transient API outage no longer kills the service; also drop the noisy "Waiting N seconds" info log printed on every poll iteration. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
Bring _update_node() in line with what the tarball service does nowadays when it publishes a checkout node: - set result to "pass" when the node becomes available - use timezone-aware datetime (datetime.now(datetime.UTC)) for the holdoff timestamp instead of the deprecated datetime.utcnow() - deep-copy the checkout node data before adding the patchset hash; the previous shallow copy mutated the kernel_revision dict shared with the checkout node object Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
No functional change: - drop unused imports (kernelci.storage, base.Service) - remove stale FIXME comments and fix comment typos - document why TAR_CREATE_CMD is overridden (extracted tarball is not a git repository, so `git archive` cannot be used) - reformat with ruff to satisfy the CI pre-commit hooks Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
Every other service defines the host.docker.internal extra host so it can reach the API running on the docker host; the patchset service was missing it and could not connect to the API in the docker-host setup. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
Patches were piped straight into patch(1) with no inspection, so a crafted mbox could write outside the source tree (path traversal, absolute paths, in-tree symlinks pointing elsewhere) or smuggle in binary content. Add a validation pass on every downloaded patch before it reaches patch(1): - enforce a maximum patch size (patchset_max_patch_size_mb setting, 10 MiB by default) - reject binary content: NUL bytes, "GIT binary patch" and "Binary files ... differ" sections - reject symlink (120000) and gitlink (160000) file modes so a patch cannot introduce links for later escapes - every path referenced by ---/+++ headers, "diff --git" lines and rename/copy headers must be relative, free of "..", outside .git/, and resolve (through realpath, catching symlink escapes) to a location inside the extracted source tree - at least one unified diff file header must be present, ruling out ed-style and context diffs entirely The parser is hunk-aware: hunk bodies are consumed using the @@ line counts so patch content that itself quotes diff headers (e.g. documentation changes) is not misparsed as file headers. Patches that touch build scripts remain allowed by design: that code runs inside the isolated build runtimes, whereas this validation protects the host running the patchset service. Add unit tests covering the accepted and rejected cases. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
patch(1) auto-detects the diff format by default, which includes ed-style scripts; those are interpreted by ed and were the vector for arbitrary command execution in CVE-2018-1000156. Force --unified so only unified diffs are ever interpreted, matching what the validation pass accepts. Also pass --force so patch never stops to ask questions when run as a service, and --no-backup-if-mismatch so fuzzy applications don't leave .orig files that would end up in the released source tarball. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
Patches uploaded through the pipeline API inline patch support are stored on the pipeline storage (e.g. files.kernelci.org), so the service must accept patch URLs pointing there. Add the hostname of the configured storage base_url to the allowed domains at startup instead of requiring it to be duplicated in the allowed_domains setting. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
The "patch" field of the patchset request was declared but returned
501 Not Implemented, so patches could only be referenced by URL from
an allowed domain. Implement it: inline patches are uploaded to the
configured pipeline storage (e.g. files.kernelci.org) and the
resulting URLs are stored as the patchset node artifacts, exactly as
if they had been passed via "patchurl".
Uploads are limited to 32 patches of up to 10 MiB each per request
and must be non-empty text without NUL bytes. Files are stored as
patchset/<userid>/<nodeid>-<timestamp>-<n>.patch
with the user ID derived from the JWT email, so uploads are easy to
attribute, browse and clean up. Deeper validation (unified diff
format, path containment) still happens in the patchset service
before the patch is applied.
The patchset service accepts these URLs through the storage domain
added to its allowed domains at startup.
Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
bhcopeland
reviewed
Jul 28, 2026
| f"Patch {patch_name} contains binary data" | ||
| ) | ||
|
|
||
| lines = patch_data.split(b"\n") |
Member
There was a problem hiding this comment.
lines = [ln.rstrip(b"\r") for ln in patch_data.split(b"\n")]
I'd normalise this to catch these use cases
Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This series restore patchset, patch bugs existed before, adapt to new changes (durable events, etc), add functionality to upload patches to storage.