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

Implement "Mixed Content" check #543

Merged
merged 1 commit into from Apr 19, 2015
Merged

Conversation

@jugglinmike
Copy link
Contributor

@jugglinmike jugglinmike commented Apr 18, 2015

Commit message:

This check ensures that resources embedded within HTML documents are not
requested using a protocol that degrades the security of the original
connection. This avoids "mixed content" warnings in browsers that
enforce consistent protocols, and it precludes the leaking of private
information in browsers that do not.

Resolves gh-542

@@ -4,6 +4,18 @@

module ::Nanoc::Extra
class LinkCollector

@@uri_attrs = {
Copy link
Member

@ddfreyne ddfreyne Apr 18, 2015

Choose a reason for hiding this comment

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

I’d prefer to have this in a constant rather than a class variable (I think Rubocop will complain anyway).

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Apr 18, 2015

This looks good! I’ll do a more in-depth review soon, but I‘m quite liking it so far.

Loading

@ddfreyne ddfreyne added this to the 3.8 milestone Apr 18, 2015
@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Apr 18, 2015

Great, thanks! I'll hold off on moving that class variable till you get a chance to finish.

Loading

@@ -4,6 +4,18 @@

module ::Nanoc::Extra
class LinkCollector

@@uri_attrs = {
'a' => :href,
Copy link
Member

@ddfreyne ddfreyne Apr 18, 2015

Choose a reason for hiding this comment

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

Does it make sense to disallow linking to a non-secure site from a secure site?

Loading

Copy link
Contributor Author

@jugglinmike jugglinmike Apr 18, 2015

Choose a reason for hiding this comment

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

  1. Prevent insertion of insecure "active content" (such as JavaScript) that could generate misleading content or steal private information
  2. Prevent passive leaking of information--HTTP requests are sent in-the-clear and might leak information about the user's browsing session

Users who navigate using insecure hyperlinks are susceptible to #2, but navigation is performed actively by the user, and it's harder to achieve the task of "saving users from themselves". Plus, the current site's maintainer has no control over the availability of SSL on remote servers.

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Apr 18, 2015

The style checks are failing. Apparently, they’re also failing on other parts of the code—I’ll fix it; ignore the style test results for now.

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Apr 18, 2015

Can you rebase onto master? That should fix the style issues (at least the ones that are not your fault, heh).

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Apr 18, 2015

Looks good otherwise!

Loading

@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Apr 18, 2015

Alrighty, I've rebased and incorporated your feedback in separate commits. I'm happy to squash when you think this is ready

Loading

@@ -0,0 +1,31 @@
# encoding: utf-8

PROTOCOL_PATTERN = /^(\w+):\/\//
Copy link
Member

@ddfreyne ddfreyne Apr 18, 2015

Choose a reason for hiding this comment

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

Could you move this into the MixedContent class? This way, PROTOCOL_PATTERN won’t be a top-level constant. Constant lookup will still work.

Loading

Copy link
Contributor Author

@jugglinmike jugglinmike Apr 18, 2015

Choose a reason for hiding this comment

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

Ah yeah, much better that way.

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Apr 18, 2015

Cool! OK to squash. I’ll properly test it out tomorrow and will merge it if it’s OK!

Loading

This check ensures that resources embedded within HTML documents are not
requested using a protocol that degrades the security of the original
connection. This avoids "mixed content" warnings in browsers that
enforce consistent protocols, and it precludes the leaking of private
information in browsers that do not.
@jugglinmike jugglinmike force-pushed the mixed-content-check branch from c980885 to e2d4feb Apr 18, 2015
@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Apr 18, 2015

Excellent--all squashed

Loading

ddfreyne added a commit that referenced this issue Apr 19, 2015
@ddfreyne ddfreyne merged commit 0e7cdad into nanoc:master Apr 19, 2015
1 check passed
Loading
@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Apr 19, 2015

Thanks!

Loading

@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Apr 19, 2015

My pleasure :)

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants