🛡️ Sentinel: [HIGH] Fix insecure anonymous session management#102
🛡️ Sentinel: [HIGH] Fix insecure anonymous session management#102guangtouwangba wants to merge 1 commit intomasterfrom
Conversation
- **Vulnerability:** Anonymous sessions were generated from User-Agent hashing, causing session collisions for users with identical browser configurations.
- **Fix:** Implemented unique session ID generation.
- Frontend: Generates a UUIDv4 (stored in localStorage) and sends it via `X-Anonymous-ID` header.
- Backend: Prioritizes `X-Anonymous-ID` for session identification, falling back to User-Agent for legacy clients.
- **Verification:** Added backend unit tests (`tests/unit/test_auth_anon.py`) verifying unique IDs are generated when the header is present.
- **Journal:** Added entry to `.jules/sentinel.md`.
Summary of ChangesHello @guangtouwangba, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security of anonymous user sessions by moving away from a User-Agent-based identification system, which was prone to collisions. The new approach introduces a robust, client-generated unique identifier that is prioritized by the backend, ensuring that each anonymous user maintains a distinct and secure session. This change mitigates the risk of data leakage and unauthorized access for anonymous users. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the insecure anonymous session management vulnerability by replacing the User-Agent-based session ID with a client-generated unique ID. The backend implementation correctly prioritizes the new X-Anonymous-ID header while maintaining backward compatibility. The addition of unit tests is also a great step towards ensuring the reliability of this security fix. I have provided a couple of suggestions to further enhance the security of the client-side ID generation and improve the test coverage for an important edge case on the backend.
| assert user.is_anonymous is True | ||
| # Hash of empty string | ||
| expected_hash = hashlib.sha256(b"").hexdigest()[:16] | ||
| assert user.user_id == f"anon-{expected_hash}" |
There was a problem hiding this comment.
The test coverage for the new anonymous session logic is good. To make it even more robust, I suggest adding a test case for when the x-anonymous-id header is present but contains an empty string. The current backend implementation should fall back to using the user-agent in this scenario, and verifying this behavior will cover an important edge case.
| assert user.user_id == f"anon-{expected_hash}" | |
| @pytest.mark.asyncio | |
| async def test_empty_header_fallback_to_user_agent(self): | |
| """Test that an empty X-Anonymous-ID header falls back to User-Agent.""" | |
| user_agent = "Mozilla/5.0 (Test)" | |
| req = MagicMock(spec=Request) | |
| req.headers = { | |
| "user-agent": user_agent, | |
| "x-anonymous-id": "" | |
| } | |
| user = await get_optional_user(request=req, authorization=None) | |
| assert user.is_anonymous is True | |
| # Verify it uses UA hash | |
| expected_hash = hashlib.sha256(user_agent.encode()).hexdigest()[:16] | |
| assert user.user_id == f"anon-{expected_hash}" |
| if (typeof crypto !== 'undefined' && crypto.randomUUID) { | ||
| id = crypto.randomUUID(); | ||
| } else { | ||
| // Fallback for older browsers / non-secure contexts | ||
| id = 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, function ( | ||
| c | ||
| ) { | ||
| const r = (Math.random() * 16) | 0, | ||
| v = c === 'x' ? r : (r & 0x3) | 0x8; | ||
| return v.toString(16); | ||
| }); |
There was a problem hiding this comment.
The fallback for UUID generation uses Math.random(), which is not cryptographically secure. This is particularly relevant because crypto.randomUUID() is unavailable in non-secure contexts (e.g., http://localhost during development), causing this insecure fallback to be used.
To enhance security, I recommend using crypto.getRandomValues(), which is cryptographically secure and more widely supported, especially in non-secure contexts. This ensures that even in development or older browsers, the generated ID has strong randomness.
if (typeof crypto !== 'undefined' && crypto.randomUUID) {
id = crypto.randomUUID();
} else if (typeof crypto !== 'undefined' && crypto.getRandomValues) {
// Fallback for browsers without crypto.randomUUID (e.g. in non-secure contexts).
// This is more secure than using Math.random().
const rnds = new Uint8Array(16);
crypto.getRandomValues(rnds);
rnds[6] = (rnds[6] & 0x0f) | 0x40; // Version 4
rnds[8] = (rnds[8] & 0x3f) | 0x80; // Variant RFC4122
const hex: string[] = [];
for (let i = 0; i < 16; ++i) {
hex.push((rnds[i] + 0x100).toString(16).substring(1));
}
id = `${hex.slice(0, 4).join('')}-${hex.slice(4, 6).join('')}-${hex.slice(6, 8).join('')}-${hex.slice(8, 10).join('')}-${hex.slice(10, 16).join('')}`;
} else {
// Insecure fallback for very old browsers without any crypto API.
console.warn('Generating insecure anonymous ID. Use a modern browser for better security.');
id = 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, function (c) {
const r = (Math.random() * 16) | 0,
v = c === 'x' ? r : (r & 0x3) | 0x8;
return v.toString(16);
});
}1 participant
X-Anonymous-IDheader.X-Anonymous-IDfor session identification, falling back to User-Agent for legacy clients.tests/unit/test_auth_anon.py) verifying unique IDs are generated when the header is present..jules/sentinel.md.