Skip to content

Commit

Permalink
[FedCM] Allow mismatching trailing slash for provider urls
Browse files Browse the repository at this point in the history 
The provider url from the API call must match the one in the manifest
list by design.

However, it's possible for developers to append a trailing slash in one
of them but not in the other one especially when there's path involved.
Besides, for GURL without path, |provider_.spec()| will append a
trailing slash automatically. Therefore we relax the requirement by
allowing mismatch on trailing slash.

(cherry picked from commit 6aa3eee)

Bug: 1316823
Change-Id: If28c8d94209a990f6136417dd6efb61ee2ba3967
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3589174
Reviewed-by: Sam Goto <goto@chromium.org>
Commit-Queue: Yi Gu <yigu@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#993368}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3594941
Auto-Submit: Yi Gu <yigu@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org>
Cr-Commit-Position: refs/branch-heads/5005@{#44}
Cr-Branched-From: 5b4d945-refs/heads/main@{#992738}
  • Loading branch information
@yi-gu
yi-gu authored and Chromium LUCI CQ committed Apr 20, 2022
1 parent 8f9fb0b commit a4d887e
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 18 deletions.
35 changes: 34 additions & 1 deletion content/browser/webid/federated_auth_request_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,40 @@ void FederatedAuthRequestImpl::OnManifestListFetched(
return;
}

if (urls.count(provider_.spec()) == 0) {
// The provider url from the API call:
// navigator.credentials.get({
// federated: {
// providers: [{
// url: "https://foo.idp.example",
// clientId: "1234"
// }],
// }
// });
// must match the one in the manifest list:
// {
// "provider_urls": [
// "https://foo.idp.example"
// ]
// }
// However, it's possible for developers to append a trailing slash in one of
// them but not in the other one especially when there's path involved.
// Besides, for GURL without path, |provider_.spec()| will append a trailing
// slash automatically. Therefore we relax the requirement by allowing
// mismatch on trailing slash.
std::string provider_url = provider_.spec();
if (provider_.path().empty() || provider_.path().back() != '/') {
std::string new_path = provider_.path() + '/';
GURL::Replacements replacements;
replacements.SetPathStr(new_path);
provider_url = provider_.ReplaceComponents(replacements).spec();
}
DCHECK_EQ(provider_url.back(), '/');

bool provider_url_is_valid = (urls.count(provider_url) != 0);
provider_url.pop_back();
provider_url_is_valid |= (urls.count(provider_url) != 0);

if (!provider_url_is_valid) {
RecordRequestIdTokenStatus(IdTokenStatus::kManifestNotInManifestList,
render_frame_host_->GetPageUkmSourceId());
CompleteRequest(FederatedAuthRequestResult::kErrorManifestNotInManifestList,
Expand Down
91 changes: 74 additions & 17 deletions content/browser/webid/federated_auth_request_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ constexpr char kPrivacyPolicyUrl[] = "https://rp.example/pp";
constexpr char kTermsOfServiceUrl[] = "https://rp.example/tos";
constexpr char kClientId[] = "client_id_123";
constexpr char kNonce[] = "nonce123";
constexpr bool kManifestInList = true;

// Values will be added here as token introspection is implemented.
constexpr char kToken[] = "[not a real token]";
Expand All @@ -90,6 +89,8 @@ static const std::initializer_list<IdentityRequestAccount> kAccounts{{
GURL() // picture
}};

static const std::set<std::string> kManifestList{kProviderUrl};

// Parameters for a call to RequestIdToken.
struct RequestParameters {
const char* provider;
Expand Down Expand Up @@ -129,17 +130,21 @@ struct MockClientIdConfiguration {
const char* terms_of_service_url;
};

struct MockManifestList {
std::set<std::string> provider_urls;
};

struct MockManifest {
FetchStatus fetch_status;
const char* accounts_endpoint;
const char* token_endpoint;
const char* client_metadata_endpoint;
const char* revocation_endpoint;
bool manifest_in_list;
};

struct MockConfiguration {
const char* token;
MockManifestList manifest_list;
MockManifest manifest;
MockClientIdConfiguration client_metadata;
FetchStatus accounts_response;
Expand All @@ -157,13 +162,13 @@ static const RequestParameters kDefaultRequestParameters{

static const MockConfiguration kConfigurationValid{
kToken,
{kManifestList},
{
FetchStatus::kSuccess,
kAccountsEndpoint,
kTokenEndpoint,
kClientMetadataEndpoint,
kRevocationEndpoint,
kManifestInList,
},
kDefaultClientMetadata,
FetchStatus::kSuccess,
Expand Down Expand Up @@ -376,11 +381,8 @@ class TestIdpNetworkRequestManager : public MockIdpNetworkRequestManager {

void FetchManifestList(FetchManifestListCallback callback) override {
fetched_endpoints_ |= FetchedEndpoint::MANIFEST_LIST;
std::set<std::string> urls;
if (config_.manifest.manifest_in_list) {
urls.insert(kProviderUrl);
}
std::move(callback).Run(FetchStatus::kSuccess, urls);
std::move(callback).Run(FetchStatus::kSuccess,
config_.manifest_list.provider_urls);
}

void FetchManifest(absl::optional<int> idp_brand_icon_ideal_size,
Expand Down Expand Up @@ -859,26 +861,81 @@ TEST_F(BasicFederatedAuthRequestImplTest, ManifestListSuccess) {
/* expected_revocation_hint=*/"");
SetNetworkRequestManager(std::move(checker));

RequestExpectations expect{kExpectationSuccess};
expect.fetched_endpoints |= FetchedEndpoint::MANIFEST_LIST;

RunAuthTest(kDefaultRequestParameters, expect, kConfigurationValid);
RunAuthTest(kDefaultRequestParameters, kExpectationSuccess,
kConfigurationValid);
}

// Test the provider url is not in the manifest list.
TEST_F(BasicFederatedAuthRequestImplTest, ManifestListNotInList) {
base::test::ScopedFeatureList list;
list.InitAndEnableFeature(features::kFedCmManifestValidation);

MockConfiguration config_not_in_list{kConfigurationValid};
config_not_in_list.manifest.manifest_in_list = false;

RequestExpectations request_not_in_list = {
RequestIdTokenStatus::kError,
FederatedAuthRequestResult::kErrorManifestNotInManifestList,
FetchedEndpoint::MANIFEST_LIST};

RunAuthTest(kDefaultRequestParameters, request_not_in_list,
config_not_in_list);
RequestParameters parameters{"https://not-in-list.example", kClientId, kNonce,
/*prefer_auto_sign_in=*/false};
RunAuthTest(parameters, request_not_in_list, kConfigurationValid);
}

// Test mismatching trailing slash is allowed.
TEST_F(BasicFederatedAuthRequestImplTest, ManifestListHasExtraTrailingSlash) {
base::test::ScopedFeatureList list;
list.InitAndEnableFeature(features::kFedCmManifestValidation);

RequestParameters parameters{"https://idp.example/", kClientId, kNonce,
/*prefer_auto_sign_in=*/false};
MockConfiguration config{kConfigurationValid};
config.manifest_list.provider_urls =
std::set<std::string>{"https://idp.example"};

RunAuthTest(parameters, kExpectationSuccess, config);
}

// Test mismatching trailing slash is allowed.
TEST_F(BasicFederatedAuthRequestImplTest, ManifestListHasNoTrailingSlash) {
base::test::ScopedFeatureList list;
list.InitAndEnableFeature(features::kFedCmManifestValidation);

RequestParameters parameters{"https://idp.example", kClientId, kNonce,
/*prefer_auto_sign_in=*/false};
MockConfiguration config{kConfigurationValid};
config.manifest_list.provider_urls =
std::set<std::string>{"https://idp.example/"};

RunAuthTest(parameters, kExpectationSuccess, config);
}

// Test mismatching trailing slash is allowed.
TEST_F(BasicFederatedAuthRequestImplTest,
ManifestListHasTrailingSlashAfterPath) {
base::test::ScopedFeatureList list;
list.InitAndEnableFeature(features::kFedCmManifestValidation);

RequestParameters parameters{"https://idp.example/foo/", kClientId, kNonce,
/*prefer_auto_sign_in=*/false};
MockConfiguration config{kConfigurationValid};
config.manifest_list.provider_urls =
std::set<std::string>{"https://idp.example/foo"};

RunAuthTest(parameters, kExpectationSuccess, config);
}

// Test mismatching trailing slash is allowed.
TEST_F(BasicFederatedAuthRequestImplTest,
ManifestListHasNoTrailingSlashAfterPath) {
base::test::ScopedFeatureList list;
list.InitAndEnableFeature(features::kFedCmManifestValidation);

RequestParameters parameters{"https://idp.example/foo", kClientId, kNonce,
/*prefer_auto_sign_in=*/false};
MockConfiguration config{kConfigurationValid};
config.manifest_list.provider_urls =
std::set<std::string>{"https://idp.example/foo/"};

RunAuthTest(parameters, kExpectationSuccess, config);
}

// Test that request fails if manifest is missing token endpoint.
Expand Down

0 comments on commit a4d887e

Please sign in to comment.