Telephone numbers: the collision limit changes the wording but not the answer, and contact pages share one bucket #374

New issue

Closed

opened 2026-10-01 10:46:14 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-01 10:46:14 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, by reading every caller of phone_numbers.collision_message and running the formset against a spent allowance.

Related to #333.

Checked again on 2026-10-01 against main at 0a5cad587: still present. The code is now at core/phone_numbers.py:326-327 (the check in BasePhoneNumberFormSet.clean), core/phone_numbers.py:305-308 (the comment on asked_by), accounts/forms.py:407-414 and jobs/forms.py:630-637 (the two clean_phone methods); core/mixins.py, core/throttle.py, api/routers/companies.py, resume/candidate.py and the test have not moved.

The limit from #142 is meant to stop one account from learning which telephone numbers are recorded on the instance. On the forms and the API it only changes the words. Once the allowance is spent, a number somebody holds is still refused and a free one is still accepted, so the refusal itself remains the answer. The CV and letter file importer already handles this correctly; the forms and the API do not. Separately, the contact pages never say who is asking, so all accounts share one allowance there.

Where:
- core/phone_numbers.py:349-350 (BasePhoneNumberFormSet.clean): if taken_elsewhere(...): form.add_error("number", collision_message(self.asked_by)). The error is added whatever the message says.
- accounts/forms.py:405-413 and jobs/forms.py:633-640 (clean_phone on the profile and contact forms) and api/routers/companies.py:123-126 (add_contact, 422): the same shape.
- core/mixins.py:145-160 (PhoneNumbersMixin.get_phone_numbers, used by the contact create and update views) builds the formset without asked_by, so collision_message(None) reaches throttle.consume with who=None, and core/throttle.py:110 keys it on str(None).
- core/phone_numbers.py:328-331 says that with no asked_by "the limit has nobody to charge and the honest sentence is given". In fact the shared None bucket is charged.
- For comparison, resume/candidate.py:840-866 (_somebody_elses) refuses every further number in the same words once the allowance is spent, "because telling them apart is the answer".
What happens: with POSTULO_NUMBER_RATE spent, a two-row formset holding one number that another account has and one that nobody has still flags only the first. The flag now reads "You have been told about a lot of numbers already recorded here…" instead of "already recorded". A formset accepts up to 1,000 rows per request, so each request tests hundreds of numbers, and spending the allowance does not stop it. On the contact pages, one person's collisions spend the instance-wide None allowance, and everyone else is then shown the too-often sentence on their own contact forms.
Who can trigger it: any signed-in account, through Your details, the contact form, or POST /api/companies/{pk}/contacts with a token. It reveals only that some account on the instance holds a number, not whose it is. That is the disclosure #142 set out to limit.
Why it matters: the documented protection does nothing. postulo.wiki/Tracking-applications.md promises that "after a score of them in an hour Postulo says when the answer will be available again instead", and the sweep #142 describes still works. The shared bucket also means one person can make every other account see the throttling message.
Fix:
1. Add a way to ask whether an account's allowance is spent without spending it, for example throttle.spent(action, who, rate).
2. In the formset, both clean_phone methods and the API, refuse every new or changed number with ASKED_TOO_OFTEN once the allowance is spent, taken or not, before calling taken_elsewhere. That is the rule _somebody_elses already follows. The API should answer 429 with Retry-After.
3. Pass asked_by=self.request.user from PhoneNumbersMixin.get_phone_numbers, and make asked_by a required argument of formset_for, so the limit is never charged to None.
4. Correct the class docstring.

Related to #142 (closed), which introduced the limit; this is an incomplete fix. tests/test_phone_verification.py:250-268 checks only that the wording changes.

Done when

