improve docs and remove check we already do in check_domain ... also … #65
Conversation
…/ is not supported
|
I can haz review ? |
| }; | ||
|
|
||
| size_t i; | ||
|
|
||
| for (i = 0; i < valid_uris_count; ++i) { | ||
| size_t len = strlen(valid_uris[i]); | ||
|
|
||
| if (link_len > len && | ||
| strncasecmp((char *)link, valid_uris[i], len) == 0 && | ||
| rinku_isalnum(link[len])) |
There was a problem hiding this comment.
I'm hesitant to remove this in case autolink_issafe gets another consumer. As the overarching "is this safe to auto-link?" method, I'd rather leave this check in.
There was a problem hiding this comment.
just because there is 1 alphanum character does not mean it's safe to link ... implementing partial checks in multiple places seems like a bad security model ... ideally this method would be "is_safe_protocol" ...
| link->start = pos; | ||
| link->end = utf8proc_find_space(data, link->end, size); | ||
|
|
||
| // move to before the protocol | ||
| while (link->start && rinku_isalpha(data[link->start - 1])) |
There was a problem hiding this comment.
I don't think these kinds of comments are helpful. (see the gist of this article)
There was a problem hiding this comment.
this is pretty cryptic and took us a while to understand ... so I think having some top-level understanding of the flow makes it easier to read
There was a problem hiding this comment.
... ideally the code would be more readable ... but I was not brave enough to do that :D
There was a problem hiding this comment.
maybe you could write some comments for these methods ... I'm just poking in here and trying to piece information together ...
| bool | ||
| autolink_issafe(const uint8_t *link, size_t link_len) | ||
| { | ||
| static const size_t valid_uris_count = 5; | ||
| static const char *valid_uris[] = { | ||
| "/", "http://", "https://", "ftp://", "mailto:" |
/ is not supported since we check if the string starts with ':' earlier
@vmg @kivikakk