ci: Add tsec_test for all ng_module targets. #24066
Conversation
|
For two source files, I have to do edits of |
|
@devversion Please take a look. |
Instead of modifying ~250 BUILD.bazel files, instrument the ng_module macro to conveniently create tsec_test for all modules. The ts_library macro is not instrumented since most of them are about testing, schematics and examples, which are not relevant to XSS. For those that are indeed security sensitive, tsec_test is manually added into individual BUILD.bazel files.
Nice, thanks for making this change. Just a couple of comments, but overall looks great!
|
@devversion Thanks for the review! I've resolved the comments. PTAL. |
LGTM, just one minor comment. I'd like to avoid the type mixup in tsec vs. actual build
| ], | ||
| "ban-element-setattribute": [ | ||
| "../src/cdk/a11y/aria-describer/aria-reference.ts", | ||
| "../src/material-experimental/mdc-checkbox/checkbox.ts", |
There was a problem hiding this comment.
Do you know if any of these exceptions should be eventually removed, or do they all contain valid reasons for this exception
There was a problem hiding this comment.
The ban-trustedtypes-createpolicy exception is expected. We won't be able to remove it until we have better support to create trusted types for SVG.
The ban-element-innerhtml-assignments exception is a false positive, because src/material/icon/trusted-types.ts defines its own TrustedTypes interface instead of pulling the type from the @types/trusted-types package. I'm not sure why it's coded that way, but technically it can be removed.
The "ban-element-setattribute" ones are tricky. I don't see anything that raise immediate alarms, but some of those exceptions are exposing the "setAttribute" interface to users of the the custom elements, which can be abused to bypass other checks (depending on the type of the underlying elements). Those are probably hard to remove, since it will require breaking changes to the programming interface of those custom elements. That said, so far we haven't seen XSS caused by those in google3, so it might not be a big issue.
|
Is there anything else I need to do to get the PR merged? |
|
@uraj I've added the appropriate labels. It should be in the merge queue now. I assume @andrewseguin's comment is answered and we could get this in regardless for now. |
* ci: Add tsec_test for all ng_module targets. Instead of modifying ~250 BUILD.bazel files, instrument the ng_module macro to conveniently create tsec_test for all modules. The ts_library macro is not instrumented since most of them are about testing, schematics and examples, which are not relevant to XSS. For those that are indeed security sensitive, tsec_test is manually added into individual BUILD.bazel files. * fixup! ci: Add tsec_test for all ng_module targets. * fixup! ci: Add tsec_test for all ng_module targets. (cherry picked from commit d93d9a3)
Instead of updating ~250
BUILD.bazelfiles, instrument theng_modulemacro to conveniently createtsec_testfor all modules. Thets_librarymacro is not instrumented since most of them are about testing, schematics and examples, which are not relevant to XSS. For those that are indeed security sensitive,tsec_testis manually added into individualBUILD.bazelfiles.The text was updated successfully, but these errors were encountered: