Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify some of the appid code, improve script tag detection and add validation #2519

Merged
merged 13 commits into from
Sep 25, 2024

Conversation

AE-MS
Copy link
Contributor

@AE-MS AE-MS commented Sep 23, 2024

Description

The codebase currently has some duplication in a few areas:

  • Multiple different ways of accepting, storing, and validation of app ids
  • Multiple (different) ways of identifying the presence of script tags

Main changes in the PR:

  1. Unify all App Id logic around the AppId type -- note: the input parameters for appids unfortunately cannot be changed from string to AppId at this time since that would be a API-breaking change.
  2. Remove incomplete implementation of HTML entity decoding (since it was missing a lot of other entities) and remove need for such functionality by just looking specifically for encoded script tags (script tag searching was the only function using the entity decoding helper)
  3. All related tests that were using a regex to test for a specific substring were updated to be case-insensitive, since they are intentionally not looking to exact match the error message (the exact error message is not part of the API contract)
  4. Explicitly added unit tests for validate the hasScriptTags function
  5. Updated unit tests for functions that were doing app id validation to no longer test on the exact error message (it's not part of the API contract) and only test for the presence of a descriptive word (in a case-insensitive way). Relatedly: updated these tests to more directly use Jest-provided affordances for testing async functions that throw errors.
  6. Updated unit test usage of domains not controlled by Microsoft to use Microsoft-owned domains

Validation

Validation performed:

  1. All existing and new unit tests were run and passed
  2. Private helper functions that previously didn't have specific unit tests now do

Unit Tests added:

Yes

End-to-end tests added:

No

Additional Requirements

Change file added:

Yes

Next/remaining steps:

There are still a few places that use the old validateId function but that I did not change to use AppId. This is because the data being validated were not actually an app ids -- they are fields like "title id" and "oauthConfigId". It's not clear why these other ID types were using the app id validation code so I can't tell whether to unify them now. I have questions out to the owners about this and will follow up in a subsequent change.

@AE-MS AE-MS changed the title Unify some of the appid code Unify some of the appid code, improve script tag detection and add validation Sep 23, 2024
@AE-MS AE-MS marked this pull request as ready for review September 23, 2024 19:45
@AE-MS AE-MS requested a review from a team as a code owner September 23, 2024 19:45
@AE-MS AE-MS enabled auto-merge (squash) September 24, 2024 23:05
Copy link
Contributor

@erinha erinha left a comment

Choose a reason for hiding this comment

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

:shipit:

@AE-MS AE-MS merged commit 60eb045 into main Sep 25, 2024
28 checks passed
@AE-MS AE-MS deleted the AE_MS/unify_appid2 branch September 25, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants