-
-
Notifications
You must be signed in to change notification settings - Fork 778
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
updates external links by adding rel #5961
updates external links by adding rel #5961
Conversation
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes.
Note that CONTRIBUTING.md cannot previewed locally; rather it should be previewed at this URL:
|
Availability: Evenings |
Availability: Monday-Friday 12–2 PM, 8–10 PM (PST) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there @sornekian , great work there all the work assigned to you in the issue is done correctly
- the to/from branch looks good
- the visual changes are completely applied in the local environment and are responsive
- the changes specified in the CodeQL is done correctly and looks good
- the issue linked is correct too
Great work there, I approve this PR.......
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sornekian, I've reviewed the changes in your branch and confirm that the code added to resolve the issue is correct, clean, and does not affect the visuals on the About page. PR is approved, nice job!
I noticed that the for loop for dynamic sponsor element creation is currently commented out, and sponsor links are manually added below. Assuming these are for testing, as the site doesn't have a public link to the page yet, they lack the rel="noopener noreferrer"
attribute. It might be good to mention this on the issue page, recommending to address this before the page goes live.
Hey @sornekian - Great job on the PR! @jphamtv Saw your comment about the sponsor links on lines 15, 18, and 21. I.e. should these also have the Good observation and thanks for bringing up the question. I was curious about why CodeQL was not flagging these links- I am guessing that CodeQL is not explicitly scanning HTML, only the embedded Liquid snippets, but I could be wrong. I would agree that if we are fixing one link we should fix the others. (Incidentally the article attached to the CodeQL alert says that modern browsers (2021+) have addressed this previous vulnerability.) I will put this on the list to discuss with @roslynwythe - January sometime. |
Thanks @t-will-gillis ! |
Fixes #5673
What changes did you make?
Why did you make the changes (we will use this info to test)?
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
Visuals after changes are applied