Repository navigation
COG-1153: AppointmentResource.confirm → POST (server + AdminPortal caller) - #336
Conversation
…tal caller Co-Authored-By: patrick.bradley <patrick.bradley@cognition.ai>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
| if (xsrfToken) { | ||
| headers.append('X-XSRF-TOKEN', xsrfToken); |
There was a problem hiding this comment.
🟡 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.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
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.
Summary
Linear: COG-1153 — W1 · AppointmentResource.confirm → POST (server + Angular caller)
AppointmentResource.confirmwas mapped with@RequestMapping("/{id}/confirm")(no method), so it answered GET.CsrfFilteronly 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.masteryet, so a small privatereadCookielives inAppointmentService; switch to the shared helper when the AdminPortal CSRF ticket lands.RequestFilter's preflightAccess-Control-Allow-Headersmust includex-xsrf-token— that belongs to the CORS/CSRF tickets, not this one.AppointmentResourceTest(standalone MockMvc + Mockito, no DB): POST → 200 and callsconfirmAppointment(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 assertsget(".../confirm")→ 200. That test will fail once this PR lands. It has to be flipped topost(...)(and renamed, e.g.confirmsAppointmentOverPost) in whichever PR merges second. I didn't edit it here because the file doesn't exist onmaster, 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):AdminPortal (Node 12):
ng buildsucceeds.ng lintshows 6 errors that also exist onmaster, in other files (navbar, app.module, specs), and none inappointment.service.ts.Running app (
mvn spring-boot:run: signed upadmin, gave it ROLE_ADMIN, logged in throughPOST /index, created an appointment throughPOST /appointment/create):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