Added memory mode support - #243
DhananjayNazare wants to merge 4 commits into
Conversation
|
You will need to elaborate on why you consider local-/sessionstorage as a vulnerability, and how storing the token in memory remedies that. Also, please run the formatter included in the project via pre-commit. |
|
Also added few more inputs on in issue section as an justification for the change. Please let me know if u have any further review comments. |
sebastianvitterso
left a comment
sebastianvitterso
left a comment
There was a problem hiding this comment.
@soofstad will make the final review, to consider whether this is something we need/want to support.
Code-wise, this looks good and proper. Fits our code style well. The inMemoryStorage definition is simple and elegant. Nice to have tests properly covering it. All-in-all very good.
|
@soofstad coud you please review the PR? |
|
Stale pull request, will be closed in 7 days unless there is further activity |
There was a problem hiding this comment.
Pull request overview
Adds in-memory authentication storage as an alternative to local and session storage.
Changes:
- Adds memory storage support and configuration validation.
- Keeps PKCE redirect data in session storage.
- Adds documentation, tests, and Biome formatting tooling.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/inMemoryStorage.ts |
Implements module-level memory storage. |
src/useBrowserStorage.ts |
Integrates memory storage with the React hook. |
src/authentication.ts |
Selects session storage for memory-mode PKCE data. |
src/authConfig.ts |
Validates the new storage option. |
src/types.ts |
Extends the public storage type. |
tests/memory-storage.test.tsx |
Tests memory storage behavior. |
README.md |
Documents memory mode. |
package.json |
Adds Biome and a formatting script. |
biome.json |
Enables formatting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // For 'memory' storage the in-memory state is lost on redirect, so PKCE | ||
| // verifier and auth state are kept in sessionStorage to survive the round-trip. | ||
| const storage = config.storage === 'local' ? localStorage : sessionStorage |
| // 'memory' - Held in a module-level Map; never written to disk or browser storage. | ||
| // State is lost on page refresh. Useful for SSR environments, incognito | ||
| // restrictions, or embedded contexts where browser storage is unavailable. | ||
| // NOTE: PKCE verifier and auth state are stored in sessionStorage during the | ||
| // login redirect so the callback page can still complete the code exchange. |
| /** | ||
| * A module-level in-memory store that mimics the Web Storage API interface. | ||
| * Unlike localStorage/sessionStorage, values are held in a plain Map and do | ||
| * not survive page refreshes or shared across tabs. This is useful when |
|
Testing your branch, it simply fails to login. I did not look too much into it, but it it probably related to the first finding by Copilot. Please make sure it actually works, and I will do another review 🙂 |
What does this pull request change?
Add support for memory storage along with local and session storage
Why is this pull request needed?
In case local storage and session storage is considered as the vulnerability, need a safe option for storage
Issues related to this change
Token stored in local storage is considered as sensitive data which is getting exposed. With mythos release, organisations are scanning all the applications leading to these kind local storage usages being triggered a vulnerabilties and auto suggestion from model is either have a inmemroy storage or build server driven solution. In most scenarios inmemory solution is a quick solution rather than build server driven solutions.