Skip to content

Sec #11

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

Open
Commando-X wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Sec #11

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
77b0f31
user3
Commando-X Feb 28, 2026
f17854b
here we go
Commando-X Feb 28, 2026
5c4bc52
here we go 2
Commando-X Feb 28, 2026
216a3e0
loan
Commando-X Feb 28, 2026
422dc35
otp update
Commando-X Feb 28, 2026
caafe23
hmm
Commando-X Feb 28, 2026
2f96571
pword
Commando-X Feb 28, 2026
58acb27
OKAY
Commando-X Feb 28, 2026
93403f1
no wonder
Commando-X Feb 28, 2026
9689240
New branch
Commando-X Feb 28, 2026
be0e8f0
key
Commando-X Feb 28, 2026
385e713
hmm
Commando-X Feb 28, 2026
94eee11
update
Commando-X Feb 28, 2026
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
2 changes: 1 addition & 1 deletion badApi/account.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const router = express.Router();
// In-memory database for storing user information (including sensitive data)
const usersInfo = [
{ username: 'user1', dob: '1990-01-01', address: '5, 3rd mainland Bridge', cardInfo: '1234-5678-9012-3456', children: ['Child1', 'Child2'], balance: 1000, role: 'user' },
{ username: 'user2', dob: '1985-05-15', address: '7, Ghost street', cardInfo: '9876-5432-1098-7654', children: ['Child3'], balance: 500, role: 'user' },
{ username: 'user3', dob: '1985-05-15', address: '7, Ghost street', cardInfo: '9876-5432-1098-7654', children: ['Child3'], balance: 500, role: 'user' },

@claude claude Bot Feb 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: HIGH — Hardcoded PII and Credit Card Numbers in Source Code + Data Inconsistency Introduced by This PR

This line contains a full, unmasked credit card number (9876-5432-1098-7654) and personally identifiable information (DoB, address, children's names) embedded directly in source code.

Additionally, this PR renames the username from user2 to user3 here, while admin.js still references user2 for the same logical record. This data inconsistency means the admin loan-grant endpoint and account endpoint now operate on different user data sets.

Impact:

  • Real card numbers / PII in source code are exposed to every developer, CI system, and anyone with repo access — violating PCI-DSS and data minimisation requirements
  • Inconsistent test data can mask authorization bypass bugs in testing

Recommendation:

  • Replace all hardcoded sensitive values with clearly fake placeholders (e.g., XXXX-XXXX-XXXX-1234)
  • Keep seed data consistent across modules, or share a single source-of-truth fixture file

Comment on lines 6 to +7

@claude claude Bot Feb 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 CRITICAL — Hardcoded PII and Payment Card Data in Source Code

Issue: Full credit card numbers (cardInfo), dates of birth, home addresses, and family information are hardcoded as static in-memory data in source code.

Risk:

  • PCI-DSS violation: Full PANs (Primary Account Numbers) in source code is an automatic PCI-DSS Level 1 failure. Even "test" card numbers can constitute a violation if they look like real card numbers.
  • GDPR/privacy violation: Real-looking PII (DOB, addresses) in committed code can constitute sensitive data exposure (Article 5 GDPR).
  • This data is permanently in git history; removing it from the file is insufficient.

Fix: Use obviously fake, clearly-marked test data that cannot be confused with real credentials:

// SAFE TEST DATA — clearly synthetic, not real PAN format
{ username: 'user1', dob: 'XXXX-XX-XX', address: 'TEST_ADDRESS', cardInfo: 'TEST-CARD-XXXX', ... }

Never store card numbers — even partial — in application code. Use a tokenization service for any payment card reference.

Reference: CWE-312 (Cleartext Storage of Sensitive Information) | OWASP A02:2021 | PCI-DSS Requirement 3

];

/**
Expand Down
2 changes: 1 addition & 1 deletion badApi/admin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const router = express.Router();
const users = []; // Simulated user storage for the sake of example
const usersInfo = [
{ username: 'user1', dob: '1990-01-01', address: '5, 3rd mainland Bridge', cardInfo: '1234-5678-9012-3456', children: ['Child1', 'Child2'], balance: 1000, role: 'user' },
{ username: 'user2', dob: '1985-05-15', address: '7, Ghost street', cardInfo: '9876-5432-1098-7654', children: ['Child3'], balance: 500, role: 'user' },
{ username: 'user2', dob: '1985-05-17', address: '7, Ghost street', cardInfo: '9876-5432-1098-7654', children: ['Child3'], balance: 500, role: 'user' },

@claude claude Bot Feb 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: CRITICAL — Broken Function-Level Authorization on Admin Endpoints

This PR changes a DoB in the seed data, but the critical issue is the surrounding module: POST /admin/hidden-admin-signup (line 150) has no authentication whatsoever. Any anonymous caller can POST /admin/hidden-admin-signup with any username/password and receive an admin-role account.

Similarly, POST /admin/grant-loan/:username (line 160) modifies user balances with no admin token check.

Impact: Complete privilege escalation — any unauthenticated user can become an admin, then grant themselves or others arbitrary balance increases.

Recommendation:

  • Both endpoints must require a valid admin JWT/session before execution
  • The "hidden" admin signup endpoint should be removed from the production surface and from Swagger docs entirely
  • Admin account creation should only be possible via a seeded initial admin or out-of-band provisioning

Comment on lines 7 to +8

@claude claude Bot Feb 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 CRITICAL — Hardcoded PII and Payment Card Data in Source Code

Issue: Same pattern as account.js — full credit card PANs, DOB, and address hardcoded in the admin module's in-memory store.

Risk: Identical to account.js: PCI-DSS violation, sensitive data exposure via git history, and GDPR risk. Admin modules often have broader data access, amplifying the exposure surface.

Fix: Replace with clearly synthetic, obviously-fake test data. Consider a shared test-fixtures module rather than duplicating data across files.

Reference: CWE-312 | OWASP A02:2021 | PCI-DSS Requirement 3

];

/**
Expand Down
2 changes: 1 addition & 1 deletion badApi/db.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const mysql = require('mysql2');
const connection = mysql.createConnection({
host: 'localhost',
user: 'ghost',
password: 'ghost_sec', //coming back here
password: 'ghost_sec1', //coming back here

@claude claude Bot Feb 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: CRITICAL — Hardcoded Database Credential

The password was changed from ghost_sec to ghost_sec1, but it is still hardcoded in source code. This commit has permanently embedded a real credential into git history.

Impact: Anyone with read access to the repository (or git history) gains the database password. Even if the password is rotated, the old value survives in history unless a full history rewrite is performed.

Recommendation:

  • Load credentials from environment variables: password: process.env.DB_PASSWORD
  • Rotate the database password immediately since both values are now in git history
  • Add db.js (or a .env file) to .gitignore if it ever contained real secrets
  • The comment //coming back here indicates the developer knew this was a problem — this must not ship

database: 'badapi_db2'

@claude claude Bot Feb 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 CRITICAL — Hardcoded Database Credential

Issue: Database password is hardcoded directly in source code. Rotating from ghost_sec → ghost_sec1 does not fix the underlying problem; both values are now in git history forever.

Risk: Anyone with read access to the repository (or any git clone / fork / leak) has the database password. Credentials committed to git cannot be truly revoked from history without a rewrite.

Fix:

Suggested change
database: 'badapi_db2'
password: process.env.DB_PASSWORD,

Store all credentials in environment variables and inject them at runtime (e.g., via .env + dotenv, GitHub Actions secrets, or a secrets manager). Rotate the current password immediately.

Reference: CWE-798 (Use of Hard-coded Credentials) | OWASP A02:2021 Cryptographic Failures

});

Expand Down
2 changes: 1 addition & 1 deletion badApi/loan.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const router = express.Router();

// In-memory database for loan applications
let loanApplications = [
{ username: 'user1', amount: 1000, status: 'pending' },
{ username: 'user1', amount: 2000, status: 'pending' },

@claude claude Bot Feb 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: HIGH — Unauthenticated Loan Submission with Client-Controlled Identity (BOLA)

The PR changes the seed amount from 1000 to 2000, but the deeper issue is the POST /loan/apply handler (line 73): it accepts username directly from the request body with no authentication and no ownership check.

Impact: Any unauthenticated caller can submit a loan application on behalf of any user, including fabricating usernames. There is also no validation preventing negative or zero amounts.

Recommendation:

  • Require authentication; derive username from the verified session/token, not from the request body
  • Validate that amount is a positive number within an allowed range
  • The endpoint should return 401 if no valid token is present

];

/**
Expand Down
4 changes: 2 additions & 2 deletions badApi/otp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const router = express.Router();
* properties:
* otp:
* type: integer
* example: 123456
* example: 1234567

@claude claude Bot Feb 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: LOW — Swagger Example Contradicts Implementation

The example was updated to 1234567 (7 digits), but the code on line 42 generates a 3-digit number (100–999). This creates misleading API documentation and masks the more serious 3-digit OTP weakness from API consumers.

Recommendation: Fix the underlying OTP generation (see comment on line 42), then update the example to reflect the corrected 6-digit format.

* 500:
* description: Internal server error
* content:
Expand All @@ -39,7 +39,7 @@ const router = express.Router();
*/

router.post('/otp', (req, res) => {
const otp = Math.floor(100 + Math.random() * 900); // Generate 3-digit random number
const otp = Math.floor(100 + Math.random() * 900); // Generate 3-digit random number.

@claude claude Bot Feb 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: CRITICAL — Insecure OTP: Weak Entropy + Trivial Brute Force + Exposed in Response

Three compounding problems on this line:

  1. Math.random() is not cryptographically secure. It is predictable and unsuitable for security tokens. Use crypto.randomInt() from Node's built-in crypto module.

  2. Only 3 digits (100–999 = 900 possible values). An attacker can exhaust the entire OTP space in seconds with no rate limiting in place. A minimum of 6 digits is required (1,000,000 combinations).

  3. OTP is returned directly in the API response. This completely negates its purpose — the OTP must be delivered via an out-of-band channel (SMS, email, authenticator app) and only a success/failure response should be returned to the caller.

Impact: OTP can be trivially predicted or brute-forced, enabling account takeover of any user.

Recommendation:

const otp = crypto.randomInt(100000, 999999); // 6-digit CSPRNG OTP
// Send via SMS/email; do NOT return in response
res.json({ message: 'OTP sent' });

Add per-IP and per-user rate limiting (e.g., max 5 attempts, 15-minute lockout).

return res.json({ otp });
Comment on lines +42 to 43

@claude claude Bot Feb 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:large_yellow_circle: HIGH — Weak OTP: Non-Cryptographic Randomness + Trivial Brute-Force Surface + Secret Returned to Caller

Issue (Line 42) — CWE-338: Math.random() is a pseudo-random number generator (PRNG) seeded predictably by the JavaScript runtime. It is not cryptographically secure. An attacker who can observe timing or output patterns may predict future OTPs.

Issue (Line 42) — 3-digit space: Math.floor(100 + Math.random() * 900) produces only 900 possible values (100–999). Even with a perfect PRNG, this is trivially exhausted by brute force in under a second.

Issue (Line 43) — Sensitive Data Exposure: The OTP is returned directly in the HTTP response to the requester. A valid OTP flow sends the secret out-of-band (SMS, email, authenticator app) to a verified channel — it must never be returned to the API caller, or it provides zero authentication value.

Fix:

const { randomInt } = require('crypto');

router.post('/otp', (req, res) => {
  // 6-digit, cryptographically secure OTP — sent out-of-band, NOT returned
  const otp = randomInt(100000, 999999);
  // sendOtpViaSms(req.user.phone, otp);  // deliver to verified channel
  return res.json({ message: 'OTP sent' }); // never return the OTP value
});

Also add rate limiting (e.g., 3 attempts per 15 minutes) and expiry (e.g., 5-minute TTL).

Reference: CWE-338 (Weak PRNG) | CWE-330 (Insufficient Randomness) | OWASP A02:2021 | NIST SP 800-63B

});

Expand Down