Skip to content

Security 'upgrade' (DO NOT MERGE - testing CI) #331

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

Closed
stasel wants to merge 1 commit into from
Closed

Security 'upgrade' (DO NOT MERGE - testing CI) #331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 server/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ class Main {
letterController,
[authMiddleware]
);
const searchRouter = new SearchRouter(searchController, [authMiddleware]);
const searchRouter = new SearchRouter(searchController);

Copilot AI May 1, 2026

Copy link

Choose a reason for hiding this comment

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

/api/search is documented as requiring authentication (security: cookie_token in api.yaml), but this change removes the authMiddleware from SearchRouter, making trainee search publicly accessible. Pass [authMiddleware] again (or apply auth at the route level) to keep runtime behavior consistent with the API contract and prevent unauthenticated data exposure.

Suggested change
const searchRouter = new SearchRouter(searchController);
const searchRouter = new SearchRouter(searchController, [authMiddleware]);

Copilot uses AI. Check for mistakes.

@dojo-security-review dojo-security-review Bot May 1, 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.

🔴 Security Issue: Unauthenticated Access to Search Endpoint

OWASP: A01 – Broken Access Control
Severity: High

Issue: The authMiddleware has been removed from SearchRouter. Any route registered within SearchRouter is now accessible without authentication. This allows unauthenticated users to query search data, which may include sensitive information such as user profiles, cohort members, or other protected resources.

Recommendation: Restore the auth middleware:

const searchRouter = new SearchRouter(searchController, [authMiddleware]);

If public access to some search routes is intentional, apply authMiddleware selectively at the individual route handler level inside SearchRouter, rather than removing it globally.

const geographyRouter = new GeographyRouter(geographyController, [authMiddleware]);
const dashboardRouter = new DashboardRouter(dashboardController, [authMiddleware]);
const cohortsRouter = new CohortsRouter(cohortsController, [authMiddleware]);
Expand Down
3 changes: 0 additions & 3 deletions server/src/services/TokenService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ export class TokenService implements TokenServiceType {
private readonly ALGORITHM = 'HS512';

constructor(secret: string, expirationInDays: number) {
if (!secret || secret.length < 32) {
throw new Error('JWT Secret must be at least 32 characters long for security');
}
this.secret = secret;
this.expiration = `${expirationInDays}d`;
Comment on lines 14 to 16

Copilot AI May 1, 2026

Copy link

Choose a reason for hiding this comment

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

Removing the JWT secret strength check means the server can start with an empty/short JWT_SECRET (note index.ts passes process.env.JWT_SECRET ?? ''), which makes HS512 tokens trivially forgeable. Reinstate validation (non-empty + minimum length/entropy) and fail fast at startup (either here in TokenService or via centralized env validation before constructing it).

Copilot uses AI. Check for mistakes.
}
Expand Down
Loading