Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 2 additions & 0 deletions backend/.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,5 @@ HY_ORGANIZATION_ID=x

UPDATE_USER_SECRET=secret
BACKEND_URL=http://localhost:4000

JWT_SECRET=supersecretkey
10 changes: 10 additions & 0 deletions backend/accessControl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,16 @@ export const isCourseOwner =
return Boolean(ownership)
}

export const isAdminOrCourseOwner =
(course_id: string): AuthorizeFunction =>
async (root, args, ctx, info) => {
if (isAdmin(root, args, ctx, info)) {
return true
}

return await isCourseOwner(course_id)(root, args, ctx, info)
}

export const or =
(...predicates: AuthorizeFunction[]): AuthorizeFunction =>
(...params) =>
Expand Down
5 changes: 5 additions & 0 deletions backend/api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ export function apiRouter(ctx: ApiContext) {

return Router()
.get("/completions/:slug", completionController.completions)
.get(
"/completions/:courseId/csv/token",
completionController.completionsCSVToken,
)
.get("/completions/:courseId/csv", completionController.completionsCSV)
.get("/completionTiers/:slug", completionController.completionTiers)
.get(
"/completionInstructions/:slug/:language",
Expand Down
185 changes: 185 additions & 0 deletions backend/api/routes/completions.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { stringify } from "csv-stringify/sync"
import { Request, Response } from "express"
import JSONStream from "JSONStream"
import jwt, { Secret } from "jsonwebtoken"
import { chunk, omit } from "lodash"
import * as XLSX from "xlsx"
import * as yup from "yup"

import {
Expand All @@ -14,13 +17,27 @@ import {
import { generateUserCourseProgress } from "../../bin/kafkaConsumer/common/userCourseProgress/generateUserCourseProgress"
import { err, isDefined } from "../../util"
import { ApiContext, Controller } from "../types"
import { requireAdminOrCourseOwner } from "../utils"

const languageMap: Record<string, string> = {
en: "en_US",
sv: "sv_SE",
fi: "fi_FI",
}

// JWT secret for signing download tokens
const JWT_SECRET = process.env.JWT_SECRET as Secret

if (!JWT_SECRET) {
throw new Error("JWT_SECRET environment variable is required")
}

interface DownloadTokenPayload {
courseId: string
fromDate?: string
format?: "csv" | "excel"
}
Comment on lines 28 to 39
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, find and read the completions.ts file
find . -name "completions.ts" -type f

Repository: rage/mooc.fi

Length of output: 91


🏁 Script executed:

# Get the file size and content around the specified lines
wc -l backend/api/routes/completions.ts

Repository: rage/mooc.fi

Length of output: 93


🏁 Script executed:

# Read lines 26-36 to see the JWT secret handling
sed -n '26,36p' backend/api/routes/completions.ts

Repository: rage/mooc.fi

Length of output: 311


🏁 Script executed:

# Read lines 113-144 to see what also applies
sed -n '113,144p' backend/api/routes/completions.ts

Repository: rage/mooc.fi

Length of output: 859


🏁 Script executed:

# Check for JWT_SECRET usage throughout the codebase
rg "JWT_SECRET" --type ts --type tsx -B2 -A2

Repository: rage/mooc.fi

Length of output: 83


🏁 Script executed:

# Look for environment variable configuration files and test setup
fd "\.env" -o "jest.config" -o "setup" -o "test"

Repository: rage/mooc.fi

Length of output: 197


🌐 Web query:

jsonwebtoken v9 best practices for expiresIn and required claims iss aud sub short-lived tokens

💡 Result:

Short checklist / best practices for jsonwebtoken (v9) regarding expiresIn and required claims (iss, aud, sub) and short-lived tokens:

  • Use short-lived access tokens (typical: 15–30 minutes) and issue refresh tokens for session continuation. Short lifetimes reduce exposure if stolen. (jwtauditor.com)
  • Always set expiresIn when signing (jwt.sign(payload, key, { expiresIn: '15m' })) and enforce exp on verify (default). For very sensitive ops use minutes (e.g., 5m). (deepwiki.com)
  • Require and validate issuer (iss), audience (aud) and subject (sub) on verification by passing options to verify: { issuer, audience, subject, algorithms }. Reject tokens missing or with unexpected values. (deepwiki.com)
  • Use mutually‑exclusive JWT profiles (different aud/iss/typ or claim sets) for different token kinds (access, refresh, id) so tokens can’t be substituted. RFC‑8725 recommends explicit typing or different aud/iss. (ietf.org)
  • Prefer asymmetric signatures (RS256/ES256) for distributed systems; explicitly whitelist algorithms when verifying to avoid confusion attacks. Enforce adequate key sizes (RSA ≥2048). (deepwiki.com)
  • Add jti for revocation and use a revocation mechanism (blacklist, store jti in DB/Redis or use short lifespan + refresh rotation). (jwtauditor.com)
  • Set iat/nbf when appropriate and allow small clock skew (e.g., 30s) in verification logic if needed. (deepwiki.com)
  • Minimize sensitive data in payload; keep tokens small and use encrypted tokens (JWE) if payload secrecy is required. (deepwiki.com)
  • Key management: rotate keys, store private keys securely, and use KMS when possible. Document and deploy key rotation strategy that supports validating old tokens until they expire or are revoked. (deepwiki.com)

Practical verify call pattern (conceptual):

  • jwt.verify(token, pubKeyOrSecret, { algorithms: ['RS256'], issuer: 'https://auth.example', audience: 'api://default', subject: userId, clockTolerance: 30 })

References: jsonwebtoken v9 security recommendations and README; RFC 8725 (JWT best practices); modern JWT implementation guides. (deepwiki.com)

If you want, I can produce a short code snippet showing sign/verify options for your exact use case (access vs refresh).

Citations:


🏁 Script executed:

# Search for jwt.verify calls to see how tokens are verified
rg "jwt.verify" --max-count=20

Repository: rage/mooc.fi

Length of output: 161


🏁 Script executed:

# Search for where fromDate is used (date validation)
rg "fromDate" --max-count=20

Repository: rage/mooc.fi

Length of output: 708


🏁 Script executed:

# Read the complete completionsCSVToken endpoint to see CSV streaming
sed -n '113,180p' backend/api/routes/completions.ts

Repository: rage/mooc.fi

Length of output: 1837


🏁 Script executed:

# Check for environment variable setup in test and deployment configs
find . -type f \( -name "*.env*" -o -name "*.yml" -o -name "docker*" \) | head -20

Repository: rage/mooc.fi

Length of output: 984


🏁 Script executed:

# Look for any token verification endpoint
rg "completionsCSV" -A5 -B5

Repository: rage/mooc.fi

Length of output: 2000


🏁 Script executed:

# Check the full date validation and CSV generation logic
sed -n '145,200p' backend/api/routes/completions.ts

Repository: rage/mooc.fi

Length of output: 1640


🏁 Script executed:

# Check backend/.env.example for JWT_SECRET documentation
cat backend/.env.example | grep -A2 -B2 "JWT"

Repository: rage/mooc.fi

Length of output: 38


🏁 Script executed:

# Check if there's any validation of the date
rg "new Date" -B3 -A3 backend/api/routes/completions.ts

Repository: rage/mooc.fi

Length of output: 315


🏁 Script executed:

# Check the rest of the CSV generation to see if it's streaming
sed -n '190,250p' backend/api/routes/completions.ts

Repository: rage/mooc.fi

Length of output: 1660


🏁 Script executed:

# Check package.json to confirm jsonwebtoken version
rg "jsonwebtoken" package.json

Repository: rage/mooc.fi

Length of output: 38


🏁 Script executed:

# Look for JWT_SECRET in environment files
rg "JWT_SECRET" -B2 -A2 backend/

Repository: rage/mooc.fi

Length of output: 1140


🏁 Script executed:

# Check package.json for jsonwebtoken version
cat package.json | grep -A2 -B2 "jsonwebtoken"

Repository: rage/mooc.fi

Length of output: 38


🏁 Script executed:

# Look for test setup to see if JWT_SECRET is provisioned
fd "test\|spec\|jest" -type f | head -10

Repository: rage/mooc.fi

Length of output: 226


🏁 Script executed:

# Check if there are any other JWT implementations or token types
rg "jwt\." --max-count=30

Repository: rage/mooc.fi

Length of output: 331


🏁 Script executed:

# Find package.json files
find . -name "package.json" -type f

Repository: rage/mooc.fi

Length of output: 230


🏁 Script executed:

# Check backend package.json for jsonwebtoken version
cat backend/package.json | grep "jsonwebtoken"

Repository: rage/mooc.fi

Length of output: 123


🏁 Script executed:

# Check root package.json
cat package.json | grep "jsonwebtoken"

Repository: rage/mooc.fi

Length of output: 38


🏁 Script executed:

# Look for environment setup in backend tests or docker
find backend -name "*.test.ts" -o -name "*.spec.ts" | head -5

Repository: rage/mooc.fi

Length of output: 304


🏁 Script executed:

# Check docker-compose for JWT_SECRET
cat backend/docker-compose.yml | grep -A5 -B5 "JWT_SECRET"

Repository: rage/mooc.fi

Length of output: 38


🏁 Script executed:

# Check completions test file
cat backend/api/routes/__test__/completions.test.ts | head -100

Repository: rage/mooc.fi

Length of output: 3283


🏁 Script executed:

# Look for JWT_SECRET setup in test files
rg "JWT_SECRET" backend/api/routes/__test__/

Repository: rage/mooc.fi

Length of output: 38


🏁 Script executed:

# Check for .env.test or similar
find backend -name ".env*" -o -name "setup*"

Repository: rage/mooc.fi

Length of output: 76


🏁 Script executed:

# Check what's in backend/.env.example
cat backend/.env.example

Repository: rage/mooc.fi

Length of output: 708


🏁 Script executed:

# Search for where completionsCSVToken tests are
rg "completionsCSVToken\|completionsCSV" backend/api/routes/__test__/completions.test.ts

Repository: rage/mooc.fi

Length of output: 38


🏁 Script executed:

# Search the entire test file for CSV-related tests
rg "CSV\|csv" backend/api/routes/__test__/completions.test.ts -A10

Repository: rage/mooc.fi

Length of output: 38


🏁 Script executed:

# Check if there's a global test setup file that sets JWT_SECRET
find backend -name "setup*" -o -name "config*" | grep -i test

Repository: rage/mooc.fi

Length of output: 38


🏁 Script executed:

# Look for where getTestContext sets up environment
rg "getTestContext" backend/tests/ -A20 | head -50

Repository: rage/mooc.fi

Length of output: 1012


🏁 Script executed:

# Check if JWT_SECRET is actually set in test environment
rg "process.env.JWT_SECRET\|JWT_SECRET =" backend/tests/ -B5 -A5

Repository: rage/mooc.fi

Length of output: 38


🏁 Script executed:

# Look for where process.env variables are mocked in tests
rg "process.env\." backend/tests/index.ts | head -20

Repository: rage/mooc.fi

Length of output: 38


🏁 Script executed:

# Check if there's a before hook that sets environment variables
cat backend/tests/index.ts | head -80

Repository: rage/mooc.fi

Length of output: 1772


🏁 Script executed:

# Check if JWT_SECRET is set anywhere in the actual code startup
rg "JWT_SECRET" backend/ --type ts | grep -v test | grep -v node_modules

Repository: rage/mooc.fi

Length of output: 483


🏁 Script executed:

# Look at the server startup to see if JWT_SECRET is required
rg "process.env" backend/server.ts -A2 -B2 | head -40

Repository: rage/mooc.fi

Length of output: 38


🏁 Script executed:

# Check if there's a date parsing issue - new Date() doesn't throw on invalid dates
node -e "console.log(new Date('invalid')); console.log(new Date('invalid') instanceof Date)"

Repository: rage/mooc.fi

Length of output: 73


🏁 Script executed:

# Look for stringify import to see CSV approach
rg "stringify" backend/api/routes/completions.ts -B5 -A1

Repository: rage/mooc.fi

Length of output: 519


Ensure JWT_SECRET is documented in .env.example and set in all environments.

The startup check is good, but JWT_SECRET is missing from .env.example, making it easy to miss during deployment setup.

Additionally, the JWT token implementation needs security improvements:

  • Pass { algorithms: ['HS256'] } to jwt.verify() to prevent algorithm confusion attacks
  • Include standard claims (iss, aud) in the signed payload and validate them on verify
  • Date validation is broken: new Date(fromDate) silently returns an Invalid Date object instead of throwing, bypassing the try-catch. Validate with isNaN(date.getTime()) before querying.
  • CSV generation loads all completions into memory via stringify(). Consider streaming with a transform pipeline (as used elsewhere in this file) for large datasets.
🤖 Prompt for AI Agents
In `@backend/api/routes/completions.ts` around lines 26 - 36, Add JWT_SECRET to
.env.example and ensure it's set in deployment configs; keep the existing
startup check using JWT_SECRET. When verifying tokens in the code that uses
jwt.verify(), pass the allowed algorithms (e.g., { algorithms: ['HS256'] }) and
include/validate standard claims (iss and aud) when signing and verifying the
token payload (refer to DownloadTokenPayload and the code paths that call
jwt.sign/jwt.verify). Replace the current naive fromDate handling by parsing and
validating the date with date.getTime() / isNaN(date.getTime()) (refer to the
variable fromDate and its parsing), rejecting invalid dates instead of relying
on try/catch. Finally, avoid loading all completions into memory via
stringify(); switch the CSV export to a streaming pipeline (the same
transform/stream approach used elsewhere in this file) to handle large datasets.


interface RegisterCompletionInput {
completion_id: string
student_number: string
Expand Down Expand Up @@ -96,6 +113,174 @@ export class CompletionController extends Controller {
return // NOSONAR
}

completionsCSVToken = async (
req: Request<{ courseId: string }>,
res: Response,
) => {
const { courseId } = req.params
const { fromDate, format } = req.query

const authRes = await requireAdminOrCourseOwner(courseId, this.ctx)(
req,
res,
)

if (authRes.isErr()) {
return authRes.error
}

const course = await this.ctx.prisma.course.findUnique({
where: { id: courseId },
})

if (!course) {
return res.status(404).json({ message: "Course not found" })
}

// Generate a signed JWT token valid for 30 seconds
const payload: DownloadTokenPayload = {
courseId,
fromDate: typeof fromDate === "string" ? fromDate : undefined,
format: format === "excel" ? "excel" : "csv",
}

const token = jwt.sign(payload, JWT_SECRET, {
expiresIn: "30s",
})

return res.status(200).json({ token })
}

completionsCSV = async (
req: Request<{ courseId: string }>,
res: Response,
) => {
const { courseId } = req.params
const { token } = req.query
const { knex } = this.ctx

// Validate token
if (!token || typeof token !== "string") {
return res.status(401).json({ message: "Invalid or missing token" })
}

let tokenData: DownloadTokenPayload
try {
tokenData = jwt.verify(token, JWT_SECRET) as DownloadTokenPayload
} catch (error) {
if (error instanceof jwt.TokenExpiredError) {
return res.status(401).json({ message: "Token expired" })
}
return res.status(401).json({ message: "Invalid token" })
}

if (tokenData.courseId !== courseId) {
return res
.status(403)
.json({ message: "Token not valid for this course" })
}

const fromDate = tokenData.fromDate
const format = tokenData.format ?? "csv"

const course = await this.ctx.prisma.course.findUnique({
where: { id: courseId },
})

if (!course) {
return res.status(404).json({ message: "Course not found" })
}

let query = knex
.select<any, any[]>(
"u.id",
"com.email",
"u.first_name",
"u.last_name",
"com.completion_date",
"com.completion_language",
"com.grade",
)
.from("completion as com")
.join("course as c", "com.course_id", "c.id")
.join("user as u", "com.user_id", "u.id")
.where("c.id", course.completions_handled_by_id ?? course.id)
.distinct("u.id", "com.course_id")
.orderBy("com.completion_date", "asc")
.orderBy("u.last_name", "asc")
.orderBy("u.first_name", "asc")
.orderBy("u.id", "asc")

if (fromDate && typeof fromDate === "string") {
try {
const date = new Date(fromDate)
query = query.where("com.completion_date", ">=", date)
} catch (e) {
return res.status(400).json({ message: "Invalid date format" })
}
}

const completions = await query

const headers = [
"User ID",
"Email",
"First Name",
"Last Name",
"Completion Date",
"Completion Language",
"Grade",
]

const rows = completions.map((row) => [
row.id,
row.email,
row.first_name,
row.last_name,
row.completion_date,
row.completion_language,
row.grade,
])

if (format === "excel") {
// Generate Excel file
const worksheet = XLSX.utils.aoa_to_sheet([headers, ...rows])
const workbook = XLSX.utils.book_new()
XLSX.utils.book_append_sheet(workbook, worksheet, "Completions")

const excelBuffer = XLSX.write(workbook, {
type: "buffer",
bookType: "xlsx",
})

res.setHeader(
"Content-Type",
"application/vnd.openxmlformats-officedocument.spreadsheetml.sheet",
)
res.setHeader(
"Content-Disposition",
`attachment; filename="completions_${
fromDate ? fromDate.toString().split("T")[0] : "all"
}.xlsx"`,
)

return res.status(200).send(excelBuffer)
}

// Default CSV format
const csvContent = stringify([headers, ...rows])

res.setHeader("Content-Type", "text/csv")
res.setHeader(
"Content-Disposition",
`attachment; filename="completions_${
fromDate ? fromDate.toString().split("T")[0] : "all"
}.csv"`,
)

return res.status(200).send(csvContent)
}

completionInstructions = async (
req: Request<{ slug: string; language: string }>,
res: Response,
Expand Down
34 changes: 34 additions & 0 deletions backend/api/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,40 @@ export function requireAdmin(ctx: BaseContext) {
}
}

export function requireAdminOrCourseOwner(courseId: string, ctx: BaseContext) {
return async function (
req: Request,
res: Response,
): Promise<Result<boolean, Response>> {
const getUserResult = await getUser(ctx)(req, res)

if (getUserResult.isErr()) {
return err(getUserResult.error)
}

const { user, details } = getUserResult.value

// Allow if user is admin
if (details.administrator) {
return ok(true)
}

// Check if user has course ownership for this course
const ownership = await ctx.knex
.select("id")
.from("course_ownership")
.where("user_id", user.id)
.andWhere("course_id", courseId)
.first()

if (ownership) {
return ok(true)
}

return err(res.status(401).json({ message: "unauthorized" }))
}
}

export function getUser({ knex, logger }: BaseContext) {
return async function (
req: Request,
Expand Down
Loading
Loading