Skip to content

COG-1153: AppointmentResource.confirm → POST (server + AdminPortal caller) - #336

Open
patrickbradley-cog wants to merge 1 commit into
masterfrom
devin/cog-1153-w1-appointmentresourceconfirm-post-server-angular-caller
Open

patrickbradley-cog wants to merge 1 commit into
masterfrom
devin/cog-1153-w1-appointmentresourceconfirm-post-server-angular-caller

Conversation

@patrickbradley-cog

@patrickbradley-cog patrickbradley-cog commented Sep 30, 2026 •

Copy link
Copy Markdown

Summary

Linear: COG-1153 — W1 · AppointmentResource.confirm → POST (server + Angular caller)

AppointmentResource.confirm was mapped with @RequestMapping("/{id}/confirm") (no method), so it answered GET. CsrfFilter only checks non-safe methods, so once W2 turns CSRF on this mutation would stay unprotected (<img src=".../api/appointment/1/confirm"> confirms with the admin's session cookie). This converts it to POST now, while CSRF is still off, together with its single caller — behaviour-neutral on Boot 1.5.4, no Boot/Java bump.

- @RequestMapping("/{id}/confirm")
+ @PostMapping("/{id}/confirm")
  public void confirmAppointment(@PathVariable("id") Long id)
  // AdminPortal/src/app/appointment.service.ts
- return this.http.get(url, { withCredentials: true });
+ // X-XSRF-TOKEN added only if an XSRF-TOKEN cookie exists (none today, CSRF is off)
+ return this.http.post(url, null, { headers, withCredentials: true });
  • No shared AdminPortal CSRF helper exists on master yet, so a small private readCookie lives in AppointmentService; switch to the shared helper when the AdminPortal CSRF ticket lands.
  • Since the header is only sent when the cookie exists, today's request is still a CORS "simple" request (no preflight). Once CSRF is on, RequestFilter's preflight Access-Control-Allow-Headers must include x-xsrf-token — that belongs to the CORS/CSRF tickets, not this one.
  • Added AppointmentResourceTest (standalone MockMvc + Mockito, no DB): POST → 200 and calls confirmAppointment(42); GET → 405 and never calls the service.

Merge order / smoke suite

Merge behind the W0 smoke suite. The appointment smoke test lives in unmerged #108 (AppointmentSmokeTest.confirmsAppointmentOverGet) and asserts get(".../confirm") → 200. That test will fail once this PR lands. It has to be flipped to post(...) (and renamed, e.g. confirmsAppointmentOverPost) in whichever PR merges second. I didn't edit it here because the file doesn't exist on master, and adding it would overlap with #108.

Verification

Build/tests — Temurin 8u504, MySQL 5.7 in docker (SPRING_DATASOURCE_URL=...?useSSL=false; JDK 8u504 rejects the old connector's TLS):

$ mvn -B -f UserFront/pom.xml verify
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0 - in com.userFront.resource.AppointmentResourceTest
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0 - in com.userFront.UserFrontApplicationTests
Tests run: 3, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

AdminPortal (Node 12): ng build succeeds. ng lint shows 6 errors that also exist on master, in other files (navbar, app.module, specs), and none in appointment.service.ts.

Running app (mvn spring-boot:run: signed up admin, gave it ROLE_ADMIN, logged in through POST /index, created an appointment through POST /appointment/create):

--- GET /api/appointment/1/confirm
{"status":405,"error":"Method Not Allowed","message":"Request method 'GET' not supported",...}
HTTP 405
id  confirmed
1   0
--- POST /api/appointment/1/confirm
HTTP 200
id  confirmed
1   1
--- unauthenticated POST
HTTP 302 -> http://localhost:8080/index

Link to Devin session: https://app.devin.ai/sessions/7a967fb2d2c64eee91e222975dbc2ecb
Open in Devin Desktop: https://app.devin.ai/desktop/session/7a967fb2d2c64eee91e222975dbc2ecb?variant=devin
Requested by: @patrickbradley-cog


Devin Review

Status Commit
⚪ Not started —

Run Devin Review

Devin Review (Staging)


Devin Review

…tal caller

Co-Authored-By: patrick.bradley <patrick.bradley@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

Devin Review

Comment on lines +19 to +20
if (xsrfToken) {
headers.append('X-XSRF-TOKEN', xsrfToken);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 CSRF cookie blocks appointment confirmation

When an XSRF-TOKEN cookie exists, confirmAppointment triggers a preflight that rejects its X-XSRF-TOKEN header. RequestFilter omits that header from its preflight response, so the appointment remains unconfirmed.

Learn more

The admin portal calls the backend across ports, so the browser applies CORS. Without the cookie, the confirmation POST has no custom header. With the cookie, confirmAppointment adds X-XSRF-TOKEN, forcing an OPTIONS preflight. RequestFilter answers OPTIONS but omits that header from Access-Control-Allow-Headers. The browser blocks the POST before the controller can confirm the appointment. This applies whenever the portal can read an XSRF-TOKEN cookie, including after a CSRF cookie is introduced.

Example: With document.cookie containing XSRF-TOKEN=abc, clicking Confirm causes an OPTIONS request asking permission for x-xsrf-token. The filter excludes it, so the browser never sends the POST and the appointment stays unconfirmed.

Recommended fix: Add x-xsrf-token to the OPTIONS Access-Control-Allow-Headers list in RequestFilter when sending the header, and cover the cookie-present cross-origin flow in a browser-level test.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is correct, but it can't happen on master today. CSRF is disabled (SecurityConfig: .csrf().disable()), so nothing ever sets an XSRF-TOKEN cookie. That means the header is never added, and the confirm POST is still a simple CORS request with no preflight. The ticket requires the header now so it's ready when CSRF is turned on.

I'm keeping the fix out of this PR on purpose. RequestFilter belongs to COG-1165 (W2), which replaces it with a declarative CorsConfigurationSource. W2 is also when CSRF (and so the cookie) gets turned on, so X-XSRF-TOKEN needs to be in that config's allowed headers. The W1 tickets are meant to be file-disjoint, so I'm leaving RequestFilter alone here and flagging this to the coordinator for COG-1165.

@devin-ai-integration

Copy link
Copy Markdown

Browser test (AdminPortal on :4200 → UserFront on :8080, MySQL 5.7, admin user)

Passed: GET /api/appointment/2/confirm returned 405 with Allow: POST and left the row unconfirmed. One click on AdminPortal Confirm changed the row to true and hid the link, and the value stayed true after another reload. The browser HAR shows the UI sent POST /api/appointment/2/confirm with no X-XSRF-TOKEN header, which is expected because no cookie is set while CSRF is off.

Before After
before after

GET 405

The browser couldn't show the POST's status code. AppointmentComponent.confirmAppointment already calls location.reload() right after subscribe() (this race predates the PR), so Chrome records the POST as net::ERR_ABORTED. The request still reached the server: the DB row was updated. The 200 status itself is shown by the curl run in the PR description (POST → HTTP 200, confirmed 0 → 1).

Not tested: behaviour when an XSRF-TOKEN cookie is present. That depends on W2 (CSRF on, plus the CORS allow-list in COG-1165).

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.

1 participant