fix(auth): use Immutable Map as fallback in logout cookie deletion#10839
fix(auth): use Immutable Map as fallback in logout cookie deletion#10839yogeshwaran-c wants to merge 4 commits intoswagger-api:masterfrom
Conversation
When logging out, the auth wrap-action used a plain JS object {} as
the default value for authorized.get(), but then called .getIn() on
it which is an Immutable.js method not available on plain objects.
This caused a TypeError: o.getIn is not a function when the
authorized entry was not found.
Changed the fallback to Map() from Immutable.js so that getIn()
calls work correctly.
Fixes swagger-api#10761
|
@yogeshwaran-c I am wondering why the authorization entry is not found in the store in this case - if it is supposed to be found but isn't, the apikey won't be removed from the cookies, thus the problem will still be there. |
Adds unit tests that exercise the logout cookie cleanup path when the payload from the Authorize popup contains security scheme names that are absent from the authorized store. This is the scenario that originally triggered issue swagger-api#10761: `authorized.get(name)` returned the fallback, and the subsequent `.getIn(...)` call crashed. The new tests lock in the fallback behavior so the cookie cleanup stays resilient.
|
@lukaszzazulak Thanks for pushing on the root cause — you're right that if an apikey we expect to delete is missing from I dug into why let auths = definitions.map((val, key) => key).toArray()
authActions.logoutWithPersistOption(auths)And the Logout button visibility check ( The Given that, the The over-broad logout payload from Tests pass locally ( |
The Authorize popup's Logout button submits every security definition in the displayed group, even ones the user never authorized. Those entries are absent from the authorized store, so the fallback Map() default is exercised on purpose. Document this so future readers don't mistake the fallback for a safety net around an unknown bug.
|
@lukaszzazulak Good question — I dug into it. The entry genuinely isn't in the store for those names, and here's why. Why the authorization entry can be missing The logoutClick =(e) => {
let { authActions, definitions } = this.props
let auths = definitions.map((val, key) => key).toArray()
...
authActions.logoutWithPersistOption(auths)
}It iterates over every security definition in the displayed group, regardless of whether the user actually filled it in. So when a spec has more than one security scheme in the same group (or any unauthorized scheme is shown alongside an authorized apiKey), the payload includes names that were never authorized and therefore never written to Does the apiKey still get removed? Yes, and the new test locks this down. I reproduced the exact sequence the reporter hit in #10761 by passing
I added a test ( Strictly speaking, the root cause is on the UI side — Commits pushed:
|
|
@yogeshwaran-c it does not seem to be the case - in the logoutClick, the definitions map contains only specific security definition, not all of them, like this one here (converted using toJS() from Immutable Map): It looks like somehow after the .map and .toArray calls, the auths array contains two elements ["ApiKeyAuth", "ApiKeyAuth"] instead of one ["ApiKeyAuth"] - and it only happens on instance described here #10761, see also my comment #10761 (comment) it looks like there are two different implementations used of immutablejs, but not sure - take into account what I've written, but challenge my assumptions and come with your own analysis |
Immutable v4 changed `Map#toArray()` to emit `[key, value]` pairs rather than values-only. The existing `definitions.map((val, key) => key).toArray()` pattern therefore produced `[[name, schema]]` instead of `[name]` when a host bundle ships Immutable v4+ alongside swagger-ui (e.g. a Vite/Rolldown build in a user app). That malformed payload: - made the LOGOUT reducer a no-op for the real scheme, so apiKey cookies were never cleared, and - made `authorized.get(payload[0])` miss, triggering `.getIn` on the default fallback and crashing logout (swagger-api#10761). Use `keySeq().toArray()` instead, which returns the flat key array under both v3 and v4. The defensive `Map()` fallback in `wrap-actions.js` stays for any other upstream callers that pass an unexpected key shape.
|
@lukaszzazulak You were right, and I was wrong about where this breaks. I went back and traced the data flow and tried to reproduce the doubled array directly. Writing up what I found. You were right on two counts
What actually breaksI pulled the deployed bundle at That bundle ships Immutable 4.3.8, not the 3.8.3 that swagger-ui pins in // Immutable v3
Map({ ApiKeyAuth: schema }).map((v, k) => k).toArray()
// → ['ApiKeyAuth']
// Immutable v4
Map({ ApiKeyAuth: schema }).map((v, k) => k).toArray()
// → [['ApiKeyAuth', 'ApiKeyAuth']]
The consumer app (Vite + Rolldown at aish.li) ends up with Immutable 4 in its final chunk most likely because its own module graph resolves a newer Immutable and the swagger-ui chunk gets tree-shaken against it. Petstore doesn't hit this because the hosted bundle still has the pinned v3. The fixPushed 39bcab9 on top of the existing branch. Two changes:
Tests updated:
Let me know if you'd prefer the |
2 participants
Description
When logging out of an API key authorization, the auth
logoutwrap-action insrc/core/plugins/auth/wrap-actions.jsused a plain JavaScript object{}as the default value forauthorized.get(authorizedName, {}). However, subsequent code calls.getIn()on this fallback value, which is an Immutable.js method not available on plain objects.This caused the error:
The fix changes the fallback from
{}toMap()(from Immutable.js), ensuring that.getIn()calls work correctly even when the authorization entry is not found in the store.Motivation and Context
Fixes #10761
The crash occurs when clicking the Logout button for an ApiKeyAuth in the Available authorizations popup, specifically when the authorized entry cannot be found in the Immutable store.
How Has This Been Tested?
Map()from Immutable.js has the.getIn()methodMap().getIn(["schema", "type"])returnsundefinedrather than throwing, which correctly prevents the cookie deletion logic from executingauthorizeaction in the same file already usesfromJS()to convert to Immutable, confirming that Immutable types are the expected data typeScreenshots (if appropriate):
N/A
Checklist
My PR contains...
src/is unmodified: changes to documentation, CI, metadata, etc.)package.json)My changes...
Documentation
Automated tests