Hi Kentaro, On Tue, Aug 03, 2021 at 11:46:50PM +0900, Kentaro Hayashi wrote: > On Fri, 30 Jul 2021 22:31:07 +0200 Salvatore Bonaccorso <car...@debian.org> > wrote: > > [1] > > https://git.claws-mail.org/?p=claws.git;a=commit;h=ac286a71ed78429e16c612161251b9ea90ccd431 > > I'm not an expert, so feedback are welcome. > I think that [1] maybe incomplete. > > [1] commit fixes two parts, checking by is_uri_string [2] and new comparison > with uri->uri. [3] > > [2] > if (is_uri_string(uri->uri) == FALSE) > - return TRUE; > + return FALSE; > > > It seems ok, if it is not valid string, it should return FALSE. > > > [3] > > + if (strlen(uri->uri) > get_uri_len(uri->uri)) > + retval = FALSE; > > Before this check, there are "if (retval == FALSE)" block, > thus the above code must be put before "if (retval == FALSE)" block, > so this fix is wrong. (at least it doesn't work as expected)
The "if (retval == FALSE)" block before [3] you mention is allowing the user to inspect the URL in a dialog and override the decission computed by the function so far. Moving the strlen/get_url_len check before that block would mean the user could also override that check. Therefore it has to be determined first if that difference in URL lengths can be also easily detected by user eyes. I suspect is not that easy, and allowing users to override that would make it easier for phishers to introduce dangerous URLs. But I may be wrong, of course. As currently, even if the used has decided to display the URL, the function will deny if the lenghts differ, so I think that's safer, but, as you say, some users may not expect that. Perhaps this unexpected behaviour requires some more info in the UI, better than opening the door to unsafe URLs. regards, -- Ricardo Mones ~ I'm sorry, my responses are limited. You must ask the right questions. A hologram
signature.asc
Description: PGP signature
