-
Notifications
You must be signed in to change notification settings - Fork 0
fix(backend): resolve critical audit bugs O3, O7, H1, A2, A3, A4, H2c, H7, H8, C2 #23
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| import { GraphQLError, ValidationContext, ASTVisitor } from 'graphql'; | ||
|
|
||
| export function depthLimit(maxDepth: number) { | ||
| return (context: ValidationContext): ASTVisitor => { | ||
| let depth = 0; | ||
| return { | ||
| Field: { | ||
| enter(node) { | ||
| depth++; | ||
| if (depth > maxDepth) { | ||
| context.reportError( | ||
| new GraphQLError( | ||
| `Query depth exceeds maximum limit of ${maxDepth}`, | ||
| { | ||
| nodes: [node], | ||
| }, | ||
| ), | ||
| ); | ||
| } | ||
| }, | ||
| leave() { | ||
| depth--; | ||
| }, | ||
| }, | ||
| }; | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,13 @@ export class AuthCredentialsService { | |
| include: { group: true }, | ||
| }); | ||
|
|
||
| if (!customer || !customer.active || this.isLocked(customer.lockedUntil)) { | ||
| if ( | ||
| !customer || | ||
| !customer.active || | ||
| this.isLocked(customer.lockedUntil) || | ||
| (customer.lockedUntil === null && | ||
| customer.failedLoginAttempts >= LOCKOUT_THRESHOLD) | ||
| ) { | ||
| return null; | ||
| } | ||
|
Comment on lines
+24
to
32
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition introduces a permanent lockout bug. If if (!customer || !customer.active || this.isLocked(customer.lockedUntil)) {
return null;
} |
||
|
|
||
|
|
@@ -44,7 +50,13 @@ export class AuthCredentialsService { | |
| include: { role: true }, | ||
| }); | ||
|
|
||
| if (!employee || !employee.active || this.isLocked(employee.lockedUntil)) { | ||
| if ( | ||
| !employee || | ||
| !employee.active || | ||
| this.isLocked(employee.lockedUntil) || | ||
| (employee.lockedUntil === null && | ||
| employee.failedLoginAttempts >= LOCKOUT_THRESHOLD) | ||
| ) { | ||
| return null; | ||
| } | ||
|
Comment on lines
+53
to
61
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition introduces a permanent lockout bug for employees. If if (!employee || !employee.active || this.isLocked(employee.lockedUntil)) {
return null;
} |
||
|
|
||
|
|
@@ -80,15 +92,20 @@ export class AuthCredentialsService { | |
| }); | ||
| if (customer.failedLoginAttempts >= LOCKOUT_THRESHOLD) { | ||
| const lockedUntil = this.lockoutDate(); | ||
| await this.prisma.customer.update({ | ||
| where: { id }, | ||
| const updated = await this.prisma.customer.updateMany({ | ||
| where: { | ||
| id, | ||
| OR: [{ lockedUntil: null }, { lockedUntil: { lt: new Date() } }], | ||
| }, | ||
| data: { lockedUntil }, | ||
| }); | ||
| await this.sendAccountLockoutAlert( | ||
| customer.email, | ||
| 'customer', | ||
| lockedUntil, | ||
| ); | ||
| if (updated.count > 0) { | ||
| await this.sendAccountLockoutAlert( | ||
| customer.email, | ||
| 'customer', | ||
| lockedUntil, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -99,15 +116,20 @@ export class AuthCredentialsService { | |
| }); | ||
| if (employee.failedLoginAttempts >= LOCKOUT_THRESHOLD) { | ||
| const lockedUntil = this.lockoutDate(); | ||
| await this.prisma.employee.update({ | ||
| where: { id }, | ||
| const updated = await this.prisma.employee.updateMany({ | ||
| where: { | ||
| id, | ||
| OR: [{ lockedUntil: null }, { lockedUntil: { lt: new Date() } }], | ||
| }, | ||
| data: { lockedUntil }, | ||
| }); | ||
| await this.sendAccountLockoutAlert( | ||
| employee.email, | ||
| 'employee', | ||
| lockedUntil, | ||
| ); | ||
| if (updated.count > 0) { | ||
| await this.sendAccountLockoutAlert( | ||
| employee.email, | ||
| 'employee', | ||
| lockedUntil, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,30 +66,40 @@ export class AuthOAuthService { | |
| throw new Error('Default customer group missing'); | ||
| } | ||
|
|
||
| const customer = await this.prisma.customer.upsert({ | ||
| where: { email: profile.email }, | ||
| update: {}, | ||
| create: { | ||
| email: profile.email, | ||
| password: await this.passwordSecurity.hashWithoutPolicy( | ||
| randomBytes(32).toString('base64url'), | ||
| ), | ||
| firstname: profile.firstname, | ||
| lastname: profile.lastname, | ||
| groupId: group.id, | ||
| }, | ||
| include: { group: true }, | ||
| }); | ||
| return this.prisma.$transaction(async (tx) => { | ||
| const customer = await tx.customer.upsert({ | ||
| where: { email: profile.email }, | ||
| update: {}, | ||
| create: { | ||
| email: profile.email, | ||
| password: await this.passwordSecurity.hashWithoutPolicy( | ||
| randomBytes(32).toString('base64url'), | ||
| ), | ||
| firstname: profile.firstname, | ||
| lastname: profile.lastname, | ||
| groupId: group.id, | ||
| }, | ||
| include: { group: true }, | ||
| }); | ||
|
Comment on lines
+69
to
+83
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Performing CPU-intensive cryptographic operations like password hashing ( const password = await this.passwordSecurity.hashWithoutPolicy(
randomBytes(32).toString('base64url'),
);
return this.prisma.$transaction(async (tx) => {
const customer = await tx.customer.upsert({
where: { email: profile.email },
update: {},
create: {
email: profile.email,
password,
firstname: profile.firstname,
lastname: profile.lastname,
groupId: group.id,
},
include: { group: true },
}); |
||
|
|
||
| await this.prisma.oAuthAccount.create({ | ||
| data: { | ||
| provider: profile.provider, | ||
| providerAccountId: profile.providerAccountId, | ||
| email: profile.email, | ||
| customerId: customer.id, | ||
| }, | ||
| await tx.oAuthAccount.upsert({ | ||
| where: { | ||
| provider_providerAccountId: { | ||
| provider: profile.provider, | ||
| providerAccountId: profile.providerAccountId, | ||
| }, | ||
| }, | ||
| update: {}, | ||
| create: { | ||
| provider: profile.provider, | ||
| providerAccountId: profile.providerAccountId, | ||
| email: profile.email, | ||
| customerId: customer.id, | ||
| }, | ||
| }); | ||
|
|
||
| return customer; | ||
| }); | ||
|
Comment on lines
+69
to
102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: sed -n '1,220p' libs/backend/core/auth/src/lib/services/auth-oauth.service.tsRepository: tahiry-dev-29/Swift-Shop Length of output: 6311 🏁 Script executed: sed -n '1,220p' libs/backend/core/auth/src/lib/services/password-security.service.tsRepository: tahiry-dev-29/Swift-Shop Length of output: 2668 Move the password hash out of the transaction. 🤖 Prompt for AI Agents |
||
| return customer; | ||
| } | ||
|
|
||
| private getOAuthConfig(provider: OAuthProvider) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,18 @@ export class MediaService implements OnModuleInit { | |
|
|
||
| try { | ||
| const image = sharp(file.buffer); | ||
| const metadata = await image.metadata(); | ||
|
|
||
| if (metadata.width && metadata.width > 4096) { | ||
| throw new BadRequestException( | ||
| `Image width of ${metadata.width}px exceeds maximum allowed size of 4096px`, | ||
| ); | ||
| } | ||
| if (metadata.height && metadata.height > 4096) { | ||
| throw new BadRequestException( | ||
| `Image height of ${metadata.height}px exceeds maximum allowed size of 4096px`, | ||
| ); | ||
| } | ||
|
Comment on lines
+40
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Specific dimension error messages are swallowed by the catch block. The Re-throw 🛡️ Proposed fix } catch (e) {
+ if (e instanceof HttpException) {
+ throw e;
+ }
this.logger.error(`Image processing failed: ${String(e)}`);
throw new BadRequestException('Could not process image');
}🤖 Prompt for AI Agents |
||
|
|
||
| // Save original as WebP | ||
| const originalPath = path.join( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,24 +12,19 @@ export class ProductStockService { | |||||
|
|
||||||
| async updateStock(input: UpdateStockInput) { | ||||||
| const { productId, combinationId, ...stockData } = input; | ||||||
| const existingStock = await this.prisma.stock.findFirst({ | ||||||
| where: { | ||||||
| OR: [ | ||||||
| { productId: productId ?? undefined }, | ||||||
| { combinationId: combinationId ?? undefined }, | ||||||
| ], | ||||||
| }, | ||||||
| }); | ||||||
|
|
||||||
| if (existingStock) { | ||||||
| return this.prisma.stock.update({ | ||||||
| where: { id: existingStock.id }, | ||||||
| data: stockData, | ||||||
| }); | ||||||
| if (!productId && !combinationId) { | ||||||
| throw new BadRequestException( | ||||||
| 'Either productId or combinationId must be provided', | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| return this.prisma.stock.create({ | ||||||
| data: { | ||||||
| const where = productId ? { productId } : { combinationId }; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When both
Suggested change
|
||||||
|
|
||||||
| return this.prisma.stock.upsert({ | ||||||
| where, | ||||||
| update: stockData, | ||||||
| create: { | ||||||
| productId, | ||||||
| combinationId, | ||||||
| ...stockData, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,30 +40,40 @@ export class GuestCheckoutService { | |
| firstname: string, | ||
| lastname: string, | ||
| ) { | ||
| const existingCustomer = await this.prisma.customer.findUnique({ | ||
| where: { email }, | ||
| }); | ||
| if (existingCustomer) { | ||
| return existingCustomer; | ||
| } | ||
|
|
||
| const group = await this.prisma.customerGroup.findFirst({ | ||
| orderBy: { name: 'asc' }, | ||
| }); | ||
| if (!group) { | ||
| throw new BadRequestException('Default customer group is missing'); | ||
| } | ||
|
|
||
| return this.prisma.customer.create({ | ||
| data: { | ||
| email, | ||
| firstname, | ||
| lastname, | ||
| password: `guest:${randomUUID()}`, | ||
| isGuest: true, | ||
| groupId: group.id, | ||
| }, | ||
| }); | ||
| try { | ||
| return await this.prisma.customer.create({ | ||
| data: { | ||
| email, | ||
| firstname, | ||
| lastname, | ||
| password: `guest:${randomUUID()}`, | ||
| isGuest: true, | ||
| groupId: group.id, | ||
| }, | ||
| }); | ||
| } catch (e: unknown) { | ||
| if ( | ||
| typeof e === 'object' && | ||
| e !== null && | ||
| 'code' in e && | ||
| (e as { code: string }).code === 'P2002' | ||
| ) { | ||
| const existingCustomer = await this.prisma.customer.findUnique({ | ||
| where: { email }, | ||
| }); | ||
| if (existingCustomer) { | ||
| return existingCustomer; | ||
| } | ||
|
Comment on lines
+68
to
+73
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Allowing guest checkout to silently proceed and merge carts with an existing registered customer account (where const existingCustomer = await this.prisma.customer.findUnique({
where: { email },
});
if (existingCustomer) {
if (!existingCustomer.isGuest) {
throw new BadRequestException(
'This email is registered to an existing account. Please log in to proceed.',
);
}
return existingCustomer;
} |
||
| } | ||
| throw e; | ||
| } | ||
|
Comment on lines
+50
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Check whether the previous version of getOrCreateGuestCustomer checked isGuest,
# and whether downstream code guards against non-guest customers.
# Search for isGuest checks in the guest checkout flow
rg -n 'isGuest' libs/backend/features/order/src/lib/guest-checkout.service.ts
# Check if attachCartToCustomer or createOrderFromCart validate customer type
rg -n 'isGuest|guest' libs/backend/features/order/src/lib/order-creation.service.tsRepository: tahiry-dev-29/Swift-Shop Length of output: 410 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Map the relevant files first, then inspect the guest checkout flow and its callers.
ast-grep outline libs/backend/features/order/src/lib/guest-checkout.service.ts --view expanded
ast-grep outline libs/backend/features/order/src/lib/order-creation.service.ts --view expanded
echo '--- guest-checkout.service.ts ---'
cat -n libs/backend/features/order/src/lib/guest-checkout.service.ts | sed -n '1,220p'
echo '--- order-creation.service.ts ---'
cat -n libs/backend/features/order/src/lib/order-creation.service.ts | sed -n '1,260p'Repository: tahiry-dev-29/Swift-Shop Length of output: 11915 Guard against non-guest accounts here If 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| private async attachCartToCustomer(cartId: string, customerId: string) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: tahiry-dev-29/Swift-Shop
Length of output: 204
🏁 Script executed:
sed -n '1,220p' apps/api/src/config/graphql-depth.util.tsRepository: tahiry-dev-29/Swift-Shop
Length of output: 787
🏁 Script executed:
rg -n "depthLimit\(" apps/api/src -SRepository: tahiry-dev-29/Swift-Shop
Length of output: 324
🌐 Web query:
graphql-js validation visitor fragment definitions traversal fragment spreads depthLimit validation rule semantics💡 Result:
In graphql-js, the validation process is managed by the validate function, which utilizes a specialized visitor to perform a depth-first traversal of the GraphQL document AST [1][2]. This traversal is orchestrated in parallel across all enabled validation rules [2]. Fragment Definitions and Traversal During validation, the engine does not automatically "inline" fragments into the AST [2]. Instead, it uses the ValidationContext to manage relationships between fragment definitions and fragment spreads [3]. The ValidationContext tracks selections and dependencies, allowing rules to inspect fragment spreads via methods like getFragmentSpreads(selectionSet) [3][4]. When a validation rule needs to analyze a fragment's contents, it typically retrieves the fragment definition by name from the document and inspects its associated selection set [3][4]. TypeInfo, a utility class, is commonly used alongside the visitor to track the schema types corresponding to the current AST nodes during this traversal [5]. DepthLimit Validation Rule There is no standard built-in DepthLimit validation rule in the core graphql-js specification-compliant ruleset [6]. While there is a MaxIntrospectionDepthRule for limiting introspection, standard query depth limiting is often implemented as a custom validation rule [7][6]. Developers frequently implement such rules by creating a visitor that tracks the current depth during traversal, incrementing it when entering a selection set and decrementing it upon leaving, then raising a GraphQLError if a threshold is exceeded [8][7]. Semantics of Validation Traversal The visitor pattern in graphql-js supports enter and leave hooks for specific AST node types [9]. Because the validation process uses visitInParallel, it ensures that all registered validation rules receive the same node during the document-wide traversal, maintaining high performance while allowing rules to maintain their own isolated state [2]. When dealing with fragment spreads, the validation engine maintains a clear separation between the operation's own selection set and the selection sets defined within fragments, relying on the context to resolve these references on-demand [3][4].
Citations:
🏁 Script executed:
Repository: tahiry-dev-29/Swift-Shop
Length of output: 239
🌐 Web query:
graphql-js visit document fragment definitions visited separately from fragment spreads order validation depth rule💡 Result:
In graphql-js, the document traversal performed by the
visitfunction is a depth-first search that treats all nodes in the AST as part of a single tree [1]. Fragment definitions and fragment spreads are visited as part of this standard document traversal; they are not inherently "separated" by the visitor logic into distinct phases or execution orders [2][1]. Regarding validation, the order in which specific rules are processed is defined by thespecifiedRulesarray, which establishes a predictable sequence for error reporting [3]. While thevisitfunction itself follows the AST structure (depth-first), individual validation rules, such asNoFragmentCyclesRuleorKnownFragmentNamesRule, may implement their own logic to track fragment dependencies and depth [3]. Key points regarding your query: 1. Traversal Order: Thevisitfunction performs a depth-first traversal of the entire document AST [1]. There is no separate "fragment definitions" phase; they are visited when the walker encounters them in the tree [1]. 2. Validation Context: Validation rules interact with the document by registering visitors that execute during this traversal [4]. Some advanced features, such as experimental fragment arguments, require initial passes (e.g., usingTypeInfo) to index fragment definitions before evaluating spreads to resolve local parameters correctly [5][6]. 3. Depth and Cycles: Rules likeNoFragmentCyclesRuleand depth-related validation must explicitly maintain state across the traversal to detect circular dependencies or exceed limits, as the standardvisitfunction simply traverses the provided tree [3]. 4. Fragment Spreads: Recent updates ingraphql-jshave refined how fragment spreads are visited, particularly concerning deferred spreads, to avoid unnecessary re-visitation during traversal [7]. If you are building a custom validation rule that requires knowledge of fragment definitions before evaluating spreads, it is common practice to perform an initial pass over the document to collect and index fragment definitions, or to useTypeInfoto maintain state during the traversal [5][6].Citations:
Named fragment spreads undercount depth.
This visitor only tracks
Fieldnodes, so selections reached through...Fragmentare measured in isolation instead of at the spread site. A query can push extra nesting into named fragments and exceedmaxDepthwithout tripping this rule. Handle fragment spreads recursively or use a depth-limit library that does.🤖 Prompt for AI Agents