Skip to content

ci: Add tsec_test for all ng_module targets. #24066

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

Merged
merged 3 commits into from
Jan 6, 2022
Merged

ci: Add tsec_test for all ng_module targets. #24066

merged 3 commits into from
Jan 6, 2022

Conversation

uraj
Copy link
Contributor

@uraj uraj commented Dec 7, 2021

Instead of updating ~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.

@uraj uraj requested review from devversion, jelbourn, mmalerba and a team as code owners December 7, 2021 23:54
@uraj
Copy link
Contributor Author

uraj commented Dec 7, 2021

For two source files, I have to do edits of setTimeout -> window.setTimeout since the transitive dependencies pull in Node.JS typing, making tsec resolve setTimeout to the Node.JS type instead of the DOM type.

@uraj
Copy link
Contributor Author

uraj commented Dec 7, 2021

@devversion Please take a look.

uraj added 2 commits December 8, 2021 00:08
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.
@devversion devversion self-assigned this Dec 8, 2021
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

Nice, thanks for making this change. Just a couple of comments, but overall looks great!

@uraj
Copy link
Contributor Author

uraj commented Dec 9, 2021

@devversion Thanks for the review! I've resolved the comments. PTAL.

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor comment. I'd like to avoid the type mixup in tsec vs. actual build

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

lgtm

@jelbourn jelbourn requested a review from andrewseguin December 9, 2021 22:23
],
"ban-element-setattribute": [
"../src/cdk/a11y/aria-describer/aria-reference.ts",
"../src/material-experimental/mdc-checkbox/checkbox.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if any of these exceptions should be eventually removed, or do they all contain valid reasons for this exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM for the tooling setup.

@uraj
Copy link
Contributor Author

uraj commented Dec 17, 2021

Is there anything else I need to do to get the PR merged?

@devversion devversion added action: merge The PR is ready for merge by the caretaker merge safe target: patch This PR is targeted for the next patch release and removed merge safe labels Dec 17, 2021
@devversion
Copy link
Member

@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.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 26, 2021
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021
@wagnermaciel wagnermaciel merged commit d93d9a3 into angular:master Jan 6, 2022
wagnermaciel pushed a commit that referenced this pull request Jan 6, 2022
* 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)
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants