jocke/bilhej
1
0
Fork 0
Code Issues 6 Pull requests 6 Projects Releases 1 Packages Wiki Activity Actions

fix(security): gate guest self-confirmed orders behind PENDING_REVIEW (#18) #30

Open
hermes wants to merge 1 commit from fix/guest-payment-pending-review into master
pull from: fix/guest-payment-pending-review
merge into: jocke:master
Conversation 0 Commits 1 Files changed 7 +45 -6
hermes commented 2026-07-18 11:49:54 +00:00
Collaborator
Copy link

Why

Issue #18: POST /api/guest-orders/{token}/pay is public (no JWT) and confirmGuestPayment simply transitions PENDING_PAYMENT -> PROCESSING without verifying any Swish payment was received. A guest can mark an order paid without paying, and BilHej eats the PostNord cost.

Approach

Interim guard (Tier 1 Swish Commerce API integration is a separate, larger effort). Self-confirmed guest orders now enter PENDING_REVIEW instead of PROCESSING, gating fulfillment on manual admin confirmation.

Changes

  • OrderStatus: add PENDING_REVIEW("pending_review")
  • OrderService.confirmGuestPayment: set PENDING_REVIEW instead of PROCESSING; do NOT call notifyOrderProcessing
  • AdminOrderStatusRules: allow PENDING_REVIEW -> PROCESSING, CANCELLED, FAILED; canRegisterShipment returns false
  • AdminOrderWorkflowService: when admin advances PENDING_REVIEW -> PROCESSING, call notifyOrderProcessing to trigger fulfillment
  • GuestOrderPage.vue: status label "Betalning mottagen, under granskning"
  • OrderServiceTest: assert PENDING_REVIEW + no notification after guest self-confirm
  • AdminOrderStatusRulesTest: 3 new tests for PENDING_REVIEW transitions

The authenticated confirmPayment path is unchanged (acknowledged Phase 0 honor-system with user traceability).

Test plan

  • ./gradlew :backend:test — BUILD SUCCESSFUL
  • npx vitest run src/__tests__/GuestOrderPage.spec.ts — 12/12 pass

Closes #18

## Why Issue #18: `POST /api/guest-orders/{token}/pay` is public (no JWT) and `confirmGuestPayment` simply transitions `PENDING_PAYMENT -> PROCESSING` without verifying any Swish payment was received. A guest can mark an order paid without paying, and BilHej eats the PostNord cost. ## Approach Interim guard (Tier 1 Swish Commerce API integration is a separate, larger effort). Self-confirmed guest orders now enter `PENDING_REVIEW` instead of `PROCESSING`, gating fulfillment on manual admin confirmation. ## Changes - **OrderStatus**: add `PENDING_REVIEW("pending_review")` - **OrderService.confirmGuestPayment**: set `PENDING_REVIEW` instead of `PROCESSING`; do NOT call `notifyOrderProcessing` - **AdminOrderStatusRules**: allow `PENDING_REVIEW -> PROCESSING, CANCELLED, FAILED`; `canRegisterShipment` returns false - **AdminOrderWorkflowService**: when admin advances `PENDING_REVIEW -> PROCESSING`, call `notifyOrderProcessing` to trigger fulfillment - **GuestOrderPage.vue**: status label "Betalning mottagen, under granskning" - **OrderServiceTest**: assert `PENDING_REVIEW` + no notification after guest self-confirm - **AdminOrderStatusRulesTest**: 3 new tests for PENDING_REVIEW transitions The authenticated `confirmPayment` path is unchanged (acknowledged Phase 0 honor-system with user traceability). ## Test plan - [x] `./gradlew :backend:test` — BUILD SUCCESSFUL - [x] `npx vitest run src/__tests__/GuestOrderPage.spec.ts` — 12/12 pass Closes #18
hermes added 1 commit 2026-07-18 11:49:55 +00:00
fix(security): gate guest self-confirmed orders behind PENDING_REVIEW
Some checks failed
CI / Lint, type check, unit tests, coverage (pull_request) Successful in 2m47s
Details
CI / E2E browser tests (pull_request) Failing after 1m44s
Details
5825723420 
POST /api/guest-orders/{token}/pay is public (no JWT) and
confirmGuestPayment simply transitions PENDING_PAYMENT -> PROCESSING
without verifying any Swish payment was received. A guest can mark an
order paid without paying, and BilHej eats the PostNord cost for a
free letter.

Interim fix (Tier 1 Swish Commerce API integration is a separate,
larger effort): self-confirmed guest orders now enter PENDING_REVIEW
instead of PROCESSING, gating fulfillment on manual admin
confirmation.

Changes:
  - OrderStatus: add PENDING_REVIEW("pending_review")
  - OrderService.confirmGuestPayment: set PENDING_REVIEW instead of
    PROCESSING; do NOT call notifyOrderProcessing (no fulfillment yet)
  - AdminOrderStatusRules: allow PENDING_REVIEW -> PROCESSING,
    CANCELLED, FAILED; canRegisterShipment returns false for
    PENDING_REVIEW
  - AdminOrderWorkflowService: when admin advances PENDING_REVIEW ->
    PROCESSING, call notifyOrderProcessing to trigger fulfillment
  - GuestOrderPage.vue: status label 'Betalning mottagen, under
    granskning' for pending_review
  - OrderServiceTest: assert PENDING_REVIEW status + no notification
    after guest self-confirm
  - AdminOrderStatusRulesTest: 3 new tests for PENDING_REVIEW
    transitions (allowed targets, valid PROCESSING transition,
    rejected SENT transition)

The authenticated confirmPayment path is unchanged (acknowledged
Phase 0 honor-system with user traceability).

Closes #18
hermes reviewed 2026-07-18 11:53:30 +00:00
hermes left a comment
Author
Collaborator
Copy link

Verdict: Approve with minor test-coverage suggestions.

Clean, minimal guard for #18. The status-machine change is correct: guest self-confirm lands in PENDING_REVIEW (no auto-fulfillment trigger), admin workflow only re-fires notifyOrderProcessing on the explicit PENDING_REVIEW -> PROCESSING advance, and the new AdminOrderStatusRules additions correctly seal off direct PENDING_REVIEW -> SENT and canRegisterShipment (so a self-confirmed order cannot slip straight to PostNord). Tests for OrderService and AdminOrderStatusRules cover the core mechanic; backward-compatible with existing PROCESSING orders.

Two test gaps and one readability nit below; none are blocking.

Critical

None.

Warnings

None. No security regressions; the autonomous notify was correctly removed from confirmGuestPayment.

Suggestions

  • Backend: cover the new notification trigger. AdminOrderWorkflowService.updateOrderStatus gains a PENDING_REVIEW -> PROCESSING -> notifyOrderProcessing branch, but AdminOrderWorkflowServiceTest was not updated. This notification is the load-bearing human-loop confirmation for the security fix — without a guard test, the branch can silently regress. Suggested test: shouldNotifyProcessingWhenAdminAdvancesFromPendingReview (mirror the existing notifyOrderFailed test).

  • Frontend: cover the new pending_review label. GuestOrderPage.spec.ts asserts labels for paid/sent/delivered/failed/cancelled but skips the new pending_review case (currently renders 'Betalning mottagen, under granskning'). Tests still pass due to the switch default, but the new label is uncovered. Add it('shows "Betalning mottagen, under granskning" for pending_review status').

  • Add a one-line comment above the new branch in AdminOrderWorkflowService.updateOrderStatus noting why this transition is special (confirmGuestPayment intentionally skips notify; this is the admin-vouched re-fire). Contrast with FAILED -> notifyOrderFailed above, which doesn't trigger notifyOrderProcessing on the FAILED -> PROCESSING revert.

Looks Good

  • OrderStatus enum insertion point between PAID and PROCESSING is harmless; every switch has a default.
  • confirmGuestPayment correctly drops the autonomous notifyOrderProcessing call; customer now sees 'under granskning' instead of 'Behandlas'.
  • AdminOrderStatusRules changes correctly seal off PENDING_REVIEW -> SENT and disable canRegisterShipment.
  • The previousStatus == PENDING_REVIEW guard is precise; it does not fire for FAILED -> PROCESSING recoveries (matching pre-existing behavior).
  • Javadoc on confirmGuestPayment clearly explains the rationale.
  • Frontend label matches the Swedish string convention used elsewhere in the file.
**Verdict: Approve with minor test-coverage suggestions.** Clean, minimal guard for #18. The status-machine change is correct: guest self-confirm lands in `PENDING_REVIEW` (no auto-fulfillment trigger), admin workflow only re-fires `notifyOrderProcessing` on the explicit `PENDING_REVIEW -> PROCESSING` advance, and the new `AdminOrderStatusRules` additions correctly seal off direct `PENDING_REVIEW -> SENT` and `canRegisterShipment` (so a self-confirmed order cannot slip straight to PostNord). Tests for `OrderService` and `AdminOrderStatusRules` cover the core mechanic; backward-compatible with existing `PROCESSING` orders. Two test gaps and one readability nit below; none are blocking. ### Critical None. ### Warnings None. No security regressions; the autonomous notify was correctly removed from `confirmGuestPayment`. ### Suggestions - **Backend: cover the new notification trigger.** `AdminOrderWorkflowService.updateOrderStatus` gains a `PENDING_REVIEW -> PROCESSING -> notifyOrderProcessing` branch, but `AdminOrderWorkflowServiceTest` was not updated. This notification is the load-bearing human-loop confirmation for the security fix — without a guard test, the branch can silently regress. Suggested test: `shouldNotifyProcessingWhenAdminAdvancesFromPendingReview` (mirror the existing `notifyOrderFailed` test). - **Frontend: cover the new `pending_review` label.** `GuestOrderPage.spec.ts` asserts labels for `paid`/`sent`/`delivered`/`failed`/`cancelled` but skips the new `pending_review` case (currently renders `'Betalning mottagen, under granskning'`). Tests still pass due to the switch `default`, but the new label is uncovered. Add `it('shows "Betalning mottagen, under granskning" for pending_review status')`. - **Add a one-line comment** above the new branch in `AdminOrderWorkflowService.updateOrderStatus` noting *why* this transition is special (confirmGuestPayment intentionally skips notify; this is the admin-vouched re-fire). Contrast with `FAILED -> notifyOrderFailed` above, which doesn't trigger notifyOrderProcessing on the `FAILED -> PROCESSING` revert. ### Looks Good - `OrderStatus` enum insertion point between `PAID` and `PROCESSING` is harmless; every switch has a `default`. - `confirmGuestPayment` correctly drops the autonomous `notifyOrderProcessing` call; customer now sees 'under granskning' instead of 'Behandlas'. - `AdminOrderStatusRules` changes correctly seal off `PENDING_REVIEW -> SENT` and disable `canRegisterShipment`. - The `previousStatus == PENDING_REVIEW` guard is precise; it does not fire for `FAILED -> PROCESSING` recoveries (matching pre-existing behavior). - Javadoc on `confirmGuestPayment` clearly explains the rationale. - Frontend label matches the Swedish string convention used elsewhere in the file.
backend/src/main/java/se/bilhalsning/service/AdminOrderWorkflowService.java
hermes commented 2026-07-18 11:53:30 +00:00
Author
Collaborator
Copy link

💡 Test gap + readability nit. This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by AdminOrderWorkflowServiceTest — the notifyOrderFailed branch above is tested, this one isn't. Consider adding shouldNotifyProcessingWhenAdminAdvancesFromPendingReview.
Also worth a one-line comment clarifying why PROCESSING from PENDING_REVIEW notifications where PROCESSING from FAILED does not (because confirmGuestPayment deliberately skipped the notify).

💡 **Test gap + readability nit.** This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by `AdminOrderWorkflowServiceTest` — the `notifyOrderFailed` branch above is tested, this one isn't. Consider adding `shouldNotifyProcessingWhenAdminAdvancesFromPendingReview`. Also worth a one-line comment clarifying *why* `PROCESSING` from `PENDING_REVIEW` notifications where `PROCESSING` from `FAILED` does not (because `confirmGuestPayment` deliberately skipped the notify).
frontend/src/pages/GuestOrderPage.vue
hermes commented 2026-07-18 11:53:30 +00:00
Author
Collaborator
Copy link

💡 Frontend test gap. GuestOrderPage.spec.ts asserts labels for paid/sent/delivered/failed/cancelled but not for the new pending_review case (renders 'Betalning mottagen, under granskning'). The switch default keeps existing tests green, but this new label is uncovered — worth adding it('shows "Betalning mottagen, under granskning" for pending_review status').

💡 **Frontend test gap.** `GuestOrderPage.spec.ts` asserts labels for `paid`/`sent`/`delivered`/`failed`/`cancelled` but not for the new `pending_review` case (renders `'Betalning mottagen, under granskning'`). The switch `default` keeps existing tests green, but this new label is uncovered — worth adding `it('shows "Betalning mottagen, under granskning" for pending_review status')`.
Some checks failed
CI / Lint, type check, unit tests, coverage (pull_request) Successful in 2m47s
Details
CI / E2E browser tests (pull_request) Failing after 1m44s
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/guest-payment-pending-review:fix/guest-payment-pending-review
git checkout fix/guest-payment-pending-review

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout master
git merge --no-ff fix/guest-payment-pending-review
git checkout fix/guest-payment-pending-review
git rebase master
git checkout master
git merge --ff-only fix/guest-payment-pending-review
git checkout fix/guest-payment-pending-review
git rebase master
git checkout master
git merge --no-ff fix/guest-payment-pending-review
git checkout master
git merge --squash fix/guest-payment-pending-review
git checkout master
git merge --ff-only fix/guest-payment-pending-review
git checkout master
git merge fix/guest-payment-pending-review
git push origin master
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: jocke/bilhej#30
No description provided.
Delete branch "fix/guest-payment-pending-review"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?