Fix overly generous matching of pushkin app IDs, introduced in 0.5.0 #269
Conversation
Prevent dots (.) from being interpreted as wildcards. Only use pushkins which match the entire app ID, rather than a substring.
squahtx
force-pushed
the
squah/fix_pushkin_matching
branch
from
bae7bff
to
14f3699
Compare
sygnal/http.py
Outdated
| # otherwise, find any pushkins whose appid patterns match | ||
| pushkins = [] | ||
| for pushkin_appid, pushkin in self.sygnal.pushkins.items(): | ||
| pattern = glob_to_regex(pushkin_appid, ignore_case=False) |
There was a problem hiding this comment.
It seems like it would be nice to precompile these, but it's less easy than the other case so I guess it depends on how ugly you think that'd be. (It also short-circuits to doing a direct check in the dict for the exact app ID, so I don't know how commonly this case is hit.)
There was a problem hiding this comment.
pushkin_appid is the same as pushkin.name in the other PR. I'm thinking of doing a refactor to have pushkins cache their compiled patterns and add Pushkin.matches_appid(str).
(Why do we have two spellings of appid and app_id in the code?)
The exact app ID match both incorrect and correct at the same time. appid is a literal while pushkin_appid is a pattern. It just so happens that ? matches a literal ? and * matches a literal *.
There was a problem hiding this comment.
I've changed the patterns to be explicitly cached on startup.
There was a problem hiding this comment.
Why do we have two spellings of appid and app_id in the code?
because life is hard and different people think different things about collapsing 'id' into the same word or not /shrug
It'll be because it needs #268 to be merged first. |
|
Nothing's really changed since the last review. It's only a merge from main followed by a switch to |
Addresses #255 together with
#268#281 and #270.Prevent dots (.) from being interpreted as wildcards.
Only use pushkins which match the entire app ID, rather than a
substring.