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

Fix Safari 11 PKCS bug #11396

Merged
merged 1 commit into from Sep 25, 2017
Merged

Fix Safari 11 PKCS bug #11396

merged 1 commit into from Sep 25, 2017

Conversation

taymonbeal
Copy link
Member

Safari 11 finally started supporting the modern unprefixed Web Crypto API, but left the old one around for backwards compatibility. This played havoc with our feature detection and caused any attempt to use public-key crypto on Safari 11 (i.e., Fast Fetch preferential rendering) to fail. This fix makes sure that the crypto service always knows which version of the API it's using.

Fixes #11394.

/** @private @const {?webCrypto.SubtleCrypto} */
this.subtle_ = getSubtle(win);
this.subtle_ = subtle;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Any reason to not just use this.subtle_ on lines 37, 39?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then it couldn't be @const anymore.

@jonkeller
Copy link
Contributor

jonkeller commented Sep 23, 2017 via email

@taymonbeal
Copy link
Member Author

Hey, anything blocking a merge at this point?

@keithwrightbos keithwrightbos merged commit af3cb0b into ampproject:master Sep 25, 2017
@taymonbeal taymonbeal deleted the safari-11-crypto-fix branch September 25, 2017 16:40
camelburrito pushed a commit that referenced this pull request Sep 25, 2017
dmvjs pushed a commit to dmvjs/amphtml that referenced this pull request Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A4A: Crypto Bug
5 participants