Conversation
Signed-off-by: Zoe Lembrik <me@immz.fish>
Signed-off-by: Zoe Lembrik <me@immz.fish>
dab8e16 to
6571c99
Compare
web_src/src/pages/workflowv2/mappers/buildkite/on_build_finished.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Zoe Lembrik <me@immz.fish>
Signed-off-by: Zoe Lembrik <me@immz.fish>
Signed-off-by: Zoe Lembrik <me@immz.fish>
Signed-off-by: Zoe Lembrik <me@immz.fish>
|
@immz4 thanks for the submission and a clear overview! As you already mentioned - creating webhooks manually is not ideal. Even though they don't support API endpoint for the organization notification webhooks - does it make sense to use pipeline webhooks? If I understand correctly |
@AleksandarCole Unfortunately these are incoming webhooks. To quote GraphQL API doc:
So, they work like this: |
|
@immz4 understood. Can we then work on improving the UX of configuring this? I did not manage to connect it based on the instructions in the UI. Even after reviewing the video I'm not clear how to connect this. We can ask the user to input the link to their organization (eg. We also need to generate webhook url for them, I am unsure what this means |
Signed-off-by: Zoe Lembrik <me@immz.fish>
Signed-off-by: Zoe Lembrik <me@immz.fish>
|
@AleksandarCole addressed UX, added setup helpers Integration setup flow: |
|
I can also record new video to demonstrate UX changes. |
Signed-off-by: Zoe Lembrik <me@immz.fish>
|
Hey @immz4 this is much better, thanks! I've managed to run it - great direction, I love that you included those env vars + metadata. Have few changes that we need to make and one problem I came across: Environment variable brokenWhen I add environment variable I cannot save the component: Wrong state on failIf the pipeline fails state should not be
Wrong component iconIcons render as puzzle (because they are not using BuildKite icon) Build metadata not setEven though I've configured build metadata it doesn't seem to be set Update details tabIn the details tab, let's have two timestamps at the top (Started at, and Completed at), and Build URL, feel free to delete the rest.
From the trigger component detail tab, let's also have timestamp at the top (single one in this case) and remove Build State (or result, having both is redundant), Build Number, Project, Organization (those are empty) Integration Setup UXThe organization URL is prefilled with user email (which is very strange) and also API token is prefilled with something also. Let's make sure those are empty and that user needs to add them. The instructions for creating the webhook appear only after I save the integration for the first time (with API token) - any way for them to be there from the start - or appear once user enters the url? Also, would be good if we had a link in the description for API token that immediately leads you to https://buildkite.com/user/api-access-tokens as it's a fixed url (same for everyone). |
|
Hey, @AleksandarCole, regarding your comments
That is indeed very strange, since I do not have this problem.
|
Signed-off-by: Zoe Lembrik <me@immz.fish>
Signed-off-by: Zoe Lembrik <me@immz.fish>
Signed-off-by: Zoe Lembrik <me@immz.fish>
Signed-off-by: Zoe Lembrik <me@immz.fish>
| sigTime := time.Unix(timestamp, 0) | ||
| if time.Since(sigTime) > MaxReplayAge { | ||
| return fmt.Errorf("timestamp too old: max age is %v", MaxReplayAge) | ||
| } |
There was a problem hiding this comment.
Webhook signature verification accepts future timestamps
Low Severity
The replay protection check time.Since(sigTime) > MaxReplayAge only rejects timestamps older than 5 minutes. For a future timestamp, time.Since returns a negative duration, which is always less than MaxReplayAge, so the check passes. This means a signature with a far-future timestamp remains valid indefinitely, weakening the replay protection window.
|
@immz4 after looking a bit deeper on this one, it seems clear to me that the webhook setup should be done as part of the trigger setup itself, and not the integration setup. A few considerations that led me to this conclusion:
For reference on how to handle webhook setup as part of trigger setup, take a look at the dockerhub.onImagePush trigger. |
Signed-off-by: Zoe Lembrik <me@immz.fish>
Signed-off-by: Zoe Lembrik <me@immz.fish>
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Pipeline filter silently bypassed on missing slug
Medium Severity
The pipeline filter uses a triple-nested optional type-assertion chain. If payload["pipeline"] is absent or present but lacks a slug key, both inner assertions fail silently and the filter is skipped entirely — the event is accepted and emitted regardless of which pipeline sent it. A webhook payload that doesn't include pipeline.slug (e.g., from a different event shape or a misconfigured Buildkite webhook) will therefore bypass the configured pipeline filter and trigger unintended workflow executions.
Signed-off-by: Zoe Lembrik <me@immz.fish>
Signed-off-by: Zoe Lembrik <me@immz.fish>
Signed-off-by: Zoe Lembrik <me@immz.fish>
|
@lucaspin Moved webhook setup to trigger. Current flow:
User needs to select the pipeline and save the trigger in order to get webhook url and token
After that user can click a button and copy over params to Buildkite |
| slugPattern := regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_-]*[a-zA-Z0-9]$`) | ||
| if slugPattern.MatchString(strings.TrimSpace(orgInput)) { | ||
| return strings.TrimSpace(orgInput), nil | ||
| } |
There was a problem hiding this comment.
Slug regex rejects valid single-character org slugs
Low Severity
The slugPattern regex ^[a-zA-Z0-9][a-zA-Z0-9_-]*[a-zA-Z0-9]$ requires at least two characters because it demands both a start and end character class match. A valid single-character org slug (e.g., "a") would be rejected with an "invalid organization format" error, even though Buildkite permits it.
Signed-off-by: Zoe Lembrik <me@immz.fish>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Pipeline filter silently bypassed on missing slug
Low Severity
The pipeline filter in HandleWebhook is nested inside two type assertions. If payload["pipeline"] is absent or not a map[string]any, or if pipeline["slug"] is not a string, both assertions fail silently — the filter is completely bypassed and the event proceeds to trigger a workflow execution regardless of which pipeline sent it. The same silent-bypass pattern applies to the branch filter. Any build.finished event that lacks expected fields passes unfiltered.
| slugPattern := regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_-]*[a-zA-Z0-9]$`) | ||
| if slugPattern.MatchString(strings.TrimSpace(orgInput)) { | ||
| return strings.TrimSpace(orgInput), nil | ||
| } |
There was a problem hiding this comment.
Single-character org slugs rejected as invalid
Low Severity
The slug validation regex ^[a-zA-Z0-9][a-zA-Z0-9_-]*[a-zA-Z0-9]$ requires a minimum of two characters (one anchored at each end with zero-or-more in between), so any single-character org slug fails validation and returns an error. This is also inconsistent: the same single-character slug succeeds if the user provides it as a full URL (e.g., https://buildkite.com/a) because the URL pattern matches first, bypassing the slug pattern entirely.
|
@lucaspin @AleksandarCole Hi, any updates on PR merge? |
|
Hey @immz4, it looks good, our internal team will need to do some final polish and updates before the merge, but I think this contract is completed successfully 👍 |













Implements #2360
This PR implements integration for the Buildkite, which allows users to:
Integration requires following:
read_organizations,read_user,read_pipelines,read_buildsandwrite_buildspermissionsbuild.finishedeventAPI keys are created in Personal Settings > API Access Tokens
Webhook is created in Organization Settings > Notification Services with following parameters:
https://<backend_address>/api/v1/integrations/<integration_id>/webhook(URL is provided during the setup)build.finishedLimitations:
Video: https://youtu.be/YOXcCU9rAS0