Add bundle size reports to the PR description #5280
Conversation
Leah-Xia-Microsoft
requested review from
JamesBurnside,
alkwa-msft,
dmceachernmsft,
carocao-msft,
edwardlee-msft,
PorterNan,
mgamis-msft,
prabhjot-msft,
jimchou-dev,
vhuseinova-msft,
jpeng-ms,
waynemo,
emlynmac,
JoshuaLai and
palatter
@azure/communication-react jest test coverage for stable.
|
@azure/communication-react jest test coverage for beta.
|
Leah-Xia-Microsoft
added
the
does not need changelog
…ription' of https://github.com/Azure/communication-ui-library into leahxia/update-ci-to-add-bundle-size-reports-to-pr-description
This reverts commit 841f4c0.
| uses: peter-evans/create-or-update-comment@v2 | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
| - name: Update PR Description |
There was a problem hiding this comment.
@edwardlee-msft given this PR #5269 do we need to ensure any santization is happening if someone injects scripts into the PR description?
Co-authored-by: James Burnside <[email protected]> Signed-off-by: Leah Xia <[email protected]>
| content: | | ||
| <!-- ${{ matrix.app }} bundle-size-report --> | ||
| ## ${{ matrix.app }} bundle size is ***${{ steps.bundles.outputs.change }}***. | ||
| - Current size: ${{ steps.bundles.outputs.current_size }} | ||
| - Base size: ${{ steps.bundles.outputs.base_size}} | ||
| - Diff size: ${{ steps.bundles.outputs.diff}} | ||
| edit-mode: replace | ||
| - Base size: ${{ steps.bundles.outputs.base_size }} | ||
| - Diff size: ${{ steps.bundles.outputs.diff }} | ||
| <!-- end-bundle-size-report --> |
There was a problem hiding this comment.
these ${{values}}, can be areas where RCE can happen if not santization is implemented.
Especially if this is passed through some kind of bash command.
Can the steps.bundles.ouputs values be manipulated or edited by a User?
There was a problem hiding this comment.
I think we're safe from attacks since it's this CI workflow that build the sample apps to generate and upload the bundle size reports to gist, then in the following steps, it downloads the reports and calculate and generate the variables we uses here, such as steps.bundles.outputs.change.
If all the variables are provided by this workflow it self, we should be ok?
@edwardlee-msft and @JamesBurnside please let me know what you think. Should we implement sanitization anyways just to be safe?
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
| - name: Update PR Description | ||
| uses: nefrob/[email protected] |
There was a problem hiding this comment.
Does nefrob/[email protected] apply sanitization?
There was a problem hiding this comment.
Unfortunately,
nefrob/[email protected] does not handle sanitization:
https://github.com/nefrob/pr-description/blob/master/index.js
What
Why
How Tested
After adding a testing commit to increase the bundle size, the chat bundle size has been updated in the description.
And the CI job fails with the correct error message
After adding the
significant bundle size changelabel, the bundleSizeCheck step is ignored and the CI job passesProcess & policy checklist
Is this a breaking change?
Calling bundle size is not changed.
CallWithChat bundle size is not changed.
Chat bundle size is not changed.