Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(prop): trusted domain #440

Merged
merged 2 commits into from
Jun 13, 2023
Merged

fix(prop): trusted domain #440

merged 2 commits into from
Jun 13, 2023

Conversation

mnicpt
Copy link
Contributor

@mnicpt mnicpt commented Jun 13, 2023

Changes trusted domain prop to accept regex rather than strings.

@mnicpt mnicpt requested a review from a team as a code owner June 13, 2023 02:10
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (5f3bf00) 95.51% compared to head (8b57e9e) 95.52%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #440   +/-   ##
=======================================
  Coverage   95.51%   95.52%           
=======================================
  Files          18       18           
  Lines        1226     1228    +2     
=======================================
+ Hits         1171     1173    +2     
  Misses         55       55           
Impacted Files Coverage Δ
src/component/props.js 93.93% <ø> (ø)
src/child/props.js 96.15% <100.00%> (+0.15%) ⬆️
src/parent/parent.js 95.66% <100.00%> (+<0.01%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -380,7 +380,9 @@ export function parentComponent<P, X, C>({

const trustedChild: boolean =
prop && prop.trustedDomains && prop.trustedDomains.length > 0
? prop.trustedDomains.includes(initialChildDomain)
? prop.trustedDomains.reduce((acc, val) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about accepting a string or regex? We may be able to reuse code from allowedParentDomains: https://github.com/search?q=repo%3Akrakenjs%2Fzoid%20allowedParentDomains&type=code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gregjopa I initially was going down this path until I realized using a string won't play nice when going from environment to environment like Regex does. We could always have it point to an enum. 🤔

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 domain prop does the same thing as allowedParentDomain in that it uses the same type def as DomainMatcher from cross-domain-utils. I just recently updated DomainMatcher to except an array of string or regex.

Copy link
Contributor Author

@mnicpt mnicpt Jun 13, 2023

Choose a reason for hiding this comment

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

@gregjopa Yeah, I can do this. I'm using matchDomain which allows for all. Great suggestion!

Copy link
Contributor

Choose a reason for hiding this comment

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

@gregjopa I initially was going down this path until I realized using a string won't play nice when going from environment to environment like Regex does.

Ahh I see. I'm glad you already considered this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gregjopa Yeah, I can do this. I'm using matchDomain which allows for all.

Great to hear!

@mnicpt mnicpt merged commit 081d0d5 into main Jun 13, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants