fix: restrict future dates in mark attendance - #4950
Conversation
Confidence Score: 5/5Both the UI and server-side paths are now guarded against future dates; the change is minimal and targeted. The two-line Python changes close a clear gap — get_unmarked_days now clamps to today, and mark_bulk_attendance rejects future dates before any DB write. The JS change is defensive-in-depth only. No unrelated logic is touched. No files require special attention. Reviews (3): Last reviewed commit: "chore: use getdate() and compute today o..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def bulk_assign(self, employees: list): | ||
| validate_bulk_tool_fields(self, ["company", "holiday_list"], employees) | ||
|
|
||
| if len(employees) <= 30: | ||
| return self._bulk_assign(employees) | ||
|
|
||
| frappe.enqueue(self._bulk_assign, timeout=3000, employees=employees) | ||
| frappe.msgprint( | ||
| _("Assignment of Holiday List has been queued. It may take a few minutes."), | ||
| alert=True, | ||
| indicator="blue", | ||
| ) | ||
|
|
||
| def _bulk_assign(self, employees: list): | ||
| success, failure = [], [] | ||
| count = 0 | ||
| savepoint = "before_holiday_list_assignment" | ||
|
|
||
| for d in employees: | ||
| try: | ||
| frappe.db.savepoint(savepoint) | ||
| employee = frappe.get_doc("Employee", d) | ||
| employee.holiday_list = self.holiday_list | ||
| employee.save() | ||
|
|
There was a problem hiding this comment.
bulk_assign: _bulk_assign iterates over the client-supplied employees list without re-validating it against the get_employees() scope. An HR User scoped to Company X can call bulk_assign with an employee ID from Company Y; validate_bulk_tool_fields only checks required fields, not company/department boundary, and frappe.get_doc("Employee", d) loads the record without check_permission=True (contrast with hrms/hr/utils.py:174 which explicitly passes it). The save then writes the out-of-scope employee's holiday list.
How this was verified: Static source-to-sink trace: the attacker-controlled employees RPC parameter reaches frappe.get_doc("Employee", d) → employee.save() with no re-query against the match-conditions applied in get_employees. Concrete exploit: user with Company X scope sends employees=["EMP099"] (Company Y record); _bulk_assign accepts and persists the assignment.
2aa9b6b to
e548f5e
Compare
|
Tick the box to add this pull request to the merge queue (same as
|
|
We don't exactly restrict future attendance marking anywhere. This change seems unnecessary. |
In the Mark Attendance dialog, the to_date field already prevents users from selecting future dates through the date picker, the future dates are disabled (greyed out). However, users can still manually enter or modify the date value. I added this validation to prevent manually entering a future date, so the behavior remains consistent regardless of how the value is provided. |
|
Then maybe we should take the out the max date in date picker as well |
If we remove the max_date restriction, users will be able to explicitly select future dates and mark attendance as Present. I'm not sure if that's the intended behavior. Also, there doesn't seem to be any validation in the Attendance DocType to prevent marking Present for a future date. If the expected behavior is to disallow future attendance for the Present status, we can add a conditional validation to throw an error when the attendance date is in the future, the status is Present, and the attendance is not linked to an Attendance Request. This will ensure the restriction is enforced consistently. Also, are there any scenarios where marking Present attendance for a future date is expected or allowed? |
2 participants
Description: The Mark Attendance dialog only greyed out future dates in the
to_datedatepicker. That's UI-only —ControlDate.validate()checks the date format, not the range — so a future date could still be typed in. Behind it the server had no guard at all:get_unmarked_days()offered future dates as options, andmark_bulk_attendance()marked whatever dates it was sent.Before:
Screencast.from.2026-07-16.20-42-03.webm
After:
Screencast.from.2026-07-16.20-39-53.webm
Backport needed for v15, v16