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

✨ [RUMF-1175] collect reports and csp violation #1332

Merged
merged 26 commits into from
Mar 4, 2022

Conversation

amortemousque
Copy link
Contributor

@amortemousque amortemousque commented Feb 11, 2022

Motivation

Collect reports and csp violation.
Feature under feature flag

Worth reviewing commit by commit

Changes

  • add a reportObserbvable
  • collect error reports in RUM
  • collect all reports in Logs

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2022

Codecov Report

Merging #1332 (7124d93) into main (e58d489) will increase coverage by 0.07%.
The diff coverage is 94.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1332      +/-   ##
==========================================
+ Coverage   89.75%   89.83%   +0.07%     
==========================================
  Files         102      105       +3     
  Lines        4207     4297      +90     
  Branches      944      956      +12     
==========================================
+ Hits         3776     3860      +84     
- Misses        431      437       +6     
Impacted Files Coverage Δ
...omain/rumEventsCollection/error/errorCollection.ts 70.83% <0.00%> (-3.08%) ⬇️
...main/rumEventsCollection/error/trackReportError.ts 77.77% <77.77%> (ø)
packages/core/test/stubReportApis.ts 93.33% <93.33%> (ø)
...ackages/core/src/domain/report/reportObservable.ts 97.36% <97.36%> (ø)
packages/core/src/tools/error.ts 90.62% <100.00%> (+0.62%) ⬆️
packages/core/src/tools/utils.ts 86.69% <100.00%> (+0.06%) ⬆️
packages/logs/src/boot/startLogs.ts 85.54% <100.00%> (+2.20%) ⬆️
packages/logs/src/domain/configuration.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e58d489...7124d93. Read the comment docs.

@amortemousque amortemousque force-pushed the aymeric/collect-reports-and-csp-violation branch from 63252da to f960da4 Compare February 11, 2022 10:57
@amortemousque amortemousque force-pushed the aymeric/collect-reports-and-csp-violation branch 4 times, most recently from 4d666f7 to ca72d57 Compare February 25, 2022 11:06
@amortemousque amortemousque marked this pull request as ready for review February 25, 2022 11:10
@amortemousque amortemousque requested a review from a team as a code owner February 25, 2022 11:10
@amortemousque amortemousque force-pushed the aymeric/collect-reports-and-csp-violation branch 2 times, most recently from 48fd096 to 9bcb3d5 Compare February 25, 2022 15:17
@amortemousque amortemousque force-pushed the aymeric/collect-reports-and-csp-violation branch from 9bcb3d5 to fcd4950 Compare February 25, 2022 15:19
packages/core/src/domain/report/reportObservable.spec.ts Outdated Show resolved Hide resolved
packages/core/src/domain/report/reportObservable.spec.ts Outdated Show resolved Hide resolved
packages/core/src/domain/report/reportObservable.ts Outdated Show resolved Hide resolved
packages/core/test/specHelper.ts Outdated Show resolved Hide resolved
packages/core/src/domain/report/reportObservable.ts Outdated Show resolved Hide resolved
@amortemousque amortemousque force-pushed the aymeric/collect-reports-and-csp-violation branch from f5e2587 to 875cdc3 Compare March 1, 2022 16:09
@amortemousque amortemousque requested a review from a team as a code owner March 1, 2022 16:23
@amortemousque amortemousque force-pushed the aymeric/collect-reports-and-csp-violation branch from f310e7b to 5555282 Compare March 2, 2022 12:53
@amortemousque amortemousque force-pushed the aymeric/collect-reports-and-csp-violation branch from df6f027 to a6f592f Compare March 2, 2022 16:46

function buildRawReportFromCspViolation(event: SecurityPolicyViolationEvent): RawReport {
const type = RawReportType.cspViolation
const message = `'${event.blockedURI}' blocked by '${event.effectiveDirective}' directive`
Copy link
Contributor Author

@amortemousque amortemousque Mar 2, 2022

Choose a reason for hiding this comment

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

After some thought, I think it is valuable to add the originalPolicy to the crafted message.
Could be done like so:
${event.blockedURI}' blocked by '${event.effectiveDirective}' directive of the policy "${event.originalPolicy}"

This way we will have message like:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Some directives could list a large number of domains 🤔
I'd say that it could be added in the stack to keep a minimal message.
We should maybe also truncate the policy to avoid sending too much data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it to the stack. Let me know what you think

Copy link
Member

Choose a reason for hiding this comment

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

LGTM!

@amortemousque amortemousque merged commit 0ef55b9 into main Mar 4, 2022
@amortemousque amortemousque deleted the aymeric/collect-reports-and-csp-violation branch March 4, 2022 15:00
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.

4 participants