asf-tooling opened a new issue, #970:
URL: https://github.com/apache/tooling-trusted-releases/issues/970
**ASVS Level(s):** [L1]
**Description:**
### Summary
The primary OAuth authentication callback at `/auth` writes new session data
without first terminating the existing session token, violating ASVS 7.2.4's
explicit requirement to 'terminate the current session token' before generating
a new one. The code calls `session.write(oauth_data)` directly without calling
`session.clear()` first. While the application's signed cookie architecture
provides inherent resistance to classical session fixation attacks, the
implementation does not follow the defense-in-depth principle demonstrated in
the admin browse-as flow where `session.clear()` is correctly called before
writing new session data.
### Details
**Affected Files and Lines:**
- `src/asfquart/generics.py:~97-100` - OAuth callback without session.clear()
- `src/asfquart/session.py:107-117` - Session write implementation
The OAuth callback writes new session data without clearing the existing
session first, violating ASVS requirements and best practices.
### Recommended Remediation
Add `session.clear()` before `session.write()` in the OAuth callback:
```python
# src/asfquart/generics.py — OAuth callback branch (line ~97)
oauth_data = await rv.json()
asfquart.session.clear() # ← ADD: Terminate current session token
(ASVS 7.2.4)
asfquart.session.write(oauth_data) # Generate new session token
```
**Alternative (Best Practice):** Create a dedicated `session.regenerate()`
function that atomically calls `clear()` then `write()` to prevent future
regressions:
```python
def regenerate(session_data: dict) -> None:
"""Atomically clear old session and write new session."""
clear()
write(session_data)
```
This function should be used at all authentication entry points.
### Acceptance Criteria
- [ ] session.clear() called before session.write()
- [ ] OAuth authentication terminates old session
- [ ] session.regenerate() function considered
- [ ] All authentication entry points reviewed
- [ ] Integration test verifies session termination
- [ ] Unit test verifying the fix
### References
- Source reports: L1:7.2.4.md
- Related findings: FINDING-005
- ASVS sections: 7.2.4
### Priority
High
---
---
**Triage notes:** audit_guidance about clear not being needed because write
effectively does a clear by OVERWRITING IT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
