OpenWF/SpaceNinjaServer
21
23
Fork 24
Code Issues 23 Pull Requests 5 Actions Packages Projects Releases Activity

fix: avoid spilling new database account fields into login response #610

Merged
Sainan merged 4 commits from login-improvement into main 2024-12-22 15:40:35 -08:00
Conversation 4 Commits 4 Files Changed 4 +41 -42
4 changed files with 41 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

43
src/controllers/api/loginController.ts
View File

@ -7,11 +7,11 @@ import buildConfig from "@/static/data/buildConfig.json";
import { toLoginRequest } from "@/src/helpers/loginHelpers"; import { toLoginRequest } from "@/src/helpers/loginHelpers";
import { Account } from "@/src/models/loginModel"; import { Account } from "@/src/models/loginModel";
import { createAccount, isCorrectPassword } from "@/src/services/loginService"; import { createAccount, isCorrectPassword } from "@/src/services/loginService";
import { ILoginResponse } from "@/src/types/loginTypes"; import { IDatabaseAccountJson, ILoginResponse } from "@/src/types/loginTypes";
import { DTLS, groups, HUB, platformCDNs } from "@/static/fixed_responses/login_static"; import { DTLS, groups, HUB, platformCDNs } from "@/static/fixed_responses/login_static";
import { logger } from "@/src/utils/logger"; import { logger } from "@/src/utils/logger";
const loginController: RequestHandler = async (request, response) => { export const loginController: RequestHandler = async (request, response) => {
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-argument // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-argument
const body = JSON.parse(request.body); // parse octet stream of json data to json object const body = JSON.parse(request.body); // parse octet stream of json data to json object
const loginRequest = toLoginRequest(body); const loginRequest = toLoginRequest(body);
@ -39,21 +39,7 @@ const loginController: RequestHandler = async (request, response) => {
Nonce: nonce Nonce: nonce
}); });
logger.debug("created new account"); logger.debug("created new account");
// eslint-disable-next-line @typescript-eslint/no-unused-vars response.json(createLoginResponse(newAccount, buildLabel));
const { email, password, LastLoginDay, ...databaseAccount } = newAccount;
const newLoginResponse: ILoginResponse = {
...databaseAccount,
Groups: groups,
platformCDNs: platformCDNs,
NRS: [config.myAddress],
DTLS: DTLS,
IRC: config.myIrcAddresses ?? [config.myAddress],
HUB: HUB,
BuildLabel: buildLabel,
MatchmakingBuildId: buildConfig.matchmakingBuildId
};
response.json(newLoginResponse);
return; return;
} catch (error: unknown) { } catch (error: unknown) {
if (error instanceof Error) { if (error instanceof Error) {
@ -76,9 +62,22 @@ const loginController: RequestHandler = async (request, response) => {
} }
await account.save(); await account.save();
const { email, password, LastLoginDay, ...databaseAccount } = account.toJSON(); response.json(createLoginResponse(account.toJSON(), buildLabel));
const newLoginResponse: ILoginResponse = { };
...databaseAccount,
const createLoginResponse = (account: IDatabaseAccountJson, buildLabel: string): ILoginResponse => {
return {
id: account.id,
DisplayName: account.DisplayName,
coderabbitai[bot] commented 2024-12-22 15:34:20 -08:00 (Migrated from github.com)
Review
Copy Link

⚠️ Potential issue

Inconsistent usage of toJSON() method

There's an inconsistency in how the account object is passed to createLoginResponse:

  • Line 42: Passes newAccount directly
  • Line 65: Calls account.toJSON()

This could lead to type mismatches or inconsistent behavior.

Suggestion to standardize the approach:

-    response.json(createLoginResponse(newAccount, buildLabel));
+    response.json(createLoginResponse(newAccount.toJSON(), buildLabel));

Also applies to: 65-65

_:warning: Potential issue_ **Inconsistent usage of toJSON() method** There's an inconsistency in how the account object is passed to `createLoginResponse`: - Line 42: Passes `newAccount` directly - Line 65: Calls `account.toJSON()` This could lead to type mismatches or inconsistent behavior. Suggestion to standardize the approach: ```diff - response.json(createLoginResponse(newAccount, buildLabel)); + response.json(createLoginResponse(newAccount.toJSON(), buildLabel)); ``` Also applies to: 65-65 <!-- This is an auto-generated comment by CodeRabbit -->
CountryCode: account.CountryCode,
ClientType: account.ClientType,
CrossPlatformAllowed: account.CrossPlatformAllowed,
ForceLogoutVersion: account.ForceLogoutVersion,
AmazonAuthToken: account.AmazonAuthToken,
AmazonRefreshToken: account.AmazonRefreshToken,
ConsentNeeded: account.ConsentNeeded,
TrackedSettings: account.TrackedSettings,
Nonce: account.Nonce,
Groups: groups, Groups: groups,
platformCDNs: platformCDNs, platformCDNs: platformCDNs,
NRS: [config.myAddress], NRS: [config.myAddress],
@ -88,8 +87,4 @@ const loginController: RequestHandler = async (request, response) => {
BuildLabel: buildLabel, BuildLabel: buildLabel,
MatchmakingBuildId: buildConfig.matchmakingBuildId MatchmakingBuildId: buildConfig.matchmakingBuildId
}; };
response.json(newLoginResponse);
}; };
export { loginController };

6
src/models/loginModel.ts
View File

@ -1,4 +1,4 @@
import { IDatabaseAccountDocument } from "@/src/types/loginTypes"; import { IDatabaseAccountJson } from "@/src/types/loginTypes";
import { model, Schema, SchemaOptions } from "mongoose"; import { model, Schema, SchemaOptions } from "mongoose";
const opts = { const opts = {
@ -20,7 +20,7 @@ const opts = {
// } // }
// } // }
const databaseAccountSchema = new Schema<IDatabaseAccountDocument>( const databaseAccountSchema = new Schema<IDatabaseAccountJson>(
{ {
email: { type: String, required: true, unique: true }, email: { type: String, required: true, unique: true },
password: { type: String, required: true }, password: { type: String, required: true },
@ -48,4 +48,4 @@ databaseAccountSchema.set("toJSON", {
virtuals: true virtuals: true
}); });
export const Account = model<IDatabaseAccountDocument>("Account", databaseAccountSchema); export const Account = model<IDatabaseAccountJson>("Account", databaseAccountSchema);

4
src/services/loginService.ts
View File

@ -1,6 +1,6 @@
import { Account } from "@/src/models/loginModel"; import { Account } from "@/src/models/loginModel";
import { createInventory } from "@/src/services/inventoryService"; import { createInventory } from "@/src/services/inventoryService";
import { IDatabaseAccount } from "@/src/types/loginTypes"; import { IDatabaseAccount, IDatabaseAccountJson } from "@/src/types/loginTypes";
import { createShip } from "./shipService"; import { createShip } from "./shipService";
import { Types } from "mongoose"; import { Types } from "mongoose";
import { Loadout } from "@/src/models/inventoryModels/loadoutModel"; import { Loadout } from "@/src/models/inventoryModels/loadoutModel";
@ -12,7 +12,7 @@ export const isCorrectPassword = (requestPassword: string, databasePassword: str
return requestPassword === databasePassword; return requestPassword === databasePassword;
}; };
export const createAccount = async (accountData: IDatabaseAccount) => { export const createAccount = async (accountData: IDatabaseAccount): Promise<IDatabaseAccountJson> => {
const account = new Account(accountData); const account = new Account(accountData);
try { try {
await account.save(); await account.save();

30
src/types/loginTypes.ts
View File

@ -1,4 +1,18 @@
export interface ILoginResponse extends Omit<IDatabaseAccountDocument, "email" | "password"> { export interface IAccountAndLoginResponseCommons {
DisplayName: string;
CountryCode: string;
ClientType: string;
CrossPlatformAllowed: boolean;
ForceLogoutVersion: number;
AmazonAuthToken?: string;
AmazonRefreshToken?: string;
ConsentNeeded: boolean;
TrackedSettings: string[];
Nonce: number;
}
coderabbitai[bot] commented 2024-12-22 15:12:10 -08:00 (Migrated from github.com)
Review
Copy Link

⚠️ Potential issue

Security concern: Sensitive fields in common interface may leak into login response

The IAccountAndLoginResponseCommons interface includes potentially sensitive fields like AmazonAuthToken and AmazonRefreshToken. By making these common between account and login response, we risk exposing sensitive data in the login response, which contradicts the PR's objective of preventing field spillage.

Consider splitting this into two separate interfaces:

interface IAccountFields {
    DisplayName: string;
    CountryCode: string;
    ClientType: string;
    CrossPlatformAllowed: boolean;
    ForceLogoutVersion: number;
    AmazonAuthToken?: string;
    AmazonRefreshToken?: string;
    ConsentNeeded: boolean;
    TrackedSettings: string[];
    Nonce: number;
}

interface ILoginResponseFields {
    DisplayName: string;
    CountryCode: string;
    ClientType: string;
    CrossPlatformAllowed: boolean;
    ForceLogoutVersion: number;
    ConsentNeeded: boolean;
    TrackedSettings: string[];
    Nonce: number;
}
_:warning: Potential issue_ **Security concern: Sensitive fields in common interface may leak into login response** The `IAccountAndLoginResponseCommons` interface includes potentially sensitive fields like `AmazonAuthToken` and `AmazonRefreshToken`. By making these common between account and login response, we risk exposing sensitive data in the login response, which contradicts the PR's objective of preventing field spillage. Consider splitting this into two separate interfaces: ```typescript interface IAccountFields { DisplayName: string; CountryCode: string; ClientType: string; CrossPlatformAllowed: boolean; ForceLogoutVersion: number; AmazonAuthToken?: string; AmazonRefreshToken?: string; ConsentNeeded: boolean; TrackedSettings: string[]; Nonce: number; } interface ILoginResponseFields { DisplayName: string; CountryCode: string; ClientType: string; CrossPlatformAllowed: boolean; ForceLogoutVersion: number; ConsentNeeded: boolean; TrackedSettings: string[]; Nonce: number; } ``` <!-- This is an auto-generated comment by CodeRabbit -->
export interface ILoginResponse extends IAccountAndLoginResponseCommons {
id: string;
Groups: IGroup[]; Groups: IGroup[];
BuildLabel: string; BuildLabel: string;
MatchmakingBuildId: string; MatchmakingBuildId: string;
@ -10,7 +24,7 @@ export interface ILoginResponse extends Omit<IDatabaseAccountDocument, "email" |
} }
// Includes virtual ID // Includes virtual ID
export interface IDatabaseAccountDocument extends IDatabaseAccount { export interface IDatabaseAccountJson extends IDatabaseAccount {
id: string; id: string;
} }
@ -19,19 +33,9 @@ export interface IGroup {
experimentGroup: string; experimentGroup: string;
} }
export interface IDatabaseAccount { export interface IDatabaseAccount extends IAccountAndLoginResponseCommons {
email: string; email: string;
password: string; password: string;
DisplayName: string;
CountryCode: string;
ClientType: string;
CrossPlatformAllowed: boolean;
ForceLogoutVersion: number;
AmazonAuthToken?: string;
AmazonRefreshToken?: string;
ConsentNeeded: boolean;
TrackedSettings: string[];
Nonce: number;
LastLoginDay?: number; LastLoginDay?: number;
} }