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

docs: update links to use HTTPS as protocol #39718

Closed
wants to merge 3 commits into from
Closed

docs: update links to use HTTPS as protocol #39718

wants to merge 3 commits into from

Conversation

Marcono1234
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Documentation, code comments and example and test code is using HTTP for links and URLs despite the target sites supporting HTTPS. When the user then opens the site, the browser first tries to create an unencrypted connection. An adversary in the same local network could abuse this.

What is the new behavior?

The links have been updated where the target site supports HTTPS (and is correctly configured); some links have been updated to their current redirect destination.

Note that this pull request might have missed a few cases. Additionally it is a draft for now to make sure that it does not break anything (since there are also a few code changes).
Though I do not mind if you think this effort is not worth it. Also feel free to pick the relevant edits from the pull request then, or let me know if you want something to be changed, or if you want this pull request to be split.

Does this PR introduce a breaking change?

  • Yes
  • No

I hope not.

@google-cla google-cla bot added the cla: yes label Nov 16, 2020
@Marcono1234 Marcono1234 changed the title (docs|fix): update links, use HTTPS as protocol docs|fix: update links, use HTTPS as protocol Nov 16, 2020
Comment on lines 144 to 146
`Content-Security-Policy` HTTP header. Read more about content security policy at the
[Web Fundamentals page](https://developers.google.com/web/fundamentals/security/csp) on the
Google Developers website.
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 sure if the wording here is good. The reason for this change is that http://www.html5rocks.com/en/tutorials/security/content-security-policy/ redirects to the Google Developers site.

Copy link
Member

Choose a reason for hiding this comment

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

I think "Web Fundamentals guide" is better. Other than that, everything sounds good to me.

@@ -222,7 +222,7 @@ modified to serve `index.html`:


* [IIS](https://www.iis.net/): add a rewrite rule to `web.config`, similar to the one shown
[here](http://stackoverflow.com/a/26152011/2116927):
[here](https://stackoverflow.com/a/26152011):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the user ID at the end. Could be added again if that user wants badges 😄

@@ -137,7 +137,7 @@ template using the `<app-hero-form>` tag.
The **Submit** button has some classes on it for styling.
At this point, the form layout is all plain HTML5, with no bindings or directives.

6. The sample form uses some style classes from [Twitter Bootstrap](http://getbootstrap.com/css/): `container`, `form-group`, `form-control`, and `btn`.
6. The sample form uses some style classes from [Twitter Bootstrap](https://getbootstrap.com/css/): `container`, `form-group`, `form-control`, and `btn`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this could also be changed to https://getbootstrap.com/, or a different link, in case this applies to version 4 as well. The current link leads to version 3.4.

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

This is amazing, @Marcono1234 ❤️
I could help taking a look, even if it's in draft state 😇

aio/content/guide/styleguide.md Outdated Show resolved Hide resolved
aio/content/tutorial/toh-pt4.md Outdated Show resolved Hide resolved
aio/content/guide/glossary.md Outdated Show resolved Hide resolved
aio/content/guide/schematics-authoring.md Outdated Show resolved Hide resolved
packages/bazel/src/ng_module.bzl Outdated Show resolved Hide resolved
modules/playground/src/hello_world/index.ts Outdated Show resolved Hide resolved
aio/content/guide/animations.md Outdated Show resolved Hide resolved
@google-cla

This comment has been minimized.

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 18, 2020
@Marcono1234
Copy link
Contributor Author

I have force-pushed to fix a merge conflict by rebasing onto master; maybe it would have been better to merge master in instead, sorry.

I can also squash the commits at the end, but for now I think having them separate makes it easier to see the changes.

@gkalpak gkalpak added action: review The PR is still awaiting reviews from at least one requested reviewer aio: preview comp: docs refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release type: bug/fix labels Nov 18, 2020
@mary-poppins
Copy link

@ngbot ngbot bot modified the milestone: needsTriage Nov 18, 2020
@gkalpak gkalpak self-requested a review November 18, 2020 19:40
@ngbot ngbot bot added this to the needsTriage milestone Nov 18, 2020
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Thx, @Marcono1234! One last comment and this should be good to go 🚀

We prefer rebasing over merging, so force-pushing to a PR branch is fine 😉

You are right that we will need the commits squashed eventually. A better way to keep incremental changes separate is using fixup commits. See here for more details. At this point, you could squash your existing commits into one and add any new changes to a fixup commit (so they are easier to review).

aio/content/guide/animations.md Outdated Show resolved Hide resolved
@google-cla

This comment has been minimized.

1 similar comment
@google-cla

This comment has been minimized.

@Marcono1234 Marcono1234 marked this pull request as ready for review November 18, 2020 21:36
@google-cla

This comment has been minimized.

Comment on lines 609 to 610
`${lowerAttrName} on element ${tagName} (see https://g.co/ng/security#xss)`);
`${lowerAttrName} on element ${tagName} ` +
`(see https://g.co/ng/security#xss)`);
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 format task wanted to wrap this in a weird way:

console.warn(
    `WARNING: ignoring unsafe attribute value ` +
    `${lowerAttrName} on element ${
        tagName} (see https://g.co/ng/security#xss)`);

I am not really familiar with TypeScript, but that did not look right to me so I manually wrapped it instead.

Copy link
Member

Choose a reason for hiding this comment

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

You should never argue with the formatter 😛
Your way is fine too, though, so heppy to go with that (as long as the formatter is happy).

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

🎉
Thx, @Marcono1234 ❤️

Reviewed-for: dev-infra, global-docs-approvers

@gkalpak

This comment has been minimized.

@google-cla google-cla bot added cla: yes and removed cla: no labels Nov 19, 2020
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Reviewed-for: dev-infra, global-docs-approvers

Copy link
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

reviewed-for: integration-tests

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

wow.. nice cleanup. thank you, @Marcono1234!

Reviewed-for: global-approvers

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 19, 2020
@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Nov 19, 2020
@IgorMinar IgorMinar removed the action: presubmit The PR is in need of a google3 presubmit label Nov 19, 2020
@Marcono1234
Copy link
Contributor Author

No problem and thanks for the review comments!

It appears the CircleCI "Details" links require you to authenticate with your GitHub account, even though you just want to see the build logs. This is rather cumbersome, especially since CircleCI requests quite a lot permissions1. Is this something you can control and potentially turn off?


1: Requested permissions:

  • "This application will be able to read and write all public repository data."
  • "This application will be able to read your organization, team membership, and private project boards."

@gkalpak gkalpak changed the title docs|fix: update links, use HTTPS as protocol docs: update links to use HTTPS as protocol Nov 20, 2020
@gkalpak
Copy link
Member

gkalpak commented Nov 20, 2020

Unfortunately that is not something we control 😞
We have raised it with CircleCI, so it's on them to fix it.

AndrewKushnir pushed a commit that referenced this pull request Nov 20, 2020
@Marcono1234 Marcono1234 deleted the marcono1234/update-links branch November 21, 2020 00:00
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 22, 2020
@pullapprove pullapprove bot removed the comp: docs label Dec 22, 2020
@ngbot ngbot bot removed this from the Backlog milestone Dec 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker aio: preview cla: yes effort2: days refactoring Issue that involves refactoring or code-cleanup risk: medium target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants