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

HTTPS considerations. #283

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from
Open

HTTPS considerations. #283

wants to merge 1 commit into from

Conversation

rsolomakhin
Copy link
Collaborator

@rsolomakhin rsolomakhin commented Apr 9, 2018

Write down the security mitigations in Chrome for other implementers to
be aware.

The following tasks have been completed:

  • web platform tests (link)
  • MDN Docs added

Implementation commitment:


Preview | Diff

Write down the security mitigations in Chrome for other implementers to
be aware.
<li>If the security properties of <var>newContext</var> are
unsatisfactory (e.g., invalid certificate), then:
<ol>
<li>Reject <var>promise</var> with <a>SecurityError</a>..
Copy link
Contributor

Choose a reason for hiding this comment

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

editorial: two periods after SecurityError.

</li>
<li>If the SSL certificate of the payment handler page is not valid
(e.g., self-signed), the user agent may cancel the payment.
</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is inconsistent with the algorithm definition, which forces abort. I suggest changing this to MUST.

<li>If the security properties of <var>newContext</var> are
unsatisfactory (e.g., invalid certificate), then:
<ol>
<li>Reject <var>promise</var> with <a>SecurityError</a>..
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<li>Reject <var>promise</var> with <a>SecurityError</a>..
<li>Reject <var>promise</var> with <a>SecurityError</a>.

@@ -1669,6 +1669,15 @@ <h2>
</li>
</ol>
</li>
<li>If the security properties of <var>newContext</var> are
Copy link
Member

Choose a reason for hiding this comment

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

This seems overly vague. Maybe @domenic can suggest better wording for this? It might be a simple as saying "not a secure context" or something link that.

Copy link

Choose a reason for hiding this comment

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

It's unclear from @rsolomakhin's text whether insecure HTTP pages are rejected or not. Invalid certificates will fail a navigation, so that would happen before this step.

If the intent is to disallow insecure HTTP, then the wording would be newContext's active document's relevant settings object is contextually secure.

@@ -2078,6 +2087,31 @@ <h2>
</li>
</ul>
</section>
<section>
Copy link
Member

Choose a reason for hiding this comment

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

This whole section doesn't feel right in this spec.

@rsolomakhin
Copy link
Collaborator Author

Thank you for the review, @domenic , @ianbjacobs , and @marcoscaceres ! My intention here is to block both HTTP (non-secure) pages and HTTPS pages where certificates are self-signed, expired, revoked, and so on. I would encourage user agent implementers to clamp this down as much as possible.

For example, take a look at https://badssl.com. Many of the red-color links there lead to pages with questionable security. Chrome will display an interstitial, but users can sometimes bypass this interstisial by clicking on [Advanced]->"Proceed (unsafe)". After the user has bypassed the interstitial, Chrome treats the page as secure, but I am proposing to make an exception for payments-related API. Payment Handler and Payment Request should not work with questionable HTTPS states.

What is the best phrasing to use in this pull request to achieve this goal?

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

Successfully merging this pull request may close these issues.

4 participants