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-320] Remove url-polyfill dependency #294

Merged
merged 5 commits into from
Mar 12, 2020
Merged

Conversation

bcaudan
Copy link
Contributor

@bcaudan bcaudan commented Mar 11, 2020

The dependency used to polyfill URL API is modifying the global scope which caused an issue for a customer.
Since we don't use a lot of URL features, replace this polyfill by a simple implementation that support only our current needs.

@bcaudan bcaudan requested a review from a team as a code owner March 11, 2020 16:03
@codecov-io
Copy link

Codecov Report

Merging #294 into master will decrease coverage by 0.22%.
The diff coverage is 87.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #294      +/-   ##
==========================================
- Coverage   86.27%   86.04%   -0.23%     
==========================================
  Files          24       25       +1     
  Lines        1362     1405      +43     
  Branches      298      304       +6     
==========================================
+ Hits         1175     1209      +34     
- Misses        187      196       +9     
Impacted Files Coverage Δ
packages/core/src/requestCollection.ts 96.66% <ø> (-0.11%) ⬇️
packages/rum/src/rum.ts 83.75% <ø> (-1.25%) ⬇️
packages/core/src/utils.ts 96.15% <71.42%> (-1.79%) ⬇️
packages/rum/src/resourceUtils.ts 96.92% <85.71%> (-1.59%) ⬇️
packages/core/src/urlPolyfill.ts 90.00% <90.00%> (ø)
packages/rum/src/rum.entry.ts 69.38% <0.00%> (-2.05%) ⬇️

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 a4eb2e1...b0e05bb. Read the comment docs.

@bcaudan bcaudan merged commit 3c525e5 into master Mar 12, 2020
@bcaudan bcaudan deleted the bcaudan/url-polyfill branch March 12, 2020 08:59
@msokk
Copy link

msokk commented Mar 12, 2020

https://github.com/DataDog/browser-sdk/pull/294/files#diff-252c841276c8ed9e5cbdc5a3865065dfR47 - appending the <base> element causes CSP violations in stricter setups. Ours has 'none' for base-uri.

https://docs.datadoghq.com/real_user_monitoring/faq/content_security_policy/?tab=us - CSP article should include base-uri 'self'

@bcaudan
Copy link
Contributor Author

bcaudan commented Mar 13, 2020

Hi @msokk,
Thanks for raising the issue.

Could you confirm that:

  • it was not happening before this release
  • it only happens on IE browsers

Thanks

@msokk
Copy link

msokk commented Mar 13, 2020

IE does not support CSP (apart from X-Content-Security-Policy: sandbox).
This didn't break anything, apart from XHR url in datadog not being normalized, just our Sentry got spammed.

Started first seeing events March 12, ~13:30 GMT

@bcaudan
Copy link
Contributor Author

bcaudan commented Mar 13, 2020

@msokk is this issue reported by sentry on every browsers?

@msokk
Copy link

msokk commented Mar 13, 2020

Well, our reports mostly (99%) consisted of Chrome 80, as we have a lot of Chrome users, but there are some Firefox and Opera.

It's not a big issue, just some extra configuration might be needed from site side if they have set up security headers. We just have a very strict Content-Security-Policy by whitelisting third-party scripts/styles/fonts/etc...

So far no third-party piece of code touched the <base> element, so we had the https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/base-uri totally blocked :)

@bcaudan
Copy link
Contributor Author

bcaudan commented Mar 13, 2020

OK, I'd prefer to not need extra csp rules if we can avoid it.

Could you provide me one url where there is the issue?

@msokk
Copy link

msokk commented Mar 13, 2020

I already adjusted our CSP rules on all environments to stop eating up Sentry quota.

Here is a codepen with <meta http-equiv="Content-Security-Policy" content="base-uri 'none'"> - https://codepen.io/msokk/pen/eYNMWxy.

Violation is in console

@bcaudan
Copy link
Contributor Author

bcaudan commented Mar 13, 2020

Thanks a lot for that, I'll have a look and keep you updated if there is any changes on this topic.

bcaudan added a commit that referenced this pull request Mar 13, 2020
URL api was never used and for customer with base-uri csp restrictions, the polyfill raised an exception.

cf [bug report](#294 (comment))
bcaudan added a commit that referenced this pull request Mar 13, 2020
URL api was never used and for customer with base-uri csp restrictions, the polyfill raised an exception.

cf [bug report](#294 (comment))
bcaudan added a commit that referenced this pull request Mar 13, 2020
URL api was never used and for customer with base-uri csp restrictions, the polyfill raised an exception.
cf [bug report](#294 (comment))

Add similar restrictions to e2e pages.
bcaudan added a commit that referenced this pull request Mar 13, 2020
URL api was never used and for customer with base-uri csp restrictions, the polyfill raised an exception.
cf [bug report](#294 (comment))

Add similar restrictions to e2e pages.
bcaudan added a commit that referenced this pull request Mar 16, 2020
URL api was never used and for customer with base-uri csp restrictions, the polyfill raised an exception.
cf [bug report](#294 (comment))

Add similar restrictions to e2e pages.
bcaudan added a commit that referenced this pull request Mar 16, 2020
URL api was never used and for customer with base-uri csp restrictions, the polyfill raised an exception.
cf [bug report](#294 (comment))

Add similar restrictions to e2e pages.
bcaudan added a commit that referenced this pull request Mar 16, 2020
URL api was never used and for customer with base-uri csp restrictions, the polyfill raised an exception.
cf [bug report](#294 (comment))

Add similar restrictions to e2e pages.
@bcaudan
Copy link
Contributor Author

bcaudan commented Mar 16, 2020

Hi