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.

Sign up for GitHub

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

[PM-6413] Add http loophole for localhost #9236

Merged
merged 7 commits into from
Jun 25, 2024
Merged

[PM-6413] Add http loophole for localhost #9236

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
07b69ed
[PM-6413] feat: add http loophole for localhost
coroiu May 17, 2024
dea8c5d
Merge branch 'main' into PM-6413-add-http-loophole-for-localhost
coroiu Jun 7, 2024
2961d8c
feat: add sanity check
coroiu Jun 7, 2024
d0a116c
feat: change fido2 filters to allow scripts on localhost
coroiu Jun 10, 2024
eeb48ac
Merge branch 'main' into PM-6413-add-http-loophole-for-localhost
coroiu Jun 10, 2024
24d3695
[PM-6413] fix: injection tests
coroiu Jun 10, 2024
691299a
Merge branch 'main' into PM-6413-add-http-loophole-for-localhost
coroiu Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions libs/common/src/platform/services/fido2/fido2-client.service.spec.ts
View file
Open in desktop
justindbaur marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
} from "../../abstractions/fido2/fido2-client.service.abstraction";
import { Utils } from "../../misc/utils";

import * as DomainUtils from "./domain-utils";
import { Fido2AuthenticatorService } from "./fido2-authenticator.service";
import { Fido2ClientService } from "./fido2-client.service";
import { Fido2Utils } from "./fido2-utils";
Expand All @@ -36,6 +37,7 @@ describe("FidoAuthenticatorService", () => {
let domainSettingsService: MockProxy<DomainSettingsService>;
let client!: Fido2ClientService;
let tab!: chrome.tabs.Tab;
let isValidRpId!: jest.SpyInstance;

beforeEach(async () => {
authenticator = mock<Fido2AuthenticatorService>();
Expand All @@ -44,6 +46,8 @@ describe("FidoAuthenticatorService", () => {
vaultSettingsService = mock<VaultSettingsService>();
domainSettingsService = mock<DomainSettingsService>();

isValidRpId = jest.spyOn(DomainUtils, "isValidRpId");

client = new Fido2ClientService(
authenticator,
configService,
Expand All @@ -58,6 +62,10 @@ describe("FidoAuthenticatorService", () => {
tab = { id: 123, windowId: 456 } as chrome.tabs.Tab;
});

afterEach(() => {
isValidRpId.mockRestore();
});

describe("createCredential", () => {
describe("input parameters validation", () => {
// Spec: If sameOriginWithAncestors is false, return a "NotAllowedError" DOMException.
Expand Down Expand Up @@ -113,6 +121,7 @@ describe("FidoAuthenticatorService", () => {
});

// Spec: If options.rp.id is not a registrable domain suffix of and is not equal to effectiveDomain, return a DOMException whose name is "SecurityError", and terminate this algorithm.
// This is actually checked by `isValidRpId` function, but we'll test it here as well
it("should throw error if rp.id is not valid for this origin", async () => {
const params = createParams({
origin: "https://passwordless.dev",
Expand All @@ -126,6 +135,20 @@ describe("FidoAuthenticatorService", () => {
await rejects.toBeInstanceOf(DOMException);
});

// Sanity check to make sure that we use `isValidRpId` to validate the rp.id
it("should throw if isValidRpId returns false", async () => {
const params = createParams();
authenticator.makeCredential.mockResolvedValue(createAuthenticatorMakeResult());
// `params` actually has a valid rp.id, but we're mocking the function to return false
isValidRpId.mockReturnValue(false);

const result = async () => await client.createCredential(params, tab);

const rejects = expect(result).rejects;
await rejects.toMatchObject({ name: "SecurityError" });
await rejects.toBeInstanceOf(DOMException);
});

it("should fallback if origin hostname is found in neverDomains", async () => {
const params = createParams({
origin: "https://bitwarden.com",
Expand All @@ -151,6 +174,16 @@ describe("FidoAuthenticatorService", () => {
await rejects.toBeInstanceOf(DOMException);
});

it("should not throw error if localhost is http", async () => {
const params = createParams({
origin: "http://localhost",
rp: { id: undefined, name: "localhost" },
});
authenticator.makeCredential.mockResolvedValue(createAuthenticatorMakeResult());

await client.createCredential(params, tab);
});

// Spec: If credTypesAndPubKeyAlgs is empty, return a DOMException whose name is "NotSupportedError", and terminate this algorithm.
it("should throw error if no support key algorithms were found", async () => {
const params = createParams({
Expand Down Expand Up @@ -360,6 +393,7 @@ describe("FidoAuthenticatorService", () => {
});

// Spec: If options.rp.id is not a registrable domain suffix of and is not equal to effectiveDomain, return a DOMException whose name is "SecurityError", and terminate this algorithm.
// This is actually checked by `isValidRpId` function, but we'll test it here as well
it("should throw error if rp.id is not valid for this origin", async () => {
const params = createParams({
origin: "https://passwordless.dev",
Expand All @@ -373,6 +407,20 @@ describe("FidoAuthenticatorService", () => {
await rejects.toBeInstanceOf(DOMException);
});

// Sanity check to make sure that we use `isValidRpId` to validate the rp.id
it("should throw if isValidRpId returns false", async () => {
const params = createParams();
authenticator.getAssertion.mockResolvedValue(createAuthenticatorAssertResult());
// `params` actually has a valid rp.id, but we're mocking the function to return false
isValidRpId.mockReturnValue(false);

const result = async () => await client.assertCredential(params, tab);

const rejects = expect(result).rejects;
await rejects.toMatchObject({ name: "SecurityError" });
await rejects.toBeInstanceOf(DOMException);
});

it("should fallback if origin hostname is found in neverDomains", async () => {
const params = createParams({
origin: "https://bitwarden.com",
Expand Down Expand Up @@ -506,6 +554,16 @@ describe("FidoAuthenticatorService", () => {
expect.anything(),
);
});

it("should not throw error if localhost is http", async () => {
const params = createParams({
origin: "http://localhost",
});
params.rpId = undefined;
authenticator.getAssertion.mockResolvedValue(createAuthenticatorAssertResult());

await client.assertCredential(params, tab);
});
});

describe("assert discoverable credential", () => {
Expand Down
10 changes: 8 additions & 2 deletions libs/common/src/platform/services/fido2/fido2-client.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,10 @@
}

params.rp.id = params.rp.id ?? parsedOrigin.hostname;
if (parsedOrigin.hostname == undefined || !params.origin.startsWith("https://")) {
if (
parsedOrigin.hostname == undefined ||
(!params.origin.startsWith("https://") && parsedOrigin.hostname !== "localhost")

Check warning on line 108 in libs/common/src/platform/services/fido2/fido2-client.service.ts

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

❌ New issue: Complex Conditional 

Fido2ClientService.createCredential has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
justindbaur marked this conversation as resolved.
Show resolved Hide resolved
) {

Check notice on line 109 in libs/common/src/platform/services/fido2/fido2-client.service.ts

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

ℹ Getting worse: Complex Method 

Fido2ClientService.createCredential increases in cyclomatic complexity from 45 to 46, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
this.logService?.warning(`[Fido2Client] Invalid https origin: ${params.origin}`);
throw new DOMException("'origin' is not a valid https origin", "SecurityError");
}
Expand Down Expand Up @@ -238,7 +241,10 @@

params.rpId = params.rpId ?? parsedOrigin.hostname;

if (parsedOrigin.hostname == undefined || !params.origin.startsWith("https://")) {
if (
parsedOrigin.hostname == undefined ||
(!params.origin.startsWith("https://") && parsedOrigin.hostname !== "localhost")

Check warning on line 246 in libs/common/src/platform/services/fido2/fido2-client.service.ts

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

❌ New issue: Complex Conditional 

Fido2ClientService.assertCredential has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
) {

Check notice on line 247 in libs/common/src/platform/services/fido2/fido2-client.service.ts

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

ℹ Getting worse: Complex Method 

Fido2ClientService.assertCredential increases in cyclomatic complexity from 29 to 30, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
this.logService?.warning(`[Fido2Client] Invalid https origin: ${params.origin}`);
throw new DOMException("'origin' is not a valid https origin", "SecurityError");
}
Expand Down
Loading