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

Use postMessage in embed mode instead of custom events #9981

Merged
merged 1 commit into from Nov 21, 2023

Conversation

LukasHirt
Copy link
Contributor

Description

We have switched to using postMessage method in the embed mode instead of emitting custom events. This mitigates the issue when running the embed mode on different origin blocked the access to the dispatchEvent method.

Motivation and Context

Solve issues with forbidden access when hosting the iframe on different origin.

How Has This Been Tested?

  • test environment: local
  • test case 1: run the embed mode and check the correct events being emitted

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@LukasHirt LukasHirt self-assigned this Nov 17, 2023
@LukasHirt LukasHirt changed the title feat!use postMessage in embed mode instead of custom events Use postMessage in embed mode instead of custom events Nov 17, 2023
@LukasHirt LukasHirt added the Category:Change Change existing functionality label Nov 17, 2023
@LukasHirt LukasHirt marked this pull request as ready for review November 17, 2023 10:33
@dschmidt
Copy link
Member

I think the Sonarcloud comments are actually legit. I'm just not sure where we want to retrieve this information from

@LukasHirt
Copy link
Contributor Author

I think the Sonarcloud comments are actually legit. I'm just not sure where we want to retrieve this information from

@dschmidt I guess we need to introduce new prop in config for that then? Should * still be used as a callback though in case it is not defined?

@dschmidt
Copy link
Member

Not sure, on the one hand that's kind of an invitation to not configure it properly... on the other hand if CSP is configured properly it should be rather safe anyhow

@LukasHirt
Copy link
Contributor Author

LukasHirt commented Nov 17, 2023

Not sure, on the one hand that's kind of an invitation to not configure it properly... on the other hand if CSP is configured properly it should be rather safe anyhow

I still added it. I don't think there is any way we can secure the value ourselves as it will always depend on the deployment. Regarding not configuring it properly, I guess this is a danger we can live with? And even if the CSP would handle it, as long as it is best practice, I would set it anyway 🤷

@dschmidt
Copy link
Member

Not sure, on the one hand that's kind of an invitation to not configure it properly... on the other hand if CSP is configured properly it should be rather safe anyhow

I still added it. I don't think there is any way we can secure the value ourselves as it will always depend on the deployment. Regarding not configuring it properly, I guess this is a danger we can live with? And even if the CSP would handle it, as long as it is best practice, I would set it anyway 🤷

My point is: having * as default is a worse than having no default. Obviously people can still set it and then it's not our fault (and in fact I hate hate hate when software tells me I can't use a value because it's unsafe even if I know what I'm doing), but we maybe should not have it as a default if users don't configure anything. I think if someone wants to use embed mode, we can request to set a config value

@JammingBen
Copy link
Collaborator

I think if someone wants to use embed mode, we can request to set a config value

I agree since there is no other default value that would make sense.

Maybe we also should move the mode config into the new embed object and rename it to something like enabled? That way all the embed-configs would be together in one place (and I can't really think of another mode to come in the future).

@LukasHirt LukasHirt force-pushed the feat/actions-post-message branch 2 times, most recently from 98c1fa3 to 519d33e Compare November 20, 2023 14:10
@LukasHirt
Copy link
Contributor Author

@dschmidt now I get it, there was a bit of a misunderstanding from my side. Default is dropped now and it is configurable.

@JammingBen changelog items updated to include the link and dropping the changes and also the mode prop moved to embed group.

@LukasHirt LukasHirt force-pushed the feat/actions-post-message branch 2 times, most recently from a59f3ab to 8506fce Compare November 20, 2023 14:56
Copy link
Collaborator

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Unit tests still fail, code LGTM though 👍

@LukasHirt
Copy link
Contributor Author

Strange that the E2E tests are failing as it seems unrelated to my changes... I'll try to see what's going on

@JammingBen
Copy link
Collaborator

Strange that the E2E tests are failing as it seems unrelated to my changes... I'll try to see what's going on

Could you try rebasing with current master?

Copy link

sonarcloud bot commented Nov 21, 2023

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

95.5% 95.5% Coverage
0.0% 0.0% Duplication

@JammingBen JammingBen merged commit 1c1d628 into owncloud:master Nov 21, 2023
4 checks passed
ownclouders pushed a commit that referenced this pull request Nov 21, 2023
Use postMessage in embed mode instead of custom events
@LukasHirt LukasHirt deleted the feat/actions-post-message branch November 21, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Change Change existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants