Limit TitleExtractor to allow only Remark42 whitelisted domains #1681
Conversation
Pull Request Test Coverage Report for Build 6475851841
💛 - Coveralls |
Allowed domains consist of `REMARK_URL` second-level domain (or whole IP in case it's IP like `127.0.0.1`) and `ALLOWED_HOSTS`. That is needed to prevent Remark42 from asking arbitrary servers and storing the page title as the comment.PostTitle. Previous behaviour allowed the caller of the API to create a comment with an arbitrary URL and learn the title of the page, which might be accessible to the server Remark42 is installed on but not to the user outside that network (CWE-918).
paskal
force-pushed
the
paskal/CWE-918
branch
from
25a8d88
to
a5c4e68
Compare
backend/app/store/service/title.go
Outdated
| } | ||
| allowed := false | ||
| for _, domain := range t.allowedDomains { | ||
| if strings.HasSuffix(u.Hostname(), domain) { |
There was a problem hiding this comment.
isn't it risky a little bit? I mean can it allow ab.example.com for allowed b.example.com?
There was a problem hiding this comment.
It's a bit risky, however I see no way around for allowing case like example.org when remark42.example.com is used for Remark42 installation for comments on example.com. I think the solution here is outlining that risk in the documentation.
Also, the current case allowed requests to notexample.org if my domain is example.org (e.g. suffixes match): I've fixed that.
There was a problem hiding this comment.
Any other behaviour would be a breaking change. It's a small part of functionality so maybe a requirement for listing your domain(s) explicitly in AllowedDomains in case Remark42 runs on a separate domain is not bad.
I can alter the logic to get second-level domain only from REMARK_URL and use ALLOWED_HOSTS as-is, that seem to make sense.
Allowed domains consist of `REMARK_URL` second-level domain (or whole IP in case it's IP like `127.0.0.1`) and `ALLOWED_HOSTS`. That is needed to prevent Remark42 from asking arbitrary servers and storing the page title as the comment.PostTitle. Previous behaviour allowed the caller of the API to create a comment with an arbitrary URL and learn the title of the page, which might be accessible to the server Remark42 is installed on but not to the user outside that network (CWE-918).
paskal
force-pushed
the
paskal/CWE-918
branch
from
1f26623
to
f0071ae
Compare
|
LGTM |
Allowed domains consist of
REMARK_URLsecond-level domain (or whole IP in case it's IP like127.0.0.1) andALLOWED_HOSTS. That is needed to prevent Remark42 from asking arbitrary servers and storing the page title as the comment.PostTitle.Previous behaviour allowed the caller of the API to create a comment with an arbitrary URL and learn the title of the page, which might be accessible to the server Remark42 is installed on but not to the user outside that network (CWE-918).
Resolves #1677.