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.

Sign up for GitHub

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

Jump to bottom

fix(ui5-link): noopener attribute value added to the rel attribute #4533

Merged
merged 4 commits into from
Jan 17, 2022
Merged

fix(ui5-link): noopener attribute value added to the rel attribute #4533

merged 4 commits into from
Jan 17, 2022

Conversation

unazko
Copy link
Contributor

@unazko unazko commented Dec 29, 2021 •
edited
Loading

  • The "noopener" text is added to the rel value attribute alongside "noreferrer" text for compatibility reasons with the openui5's
    sap.m.Link implementation.
  • Explicit specification of rel "noopener" helps protect users from tabnabbing in legacy browsers including Edge Legacy and Internet Explorer.

@unazko unazko requested review from terezamch and fifoosid December 31, 2021 08:02
terezamch
terezamch reviewed Dec 31, 2021
View reviewed changes
Copy link
Contributor

@terezamch terezamch left a comment

Choose a reason for hiding this comment

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

Should there be a test about the change?

@unazko unazko requested a review from terezamch December 31, 2021 14:24
terezamch
terezamch approved these changes Jan 4, 2022
View reviewed changes
Copy link
Contributor

@terezamch terezamch left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@unazko unazko requested review from ilhan007 and vladitasev January 14, 2022 15:23
@ilhan007 ilhan007 requested a review from kineticjs January 17, 2022 09:01
@ilhan007
Copy link
Member

ilhan007 commented Jan 17, 2022 •
edited
Loading

Hello @kineticjs,
this change is relevant for the security topic and although the Link is not in your team's. ownership I thought you might have overview. Is this the recommended approach - setting "noreferrer noopener" for all links, except for those with target="_self"?

Thanks in advance!

(both not supported on IE, but I guess they won't do any harm on IE - just won't take any effect, so this should be fine).

kineticjs
kineticjs approved these changes Jan 17, 2022
View reviewed changes
Copy link
Contributor

@kineticjs kineticjs left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Yes, adding "noopener" (in addition to "noreferrer" ) explicitly aligns us better with the general recommendations and the associated fix in openui5.

Extending the check to all windows other than "_self" is also an enhancement.

@ilhan007
Copy link
Member

Hello @unazko please consider Diana's feedback for further enhancement, merging this one.

@ilhan007 ilhan007 merged commit 3f2c3cd into SAP:master Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terezamch terezamch terezamch approved these changes

@kineticjs kineticjs kineticjs approved these changes

@fifoosid fifoosid Awaiting requested review from fifoosid

@ilhan007 ilhan007 Awaiting requested review from ilhan007

@vladitasev vladitasev Awaiting requested review from vladitasev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@unazko @ilhan007 @kineticjs @terezamch