3 Commits

Author	SHA1	Message	Date
cproudlock	7260244b7e	Key the client address on the connection, not a header the caller sends ... client_ip() read the first X-Forwarded-For hop unconditionally. The login rate limit and the account lockout are keyed on that value and the audit log records it, so rotating the header per request bought an unbounded budget and an untraceable trail. This walked around TRUSTED_PROXY_COUNT, which exists to decide exactly this. The app leaves ProxyFix off by default and says why, ten lines of comment away: ProxyFix trusts X-Forwarded-*, a client can send those itself, and the login limiter is keyed on the address. Then client_ip read the header anyway, so the stated protection never applied. Resolution is now from remote_addr, which ProxyFix has already filled from the header wherever a proxy is genuinely configured. Both cases end at the same trustworthy source, and the port still comes off because ARR forwards clientip:port and ProxyFix passes it through verbatim. geenforce and tools had each hand-rolled the same mistake for their own limiters. Rather than duplicate security-relevant logic a third time, client_ip and strip_port join the shopdb.api surface (contract 0.28.0). geenforce's _trusted_client_ip, which already refused the raw header for its IP allowlist, is untouched and was the model for this. The stability table was also missing rows for 0.26.0 and 0.27.0. INSTALL-WINDOWS-IIS.md never mentioned TRUSTED_PROXY_COUNT, so a site installed from it runs behind IIS with the setting at 0: every request looks like it came from IIS, the whole site shares one rate-limit bucket, and the audit log records the proxy for every action. It now says so, and says not to set it when nothing is in front. Three test modules simulated separate callers by setting X-Forwarded-For, which is the header a real attacker rotates to escape the limit. One of them, test_client_ip_prefers_forwarded_first_hop_ stripped, asserted the defect outright. The suite could not have caught this. They now drive REMOTE_ADDR, with a regression guard that forging the header buys nothing. Scope: product	2026-08-31 07:02:17 -04:00
cproudlock	8750ba2e74	Let a site open secure send to guests, and choose how many times a link opens ... Two asks, and the first is a real security decision so it is a SETTING that defaults to the safe answer rather than a code change somebody inherits. securesend_allow_guests, OFF by default. With it off the model is unchanged: a login plus tools.secrets. With it on, somebody with no account - at a shared bay terminal, a contractor - can create a link too. The cost is named in the setting's own description and pinned by a test: a guest secret has NO CREATOR, so the audit trail can say when and from where but never who. That is the whole reason it is a switch a person turns on deliberately. IT FAILS CLOSED. A missing row, a half-migrated site, an unhappy database - the answer is no. An error must never be the thing that opens an unauthenticated write endpoint. A SIGNED-IN USER STILL NEEDS THE PERMISSION even when guests are allowed, which is surprising enough that a test pins it: opening the door to guests is a decision about the door, not about revoking what a role holds. Guest creation is rate limited per address, same shape as the login limiter. Not a security boundary - a determined person has more addresses - but an open endpoint should not be an easy place to park data or fill a disk. VIEWS ARE NOW A NUMBER, 1 to 100, instead of a four-item dropdown. How many people need a password is the sender's business: a shift handover is not one colleague on a call. The floor is what matters, not the ceiling - unlimited views would just be a password on a web page. Also folded in, from the retention question: nothing removed a secret that EXPIRED WITHOUT BEING OPENED. The recipient goes on holiday, the link is never clicked, and the ciphertext sat there for good - not readable, but it made "nothing lingers" untrue and grew the table in exactly the case where the feature failed. Any create now sweeps expired rows and a late click tidies its own, so it stays bounded without a scheduler an air-gapped site would never add. Two gates caught mistakes on the way: current_user is a LocalProxy and is never None, so `current_user is None` silently took the signed-in branch for a guest; and cache was imported straight from shopdb.extensions when it is already on the shopdb.api contract surface. 1836 tests green.	2026-08-21 15:53:30 -04:00
cproudlock	d4b49b91b4	Move secure send into Tech Tools, where somebody would look for it ... 9845867 gave it its own plugin because Tech Tools is browser-only and this needs storage. That was the right reading of the code and the wrong answer for the person using it: Tech Tools is where you go for a utility, and a tool nobody can find is a tool nobody uses. So Tech Tools now has a blueprint, a table and a migration chain where it had none. Its docstring and manifest say why, because "every tool here runs in the browser" was load-bearing documentation and is no longer true. WHAT MOVED /api/securesend -> /api/tools/secrets securesend.create -> tools.secrets securesend0001baseline -> tools0001securesend PLUGIN_TABLE_OWNERS, EXPECTED_HEAD_REVISION, EXEMPT_ENDPOINTS, the universal profile and the api-inventory surface all follow it. THE OLD "NO BACKEND" TEST IS NARROWED, NOT DELETED. It asserted get_blueprint() is None, which is the sort of guard that gets removed when it becomes inconvenient. It now asserts the blueprint serves EXACTLY the three secret routes, with their methods - so a future tool that quietly grows an endpoint or a table has to edit this test, and therefore make the decision on purpose rather than by drift. Asserting methods rather than paths also pins the detail this design turns on: redeeming is POST-ONLY. Chat and mail clients prefetch GET urls to build previews, and a preview that could redeem would consume a one-view secret before its recipient opened the link. Previously that was a comment; now it fails a test. UNINSTALLING THE OLD PLUGIN COULD NOT BE DONE WITH THE PLUGIN TOOLING. `flask plugin uninstall securesend` loads a plugin before uninstalling it, so a plugin whose directory has been deleted cannot be removed by the command that manages plugins - the registry entry had to be edited by hand. Worth fixing separately; it is the same registry-versus-code divergence the device work hit from the other side. MIGRATING A CHAIN THAT HAS ALREADY RUN needs the old table and its version row dropped first, because the relocated chain has no stamp and tries to create a table that exists. Only relevant to anyone who installed securesend during the hour it existed - it did reach the publication repo in 4242028 - and the fix is DROP TABLE securesendsecrets plus DROP TABLE alembic_version_securesend before running plugin upgrade-all. 1829 tests green. Still backend only: there are no pages yet, so nothing here is reachable by a person.	2026-08-21 14:17:57 -04:00