update mini sysbench to use kubernetes - #3056
Open
jycor wants to merge 2 commits into
Open
Conversation
Contributor
Author
|
#benchmark |
Contributor
Author
|
#benchmark |
|
SummaryCoverage spans authorization boundaries, benchmark dispatch rules, external result retrieval, pull-request reporting, failure handling, and comment lifecycle behavior. It includes normal flows plus malformed-input, missing-data, access-denial, stale-result, and cross-identity edge cases, with generally healthy behavior aside from important publication-safety gaps. Not safe to merge yet — this PR introduces a high-severity risk that results can be published to the wrong pull request, along with medium-severity malformed issue handling that can route publication to an invalid target. An unrelated medium-severity concurrency issue remains a flag for later and does not drive the merge decision. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Overlapping reports can duplicate or overwrite comments
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
| runs-on: ubuntu-latest | ||
|
|
||
| runs-on: ubuntu-22.04 | ||
| if: ${{ github.event.client_payload.issue_number != -1 }} |
There was a problem hiding this comment.
Malformed issue values reach report publication
What failed: The workflow did not reject malformed issue numbers before starting the report publication path. Missing, empty, and non-numeric values were treated as eligible, then became an invalid issue target.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: When the issue number is missing or malformed, the report job can fail or try to publish to an invalid issue instead of stopping safely. Reports for affected runs may not be posted to the intended pull request.
- Steps to Reproduce:
- Create a repository-dispatch payload with
issue_numberabsent, empty, or set to a non-numeric value such asabc. - Run the
mini-sysbenchworkflow with otherwise valid benchmark metadata. - Observe that the job condition allows the job to proceed instead of rejecting the payload.
- Observe that the publication script parses the value as
NaNand passes it to the GitHub comments API calls.
- Create a repository-dispatch payload with
- Stub / mock content: The test used a local workflow simulator with mocked S3 retrieval and pull-request comment calls; no GitHub credentials or production endpoints were used.
- Code Analysis: The changed job condition at
.github/workflows/mini-sysbench.yml:14only excludes the numeric sentinel-1:${{ github.event.client_payload.issue_number != -1 }}. Forundefined,'', and'abc', this expression is true, so the job continues. In theactions/github-scriptstep, line 37 readsconst issue_number = parseInt(ISSUE_NUMBER, 10);; each of those malformed values producesNaN. The value is then supplied asissue_numbertogithub.rest.issues.listCommentsat lines 45-49 and togithub.rest.issues.createCommentat lines 64-68. The local executable reproduction confirmed eligible=true and parsed=NaN for the malformed cases, while the numeric string'42'remained eligible and parsed to 42. The smallest practical fix is to validate that the payload is a positive integer before the job proceeds, or to add an explicit early exit in the script before any GitHub API call; the validation should also reject the string sentinel'-1'and other non-numeric values. - Why this is likely a bug: The test's expected behavior is to skip or visibly reject malformed values before publication, and the source path contradicts that requirement in an executable way. The gate admits malformed input,
parseIntconverts it toNaN, and the result is used as the target for GitHub issue API requests. This is a production workflow defect rather than a browser or simulator artifact because the same behavior follows directly from the checked-in expression and JavaScript parsing semantics. A positive numeric string remains supported, so the targeted fix is input validation at the new gate or immediately before the API calls, not a rewrite of the publication logic.
Relevant code
.github/workflows/mini-sysbench.yml:14
if: ${{ github.event.client_payload.issue_number != -1 }}.github/workflows/mini-sysbench.yml:35-37
const { ACTOR, FORMAT, ISSUE_NUMBER, JOB_TYPE, GITHUB_WORKSPACE } = process.env;
const { owner, repo } = context.repo;
const issue_number = parseInt(ISSUE_NUMBER, 10);.github/workflows/mini-sysbench.yml:45-49
const { data: comments } = await github.rest.issues.listComments({
issue_number: issue_number,
owner: context.repo.owner,
repo: context.repo.repo
});.github/workflows/mini-sysbench.yml:64-68
await github.rest.issues.createComment({
issue_number: issue_number,
owner: context.repo.owner,
repo: context.repo.repo,
body: `${commentMarker}\n@${ACTOR} ${FORMAT}\n${resData}`
});Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Malformed issue values reach report publication**
**What failed:** The workflow did not reject malformed issue numbers before starting the report publication path. Missing, empty, and non-numeric values were treated as eligible, then became an invalid issue target.
- **Impact:** When the issue number is missing or malformed, the report job can fail or try to publish to an invalid issue instead of stopping safely. Reports for affected runs may not be posted to the intended pull request.
- **Steps to reproduce:**
1. Create a repository-dispatch payload with `issue_number` absent, empty, or set to a non-numeric value such as `abc`.
2. Run the `mini-sysbench` workflow with otherwise valid benchmark metadata.
3. Observe that the job condition allows the job to proceed instead of rejecting the payload.
4. Observe that the publication script parses the value as `NaN` and passes it to the GitHub comments API calls.
- **Stub / mock content:** The test used a local workflow simulator with mocked S3 retrieval and pull-request comment calls; no GitHub credentials or production endpoints were used.
- **Code analysis:** The changed job condition at `.github/workflows/mini-sysbench.yml:14` only excludes the numeric sentinel `-1`: `${{ github.event.client_payload.issue_number != -1 }}`. For `undefined`, `''`, and `'abc'`, this expression is true, so the job continues. In the `actions/github-script` step, line 37 reads `const issue_number = parseInt(ISSUE_NUMBER, 10);`; each of those malformed values produces `NaN`. The value is then supplied as `issue_number` to `github.rest.issues.listComments` at lines 45-49 and to `github.rest.issues.createComment` at lines 64-68. The local executable reproduction confirmed eligible=true and parsed=NaN for the malformed cases, while the numeric string `'42'` remained eligible and parsed to 42. The smallest practical fix is to validate that the payload is a positive integer before the job proceeds, or to add an explicit early exit in the script before any GitHub API call; the validation should also reject the string sentinel `'-1'` and other non-numeric values.
- **Why this is likely a bug:** The test's expected behavior is to skip or visibly reject malformed values before publication, and the source path contradicts that requirement in an executable way. The gate admits malformed input, `parseInt` converts it to `NaN`, and the result is used as the target for GitHub issue API requests. This is a production workflow defect rather than a browser or simulator artifact because the same behavior follows directly from the checked-in expression and JavaScript parsing semantics. A positive numeric string remains supported, so the targeted fix is input validation at the new gate or immediately before the API calls, not a rewrite of the publication logic.
**Relevant code:**
`.github/workflows/mini-sysbench.yml:14`
~~~yaml
if: ${{ github.event.client_payload.issue_number != -1 }}
~~~
`.github/workflows/mini-sysbench.yml:35-37`
~~~javascript
const { ACTOR, FORMAT, ISSUE_NUMBER, JOB_TYPE, GITHUB_WORKSPACE } = process.env;
const { owner, repo } = context.repo;
const issue_number = parseInt(ISSUE_NUMBER, 10);
~~~
`.github/workflows/mini-sysbench.yml:45-49`
~~~javascript
const { data: comments } = await github.rest.issues.listComments({
issue_number: issue_number,
owner: context.repo.owner,
repo: context.repo.repo
});
~~~
`.github/workflows/mini-sysbench.yml:64-68`
~~~javascript
await github.rest.issues.createComment({
issue_number: issue_number,
owner: context.repo.owner,
repo: context.repo.repo,
body: `${commentMarker}\n@${ACTOR} ${FORMAT}\n${resData}`
});
~~~
| aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }} | ||
| aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | ||
| aws-region: us-west-2 | ||
| - name: Get benchmark results |
There was a problem hiding this comment.
Benchmark results can reach the wrong pull request
What failed: The workflow posted a benchmark result from another pull request to issue 42 instead of rejecting the mismatched object and issue.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: Reviewers may see benchmark results from a different pull request posted on their issue. This can mislead approval decisions and expose unrelated results across pull requests.
- Steps to Reproduce:
- Provide a valid local result object under the key other-pr/results.log with the content RESULT-FOR-B.
- Send a payload with that bucket and key but set the issue number to 42, representing a different pull request.
- Run the reporting workflow and inspect the published benchmark comment for issue 42.
- Confirm that the workflow exits successfully and the comment contains RESULT-FOR-B instead of rejecting the mismatched identities.
- Stub / mock content: The run used an isolated local AWS stub and controlled result fixtures to avoid real AWS credentials and services; no production systems or customer data were used.
- Code Analysis: The changed .github/workflows/mini-sysbench.yml lines 24-29 pass github.event.client_payload.bucket and github.event.client_payload.key independently to aws s3api get-object, so any readable object selected by the payload becomes results.log. In the later github-script at lines 35-40, ISSUE_NUMBER is read from the same payload and parsed separately while results.log is read without checking its key, metadata, producer identity, or issue association. Lines 44-68 then use that independently parsed issue_number for both issues.listComments and issues.createComment/updateComment. The PR diff replaced the old local PR/main benchmark flow with this externally selected S3 handoff, and the new changed path contains no binding, signature, or consistency check. The smallest practical fix is to carry a trusted issue identity with the produced object, verify it against ISSUE_NUMBER before the GitHub API calls, and fail the job before publication when they differ; deriving the target issue from a trusted object key is another targeted option.
- Why this is likely a bug: The controlled reproduction paired issue 42 with other-pr/results.log, and the workflow returned exit 0 while publishing RESULT-FOR-B to issue 42. This is not explained by the unavailable browser page: the workflow source independently shows that the object contents are read and then sent to the issue number without an identity comparison. A benchmark comment is user-visible evidence used to assess a pull request, so accepting a valid but unrelated object can mislead reviewers and disclose results across pull requests. Rejecting the pair or verifying a trusted binding before list/update/createComment directly prevents the observed failure without requiring a broad redesign.
Relevant code
.github/workflows/mini-sysbench.yml:24-29
- name: Get benchmark results
id: get-results
run: aws s3api get-object --bucket="$BUCKET" --key="$KEY" results.log
env:
KEY: ${{ github.event.client_payload.key }}
BUCKET: ${{ github.event.client_payload.bucket }}.github/workflows/mini-sysbench.yml:35-40
const { ACTOR, FORMAT, ISSUE_NUMBER, JOB_TYPE, GITHUB_WORKSPACE } = process.env;
const { owner, repo } = context.repo;
const issue_number = parseInt(ISSUE_NUMBER, 10);
const fs = require('fs').promises;
const resData = await fs.readFile(`${GITHUB_WORKSPACE}/results.log`, 'utf8');.github/workflows/mini-sysbench.yml:44-68
const { data: comments } = await github.rest.issues.listComments({
issue_number: issue_number,
owner: context.repo.owner,
repo: context.repo.repo
});
...
await github.rest.issues.createComment({
issue_number: issue_number,
owner: context.repo.owner,
repo: context.repo.repo,
body: `${commentMarker}\n@${ACTOR} ${FORMAT}\n${resData}`
});Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — Benchmark results can reach the wrong pull request**
**What failed:** The workflow posted a benchmark result from another pull request to issue 42 instead of rejecting the mismatched object and issue.
- **Impact:** Reviewers may see benchmark results from a different pull request posted on their issue. This can mislead approval decisions and expose unrelated results across pull requests.
- **Steps to reproduce:**
1. Provide a valid local result object under the key other-pr/results.log with the content RESULT-FOR-B.
2. Send a payload with that bucket and key but set the issue number to 42, representing a different pull request.
3. Run the reporting workflow and inspect the published benchmark comment for issue 42.
4. Confirm that the workflow exits successfully and the comment contains RESULT-FOR-B instead of rejecting the mismatched identities.
- **Stub / mock content:** The run used an isolated local AWS stub and controlled result fixtures to avoid real AWS credentials and services; no production systems or customer data were used.
- **Code analysis:** The changed .github/workflows/mini-sysbench.yml lines 24-29 pass github.event.client_payload.bucket and github.event.client_payload.key independently to aws s3api get-object, so any readable object selected by the payload becomes results.log. In the later github-script at lines 35-40, ISSUE_NUMBER is read from the same payload and parsed separately while results.log is read without checking its key, metadata, producer identity, or issue association. Lines 44-68 then use that independently parsed issue_number for both issues.listComments and issues.createComment/updateComment. The PR diff replaced the old local PR/main benchmark flow with this externally selected S3 handoff, and the new changed path contains no binding, signature, or consistency check. The smallest practical fix is to carry a trusted issue identity with the produced object, verify it against ISSUE_NUMBER before the GitHub API calls, and fail the job before publication when they differ; deriving the target issue from a trusted object key is another targeted option.
- **Why this is likely a bug:** The controlled reproduction paired issue 42 with other-pr/results.log, and the workflow returned exit 0 while publishing RESULT-FOR-B to issue 42. This is not explained by the unavailable browser page: the workflow source independently shows that the object contents are read and then sent to the issue number without an identity comparison. A benchmark comment is user-visible evidence used to assess a pull request, so accepting a valid but unrelated object can mislead reviewers and disclose results across pull requests. Rejecting the pair or verifying a trusted binding before list/update/createComment directly prevents the observed failure without requiring a broad redesign.
**Relevant code:**
`.github/workflows/mini-sysbench.yml:24-29`
~~~yaml
- name: Get benchmark results
id: get-results
run: aws s3api get-object --bucket="$BUCKET" --key="$KEY" results.log
env:
KEY: ${{ github.event.client_payload.key }}
BUCKET: ${{ github.event.client_payload.bucket }}
~~~
`.github/workflows/mini-sysbench.yml:35-40`
~~~javascript
const { ACTOR, FORMAT, ISSUE_NUMBER, JOB_TYPE, GITHUB_WORKSPACE } = process.env;
const { owner, repo } = context.repo;
const issue_number = parseInt(ISSUE_NUMBER, 10);
const fs = require('fs').promises;
const resData = await fs.readFile(`${GITHUB_WORKSPACE}/results.log`, 'utf8');
~~~
`.github/workflows/mini-sysbench.yml:44-68`
~~~javascript
const { data: comments } = await github.rest.issues.listComments({
issue_number: issue_number,
owner: context.repo.owner,
repo: context.repo.repo
});
...
await github.rest.issues.createComment({
issue_number: issue_number,
owner: context.repo.owner,
repo: context.repo.repo,
body: `${commentMarker}\n@${ACTOR} ${FORMAT}\n${resData}`
});
~~~
Contributor
Footnotes
|
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
1 participant
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.
No description provided.