microsoft / PowerApps-TestEngine

MIT License
100 stars 30 forks source link

Use JSON encoding instead of URI encoding for SetProperty #193

Closed clairewu0218 closed 1 year ago

clairewu0218 commented 1 year ago

Pull Request Template

Description

We used to use URI encoding to prevent JS script injection. This change is to using JSON encoding instead due to the fact that it's about JS code.

Checklist

NavneetThekkumpat commented 1 year ago

Do we have a plan on how to make these changes be consistent with our feature/JSSDK-server-side branch? I see this is branched off the main branch. Any reason why?

github-actions[bot] commented 1 year ago

Code Coverage

Package Line Rate Branch Rate Complexity Health
Microsoft.PowerApps.TestEngine 93% 91% 764
Summary 93% (2032 / 2176) 91% (471 / 518) 764

Minimum allowed line rate is 50%

clairewu0218 commented 1 year ago

Do we have a plan on how to make these changes be consistent with our feature/JSSDK-server-side branch? I see this is branched off the main branch. Any reason why?

Previously we used Playwright Page.EvaluateAsync with expression and arguments for handling encoding cases. As we don't have full control over the parameters, it runs into format problems with TypeScript in the new SDK. This PR will make most of the SetProperty the same as the JSSDK feature branch so that we can merge these two in the near future. The only thing left would be the new change regarding JS encoding.

NavneetThekkumpat commented 1 year ago

Do we have a plan on how to make these changes be consistent with our feature/JSSDK-server-side branch? I see this is branched off the main branch. Any reason why?

Previously we used Playwright Page.EvaluateAsync with expression and arguments for handling encoding cases. As we don't have full control over the parameters, it runs into format problems with TypeScript in the new SDK. This PR will make most of the SetProperty the same as the JSSDK feature branch so that we can merge these two in the near future. The only thing left would be the new change regarding JS encoding.

Okay that's great then. Given that we are supporting legacy (what main does now) even in our feature JSSDK branch. Thank you