From 46800094bb2a677aff6c8deaf9c8fbbcb7cc11a1 Mon Sep 17 00:00:00 2001 From: Skyler Grey Date: Sat, 27 Jul 2024 12:48:41 +0000 Subject: [PATCH 1/2] feat!: Update output path writing - Allow pull requests to create output paths Previously, it was not possible to get unmerged pull request outputs. I would like to access these (e.g. for hosting a preview version of a website in a similar way to https://docs.netlify.com/site-deploys/deploy-previews/ or https://vercel.com/docs/deployments/preview-deployments) These are now surfaced under `{owner}/{repo}/pulls/{pr number}` - Fix repository attribute name conflicts Previously there was no difference between samely-named attributes in different repositories. This has been changed so outputs are under `{owner}/{repo}/{branch}` - Allow full attribute names, still stripping path traversal Previously, presumably to prevent path traversal, if your attribute name contained slashes buildbot-nix would only take the last segment as an output. This has been replaced by interpreting slashes as subdirectories and refusing any segments which don't descend into a subdirectory (i.e. are attempting path traversal) - Still create outputs on skipped builds Previously when something was skipped, for example a build that was completed in a pull request, the output wouldn't be updated. This made the outputs directory quite unreliable. Outputs will now always be updated, no matter whether a build was actually executed. BREAKING-CHANGE: This stops old output locations being outputted. If you rely on these locations, you will need to update whatever relies on them. --- buildbot_nix/__init__.py | 59 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 53 insertions(+), 6 deletions(-) diff --git a/buildbot_nix/__init__.py b/buildbot_nix/__init__.py index 932708b..d048ec9 100644 --- a/buildbot_nix/__init__.py +++ b/buildbot_nix/__init__.py @@ -310,16 +310,54 @@ class UpdateBuildOutput(steps.BuildStep): self.project = project self.path = path + def join_traversalsafe(self, root: Path, joined: Path) -> Path: + root = root.resolve() + + for part in joined.parts: + new_root = (root / part).resolve() + + if not new_root.is_relative_to(root): + msg = f"Joined path attempted to traverse upwards when processing {root} against {part} (gave {new_root})" + raise ValueError(msg) + + root = new_root + + return root + + def join_all_traversalsafe(self, root: Path, *paths: Path) -> Path: + for path in paths: + root = self.join_traversalsafe(root, path) + + return root + + @defer.inlineCallbacks def run(self) -> Generator[Any, object, Any]: props = self.build.getProperties() - if props.getProperty("branch") != self.project.default_branch: + + pr = props.getProperty("pr_number") + + if not pr and props.getProperty("branch") != self.project.default_branch: return util.SKIPPED - attr = Path(props.getProperty("attr")).name + owner = Path(props.getProperty("owner")) + repo = Path(props.getProperty("repository_name")) + + target = Path(props.getProperty("branch")) if not pr else Path(f"pulls/{pr}") + + attr = Path(props.getProperty("attr")) + + try: + file = self.join_all_traversalsafe(self.path, owner, repo, target, attr) + except ValueError as e: + error_log: StreamLog = yield self.addLog("path_error") + error_log.addStderr(f"Path traversal prevented ... skipping update: {e}") + return util.FAILURE + + file.parent.mkdir(parents=True, exist_ok=True) + out_path = props.getProperty("out_path") - # XXX don't hardcode this - self.path.mkdir(parents=True, exist_ok=True) - (self.path / attr).write_text(out_path) + file.write_text(out_path) + return util.SUCCESS @@ -563,6 +601,7 @@ def nix_build_config( def nix_skipped_build_config( project: GitProject, worker_names: list[str], + outputs_path: Path | None = None, ) -> BuilderConfig: """Dummy builder that is triggered when a build is skipped.""" factory = util.BuildFactory() @@ -601,6 +640,14 @@ def nix_skipped_build_config( set_properties={"report_status": False}, ), ) + if outputs_path is not None: + factory.addStep( + UpdateBuildOutput( + project=project, + name="Update build output", + path=outputs_path, + ), + ) return util.BuilderConfig( name=f"{project.name}/nix-skipped-build", project=project.name, @@ -759,7 +806,7 @@ def config_for_project( retries=build_retries, post_build_steps=post_build_steps, ), - nix_skipped_build_config(project, [SKIPPED_BUILDER_NAME]), + nix_skipped_build_config(project, [SKIPPED_BUILDER_NAME], outputs_path), nix_register_gcroot_config(project, worker_names), nix_build_combined_config(project, worker_names), ], -- 2.45.2 From a5b34ea356ec66fe879e725489935322b85da448 Mon Sep 17 00:00:00 2001 From: Skyler Grey Date: Wed, 11 Sep 2024 20:01:13 +0000 Subject: [PATCH 2/2] fix: Correct output path with failed builds Previously, if a build failed to produce any output we would try to write "None" to the output file. This doesn't work, causing the job to error. Instead, we should skip writing the output and preserve the original "failed" status. --- buildbot_nix/__init__.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/buildbot_nix/__init__.py b/buildbot_nix/__init__.py index d048ec9..64ed144 100644 --- a/buildbot_nix/__init__.py +++ b/buildbot_nix/__init__.py @@ -339,6 +339,11 @@ class UpdateBuildOutput(steps.BuildStep): if not pr and props.getProperty("branch") != self.project.default_branch: return util.SKIPPED + out_path = props.getProperty("out_path") + + if not out_path: # if, e.g., the build fails and doesn't produce an output + return util.SKIPPED + owner = Path(props.getProperty("owner")) repo = Path(props.getProperty("repository_name")) @@ -355,7 +360,6 @@ class UpdateBuildOutput(steps.BuildStep): file.parent.mkdir(parents=True, exist_ok=True) - out_path = props.getProperty("out_path") file.write_text(out_path) return util.SUCCESS -- 2.45.2