Skip to content

Fix: Unauthenticated access to customer order confirmation PDFs via /ftp - #365

Closed
WesternConcrete wants to merge 4 commits into
developfrom
devin/1789370006-order-pdf-owner-check-develop
Closed

WesternConcrete wants to merge 4 commits into
developfrom
devin/1789370006-order-pdf-owner-check-develop

Conversation

@WesternConcrete

@WesternConcrete WesternConcrete commented Sep 14, 2026

Copy link
Copy Markdown

Description

Finding: Unauthenticated access to customer order confirmation PDFs via /ftp
Repo: COG-GTM/juice-shop
Fix: servePublicFiles() in routes/fileServer.ts now requires a signature-verified JWT (tried from the token cookie — parsed from the raw Cookie header, since cookieParser is mounted after the /ftp routes — then from the Bearer header) and only serves order_<orderId>.pdf when the order stored in ordersCollection for that orderId belongs to the JWT's email; anonymous requests get 401, other customers / unknown orders get 403.

placeOrder() writes ftp/order_<md5(email)[0:4]>-<random>.pdf (customer email, delivery address, line items) and /ftp/:file allowlisted every *.pdf with no auth, so any leaked/shared order link was readable by anyone.

Ownership = stored order's vowel-masked email matches the JWT email and the orderId carries the md5(email)[0:4]- prefix placeOrder derives from the real email (the masked email alone is not unique; the prefix alone is 16 bits). The check runs on the null-byte-stripped filename so the existing %00 bypass cannot skip it. A stale/malformed cookie does not mask a valid Bearer token and never throws. cleanupFtpFolder removes order_*.pdf on startup, so PDFs and the in-memory order records share a lifetime. Existing /ftp challenge behaviour for non-order files is unchanged.

Tests: test/server/fileServerSpec.ts (anonymous → 401, other customer → 403, unknown order → 403, masked-email match with wrong prefix → 403, owner → served, invalid cookie + valid Bearer → served, malformed cookie → 401) and test/api/ftp-folder.test.ts through the real Express stack (anonymous → 401, valid cookie + unknown order → 403). mocha, API tests, eslint, tsc --noEmit and npm run rsn pass.

Supersedes #364 (closed by the contribution bot for targeting master instead of develop).

Resolved or fixed issue: none

AI Tool Disclosure

  • My contribution does not include any AI-generated content
  • My contribution includes AI-generated content, as disclosed below:
    • AI Tools: Devin
    • LLMs and versions: Devin (Cognition)
    • Prompts: Daily security sweep: substantiate and fix new vulnerabilities in COG-GTM/juice-shop

Affirmation

Link to Devin session: https://app.devin.ai/sessions/a749d64c0c0e49149bdaff3d2605b5c2
Open in Devin Desktop: https://app.devin.ai/desktop/session/a749d64c0c0e49149bdaff3d2605b5c2?variant=devin
Requested by: @WesternConcrete


Devin Review

Signed-off-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Wes Convery <2wconvery@gmail.com>
@devin-ai-integration

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration Bot and others added 2 commits September 14, 2026 07:20
Signed-off-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Wes Convery <2wconvery@gmail.com>
Signed-off-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Wes Convery <2wconvery@gmail.com>
devin-ai-integration[bot]

This comment was marked as resolved.

Signed-off-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Wes Convery <2wconvery@gmail.com>
Comment thread routes/fileServer.ts
const orderOwnerEmail = (req: Request): string | undefined => {
for (const token of [cookieToken(req), utils.jwtFrom(req)]) {
try {
if (token && security.verify(token)) {
@devin-ai-integration

Copy link
Copy Markdown

CI note: the red CodeQL check is "4 new alerts in code changed by this PR", all in the new authorization block of routes/fileServer.ts (plus one re-anchored on lib/insecurity.ts:57):

  • js/user-controlled-bypass — the JWT/order-ownership condition is, by definition, controlled by user-provided input (the token and the requested filename); the token is signature-verified with security.verify and the order is matched against server-side data, so this is the intended check, not a bypass.
  • js/path-injection at the existing verify(file, res, next) call — this is the pre-existing /ftp/:file behaviour (intentional directory-traversal/null-byte challenges); it only shows as "new" because the surrounding function was rewritten.

I don't have permission to dismiss code-scanning alerts on this repo; a maintainer can dismiss them as "used in tests / won't fix". The other failing checks (check-compliance slack_webhook_url, custom-config-test mocha not found, docker-test Angular smoke, api-test StackOverflow 403, e2e redirect React juice-shop#418) fail identically on #363/#366 against develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants