Skip to content

Revert "[SINT-4157] sanitise ultimate-pipeline.yml"#7627

Merged
link04 merged 1 commit into
masterfrom
revert-7582-ikraemer/SINT-4157-sanitize-azure-pipeline
Oct 6, 2025
Merged

Revert "[SINT-4157] sanitise ultimate-pipeline.yml"#7627
link04 merged 1 commit into
masterfrom
revert-7582-ikraemer/SINT-4157-sanitize-azure-pipeline

Conversation

@bouwkast

@bouwkast bouwkast commented Oct 6, 2025

Copy link
Copy Markdown
Collaborator

Reverts #7582

This is preventing us to run the action as expected as the regex appears to be incorrect.
Since we must trigger this ourselves there really isn't a risk here at the moment, will let the SINT team follow up to resolve.

This is blocking the testing of #7514

https://dev.azure.com/datadoghq/dd-trace-dotnet/_build/results?buildId=188595&view=logs&j=8299a7f3-61e2-5626-a12e-85b7b20deab1&t=e26f8d61-df24-5d93-564c-322f7221200d

@bouwkast
bouwkast requested a review from a team as a code owner October 6, 2025 20:03
@bouwkast
bouwkast requested a review from ikraemer-dd October 6, 2025 20:03
@github-actions github-actions Bot added the area:builds project files, build scripts, pipelines, versioning, releases, packages label Oct 6, 2025
@bouwkast

bouwkast commented Oct 6, 2025

Copy link
Copy Markdown
Collaborator Author

Since we must trigger this ourselves there really isn't a risk here at the moment, will let the SINT team follow up to resolve.

This is blocking the testing of #7514

https://dev.azure.com/datadoghq/dd-trace-dotnet/_build/results?buildId=188595&view=logs&j=8299a7f3-61e2-5626-a12e-85b7b20deab1&t=e26f8d61-df24-5d93-564c-322f7221200d

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

https://api.github.com/repos/DataDog/dd-trace-dotnet/actions/workflows/create-system-test-docker-base-images.yml/dispatches \
-d "{\"ref\":\"$REF\",\"inputs\":{\"azdo_build_id\":\"$(Build.BuildId)\",\"branch_name\":\"$BRANCH_NAME\"}}"

P1 Badge Restore branch name sanitization before invoking curl payload

The revert removes the branch-name regex entirely, yet the branch value is still injected directly into the JSON body sent via curl -d. Git permits branch names containing characters such as quotes or braces, so a PR from a fork could supply a value that breaks the JSON or injects extra fields (e.g., toggling is_release_version) when the payload is built here. Rather than dropping the check, the branch name should be validated or JSON-escaped before constructing the dispatch request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

@link04
link04 enabled auto-merge (squash) October 6, 2025 20:22
@link04
link04 merged commit 3e4b2e3 into master Oct 6, 2025
151 of 152 checks passed
@link04
link04 deleted the revert-7582-ikraemer/SINT-4157-sanitize-azure-pipeline branch October 6, 2025 21:39
@github-actions github-actions Bot added this to the vNext-v3 milestone Oct 6, 2025
ikraemer-dd added a commit that referenced this pull request Oct 8, 2025
## Summary of changes

## Reason for change

Follow-up of #7582, that
got reverted in #7627.
The present PR updates the regular expression: the hyphen should be at
the end or escaped to avoid being interpreted as a range operator.

## Implementation details

From
[source](https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap09.html):

> The <hyphen-minus> character shall be treated as itself if it occurs
first (after an initial '^', if any) or last in the list, or as an
ending range point in a range expression.

## Test coverage

Tested locally with this script
[test-branch-name.sh](https://github.com/user-attachments/files/22740574/test-branch-name.sh)
and got the following result, which reproduces the error in the PR and
validates the fix from this PR. In particular:
`maximo/otel-metrics-api-exporter` and
`maximo/otel-metrics-api-exporter.` are authorized patterns, while
`maximo/otel-metrics-api-exporter\;` is not. Test results confirm that
the corrected regex now implements this behavior correctly

```
$ bash ./test-branch-name.sh maximo/otel-metrics-api-exporter
Handling maximo/otel-metrics-api-exporter
reverted regex NOT matched (branch name deemed UNSAFE, will exit)
corrected regex matched (branch name deemed safe)

$ bash ./test-branch-name.sh maximo/otel-metrics-api-exporter.
Handling maximo/otel-metrics-api-exporter.
reverted regex NOT matched (branch name deemed UNSAFE, will exit)
corrected regex matched (branch name deemed safe)

$ bash ./test-branch-name.sh maximo/otel-metrics-api-exporter\;
Handling maximo/otel-metrics-api-exporter;
reverted regex NOT matched (branch name deemed UNSAFE, will exit)
corrected regex NOT matched (branch name deemed UNSAFE, will exit)
```


## Other details
<!-- Fixes #{issue} -->


<!--  ⚠️ Note:

Where possible, please obtain 2 approvals prior to merging. Unless
CODEOWNERS specifies otherwise, for external teams it is typically best
to have one review from a team member, and one review from apm-dotnet.
Trivial changes do not require 2 reviews.

MergeQueue is NOT enabled in this repository. If you have write access
to the repo, the PR has 1-2 approvals (see above), and all of the
required checks have passed, you can use the Squash and Merge button to
merge the PR. If you don't have write access, or you need help, reach
out in the #apm-dotnet channel in Slack.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:builds project files, build scripts, pipelines, versioning, releases, packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants