Skip to content

Commit

Permalink
Revert "[Secure Payment Confirmation] Enable enrolling from an iframe."
Browse files Browse the repository at this point in the history
This reverts commit 3304e4d.

Reason for revert: This is causing a failure on a ChromeOS bot here: https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-dbg/22447

It may be the the check to base::FeatureList::IsEnabled(features::kSecurePaymentConfirmation) is somehow interacting with https://source.chromium.org/chromium/chromium/src/+/master:components/payments/content/payment_app_service.cc;l=38 to cause the feature identity check to fail.

Original change's description:
> [Secure Payment Confirmation] Enable enrolling from an iframe.
>
> Before this patch, no WebAuthn credential could be created from within a
> cross origin iframe.
>
> This patch adds a boolean field `is_payment_credential_creation` to the
> mojo parameters for making credentials and special cases WebAuthn to
> allow making credentials when that boolean is true. Blink sets that
> boolean to true when the website is creating a payment credential and
> the execution context has the "payment" feature policy enabled and the
> "SecurePaymentConfirmation" feature is enabled.
>
> After this patch, a cross origin iframe with "payment" feature policy
> can create payment credentials in WebAuthn, while all other types of
> credentials are still prohibited from being created in a cross origin
> iframe, regardless of feature policy.
>
> Bug: 1173184
> Change-Id: I852c31babe858b825365d24781b00589b3940d0a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2655605
> Commit-Queue: Rouslan Solomakhin <rouslan@chromium.org>
> Reviewed-by: Ken Buchanan <kenrb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#849597}

TBR=kenrb@chromium.org,rouslan@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

Change-Id: I3c8181c2115ae6bb703eae6d129fdd284c4fa4f2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1173184
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2669027
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#849677}
  • Loading branch information
clelland authored and Chromium LUCI CQ committed Feb 2, 2021
1 parent cceaee0 commit 846fc14
Show file tree
Hide file tree
Showing 8 changed files with 9 additions and 266 deletions.
5 changes: 1 addition & 4 deletions content/browser/webauth/authenticator_common.cc
Original file line number Diff line number Diff line change
Expand Up @@ -806,10 +806,7 @@ void AuthenticatorCommon::MakeCredential(
blink::mojom::AuthenticatorStatus status =
security_checker_->ValidateAncestorOrigins(
caller_origin,
options->is_payment_credential_creation
? WebAuthRequestSecurityChecker::RequestType::
kMakePaymentCredential
: WebAuthRequestSecurityChecker::RequestType::kMakeCredential,
WebAuthRequestSecurityChecker::RequestType::kMakeCredential,
&is_cross_origin);
if (status != blink::mojom::AuthenticatorStatus::SUCCESS) {
InvokeCallbackAndCleanup(std::move(callback), status);
Expand Down
3 changes: 1 addition & 2 deletions content/browser/webauth/webauth_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -521,8 +521,7 @@ class WebAuthLocalClientBrowserTest : public WebAuthBrowserTestBase {
/*hmac_create_secret=*/false, /*prf_enable=*/false,
blink::mojom::ProtectionPolicy::UNSPECIFIED,
/*enforce_protection_policy=*/false, /*appid_exclude=*/base::nullopt,
/*cred_props=*/false, device::LargeBlobSupport::kNotRequested,
/*is_payment_credential_creation=*/false);
/*cred_props=*/false, device::LargeBlobSupport::kNotRequested);

return mojo_options;
}
Expand Down
13 changes: 4 additions & 9 deletions content/browser/webauth/webauth_request_security_checker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

#include "base/feature_list.h"
#include "base/metrics/histogram_macros.h"
#include "components/payments/core/features.h"
#include "content/browser/renderer_host/render_frame_host_impl.h"
#include "content/public/browser/render_frame_host.h"
#include "device/fido/features.h"
Expand Down Expand Up @@ -135,16 +134,12 @@ WebAuthRequestSecurityChecker::ValidateAncestorOrigins(
RequestType type,
bool* is_cross_origin) {
*is_cross_origin = !IsSameOriginWithAncestors(origin);
if ((type != RequestType::kGetAssertion ||
if ((type == RequestType::kMakeCredential ||
!base::FeatureList::IsEnabled(
device::kWebAuthGetAssertionFeaturePolicy) ||
!render_frame_host_->IsFeatureEnabled(
blink::mojom::FeaturePolicyFeature::kPublicKeyCredentialsGet)) &&
(type != RequestType::kMakePaymentCredential ||
!base::FeatureList::IsEnabled(
payments::features::kSecurePaymentConfirmation) ||
!render_frame_host_->IsFeatureEnabled(
blink::mojom::FeaturePolicyFeature::kPayment)) &&
!static_cast<RenderFrameHostImpl*>(render_frame_host_)
->IsFeatureEnabled(blink::mojom::FeaturePolicyFeature::
kPublicKeyCredentialsGet)) &&
*is_cross_origin) {
return blink::mojom::AuthenticatorStatus::NOT_ALLOWED_ERROR;
}
Expand Down
8 changes: 2 additions & 6 deletions content/browser/webauth/webauth_request_security_checker.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,7 @@ class RenderFrameHost;
class CONTENT_EXPORT WebAuthRequestSecurityChecker
: public base::RefCounted<WebAuthRequestSecurityChecker> {
public:
enum class RequestType {
kMakeCredential,
kMakePaymentCredential,
kGetAssertion
};
enum class RequestType { kMakeCredential, kGetAssertion };

explicit WebAuthRequestSecurityChecker(RenderFrameHost* host);
WebAuthRequestSecurityChecker(const WebAuthRequestSecurityChecker&) = delete;
Expand All @@ -46,7 +42,7 @@ class CONTENT_EXPORT WebAuthRequestSecurityChecker
// Returns blink::mojom::AuthenticatorStatus::SUCCESS if |origin| is
// same-origin with all ancestors in the frame tree, or else if
// requests from cross-origin embeddings are allowed by policy and the
// RequestType is |kGetAssertion| or |kMakePaymentCredential|.
// RequestType is |kGetAssertion|.
// Returns blink::mojom::AuthenticatorStatus::NOT_ALLOWED_ERROR otherwise.
// |is_cross_origin| is an output parameter that is set to true if there is
// a cross-origin embedding, regardless of policy, and false otherwise.
Expand Down
235 changes: 0 additions & 235 deletions content/browser/webauth/webauth_request_security_checker_unittest.cc

This file was deleted.

2 changes: 0 additions & 2 deletions content/test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -2496,11 +2496,9 @@ test("content_unittests") {
"../browser/hid/hid_service_unittest.cc",
"../browser/webauth/authenticator_impl_unittest.cc",
"../browser/webauth/authenticator_mojom_traits_unittest.cc",
"../browser/webauth/webauth_request_security_checker_unittest.cc",
]
deps += [
"//components/autofill/content/browser/webauthn",
"//components/payments/core",
"//device/base",
"//device/fido",
"//device/fido:cablev2_authenticator",
Expand Down
3 changes: 0 additions & 3 deletions third_party/blink/public/mojom/webauthn/authenticator.mojom
Original file line number Diff line number Diff line change
Expand Up @@ -456,9 +456,6 @@ struct PublicKeyCredentialCreationOptions {
// The level of largeBlob support requested by the relying party.
// https://w3c.github.io/webauthn/#sctn-large-blob-extension
LargeBlobSupport large_blob_enable;

// Whether this request is for making a PaymentCredential.
bool is_payment_credential_creation;
};

enum PublicKeyCredentialType {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -865,8 +865,6 @@ void CreatePublicKeyCredentialForPaymentCredential(
return;
}

mojo_options->is_payment_credential_creation = true;

auto* authenticator =
CredentialManagerProxy::From(resolver->GetScriptState())->Authenticator();
authenticator->MakeCredential(
Expand Down Expand Up @@ -1164,9 +1162,7 @@ ScriptPromise CredentialsContainer::create(
FederatedCredential::Create(options->federated(), exception_state));
} else if (options->hasPayment()) {
if (RuntimeEnabledFeatures::SecurePaymentConfirmationEnabled(
resolver->GetExecutionContext()) &&
resolver->GetExecutionContext()->IsFeatureEnabled(
mojom::blink::FeaturePolicyFeature::kPayment)) {
resolver->GetExecutionContext())) {
CreatePublicKeyCredentialForPaymentCredential(options->payment(),
resolver);
} else {
Expand Down

0 comments on commit 846fc14

Please sign in to comment.