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

Changes streamsavers service worker, and fixes vscode TS error #176

Merged
merged 6 commits into from
May 18, 2023

Conversation

danielRicaud
Copy link
Contributor

@danielRicaud danielRicaud commented Apr 14, 2023

This PR should not be merged until the following PR has been promoted to prod (currently in staging): https://github.com/virtru-corp/zzz-secure-virtru-com/commit/370beb79875a42665ec461e59cd11467d19d6cbb

This PR changes which mitm.html (man in the middle) and service worker StreamSaver references over to one Virtru is hosting themselves rather then relying on the library creator to host mitm.html and service worker. This protects us from any upstream library changes in StreamSaver.

@danielRicaud danielRicaud self-assigned this Apr 14, 2023
@danielRicaud danielRicaud requested a review from a team as a code owner April 14, 2023 22:14
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes the following VScode error, has anyone else seen this at the top of .ts files in client-web?

Screenshot 2023-04-14 at 5 08 48 PM

@@ -22,6 +22,8 @@ export class BrowserTdfStream extends DecoratedReadableStream {
return;
}

streamSaver.mitm = 'https://secure.virtru.com/rca/stream-saver/index.html';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any place we can un-hardcode this and make it dynamic per environment? Although Auth-Widget is hardcoded to prod so maybe this isn't a deal breaker?

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion: Delete this method and just do it manually. We can move the code over to web-app as sample code, and implement it in virtru-sdk if we really need it

Copy link
Member

Choose a reason for hiding this comment

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

no Virtru urls in OpenTDF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any opinions on where this should live? Since stream-saver is only utilized in client-web?

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@danielRicaud danielRicaud merged commit 971aed7 into main May 18, 2023
@danielRicaud danielRicaud deleted the feature/update-stream-saver-service-worker-url branch May 18, 2023 16:40
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.

3 participants