security/add correct token behavior - #93
Open
pablo-schmeiser wants to merge 19 commits into
Open
Conversation
- Add non-field error handling and improve already present error handling in shift creation - Remove green hghlighting for valid, but optional form fields and removing green checkmark from optional form select fields
This is a combination of 5 commits. - chore: Create Mixins for shared, inherited behavior - chore: Refactor shared membership inheritance - chore: Register models to admin view in helper function - chore: Refactor URL patterns to use shared organization CRUD helper - chore: Refactor URL patterns to utilize organization_crud_paths for events and shifts
This is a combination of 3 commits. - fix: Group details were not reachable due to same url - fix: Correct inheritance order for OrganizationCreateUpdateMixin - fix: Refactor delete view classes to fix inheritance issue
This is a combination of 3 commits. - chore: Refactor form templates to use shared remove/restore button component - chore: Refactor often reused buttons to deduplicate code and fix modal backdrop behavior NOTE: Modal is now top-level to correct behavior. Temporary patch for iOS was removed and should be tested on iOS in the future. - chore: Refactor modal trigger components to reduce duplication and improve maintainability
This is a combination of 9 commits. - chore: Add coverage configuration and update dependencies for testing - test: Add set_language to tests - test: Add unit tests for user and membership forms to improve coverage - test: Add unit tests for user authentication and email confirmation flows - fix: Update SSO login condition to check if OAUTH is enabled - test: Add unit tests for SSO login and protected content views - test: Consolidate DOM manipulation tests and improve structure - Introduced a new `dom-harness.cjs` file to centralize DOM-related utilities for testing. - Created various test files for different components, including formset, modal handlers, navigation, sliders, save and restore functionality, and theme toggle. - Refactored class list and element creation logic to reduce duplication across tests. - Enhanced test setups to better simulate user interactions and DOM events. - Improved assertions for clarity and accuracy in testing expected behaviors. - test: Add unit tests for BaseMixin and permission handling in views - docs: Add Testing Quality Scorecard documentation
- Removed member_shift_summary template and integrated its functionality into shift_users_summary. - Updated templates to use shift_users_summary for displaying user shifts. - Added non-org participants to summary. - Introduced tests for shift_users_summary functionality and filtering behavior.
…confirmation with logging and rate limiting
pablo-schmeiser
commented
Aug 5, 2026
pablo-schmeiser
left a comment
pablo-schmeiser
left a comment
Collaborator
Author
There was a problem hiding this comment.
Added a review and found some stuff to improve
|
|
||
| if ip_attempts >= self.max_failed_attempts_per_ip: | ||
| logger.warning( | ||
| f'Rate limit exceeded for IP {ip_address}: ' |
Collaborator
Author
There was a problem hiding this comment.
IP addresses are PII and should not be logged directly (logging a hash may be sufficient) or only in combination with a data retention policy.
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.
Description
This PR introduces robust security enhancements for token-based authentication flows (password reset and email confirmation), mitigating risks related to token guessing, brute-forcing, and replay attacks.
Security Mitigations (CWEs)
Key Changes
TokenSecurityValidator): Added a centralized security validator to comprehensively check tokens, manage rate limits, and safely mark tokens as used.FailedTokenAttempt,TokenUsageLog) to track failed validation attempts and successful token usage.PasswordResetConfirmView: Integrated theTokenSecurityValidatorto check rate limits before token validation and correctly mark tokens as used.ConfirmEMailView: Updated to handle and log invalid UID/token attempts and mark confirmation links as used. Client IPs are extracted and logged appropriately.local_settings.sample.py(and defaultsettings.py) to easily control max failed attempts and time windows (e.g.,TOKEN_SECURITY_MAX_FAILED_ATTEMPTS_PER_IP,TOKEN_SECURITY_MAX_TOKEN_LENGTH).test_token_security.py) to verify the new token security logic and rate limits, alongside updates to existing user and password view tests.NOTE TO REVIEWERS
This should be merged after #92