Skip to content

CPLAT-7608 [3.1.0] Improve handling of “repeat” errors thrown from components wrapped by an ErrorBoundary#353

Merged
rmconsole3-wf merged 8 commits into3.1.0-wipfrom
3.1.0/error-boundary-improvements
Oct 11, 2019
Merged

CPLAT-7608 [3.1.0] Improve handling of “repeat” errors thrown from components wrapped by an ErrorBoundary#353
rmconsole3-wf merged 8 commits into3.1.0-wipfrom
3.1.0/error-boundary-improvements

Conversation

@aaronlademann-wf
Copy link
Contributor

@aviary-wf
Copy link

aviary-wf commented Oct 3, 2019

Security Insights

The items listed below may not capture all security relevant changes. Before providing a security review, be sure to review the entire PR for security impact.

(3) Security relevant changes were detected
  • Watched keyword innerHtml in lib/src/component/error_boundary_mixins.dart line(s) ['258'] added
  • Watched keyword InnerHtml in lib/src/component/error_boundary_mixins.dart line(s) ['282'] added
  • Watched keyword dangerouslySetInnerHTML in lib/src/component/error_boundary_mixins.dart line(s) ['283'] added
  • Action Items

    • Obtain a security review; reviewer should pay special attention to insights listed above
    • Verify aviary.yaml coverage of security relevant code

    Questions or Comments? Reach out on Slack: #support-infosec.

    return props.children;
    }

    // TODO: Add PropTypes
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    Not in this PR... I just noticed that validateProps had been removed without having an analogous propTypes replacement.

    @aaronlademann-wf aaronlademann-wf force-pushed the 3.1.0/error-boundary-improvements branch from 53ddc19 to ceda402 Compare October 3, 2019 23:35
    @aaronlademann-wf aaronlademann-wf added this to the 3.1.0 milestone Oct 3, 2019
    @maxwellpeterson-wf
    Copy link
    Member

    +1 security

    • Our CSP protects us from any potential XSS made possible by this change.

    + and add more test coverage for edge cases involving `dangerouslySetInnerHTML`
    # Conflicts:
    #	test/over_react/component/fixtures/flawed_component_on_mount.over_react.g.dart
    #	test/over_react/component/fixtures/flawed_component_that_renders_a_string.over_react.g.dart
    #	test/over_react/component/fixtures/flawed_component_that_renders_nothing.over_react.g.dart
    Copy link
    Contributor

    @joebingham-wk joebingham-wk left a comment

    Choose a reason for hiding this comment

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

    Looks great! Just a tiny question.

    });
    });

    // group('throws a PropError when', () {
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Should we add a TODO here to mention that the validate props needs to be added back?

    @kealjones-wk
    Copy link
    Contributor

    +10

    @kealjones-wk
    Copy link
    Contributor

    @Workiva/release-management-p

    @kealjones-wk
    Copy link
    Contributor

    @Workiva/release-management-p

    1 similar comment
    @kealjones-wk
    Copy link
    Contributor

    @Workiva/release-management-p

    @maxwellpeterson-wf
    Copy link
    Member

    +1 security refresh

    @rmconsole3-wf rmconsole3-wf merged commit e0d033f into 3.1.0-wip Oct 11, 2019
    @rmconsole3-wf rmconsole3-wf deleted the 3.1.0/error-boundary-improvements branch October 11, 2019 20:17
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    7 participants