Skip to content

Moved taint checks into SimpleTest.js#297

Merged
leeN merged 4 commits intoSAP:mainfrom
leeN:unify_mochi_taint_tests
Jun 10, 2025
Merged

Moved taint checks into SimpleTest.js#297
leeN merged 4 commits intoSAP:mainfrom
leeN:unify_mochi_taint_tests

Conversation

@leeN
Copy link
Collaborator

@leeN leeN commented May 28, 2025

Currently, we have a ton of duplication across the mochitests. Additionally, they contain slight differences, e.g., nullable property access vs hard property access.

This PR aims to unify this.
Tasks to be done:

  • unify taint check functions
  • Look for duplication and add new standard taint check functions (e.g., source checks)
  • Generally, unify coding style, some tests are manual and do not require manually calling finish().

@leeN leeN requested a review from tmbrbr May 28, 2025 21:06
@leeN leeN self-assigned this May 28, 2025
@leeN leeN marked this pull request as draft May 28, 2025 21:06
For every test where we do not check for __taintreport events, this unifies the usage of the mochitest approaches.

We now generally favor the following kind of test style:
```JavaScript
add_task(async function test_something() {
  ...
});
```

over the way where you manually have to call `SimpleTest.finish()` despite only doing non event based testing.
@leeN
Copy link
Collaborator Author

leeN commented May 28, 2025

It is rather ugly that the taint checks are located in SimpleTest.js. However, when trying to create a helper script that contains the taint checks, this did fail with function not defined errors. Not quite sure what went wrong there, as the same works for DOMPurify..

@leeN
Copy link
Collaborator Author

leeN commented May 28, 2025

One side effect of the second commit is the increased number of tests, but I feel that is not a problem. It is rather positive, as it avoids masking some errors in case of exceptions.

@leeN leeN mentioned this pull request Jun 2, 2025
leeN added 2 commits June 6, 2025 08:43
Before we had a ton of copy and pasted code for the purpose of checking whether some piece of JS code triggers a __taintreport event and whether that has the correct sink.

This unifies the handling/setup into one method that is used across our tests now.
@leeN leeN marked this pull request as ready for review June 6, 2025 09:13
@leeN leeN changed the title WIP: Moved taint checks into SimpleTest.js Moved taint checks into SimpleTest.js Jun 8, 2025
Copy link
Contributor

@tmbrbr tmbrbr left a comment

Choose a reason for hiding this comment

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

Looks great!

@leeN leeN merged commit f748e85 into SAP:main Jun 10, 2025
11 of 12 checks passed
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.

2 participants