A test in tests/test_phone_verification.py spends the allowance and then submits a formset with one taken and one free number. It expects both rows to be refused with the same sentence. Today the free row is accepted.
A test posts to the contact create view as two accounts in turn and shows that the first account's collisions do not spend the second's allowance.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, by reading every caller of `phone_numbers.collision_message` and running the formset against a spent allowance. Related to #333. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. The code is now at `core/phone_numbers.py:326-327` (the check in `BasePhoneNumberFormSet.clean`), `core/phone_numbers.py:305-308` (the comment on `asked_by`), `accounts/forms.py:407-414` and `jobs/forms.py:630-637` (the two `clean_phone` methods); `core/mixins.py`, `core/throttle.py`, `api/routers/companies.py`, `resume/candidate.py` and the test have not moved. The limit from #142 is meant to stop one account from learning which telephone numbers are recorded on the instance. On the forms and the API it only changes the words. Once the allowance is spent, a number somebody holds is still refused and a free one is still accepted, so the refusal itself remains the answer. The CV and letter file importer already handles this correctly; the forms and the API do not. Separately, the contact pages never say who is asking, so all accounts share one allowance there. - **Where:** - `core/phone_numbers.py:349-350` (`BasePhoneNumberFormSet.clean`): `if taken_elsewhere(...): form.add_error("number", collision_message(self.asked_by))`. The error is added whatever the message says. - `accounts/forms.py:405-413` and `jobs/forms.py:633-640` (`clean_phone` on the profile and contact forms) and `api/routers/companies.py:123-126` (`add_contact`, 422): the same shape. - `core/mixins.py:145-160` (`PhoneNumbersMixin.get_phone_numbers`, used by the contact create and update views) builds the formset without `asked_by`, so `collision_message(None)` reaches `throttle.consume` with `who=None`, and `core/throttle.py:110` keys it on `str(None)`. - `core/phone_numbers.py:328-331` says that with no `asked_by` "the limit has nobody to charge and the honest sentence is given". In fact the shared `None` bucket is charged. - For comparison, `resume/candidate.py:840-866` (`_somebody_elses`) refuses every further number in the same words once the allowance is spent, "because telling them apart is the answer". - **What happens:** with `POSTULO_NUMBER_RATE` spent, a two-row formset holding one number that another account has and one that nobody has still flags only the first. The flag now reads "You have been told about a lot of numbers already recorded here…" instead of "already recorded". A formset accepts up to 1,000 rows per request, so each request tests hundreds of numbers, and spending the allowance does not stop it. On the contact pages, one person's collisions spend the instance-wide `None` allowance, and everyone else is then shown the too-often sentence on their own contact forms. - **Who can trigger it:** any signed-in account, through *Your details*, the contact form, or `POST /api/companies/{pk}/contacts` with a token. It reveals only that some account on the instance holds a number, not whose it is. That is the disclosure #142 set out to limit. - **Why it matters:** the documented protection does nothing. `postulo.wiki/Tracking-applications.md` promises that "after a score of them in an hour Postulo says when the answer will be available again instead", and the sweep #142 describes still works. The shared bucket also means one person can make every other account see the throttling message. - **Fix:** 1. Add a way to ask whether an account's allowance is spent without spending it, for example `throttle.spent(action, who, rate)`. 2. In the formset, both `clean_phone` methods and the API, refuse every new or changed number with `ASKED_TOO_OFTEN` once the allowance is spent, taken or not, before calling `taken_elsewhere`. That is the rule `_somebody_elses` already follows. The API should answer 429 with `Retry-After`. 3. Pass `asked_by=self.request.user` from `PhoneNumbersMixin.get_phone_numbers`, and make `asked_by` a required argument of `formset_for`, so the limit is never charged to `None`. 4. Correct the class docstring. Related to #142 (closed), which introduced the limit; this is an incomplete fix. `tests/test_phone_verification.py:250-268` checks only that the wording changes. ## Done when - A test in `tests/test_phone_verification.py` spends the allowance and then submits a formset with one taken and one free number. It expects both rows to be refused with the same sentence. Today the free row is accepted. - A test posts to the contact create view as two accounts in turn and shows that the first account's collisions do not spend the second's allowance.

tiagoagueda added this to the 0.5.0 milestone

2026-10-01 10:46:14 +00:00

tiagoagueda added the

security

bug

labels

2026-10-01 10:46:14 +00:00

tiagoagueda referenced this issue from a commit

2026-10-04 10:04:57 +00:00

Refuse every new number once the collision allowance is spent

tiagoagueda closed this issue

2026-10-04 10:04:57 +00:00