Feature ETP-4576: Re-land the cookie session behind a credential preference - #133
Draft
RomanMagnoli wants to merge 25 commits into
Draft
Feature ETP-4576: Re-land the cookie session behind a credential preference#133RomanMagnoli wants to merge 25 commits into
RomanMagnoli wants to merge 25 commits into
Conversation
…point Bearer and the cookie session have to coexist while the migration lands, and the switch between them has to be a database change rather than a redeploy — that is what makes it safe to test CSRF and back out of it if something is unfinished. sessionCredentials.js is that switch. It publishes the active scheme plus both credentials, and exposes the two header builders every request goes through: jsonHeaders() for safe methods, writeHeaders() for unsafe ones. Bearer is the default, so an instance that has not opted in behaves exactly as it does today and an unreachable control plane degrades to the working scheme instead of breaking every request. It keeps module state rather than taking parameters on purpose: threading the mode and both credentials to the fetch would touch ~170 call sites across the core and the host, whereas reading it here means a call site only has to stop hand-building headers — it never learns which scheme is active. AuthProvider is the only writer, publishing on each change so a token refresh, an environment switch or a preference flip all take effect without a reload. The host resolves the credentialMode prop from the backend preference and passes it down; sourcing it stays the host's job so the core is unaware of how the instance is configured. Tested by mutation, not by a green run: the suite catches an inverted default, a CSRF proof leaking onto reads, an unguarded token interpolating to 'Bearer undefined', a partial-update merge, and a stale bearer token surviving into cookie mode. 779 node tests pass and the host builds against this core.
…lders buildHeaders() returned only a content type and a locale — no credential at all. That was correct while the cookie session was the only scheme, and became a silent 401 the moment the bearer scheme could be selected again: every caller of it, in both the core and the host, sent an unauthenticated read. It now adds the locale on top of jsonHeaders(), so the active scheme decides. Adds buildWriteHeaders() as its write-path pair. Its absence is why several host sites hand-appended the proof, and why two DELETEs used the read builder and got no proof at all. Never buildHeaders() on an unsafe method: under the cookie session that omits the CSRF proof and the backend answers 403. createApiFetch needs no change — it already builds from buildHeaders() and adds the proof per method safety, so fixing buildHeaders makes it dual-mode too. api.test.js: the Content-Type assertion is replaced by a delegation assertion. Pinning the literal headers here is what let buildHeaders drift into ignoring the scheme in the first place; asserting that it delegates is what keeps the preference able to switch anything. New credentialPublishing.test.jsx covers the one link that exists only in production: AuthProvider publishing to sessionCredentials. Every host suite replaces the provider with a mock that publishes on its behalf, so the host proves the builders honour what is published and nothing proved the provider publishes the right thing. Notable: the bearer token arrives through login(), never through restoreSession — mapRestoredSession deliberately drops it — which is why a backend-only bridge between the two schemes cannot work.
…anch Brings in 105 commits of epic work. The four conflicts were all the same fight: this branch carries the ETP-4576 cookie session (merged as PR #101) and the epic carries its revert (PR #111, commit 9ce61ac), so every auth file that #101 touched came back as "they deleted, we modified". All four are resolved in the EPIC's favour on purpose — the revert applies cleanly here and the cookie session is restored as an explicit, reviewable commit on top, rather than by silently declining a revert inside a merge resolution. That ordering matters for a second reason: the revert spans 41 files and −3941 lines, and taking "ours" wholesale would have discarded four unrelated epic commits that landed in the same files (ETP-4749, ETP-4665, ETP-4664, ETP-4663, all onboarding). Intermediate state, on purpose: sessionCredentials.js and its two test files survive the merge as new files but the wiring they need is gone until the next two commits land.
Reverts 9ce61ac ("Revert ETP-4576"), restoring across 41 files the backend-managed session the epic rolled back: fetchCookieSession / deleteCookieSession, the session-restore path in AuthContext, the CSRF proof in createApiFetch, and the onboarding flow's cookie handoff. PR #101 cannot be reopened — GitHub will not reopen a merged PR — so this is the mechanical inverse of its revert, kept as one commit so a reviewer can read it as exactly that and nothing else. Applied with no conflicts, and the four unrelated epic commits that had landed in these same files were verified to survive rather than assumed to: distinctive added lines from each of ETP-4749 (onboarding Tax ID), ETP-4665 (field length caps), ETP-4664 (email validation and error i18n) and ETP-4663 (language selector on the Register step) are all still present. This restores the cookie session AS #101 SHIPPED IT, which means it is once again the default: `restoreSession` defaults to fetchCookieSession, so any host gets the cookie session on mount whether or not it opted in. That is what broke the app a week ago. The next commit makes it opt-in — the two are split so this one stays a pure, verifiable inverse.
…ial scheme The previous commit restored ETP-4576 exactly as PR #101 shipped it, which means `restoreSession` defaulted to the platform cookie fetcher unconditionally: every host requested a server session on mount whether or not it had migrated. That is what broke the app when #101 landed without its backend, and it contradicts the bearer default the credential preference is built on. The default is now DERIVED from `credentialMode` — cookie selects the fetcher, bearer selects none — so one switch governs the scheme and the restore together and a host cannot end up half-migrated: asking for a cookie session on mount while its requests still authenticate with a bearer token, or the reverse. That default existed for a real reason, which this keeps satisfied. #101's own comment records it: tools/etendo-go-ar/app-shell mounts <AuthProvider> with no props, and once onboarding stopped writing the sf_auth_* handoff keys that host had nothing left to read — isAuthenticated false forever. It is the onboarding flow itself, where the cookie is issued, so it now opts in explicitly rather than relying on a default that also spoke for everyone else. AppShellProviders forwards `credentialMode`, which it did not: without it the functional host could select the scheme for its own requests but not for the provider that publishes them. Also re-applies the shared-builder work (sessionCredentials, buildWriteHeaders, buildHeaders delegating) that the merge resolution had dropped in favour of the epic — see the merge commit's note. Tests: AuthContext's restore describe now declares the cookie scheme instead of assuming it; what it verifies is unchanged. credentialPublishing gains four cases pinning the derived default on the network, in both directions — mutation-checked, since restoring the unconditional default kills three of them and disabling the restore entirely kills one. Suite: 812/812 node:test, and vitest clean except four UI files that fail identically on the epic (three tests whose component does not exist there either, plus add-line-button) — verified identical, not assumed.
… schemes `purgeLegacyAuthStorage()` only ever ran inside the session-restore effect, so it was reachable only when a restore was scheduled. Making the restore opt-in per credential scheme (previous commit) therefore left a real hole: under the bearer scheme nothing purges, and `authStorage.clear()` covers only the storage the host injected — which under the default (memory) is not where the sf_auth_* / sf_platform_* keys live. A stale credential left by an earlier version, or by the onboarding app, survived a logout untouched. Fixed in clearLocalSession rather than logout, so the fail-closed restore path gets it too: when the server says there is no session, whatever the client still holds is stale by definition. Found by the host, not here: eight logout specs in runtime-routes-integration went red as soon as the restore stopped being unconditional. Pinned here now, where the behaviour lives, for both schemes. Mutation-checked, and the first version of the test did NOT survive that check: it seeded the keys before mounting, so the mount purge cleared them and the test passed with no logout purge at all. Seeding after mount is what makes it discriminating — and is the realistic case anyway (another tab writing a key into a session that is already running).
… change The domain boundary check blocks this branch with two findings, and both are correct: app-shell-core changes are mixed with consumer wiring (APP_SHELL_CORE_MIXED_SCOPE), and 16 changed files under packages/etendo-go-core/ and tools/etendo-go-ar/ are unclassified by the policy (UNKNOWN_WITH_FEATURE). Clearing them needs a cross-domain plan, which `hasCrossDomainPlan` accepts either as docs/plans/<ticket>-cross-domain.md or as four keywords in the PR body. Written as the file rather than as a PR body, because the reasoning outlives the PR: this change crosses three domains on purpose — the thing being introduced IS a boundary-crossing decision (which credential authenticates a request), and it only works if exactly one place decides it. The plan records what each domain gets, the test coverage per layer, and the rollback story, which is the strongest part of the design: the scheme comes from a backend preference, so rolling back is a database change rather than a redeploy. It also states plainly that the preference must NOT be turned on yet — roughly 92 of 133 unsafe request sites in the host still carry no CSRF proof and would answer 403 — and which single commit to revert if the whole approach has to come out. Notes the `unknown` scope for what it is: packages/etendo-go-core and tools/etendo-go-ar are real owned directories that the classifier policy has no rule for yet. A follow-up in the policy, not a reason to split this change.
RomanMagnoli
added
the
cross-domain-approved
…icitly
CI's "Etendo Go core package tests" caught what my local verification missed: I ran
app-shell-core and schema-forge-core but never packages/etendo-go-core, and `make
test` does not include it either — CI checks it as a separate job. The
revert-of-the-revert touches that package's onboarding heavily, so it was exactly
the wrong one to skip.
The failing assertion was `<AuthProvider>` "mounted prop-free", which the opt-in
commit deliberately overturned: the AR onboarding host now passes
`credentialMode={CREDENTIAL_MODES.cookie}`.
Half the test is inverted and half is kept, and the comment says which is which.
Kept: no `restoreSession` prop — moving the fetcher into app-shell-core was meant
to stop every host wiring its own, and that still holds. Inverted: the provider is
no longer prop-free, because relying on an unconditional default repaired this host
without migrating it but forced the cookie scheme on every other host too, which is
why ETP-4576 was reverted. Also pins that CREDENTIAL_MODES comes from the platform
rather than being a local string literal, so a typo cannot silently degrade this
host to bearer and leave it anonymous forever.
All workspaces green: app-shell-core 812, etendo-go-core 251, schema-forge-core 38,
apps-sdk 4, apps-sdk-bff 12, agent-context 3, stack 4, quick-order-app 14.
Seven epic commits, two conflicts, and unlike the mechanical ones this branch has
been collecting these carry a real contract decision.
ETP-4905 added extension points to the onboarding steps — `onAuthenticated`,
`onRegistered`, `registerHandler`, and a `{ route }` option — so a host can embed
the flow and take over routing. Nothing in either repo supplies them yet; they are
API surface for a future consumer. All kept.
What they conflicted with is the field name. The epic passes `data.token`, this
branch passes `data.csrfToken`, and only one can be right: the login response for
POST /sws/go/session carries `{ status, account, csrfToken }` because the session
itself is the `__Host-` cookie the browser cannot read. There is no bearer token to
hand anyone. So the new callbacks receive the CSRF proof as their first argument,
matching handleAuthSuccess's own parameter — which the merge had already resolved
to `csrfToken` on its own, since ETP-4905 only touched its `route` option.
Worth flagging for whoever wires those callbacks: a host-supplied `registerHandler`
must return `{ csrfToken, account }`, not `{ token }`, or the `if (data.csrfToken)`
gate silently treats a successful registration as a failure.
Two source-reading tests updated, and only for arity: they pinned
`handleAuthSuccess(data.csrfToken, data.account)` with the closing paren as the
anchor, which the third options argument breaks. Their real intent — csrfToken and
never token — is unchanged and still asserted, including the `doesNotMatch` on
`if (data.token)`.
Suites: app-shell-core 824, etendo-go-core 251, schema-forge-core 38, all green.
Vitest still red on the same four files that fail identically on the epic (three
whose component does not exist there either, plus add-line-button).
The `./auth` barrel re-exports AuthContext.jsx, so every module that only wanted a header builder pulled React and a `.jsx` file into its graph. Vite does not care; plain Node does — `node --test` has no JSX loader, so a host unit test whose subject transitively imports `jsonHeaders`/`writeHeaders` died with ERR_UNKNOWN_FILE_EXTENSION before running a single assertion. Two host test files went dark this way as soon as a util started asking for headers, and the same would happen to every remaining call site migrated off hand-built headers. `sessionCredentials.js` already had no imports, so exposing it directly costs nothing and lets callers depend on the leaf instead of the barrel: asking "what credential does a request carry?" should not require a provider. Guarded on both sides, since the fix only holds while the module stays a leaf and a single added import would break the host's suite far from the cause: public-api.test.js now asserts the subpath is published AND that the module declares no imports and no re-exports. Verified by mutation — an added import, an added re-export, and a removed exports entry each fail it.
`handleRegister` gated success on `data.csrfToken` alone. That is right for `/sws/go/session/register`, which sets the `__Host-` cookie, but a host may supply its own `registerHandler` fronting an endpoint that never joined the session family. Etendo GO's `/company-invitations/register-and-accept` (ETP-4894) is exactly that: it still answers with a bearer `token` and issues no cookie, so its success path was unreachable — the account WAS created and the invitation accepted server-side while the UI reported a registration failure. Accepting either kind is the dual-scheme promise applied to this step. What it deliberately does NOT do is relabel one as the other: a bearer token belongs in `Authorization` and a proof in `X-Go-CSRF`, so the value is passed through unchanged to both `handleAuthSuccess` and the host's `onRegistered`, and the caller owns its interpretation. `csrfToken` wins when both are present. The SSO branch is left cookie-only on purpose — it posts to `/sws/go/session/sso/*`, which always issues the cookie, and there is no host hook there to front a legacy endpoint. The contract test asserted the old gate, so it could not have caught this; it now asserts the pass-through as well. Verified by mutation: reverting to the cookie-only gate and relabelling `credential` as `data.token` each fail it. app-shell-core 825, etendo-go-core 251, schema-forge-core 38 — all green.
…entication)
One conflict. The epic's merge block routed both LoginStep paths through a new
`completeAuthentication({ token, account, authMethod, persistAuth,
onAuthenticated })` helper, which centralises the persist + hand-off pair our
branch had inlined twice. Their refactor wins — it removes the duplication —
and the credential we feed it is ours.
Their helper passes `token` straight to `persistAuth` and then to
`onAuthenticated`; it touches no storage of its own, so it does not reintroduce
the `sf_platform_token` write this task deleted (that lives in
`handleAuthSuccess`, already migrated, and it ignores the `authMethod` option
the helper forwards).
`token` there is the generic credential slot, and what travels in it on these
paths is the CSRF proof: both endpoints they call (`/sws/go/session` and
`/sws/go/session/sso/*`) belong to the session family, so the session is the
`__Host-` cookie the page cannot read. Unlike password REGISTRATION, this path
needs no either-kind fallback — there is no host hook here that could front a
legacy bearer endpoint.
The two contract assertions asserted the pre-refactor call shape and are
rewritten against the helper, keeping the invariant that matters: the proof goes
in, not `data.token`. Verified by mutation — swapping it fails both.
app-shell-core 825, etendo-go-core 258, schema-forge-core 38 — all green.
simSearch started with `if (!token || ...) return nulls` and hand-built its own Authorization header. Under a cookie session no caller holds a token, so the guard cancelled every similarity search and returned the same all-nulls array a genuine no-match produces: no request on the wire, nothing logged, and valid CSV rows surfacing as "needs review" downstream. The credential now comes from the active scheme via a new readCredentialHeaders() on the sessionCredentials leaf, which strips Content-Type — this is a bodyless GET and application/json is not CORS-safelisted, so declaring it would force a preflight on every call. The entity/items checks stay; only the token one is gone. Callers may still pass token; it is ignored. Guarded by behaviour, not source: the cookie case asserts a request IS issued.
Two assumptions written into ETP-4576's own comments were wrong, and the ETP-4958 continuation tests are what caught them. 1. "This path needs no either-kind fallback." Gating LoginStep on data.csrfToken alone turned a bearer login into "invalid credentials" — a login that fails with the right password. Which scheme is active comes from a backend preference, so the frontend can ship before it is switched on and both shapes are reachable by design. Both LoginStep branches and RegisterStep's SSO branch now resolve csrfToken ?? token; RegisterStep's SSO branch had the same defect and no test reached it. 2. "Stop persisting anything to localStorage." Too broad: it swept up sf_platform_auth_method, which is not a credential. UserAvatarButton reads it to hide change-password from SSO users, so with nothing written an SSO user is offered a password they never set. The method write is restored and the SSO registration path now marks itself; the credential stays unstored. The guard assertions are asymmetric on purpose now — absence for the token, presence for the method — and they strip comments first, since both key names appear in the prose above the code that writes them.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Copilot PR Review
Outcome: Request changes
Blocking findings must be resolved before merge.
Blocking findings
- Duplicated added block (
DUPLICATED_BLOCK)
The same normalized block of 7 added lines appears in multiple places. Extract shared logic or remove the copy-paste. packages/etendo-go-core/src/onboarding/steps/LoginStep.jsx:69-75packages/etendo-go-core/src/onboarding/steps/RegisterStep.jsx:46-52
Copilot flagged the block as duplicated, correctly: the setItem and the six comment lines explaining WHY that key survives were pasted into LoginStep and RegisterStep verbatim. postAuth.js already owns the shared post-authentication contract, so the write and its rationale live there now as persistAuthMethod, and each step keeps only the part it actually owns — calling it with the method it authenticated by. The guard test followed the code: it asserted the setItem literal, which a step no longer contains. It now asserts the delegation, plus that neither step writes the key directly, and the key name and write are covered behaviourally next to the other postAuth tests.
github-actions
Bot
dismissed
their stale review
August 24, 2026 15:02
Blocking findings resolved by the latest Copilot PR Review run.
Declaring the scheme by hand is a claim about the BACKEND that the frontend has no way to verify, and both hard-coded values were wrong in the same way, in opposite directions. Left at the `bearer` default while `restoreSession` was already wired for cookies, `mapRestoredSession` dropped the token, so writeHeaders() had no bearer to send AND skipped `X-Go-CSRF` (it only adds the proof under `cookie`). Pinning `cookie` broke the mirror image: a backend not yet redeployed still answers login with a bearer token and issues no CSRF, so again there was no proof to send. Both times reads kept working — the browser attaches whatever session cookie exists on its own — and only writes came back 403, which is why no unit test saw either one. `auto` reads the answer instead of being told it: a cookie session issues a CSRF token, a bearer backend does not, so holding one IS the scheme. resolveMode() collapses it to `bearer` or `cookie` before any header builder sees it, and the explicit modes stay available for pinning one in a test or rolling back without a redeploy. `auto` must also default `restoreSession` on, since the restore is how the scheme gets decided at all: its response either carries a CSRF token or fails, and a failed restore leaves none, resolving to bearer. A bearer instance therefore costs one 401 on boot and then behaves exactly as it did before.
…4576 The epic grew ETP-4798 (the confirm-your-email wall and its resend rate-limit) on onboarding, which this branch had already moved onto the cookie session. Every conflict was the same shape: the epic's side is still the bearer/localStorage world, so the feature was grafted onto the cookie skeleton rather than either side being taken whole. - api.js: kept both new endpoints. `verifyEmail` stays unauthenticated — the mailed token IS the credential and the link is opened without a session — so it takes neither the cookie nor a CSRF proof, sitting with requestPasswordReset/confirmPasswordReset. `resendVerifyEmail` IS authenticated (signed in, just not confirmed) and arrived passing the bearer to buildAuthHeaders, which under this scheme would have shipped it as `X-Go-CSRF` — a header the backend rejects — while sending no session at all; it now takes the CSRF proof and `credentials: 'include'`. - OnboardingFlow.jsx: the mount reads /session, not /me, because that is where the CSRF proof comes from. The wall's decision needs /me, so routeByEnvironments reads it — which satisfies the epic's "never twice" rule from the other side, and its handover parameter is kept for callers that already hold the payload. The ETP-4798 ordering is preserved verbatim: the confirmation settles before the bootstrap runs. - Tests: three source-reading guards anchored on the bearer shapes (`fetchAccount(fetch, apiBase, currentToken)`, `routeByEnvironments(currentToken, data)`, `Authorization: Bearer`). The guarantees still hold, so each was re-anchored rather than dropped — including the epic's own warning about slicing from the first `.catch(` in the mount, which now applies to verifyEmail's. etendo-go-core 292/292, app-shell-core 836/836, schema-forge-core 38/38, vitest 789/791 + 6/6. The two vitest failures and three erroring files are the epic's own, unchanged by this merge. `cli/test/report-qr.test.js` fails on both sides for an undeclared `qrcode` dependency.
…4576 No conflicts: the epic only advanced the report QR helpers (ETP-4912), which this branch does not touch. app-shell-core 836/836, etendo-go-core 292/292, schema-forge-core 38/38. `cli/test/report-qr.test.js` still fails on both sides — the epic's `report-html-helpers.js` imports `qrcode`, which no package.json in the repo declares. Unrelated to this branch and unchanged by this merge.
…th surface
develop rebuilt exactly what this branch had replaced: ETP-5022 introduced
apiFetch, resolveApiUrl and the ambient session, and reworked authHeaders around
Accept-Language. Both belong. develop supplies the structure, this branch
supplies where the credential comes from.
- api.js keeps develop's whole surface; authHeaders and buildHeaders stop
taking a token and read the active scheme instead. Neither loses
Accept-Language, which ETP-4685/ETP-5022 added because without it the
backend silently answers in the account's AD language.
- apiFetch picks its headers by METHOD, not by whether a body is present. A
bodyless DELETE is still unsafe and needs the write proof, and develop's
body check would have sent it without one — a 403 under the cookie scheme.
- simSearch takes both: the scheme's credential and the locale header.
- onboarding/api.js keeps its cookie POST to /session/environment rather than
develop's GET /login?userId=, and routes it through buildAuthHeaders so it
is localized like every other call.
- isTokenExpired is gone with the token it asked about, so the barrel no
longer re-exports it.
Both simSearch test suites are kept: the ETP-4576 guard proving a request is
still issued with no token, and develop's request/locale coverage. api.test's
Bearer assertion is restated as the dual-scheme contract it replaced.
3209 tests pass.
…elpers
ETP-5022 and this task solved the same problem one release apart: stop call
sites from hand-building a credential. 5022 centralised it in api.js with a
guardrail against the literal outside auth/api.js; this task moved the decision
one level down, into sessionCredentials, so a scheme preference can switch
bearer and cookie without touching a call site. The merge had them fighting
because the first pass kept only one half of each.
They compose once the two concerns are separated: WHERE the credential comes
from is sessionCredentials' call, and WHAT ELSE a request carries is api.js's.
- createApiFetch regains 5022's full contract — on401, baseUrl and credentials
overrides, resolveApiUrl, and not leaking its own options into fetch — and
keeps this task's CSRF proof on unsafe methods.
- Header choice is per METHOD and per BODY, which are independent: a bodyless
request declares no Content-Type (5022) and an unsafe one still carries the
proof (4576), so a bodyless DELETE needs both.
- useApiFetch keeps 5022's optional context and ambient fallback while handing
over csrfToken rather than a client-held credential.
- Two 4576 guards were restated, not dropped: the useAuth() one now reads the
optional context, and the DEFAULT_BASE_URL one follows 5022's lazy resolver,
which exists because reading window at import time made the module unloadable
under plain node --test.
892 tests, 884 passing — up from 873 when the merge landed. The 8 that remain
are one open question, in the commit that follows this one.
Declaring the ambient session now also publishes its credential to the one place
that decides, so ETP-5022's behaviour survives the scheme switch instead of
being reimplemented beside it.
What made this non-obvious: registerApiSession reads the token on EVERY request
so a re-login is picked up without re-registering. Storing a snapshot would have
frozen it at whatever was held when the session was declared, and the bug would
only show after a re-login — the worst kind. So `token` accepts a provider as
well as a value, and the scheme resolves it per request. The mode is left
untouched: under `cookie` that token is simply never consulted, which is what
makes the preference switchable at all.
The per-call `token` option survives the same way, through
credentialHeadersForToken: the literal stays inside the scheme, so a plain
module handed a token by its caller still authenticates with it under `bearer`
and rides the cookie under `cookie`.
resetApiSessionForTests now clears the published credential too. Without that,
one test's token authenticated the next test's supposedly anonymous request —
which is exactly what it caught.
Three of develop's assertions were restated rather than deleted:
- simSearch's `!token` short-circuit is gone on purpose. Under a cookie session
nobody holds a token, so it cancelled every search and returned the all-nulls
array a real no-match produces, with no request and nothing logged. A missing
entity or an empty item list still short-circuits — those are real.
- The two that asserted a caller-passed token reaches the wire now assert the
scheme decides. Their actual subject, Accept-Language, is untouched.
That simSearch suite had also been dead: joining both describes left it
unbalanced and node reported a SyntaxError for the file rather than a failure,
so its 21 tests never ran.
913/913 in app-shell-core, 3209 in the root suite.
…pass Three of the onboarding readers — fetchAccount, fetchEnvironments and fetchOnboardingDraft — were left calling buildAuthHeaders(csrfToken) with no such parameter in scope. They are GETs and never had one: a read needs no proof, so they take none and pass none. It threw a ReferenceError before the request went out, which is why the failures carried no assertion message — the tests never got as far as asserting anything. Their header assertions also move off deepEqual: ETP-5022 added Accept-Language to buildAuthHeaders so auth errors stop arriving in English under a Spanish UI, and an exact-shape comparison treats that as a defect. What ETP-4576 asserts is unchanged and now stated directly — never an Authorization header, X-Go-CSRF only when a proof is held. 292/292 here, and green across the other three suites: 913 in app-shell-core, 12 in apps-sdk-bff, 3209 at the root.
# Conflicts: # packages/app-shell-core/src/auth/__tests__/api.test.js # packages/app-shell-core/src/auth/api.js
createApiFetch puts whatever its second argument returns into X-Go-CSRF on unsafe methods. Both callers handed it a credential instead: apiFetch passed session.getToken and useApiFetch fell back to getAmbientToken when rendered outside a provider. That slot is the proof of intent, not the credential. Under the bearer scheme this shipped the token in a header that has no business carrying it — harmless on the wire, since the backend ignores the proof there, which is exactly why it went unnoticed. Under the cookie scheme it is a real failure: the value sent is not the proof the backend issued, so every unsafe request from a component rendered outside a provider comes back 403 — the scheme ETP-4576 exists to enable. Both now read getSessionCsrfToken() off the active scheme, which registerApiSession keeps current. A source-reading guard covers the wiring, since nothing on the wire complains under bearer and that is how this regressed unnoticed.
|
0 New Issues
0 Fixed Issues
0 Accepted Issues
No data about coverage
Contributor
|
📦 Preview packages published — dist-tag To exercise the published-package path (no make bump-core-version VERSION=0.3.43-preview.feature-ETP-4576.20260831193227.afa194fRe-posted on each push to this branch; supersedes older previews. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Re-lands the ETP-4576 cookie session in the core packages, this time behind a
runtime credential preference so it is safe to ship without its consumers being
migrated.
PR #101 landed this and was reverted the next day by PR #111, because it made the
cookie session unconditional:
AuthProvider'srestoreSessiondefaulted to theplatform cookie fetcher, so every host asked the server for a session on mount
whether or not it had opted in — while the backend (ETP-4575) had not landed. A
merged PR cannot be reopened, so this is a new PR on the same branch.
How it is structured
Three commits, deliberately separable:
Merge epic/ETP-3504 into the core cookie-session branchRe-apply the core cookie session reverted by PR #1119ce61ac65, 41 files, nothing else. If this approach ever has to come out again, this is the single commit to revert.Make the restored cookie session opt-in per credential schemerestoreSession's default is now derived fromcredentialMode, so one switch governs both the scheme and whether a session is restored. Default staysbearer, so shipping this is a no-op for anyone who has not opted in.Plus
Purge the legacy credential keys on logout, in both schemesand thecross-domain plan.
Roughly 92 of 133 unsafe request sites in the functional host still carry no
CSRF proof and would answer 403 under the cookie scheme. ETP-4576 migrated a slice
of the app, not the app. The host PR adds ratchets that record that debt so it can
only shrink; turning the preference on is a separate, later decision.
Cross-domain
Labelled
cross-domain-approved, with the plan indocs/plans/ETP-4576-cross-domain.md. The change spansapp-shell-core/auth,app-shell-core/runtime,etendo-go-core/onboardingandtools/etendo-go-ar/app-shell— not because a feature leaked across boundaries,but because the thing being introduced is a boundary-crossing decision (which
credential authenticates a request) and it only works if exactly one place decides
it. The plan has the per-domain breakdown, the test map and the rollback story.
The boundary checker reports 16 files as
unknownscope: those arepackages/etendo-go-core/**andtools/etendo-go-ar/**, real owned directoriesthat the classifier policy has no rule for yet. Worth a follow-up in the policy.
A real bug this surfaced
purgeLegacyAuthStorage()only ever ran inside the session-restore effect, somaking the restore opt-in left a hole: under the bearer scheme nothing purged, and
authStorage.clear()covers only the storage the host injected — which under thedefault (memory) is not where the
sf_auth_*/sf_platform_*keys live. A stalecredential survived a logout untouched. Found by the functional host (eight logout
specs went red the moment the restore stopped being unconditional), fixed here in
clearLocalSessionso the fail-closed restore path gets it too.Testing
sessionCredentials.test.js— the decision point, 14 cases, mutation-validated 5/5.credentialPublishing.test.jsx—AuthProviderpublishing tosessionCredentials:the one link that exists only in production, since every host suite replaces the
provider with a mock. Includes the derived default asserted on the network in
both directions, and the logout purge in both schemes. Also mutation-validated —
the first version of the purge test passed with the bug present, because it seeded
the keys before mounting and the mount purge cleared them.
AuthContext.test.jsx— the restore contract now declares the cookie schemeinstead of assuming it.
Suite: 812/812 node:test. Vitest is clean except four files that fail
identically on
epic/ETP-3504(three whose component does not exist there either,from the ETP-4135 ownership split, plus
add-line-button) — verified identical, notassumed.
Ships together with
com.etendoerp.go— ETP-4575, the backend session (PR #777)etendo_schema_forge— ETP-4576, the functional host (PR #1045)The host cannot build against a published core until this lands:
buildWriteHeadersdoes not exist in 0.3.34.